feat: add BIP353 DNS payment instructions by sdmg15 · Pull Request #236 · bitcoindevkit/bdk-cli

sdmg15 · 2026-01-07T22:18:27Z

Description

This PR adds support for BIP353 payment instructions.
The following notable changes are done:

Adding of an option resolve_dns_recipient which receives a Human Readable Name (HRN) and returns the associated address
Modify the create_tx for it to support user specifying recipients as HRN.

Notes to the reviewers

Here is a list of HRN that could be used to test the implementation:

pay@dunxen.dev: Supports BOLT 12 Payment method only (CAUTION: mainnet)
send.some@satsto.me: Supports BOLT 12 payment and On Chain (CAUTION: mainnet)
Testing resolving: bdk-cli -n testnet resolve_dns_recipient testnet@bitdevsyde.org
Testing create tx: bdk-cli -n testnet wallet -w regtest_default_wallet create_tx --to "bcrt1qe497k549sgw9trzym50tdlegq3xx4apjqynjqm:1000" --to_dns "testnet@bitdevsyde.org:5000"

Changelog notice

Add top level command resolve_dns_recipient to resolve the DNS payment instructions
Add option --to_dns to create_tx command to allow sending to a Human Readable Name (HRN)

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

vadim-anfv · 2026-02-07T20:13:21Z

@sdmg15 Hi! Could you make it compile?

coveralls · 2026-02-09T14:38:13Z

Pull Request Test Coverage Report for Build 21818410493

Details

0 of 120(0.0%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.5%) to 10.237%

Changes Missing Coverage	Changed/Added Lines	%
src/utils.rs	15	0.0%
src/handlers.rs	36	0.0%
src/dns_payment_instructions.rs	69	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	12.57%

Totals
Change from base Build 21153868360:	-0.5%
Covered Lines:	268
Relevant Lines:	2618

💛 - Coveralls

sdmg15 · 2026-02-11T04:53:07Z

@va-an Thanks for taking a look. I think it should be resolved now.

vadim-anfv

Thanks for working on this @sdmg15!
I have left a few suggestions, please have a look.

sdmg15 · 2026-02-13T09:35:00Z

@va-an Applied some reviews and recommendation on 6724be5

I've also updated the PR description.

vadim-anfv

@sdmg15 thanks for update and for the detailed description!
Please take a look at the comments.

vadim-anfv

@sdmg15 thanks for update!
I've added some suggestions, pls have a look.

vadim-anfv · 2026-03-28T21:34:18Z

@sdmg15 thanks for updates!
Could you rebase?

vadim-anfv

Thanks for addressing the previous round of feedback. Before tackling more changes - branch has merge conflicts with master, would you mind rebasing first? A few more comments below.

Payment struct design

Payment is shared between two flows with different needs.

In the send flow (process_instructions -> call site in handlers.rs) only receiving_addr is read, everything else is built and discarded. .unwrap() is needed only because the field is Option<Address> despite always being Some here.

In the resolve flow (resolve_dns_recipient) the same struct is filled with the opposite subset: receiving_addr/expected_amount always None, metadata fields populated for display. So every Option<T> here is "always Some" in one flow and "always None" in the other - the type system encodes nullability that doesn't actually exist in either path.

Suggested split:

// resolve flow - display onlypubstructResolvedPaymentInfo{pubhrn:String,pubpayment_methods:Vec<PaymentMethod>,pubdescription:Option<String>,pubmin_amount:Option<Amount>,pubmax_amount:Option<Amount>,pubnotes:String,}// `display(pretty)` moves here// send flow - return what's actually usedpubasyncfnprocess_instructions(...) -> Result<(Address,Amount),Error>

This resolves several inline comments:

unwrap on receiving_addr
the FixedAmount amount mismatch
both unwraps on human_readable_name().

One small process note: please leave review threads open after addressing them - the convention in this repo is for the reviewer to resolve them once they confirm the fix.

vadim-anfv · 2026-05-15T10:52:51Z

+ let mut methods: Vec<String> = Vec::new();
+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }
+ });


Consider map + collect here - drops the mutable accumulator and lets match return the String directly. Would need PaymentMethod::* in the use block.
Suggested change
letmut methods:Vec<String> = Vec::new();
self.payment_methods.iter().for_each(|pm| match pm {
bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
methods.push(format!("Bolt 12 invoice ({})", shorten(offer,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
methods.push(format!("On chain ({})", address))
}
bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
methods.push(format!("Cashu payment ({})", shorten(csh,20,15)))
}
});
let methods:Vec<String> = self
.payment_methods
.iter()
.map(|pm| match pm {
LightningBolt11(bolt11) => {
format!("Bolt 11 invoice ({})", shorten(bolt11,20,15))
}
LightningBolt12(offer) => format!("Bolt 12 invoice ({})", shorten(offer,20,15)),
OnChain(address) => format!("On chain ({})", address),
Cashu(csh) => format!("Cashu payment ({})", shorten(csh,20,15)),
})
.collect();

vadim-anfv · 2026-05-15T11:15:57Z

+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }


shorten runs before the if pretty check, so JSON output contains truncated Bolt 11 / Bolt 12 invoices and Cashu tokens - downstream tools can't use them.
Non-pretty output:
-> % cargo run --all-features -- -n bitcoin resolve_dns_recipient send.some@satsto.me{"description": null,"expected_amount_to_send": null,"hrn":"send.some@satsto.me","max_amount": null,"min_amount": null,"notes":"This is configurable payment instructions. You must send an amount between min_amount and max_amount if set.","payment_methods":["On chain (bc1qztwy6xen3zdtt7z0vrgapmjtfz8acjkfp5fp7l)","Bolt 12 invoice (lno1zr5qyugqgskrk70k...ph4xrxtk2xc3lpq)"]}
The shorten calls must be moved into the if pretty branch only.

vadim-anfv · 2026-05-15T11:37:45Z

+ dns_recipients: Vec<(String, u64)>,
+ #[cfg(feature = "dns_payment")]
+ /// Custom resolver DNS IP to be used for resolution.
+ #[arg(long = "dns_resolver", default_value = "8.8.8.8:53")]


The default DNS resolver value is inconsistent between the two commands:
ResolveDnsRecipient::resolver -> "8.8.8.8"
CreateTx::dns_resolver -> "8.8.8.8:53"
parse_dns_instructions already appends :53 when no port is present, so the :53 suffix in CreateTx is redundant. Drop it to keep the defaults aligned.
Also worth aligning the flag names themselves - --resolver in one command and --dns_resolver in the other forces users to remember two names for the same thing.
Same value also keeps getting renamed as it travels through the call stack. For the resolve command:
commands.rs -> resolver
handlers.rs::handle_resolve_dns_recipient_command(... resolver: String ...)
dns_payment_instructions.rs::resolve_dns_recipient(... ip: String)
dns_payment_instructions.rs::parse_dns_instructions(... resolver_ip: String)
Three different names for the same value makes the flow harder to follow. Use dns_resolver everywhere - both as the CLI flag (--dns_resolver) and as the field/parameter name across all functions.

vadim-anfv · 2026-05-15T21:27:43Z

+ .map_err(|e| Error::Generic(format!("Parsing error occured {e:#?}")))?;
+ let payment = process_instructions(amount, &instructions, resolver).await?;
+
+ recipients.push((payment.receiving_addr.unwrap().into(), amount));


Two issues here:
amount is the user-supplied value, not payment.expected_amount. For FixedAmount instructions the recipient sets an exact amount, and the tx silently goes out with whatever the user typed instead.
.unwrap() relies on process_instructions always setting receiving_addr: Some(...) - an implicit invariant across two files.
Both resolved by the structural change in the top-level comment.

vadim-anfv · 2026-05-15T21:52:03Z

@sdmg15 when addressing the comments, could you split the changes into separate commits per independent fix instead of squashing everything into one? It makes the review much easier to follow (each commit maps to one review thread), and helps with bisect/revert later if needed. They can always be squashed at merge time.

vadim-anfv

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.

Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.

@tvpeter please correct me if my assumption is wrong.

tvpeter · 2026-05-22T10:01:00Z

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.
Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.
@tvpeter please correct me if my assumption is wrong.

Yes, that's correct. The --pretty flag is been removed. But you don't have to worry about removing it at your end. Once it is merged, I will clean it out.

tvpeter

@sdmg15 thank you for working on this.

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

Thank you

tvpeter · 2026-05-22T17:43:44Z

+ for recipient in dns_recipients {
+ use crate::dns_payment_instructions::{
+ parse_dns_instructions, process_instructions,
+ };
+ let amount = Amount::from_sat(recipient.1);
+ let (resolver, instructions) = parse_dns_instructions(
+ &recipient.0,
+ cli_opts.network,
+ dns_resolver.clone(),
+ )


If it is possible, can you handle this DNS resolution concurrently? Else, print the status on the terminal so that users are aware that it is resolving the DNS.

tvpeter · 2026-05-22T17:47:28Z

+ let PaymentMethod::OnChain(addr) = instructions
+ .methods()
+ .iter()
+ .find(|ix| matches!(ix, PaymentMethod::OnChain(_)))


I think you will have to reconsider prioritizing SP code for privacy concerns.

SP is still of type OnChain it's just the address derived that will change.

sdmg15 · 2026-05-25T12:42:37Z

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Yes I think this will be great but the issue here is this will require some upstream changes to the Address or PaymentMethod type in order to be able to support the sp codes formats which I'm not sure they want to support experimental features/crates.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

I've updated the examples in the description with testnet DNS address

vadim-anfv

@sdmg15 thanks for the work on this, and for applying the previous round - the Payment split into ResolvedPaymentInfo and process_instructions -> (Address, Amount) came out clean.

Left a few inline comments below, plus one bigger-picture suggestion: unifying the recipients before the send logic. Right now --to and --to_dns are handled in separate places - DNS recipients are only resolved inside the non-send_all branch, so the building logic sees two different sources at two different points. If instead all --to_dns entries were resolved to (address, amount) and merged into the single recipients list up front - before any tx-building or send_all branching - the send path would just work off one unified list and wouldn't need to know where a recipient came from.

This would naturally fix a couple of the inline issues:

--to no longer has to be required (validate "at least one recipient" on the merged list instead),
--send_all would stop silently dropping --to_dns recipients (it currently only looks at recipients[0]).

Separating recipient parsing/resolution from the payment logic keeps the tx-building code simpler and removes these edge cases by construction.

Thanks again!

vadim-anfv

tACK 2ca719b

Tested create_tx --to_dns end-to-end on regtest (with a local patch to get past the network check). Works as expected - requesting 5000 sats and 250000 sats each produced an output of exactly that amount.

Right now just pre-push does not pass:

error: variable does not need to be mutable
--> src/handlers.rs:532:13
|
532 | mut recipients,
| ----^^^^^^^^^^
| |
| help: remove this `mut`
|
= note: `-D unused-mut` implied by `-D warnings`

Other than that everything looks great.

sdmg15 · 2026-06-23T18:04:03Z

@va-an Thanks for the reviews. I've addressed that. We should be good to go.

vadim-anfv

Thanks a lot for all the work on this @sdmg15, great job - this has come together really nicely!

Just one small thing left: Cargo.lock contains changes unrelated to this PR - a stray cargo update bumped a lot of unrelated crates, including bdk_wallet 2.1.0 -> 2.4.0. Could you regenerate the lock from master so only the new dependency's entries land?

git checkout master -- Cargo.lock
cargo build --all-features

The lock also bumps bdk_wallet to 2.4.0 - this might not pass review from @tvpeter, see #225 (review).

Thanks again!

codecov · 2026-06-28T03:13:52Z

Codecov Report

❌ Patch coverage is 0% with 259 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.97%. Comparing base (2f14a36) to head (943b4b6).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/dns_payment_instructions.rs	0.00%	145 Missing ⚠️
src/handlers.rs	0.00%	106 Missing ⚠️
src/utils.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #236 +/- ##
==========================================
- Coverage 28.87% 26.97% -1.91% 
==========================================
Files 9 10 +1 Lines 3626 3882 +256 ==========================================
Hits 1047 1047 - Misses 2579 2835 +256

Flag	Coverage Δ
rust	`26.97% <0.00%> (-1.91%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tvpeter

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method. So I think it is either you consider using another library that supports SP or provides mappings to non-reusable addresses or you handling that with the available toolset. But I don't think this PR as it is fully accomplishes the purpose of BIP353.

tvpeter · 2026-06-29T16:06:35Z

 /// Adds a recipient to the transaction.
 // Clap Doesn't support complex vector parsing https://github.com/clap-rs/clap/issues/1704.
 // Address and amount parsing is done at run time in handler function.
- #[arg(env = "ADDRESS:SAT", long = "to", required = true, value_parser = parse_recipient)]


Making this optional here removes the validation that clap offers

The validation is still done but at the command implementation level

tvpeter · 2026-06-29T16:08:56Z

+ #[cfg(feature = "dns_payment")]
+ mut recipients,
+ #[cfg(not(feature = "dns_payment"))]
 recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_resolver,


I feel this introduces too many variables to combine with the existing CreateTx, so it will be better to add a new wallet command that handles DNS payment separately.

I don't know your final take on this but I tried doing it by adding CreateDNSTx or something similar which happens to just be a duplicate of CreateTx and introduce more codes. So let me know what you think should be best approach but I think they way it's done right isn't that much harmful

sdmg15 · 2026-06-30T09:29:59Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.

The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.

But I don't think this PR as it is fully accomplishes the purpose of BIP353.

I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

tvpeter · 2026-06-30T11:14:38Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.
The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.
But I don't think this PR as it is fully accomplishes the purpose of BIP353.
I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

So the whole point of my comment is that, if you can find a way around address reuse, it will serve the purpose of the BIP better. See the BIP section on it here.

Thank you.

sdmg15 · 2026-06-30T14:50:12Z

Yeah I get the point.
I think that section is more a caution targeting users setting their DNS payment wallet.

It's not meant for the application/resolver as you don't have control over that. Only the user will responsibly go and set a different wallet address in the DNS records.

That's why BIP353 combined now with BIP352 effectively solves that as the user won't need to update the dns record any more but will get a new fresh Bitcoin address at any request.

tvpeter

utACK d279097

sdmg15 · 2026-01-07T22:18:27Z

Description

This PR adds support for BIP353 payment instructions.
The following notable changes are done:

Adding of an option resolve_dns_recipient which receives a Human Readable Name (HRN) and returns the associated address
Modify the create_tx for it to support user specifying recipients as HRN.

Notes to the reviewers

Here is a list of HRN that could be used to test the implementation:

pay@dunxen.dev: Supports BOLT 12 Payment method only (CAUTION: mainnet)
send.some@satsto.me: Supports BOLT 12 payment and On Chain (CAUTION: mainnet)
Testing resolving: bdk-cli -n testnet resolve_dns_recipient testnet@bitdevsyde.org
Testing create tx: bdk-cli -n testnet wallet -w regtest_default_wallet create_tx --to "bcrt1qe497k549sgw9trzym50tdlegq3xx4apjqynjqm:1000" --to_dns "testnet@bitdevsyde.org:5000"

Changelog notice

Add top level command resolve_dns_recipient to resolve the DNS payment instructions
Add option --to_dns to create_tx command to allow sending to a Human Readable Name (HRN)

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

vadim-anfv · 2026-02-07T20:13:21Z

@sdmg15 Hi! Could you make it compile?

coveralls · 2026-02-09T14:38:13Z

Pull Request Test Coverage Report for Build 21818410493

Details

0 of 120(0.0%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.5%) to 10.237%

Changes Missing Coverage	Changed/Added Lines	%
src/utils.rs	15	0.0%
src/handlers.rs	36	0.0%
src/dns_payment_instructions.rs	69	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	12.57%

Totals
Change from base Build 21153868360:	-0.5%
Covered Lines:	268
Relevant Lines:	2618

💛 - Coveralls

sdmg15 · 2026-02-11T04:53:07Z

@va-an Thanks for taking a look. I think it should be resolved now.

vadim-anfv

Thanks for working on this @sdmg15!
I have left a few suggestions, please have a look.

sdmg15 · 2026-02-13T09:35:00Z

@va-an Applied some reviews and recommendation on 6724be5

I've also updated the PR description.

vadim-anfv

@sdmg15 thanks for update and for the detailed description!
Please take a look at the comments.

vadim-anfv

@sdmg15 thanks for update!
I've added some suggestions, pls have a look.

vadim-anfv · 2026-03-28T21:34:18Z

@sdmg15 thanks for updates!
Could you rebase?

vadim-anfv

Thanks for addressing the previous round of feedback. Before tackling more changes - branch has merge conflicts with master, would you mind rebasing first? A few more comments below.

Payment struct design

Payment is shared between two flows with different needs.

In the send flow (process_instructions -> call site in handlers.rs) only receiving_addr is read, everything else is built and discarded. .unwrap() is needed only because the field is Option<Address> despite always being Some here.

In the resolve flow (resolve_dns_recipient) the same struct is filled with the opposite subset: receiving_addr/expected_amount always None, metadata fields populated for display. So every Option<T> here is "always Some" in one flow and "always None" in the other - the type system encodes nullability that doesn't actually exist in either path.

Suggested split:

// resolve flow - display onlypubstructResolvedPaymentInfo{pubhrn:String,pubpayment_methods:Vec<PaymentMethod>,pubdescription:Option<String>,pubmin_amount:Option<Amount>,pubmax_amount:Option<Amount>,pubnotes:String,}// `display(pretty)` moves here// send flow - return what's actually usedpubasyncfnprocess_instructions(...) -> Result<(Address,Amount),Error>

This resolves several inline comments:

unwrap on receiving_addr
the FixedAmount amount mismatch
both unwraps on human_readable_name().

One small process note: please leave review threads open after addressing them - the convention in this repo is for the reviewer to resolve them once they confirm the fix.

vadim-anfv · 2026-05-15T10:52:51Z

+ let mut methods: Vec<String> = Vec::new();
+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }
+ });


Consider map + collect here - drops the mutable accumulator and lets match return the String directly. Would need PaymentMethod::* in the use block.
Suggested change
letmut methods:Vec<String> = Vec::new();
self.payment_methods.iter().for_each(|pm| match pm {
bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
methods.push(format!("Bolt 12 invoice ({})", shorten(offer,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
methods.push(format!("On chain ({})", address))
}
bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
methods.push(format!("Cashu payment ({})", shorten(csh,20,15)))
}
});
let methods:Vec<String> = self
.payment_methods
.iter()
.map(|pm| match pm {
LightningBolt11(bolt11) => {
format!("Bolt 11 invoice ({})", shorten(bolt11,20,15))
}
LightningBolt12(offer) => format!("Bolt 12 invoice ({})", shorten(offer,20,15)),
OnChain(address) => format!("On chain ({})", address),
Cashu(csh) => format!("Cashu payment ({})", shorten(csh,20,15)),
})
.collect();

vadim-anfv · 2026-05-15T11:15:57Z

+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }


shorten runs before the if pretty check, so JSON output contains truncated Bolt 11 / Bolt 12 invoices and Cashu tokens - downstream tools can't use them.
Non-pretty output:
-> % cargo run --all-features -- -n bitcoin resolve_dns_recipient send.some@satsto.me{"description": null,"expected_amount_to_send": null,"hrn":"send.some@satsto.me","max_amount": null,"min_amount": null,"notes":"This is configurable payment instructions. You must send an amount between min_amount and max_amount if set.","payment_methods":["On chain (bc1qztwy6xen3zdtt7z0vrgapmjtfz8acjkfp5fp7l)","Bolt 12 invoice (lno1zr5qyugqgskrk70k...ph4xrxtk2xc3lpq)"]}
The shorten calls must be moved into the if pretty branch only.

vadim-anfv · 2026-05-15T11:37:45Z

+ dns_recipients: Vec<(String, u64)>,
+ #[cfg(feature = "dns_payment")]
+ /// Custom resolver DNS IP to be used for resolution.
+ #[arg(long = "dns_resolver", default_value = "8.8.8.8:53")]


The default DNS resolver value is inconsistent between the two commands:
ResolveDnsRecipient::resolver -> "8.8.8.8"
CreateTx::dns_resolver -> "8.8.8.8:53"
parse_dns_instructions already appends :53 when no port is present, so the :53 suffix in CreateTx is redundant. Drop it to keep the defaults aligned.
Also worth aligning the flag names themselves - --resolver in one command and --dns_resolver in the other forces users to remember two names for the same thing.
Same value also keeps getting renamed as it travels through the call stack. For the resolve command:
commands.rs -> resolver
handlers.rs::handle_resolve_dns_recipient_command(... resolver: String ...)
dns_payment_instructions.rs::resolve_dns_recipient(... ip: String)
dns_payment_instructions.rs::parse_dns_instructions(... resolver_ip: String)
Three different names for the same value makes the flow harder to follow. Use dns_resolver everywhere - both as the CLI flag (--dns_resolver) and as the field/parameter name across all functions.

vadim-anfv · 2026-05-15T21:27:43Z

+ .map_err(|e| Error::Generic(format!("Parsing error occured {e:#?}")))?;
+ let payment = process_instructions(amount, &instructions, resolver).await?;
+
+ recipients.push((payment.receiving_addr.unwrap().into(), amount));


Two issues here:
amount is the user-supplied value, not payment.expected_amount. For FixedAmount instructions the recipient sets an exact amount, and the tx silently goes out with whatever the user typed instead.
.unwrap() relies on process_instructions always setting receiving_addr: Some(...) - an implicit invariant across two files.
Both resolved by the structural change in the top-level comment.

vadim-anfv · 2026-05-15T21:52:03Z

@sdmg15 when addressing the comments, could you split the changes into separate commits per independent fix instead of squashing everything into one? It makes the review much easier to follow (each commit maps to one review thread), and helps with bisect/revert later if needed. They can always be squashed at merge time.

vadim-anfv

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.

Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.

@tvpeter please correct me if my assumption is wrong.

tvpeter · 2026-05-22T10:01:00Z

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.
Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.
@tvpeter please correct me if my assumption is wrong.

Yes, that's correct. The --pretty flag is been removed. But you don't have to worry about removing it at your end. Once it is merged, I will clean it out.

tvpeter

@sdmg15 thank you for working on this.

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

Thank you

tvpeter · 2026-05-22T17:43:44Z

+ for recipient in dns_recipients {
+ use crate::dns_payment_instructions::{
+ parse_dns_instructions, process_instructions,
+ };
+ let amount = Amount::from_sat(recipient.1);
+ let (resolver, instructions) = parse_dns_instructions(
+ &recipient.0,
+ cli_opts.network,
+ dns_resolver.clone(),
+ )


If it is possible, can you handle this DNS resolution concurrently? Else, print the status on the terminal so that users are aware that it is resolving the DNS.

tvpeter · 2026-05-22T17:47:28Z

+ let PaymentMethod::OnChain(addr) = instructions
+ .methods()
+ .iter()
+ .find(|ix| matches!(ix, PaymentMethod::OnChain(_)))


I think you will have to reconsider prioritizing SP code for privacy concerns.

SP is still of type OnChain it's just the address derived that will change.

sdmg15 · 2026-05-25T12:42:37Z

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Yes I think this will be great but the issue here is this will require some upstream changes to the Address or PaymentMethod type in order to be able to support the sp codes formats which I'm not sure they want to support experimental features/crates.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

I've updated the examples in the description with testnet DNS address

vadim-anfv

@sdmg15 thanks for the work on this, and for applying the previous round - the Payment split into ResolvedPaymentInfo and process_instructions -> (Address, Amount) came out clean.

Left a few inline comments below, plus one bigger-picture suggestion: unifying the recipients before the send logic. Right now --to and --to_dns are handled in separate places - DNS recipients are only resolved inside the non-send_all branch, so the building logic sees two different sources at two different points. If instead all --to_dns entries were resolved to (address, amount) and merged into the single recipients list up front - before any tx-building or send_all branching - the send path would just work off one unified list and wouldn't need to know where a recipient came from.

This would naturally fix a couple of the inline issues:

--to no longer has to be required (validate "at least one recipient" on the merged list instead),
--send_all would stop silently dropping --to_dns recipients (it currently only looks at recipients[0]).

Separating recipient parsing/resolution from the payment logic keeps the tx-building code simpler and removes these edge cases by construction.

Thanks again!

vadim-anfv

tACK 2ca719b

Tested create_tx --to_dns end-to-end on regtest (with a local patch to get past the network check). Works as expected - requesting 5000 sats and 250000 sats each produced an output of exactly that amount.

Right now just pre-push does not pass:

error: variable does not need to be mutable
--> src/handlers.rs:532:13
|
532 | mut recipients,
| ----^^^^^^^^^^
| |
| help: remove this `mut`
|
= note: `-D unused-mut` implied by `-D warnings`

Other than that everything looks great.

sdmg15 · 2026-06-23T18:04:03Z

@va-an Thanks for the reviews. I've addressed that. We should be good to go.

vadim-anfv

Thanks a lot for all the work on this @sdmg15, great job - this has come together really nicely!

Just one small thing left: Cargo.lock contains changes unrelated to this PR - a stray cargo update bumped a lot of unrelated crates, including bdk_wallet 2.1.0 -> 2.4.0. Could you regenerate the lock from master so only the new dependency's entries land?

git checkout master -- Cargo.lock
cargo build --all-features

The lock also bumps bdk_wallet to 2.4.0 - this might not pass review from @tvpeter, see #225 (review).

Thanks again!

codecov · 2026-06-28T03:13:52Z

Codecov Report

❌ Patch coverage is 0% with 259 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.97%. Comparing base (2f14a36) to head (943b4b6).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/dns_payment_instructions.rs	0.00%	145 Missing ⚠️
src/handlers.rs	0.00%	106 Missing ⚠️
src/utils.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #236 +/- ##
==========================================
- Coverage 28.87% 26.97% -1.91% 
==========================================
Files 9 10 +1 Lines 3626 3882 +256 ==========================================
Hits 1047 1047 - Misses 2579 2835 +256

Flag	Coverage Δ
rust	`26.97% <0.00%> (-1.91%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tvpeter

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method. So I think it is either you consider using another library that supports SP or provides mappings to non-reusable addresses or you handling that with the available toolset. But I don't think this PR as it is fully accomplishes the purpose of BIP353.

tvpeter · 2026-06-29T16:06:35Z

 /// Adds a recipient to the transaction.
 // Clap Doesn't support complex vector parsing https://github.com/clap-rs/clap/issues/1704.
 // Address and amount parsing is done at run time in handler function.
- #[arg(env = "ADDRESS:SAT", long = "to", required = true, value_parser = parse_recipient)]


Making this optional here removes the validation that clap offers

The validation is still done but at the command implementation level

tvpeter · 2026-06-29T16:08:56Z

+ #[cfg(feature = "dns_payment")]
+ mut recipients,
+ #[cfg(not(feature = "dns_payment"))]
 recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_resolver,


I feel this introduces too many variables to combine with the existing CreateTx, so it will be better to add a new wallet command that handles DNS payment separately.

I don't know your final take on this but I tried doing it by adding CreateDNSTx or something similar which happens to just be a duplicate of CreateTx and introduce more codes. So let me know what you think should be best approach but I think they way it's done right isn't that much harmful

sdmg15 · 2026-06-30T09:29:59Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.

The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.

But I don't think this PR as it is fully accomplishes the purpose of BIP353.

I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

tvpeter · 2026-06-30T11:14:38Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.
The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.
But I don't think this PR as it is fully accomplishes the purpose of BIP353.
I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

So the whole point of my comment is that, if you can find a way around address reuse, it will serve the purpose of the BIP better. See the BIP section on it here.

Thank you.

sdmg15 · 2026-06-30T14:50:12Z

Yeah I get the point.
I think that section is more a caution targeting users setting their DNS payment wallet.

It's not meant for the application/resolver as you don't have control over that. Only the user will responsibly go and set a different wallet address in the DNS records.

That's why BIP353 combined now with BIP352 effectively solves that as the user won't need to update the dns record any more but will get a new fresh Bitcoin address at any request.

tvpeter

utACK d279097

sdmg15 · 2026-01-07T22:18:27Z

Description

This PR adds support for BIP353 payment instructions.
The following notable changes are done:

Adding of an option resolve_dns_recipient which receives a Human Readable Name (HRN) and returns the associated address
Modify the create_tx for it to support user specifying recipients as HRN.

Notes to the reviewers

Here is a list of HRN that could be used to test the implementation:

pay@dunxen.dev: Supports BOLT 12 Payment method only (CAUTION: mainnet)
send.some@satsto.me: Supports BOLT 12 payment and On Chain (CAUTION: mainnet)
Testing resolving: bdk-cli -n testnet resolve_dns_recipient testnet@bitdevsyde.org
Testing create tx: bdk-cli -n testnet wallet -w regtest_default_wallet create_tx --to "bcrt1qe497k549sgw9trzym50tdlegq3xx4apjqynjqm:1000" --to_dns "testnet@bitdevsyde.org:5000"

Changelog notice

Add top level command resolve_dns_recipient to resolve the DNS payment instructions
Add option --to_dns to create_tx command to allow sending to a Human Readable Name (HRN)

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

vadim-anfv · 2026-02-07T20:13:21Z

@sdmg15 Hi! Could you make it compile?

coveralls · 2026-02-09T14:38:13Z

Pull Request Test Coverage Report for Build 21818410493

Details

0 of 120(0.0%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.5%) to 10.237%

Changes Missing Coverage	Changed/Added Lines	%
src/utils.rs	15	0.0%
src/handlers.rs	36	0.0%
src/dns_payment_instructions.rs	69	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	12.57%

Totals
Change from base Build 21153868360:	-0.5%
Covered Lines:	268
Relevant Lines:	2618

💛 - Coveralls

sdmg15 · 2026-02-11T04:53:07Z

@va-an Thanks for taking a look. I think it should be resolved now.

vadim-anfv

Thanks for working on this @sdmg15!
I have left a few suggestions, please have a look.

sdmg15 · 2026-02-13T09:35:00Z

@va-an Applied some reviews and recommendation on 6724be5

I've also updated the PR description.

vadim-anfv

@sdmg15 thanks for update and for the detailed description!
Please take a look at the comments.

vadim-anfv

@sdmg15 thanks for update!
I've added some suggestions, pls have a look.

vadim-anfv · 2026-03-28T21:34:18Z

@sdmg15 thanks for updates!
Could you rebase?

vadim-anfv

Thanks for addressing the previous round of feedback. Before tackling more changes - branch has merge conflicts with master, would you mind rebasing first? A few more comments below.

Payment struct design

Payment is shared between two flows with different needs.

In the send flow (process_instructions -> call site in handlers.rs) only receiving_addr is read, everything else is built and discarded. .unwrap() is needed only because the field is Option<Address> despite always being Some here.

In the resolve flow (resolve_dns_recipient) the same struct is filled with the opposite subset: receiving_addr/expected_amount always None, metadata fields populated for display. So every Option<T> here is "always Some" in one flow and "always None" in the other - the type system encodes nullability that doesn't actually exist in either path.

Suggested split:

// resolve flow - display onlypubstructResolvedPaymentInfo{pubhrn:String,pubpayment_methods:Vec<PaymentMethod>,pubdescription:Option<String>,pubmin_amount:Option<Amount>,pubmax_amount:Option<Amount>,pubnotes:String,}// `display(pretty)` moves here// send flow - return what's actually usedpubasyncfnprocess_instructions(...) -> Result<(Address,Amount),Error>

This resolves several inline comments:

unwrap on receiving_addr
the FixedAmount amount mismatch
both unwraps on human_readable_name().

One small process note: please leave review threads open after addressing them - the convention in this repo is for the reviewer to resolve them once they confirm the fix.

vadim-anfv · 2026-05-15T10:52:51Z

+ let mut methods: Vec<String> = Vec::new();
+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }
+ });


Consider map + collect here - drops the mutable accumulator and lets match return the String directly. Would need PaymentMethod::* in the use block.
Suggested change
letmut methods:Vec<String> = Vec::new();
self.payment_methods.iter().for_each(|pm| match pm {
bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
methods.push(format!("Bolt 12 invoice ({})", shorten(offer,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
methods.push(format!("On chain ({})", address))
}
bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
methods.push(format!("Cashu payment ({})", shorten(csh,20,15)))
}
});
let methods:Vec<String> = self
.payment_methods
.iter()
.map(|pm| match pm {
LightningBolt11(bolt11) => {
format!("Bolt 11 invoice ({})", shorten(bolt11,20,15))
}
LightningBolt12(offer) => format!("Bolt 12 invoice ({})", shorten(offer,20,15)),
OnChain(address) => format!("On chain ({})", address),
Cashu(csh) => format!("Cashu payment ({})", shorten(csh,20,15)),
})
.collect();

vadim-anfv · 2026-05-15T11:15:57Z

+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }


shorten runs before the if pretty check, so JSON output contains truncated Bolt 11 / Bolt 12 invoices and Cashu tokens - downstream tools can't use them.
Non-pretty output:
-> % cargo run --all-features -- -n bitcoin resolve_dns_recipient send.some@satsto.me{"description": null,"expected_amount_to_send": null,"hrn":"send.some@satsto.me","max_amount": null,"min_amount": null,"notes":"This is configurable payment instructions. You must send an amount between min_amount and max_amount if set.","payment_methods":["On chain (bc1qztwy6xen3zdtt7z0vrgapmjtfz8acjkfp5fp7l)","Bolt 12 invoice (lno1zr5qyugqgskrk70k...ph4xrxtk2xc3lpq)"]}
The shorten calls must be moved into the if pretty branch only.

vadim-anfv · 2026-05-15T11:37:45Z

+ dns_recipients: Vec<(String, u64)>,
+ #[cfg(feature = "dns_payment")]
+ /// Custom resolver DNS IP to be used for resolution.
+ #[arg(long = "dns_resolver", default_value = "8.8.8.8:53")]


The default DNS resolver value is inconsistent between the two commands:
ResolveDnsRecipient::resolver -> "8.8.8.8"
CreateTx::dns_resolver -> "8.8.8.8:53"
parse_dns_instructions already appends :53 when no port is present, so the :53 suffix in CreateTx is redundant. Drop it to keep the defaults aligned.
Also worth aligning the flag names themselves - --resolver in one command and --dns_resolver in the other forces users to remember two names for the same thing.
Same value also keeps getting renamed as it travels through the call stack. For the resolve command:
commands.rs -> resolver
handlers.rs::handle_resolve_dns_recipient_command(... resolver: String ...)
dns_payment_instructions.rs::resolve_dns_recipient(... ip: String)
dns_payment_instructions.rs::parse_dns_instructions(... resolver_ip: String)
Three different names for the same value makes the flow harder to follow. Use dns_resolver everywhere - both as the CLI flag (--dns_resolver) and as the field/parameter name across all functions.

vadim-anfv · 2026-05-15T21:27:43Z

+ .map_err(|e| Error::Generic(format!("Parsing error occured {e:#?}")))?;
+ let payment = process_instructions(amount, &instructions, resolver).await?;
+
+ recipients.push((payment.receiving_addr.unwrap().into(), amount));


Two issues here:
amount is the user-supplied value, not payment.expected_amount. For FixedAmount instructions the recipient sets an exact amount, and the tx silently goes out with whatever the user typed instead.
.unwrap() relies on process_instructions always setting receiving_addr: Some(...) - an implicit invariant across two files.
Both resolved by the structural change in the top-level comment.

vadim-anfv · 2026-05-15T21:52:03Z

@sdmg15 when addressing the comments, could you split the changes into separate commits per independent fix instead of squashing everything into one? It makes the review much easier to follow (each commit maps to one review thread), and helps with bisect/revert later if needed. They can always be squashed at merge time.

vadim-anfv

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.

Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.

@tvpeter please correct me if my assumption is wrong.

tvpeter · 2026-05-22T10:01:00Z

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.
Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.
@tvpeter please correct me if my assumption is wrong.

Yes, that's correct. The --pretty flag is been removed. But you don't have to worry about removing it at your end. Once it is merged, I will clean it out.

tvpeter

@sdmg15 thank you for working on this.

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

Thank you

tvpeter · 2026-05-22T17:43:44Z

+ for recipient in dns_recipients {
+ use crate::dns_payment_instructions::{
+ parse_dns_instructions, process_instructions,
+ };
+ let amount = Amount::from_sat(recipient.1);
+ let (resolver, instructions) = parse_dns_instructions(
+ &recipient.0,
+ cli_opts.network,
+ dns_resolver.clone(),
+ )


If it is possible, can you handle this DNS resolution concurrently? Else, print the status on the terminal so that users are aware that it is resolving the DNS.

tvpeter · 2026-05-22T17:47:28Z

+ let PaymentMethod::OnChain(addr) = instructions
+ .methods()
+ .iter()
+ .find(|ix| matches!(ix, PaymentMethod::OnChain(_)))


I think you will have to reconsider prioritizing SP code for privacy concerns.

SP is still of type OnChain it's just the address derived that will change.

sdmg15 · 2026-05-25T12:42:37Z

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Yes I think this will be great but the issue here is this will require some upstream changes to the Address or PaymentMethod type in order to be able to support the sp codes formats which I'm not sure they want to support experimental features/crates.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

I've updated the examples in the description with testnet DNS address

vadim-anfv

@sdmg15 thanks for the work on this, and for applying the previous round - the Payment split into ResolvedPaymentInfo and process_instructions -> (Address, Amount) came out clean.

Left a few inline comments below, plus one bigger-picture suggestion: unifying the recipients before the send logic. Right now --to and --to_dns are handled in separate places - DNS recipients are only resolved inside the non-send_all branch, so the building logic sees two different sources at two different points. If instead all --to_dns entries were resolved to (address, amount) and merged into the single recipients list up front - before any tx-building or send_all branching - the send path would just work off one unified list and wouldn't need to know where a recipient came from.

This would naturally fix a couple of the inline issues:

--to no longer has to be required (validate "at least one recipient" on the merged list instead),
--send_all would stop silently dropping --to_dns recipients (it currently only looks at recipients[0]).

Separating recipient parsing/resolution from the payment logic keeps the tx-building code simpler and removes these edge cases by construction.

Thanks again!

vadim-anfv

tACK 2ca719b

Tested create_tx --to_dns end-to-end on regtest (with a local patch to get past the network check). Works as expected - requesting 5000 sats and 250000 sats each produced an output of exactly that amount.

Right now just pre-push does not pass:

error: variable does not need to be mutable
--> src/handlers.rs:532:13
|
532 | mut recipients,
| ----^^^^^^^^^^
| |
| help: remove this `mut`
|
= note: `-D unused-mut` implied by `-D warnings`

Other than that everything looks great.

sdmg15 · 2026-06-23T18:04:03Z

@va-an Thanks for the reviews. I've addressed that. We should be good to go.

vadim-anfv

Thanks a lot for all the work on this @sdmg15, great job - this has come together really nicely!

Just one small thing left: Cargo.lock contains changes unrelated to this PR - a stray cargo update bumped a lot of unrelated crates, including bdk_wallet 2.1.0 -> 2.4.0. Could you regenerate the lock from master so only the new dependency's entries land?

git checkout master -- Cargo.lock
cargo build --all-features

The lock also bumps bdk_wallet to 2.4.0 - this might not pass review from @tvpeter, see #225 (review).

Thanks again!

codecov · 2026-06-28T03:13:52Z

Codecov Report

❌ Patch coverage is 0% with 259 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.97%. Comparing base (2f14a36) to head (943b4b6).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/dns_payment_instructions.rs	0.00%	145 Missing ⚠️
src/handlers.rs	0.00%	106 Missing ⚠️
src/utils.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #236 +/- ##
==========================================
- Coverage 28.87% 26.97% -1.91% 
==========================================
Files 9 10 +1 Lines 3626 3882 +256 ==========================================
Hits 1047 1047 - Misses 2579 2835 +256

Flag	Coverage Δ
rust	`26.97% <0.00%> (-1.91%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tvpeter

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method. So I think it is either you consider using another library that supports SP or provides mappings to non-reusable addresses or you handling that with the available toolset. But I don't think this PR as it is fully accomplishes the purpose of BIP353.

tvpeter · 2026-06-29T16:06:35Z

 /// Adds a recipient to the transaction.
 // Clap Doesn't support complex vector parsing https://github.com/clap-rs/clap/issues/1704.
 // Address and amount parsing is done at run time in handler function.
- #[arg(env = "ADDRESS:SAT", long = "to", required = true, value_parser = parse_recipient)]


Making this optional here removes the validation that clap offers

The validation is still done but at the command implementation level

tvpeter · 2026-06-29T16:08:56Z

+ #[cfg(feature = "dns_payment")]
+ mut recipients,
+ #[cfg(not(feature = "dns_payment"))]
 recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_resolver,


I feel this introduces too many variables to combine with the existing CreateTx, so it will be better to add a new wallet command that handles DNS payment separately.

I don't know your final take on this but I tried doing it by adding CreateDNSTx or something similar which happens to just be a duplicate of CreateTx and introduce more codes. So let me know what you think should be best approach but I think they way it's done right isn't that much harmful

sdmg15 · 2026-06-30T09:29:59Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.

The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.

But I don't think this PR as it is fully accomplishes the purpose of BIP353.

I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

tvpeter · 2026-06-30T11:14:38Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.
The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.
But I don't think this PR as it is fully accomplishes the purpose of BIP353.
I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

So the whole point of my comment is that, if you can find a way around address reuse, it will serve the purpose of the BIP better. See the BIP section on it here.

Thank you.

sdmg15 · 2026-06-30T14:50:12Z

Yeah I get the point.
I think that section is more a caution targeting users setting their DNS payment wallet.

It's not meant for the application/resolver as you don't have control over that. Only the user will responsibly go and set a different wallet address in the DNS records.

That's why BIP353 combined now with BIP352 effectively solves that as the user won't need to update the dns record any more but will get a new fresh Bitcoin address at any request.

tvpeter

utACK d279097

sdmg15 · 2026-01-07T22:18:27Z

Description

This PR adds support for BIP353 payment instructions.
The following notable changes are done:

Adding of an option resolve_dns_recipient which receives a Human Readable Name (HRN) and returns the associated address
Modify the create_tx for it to support user specifying recipients as HRN.

Notes to the reviewers

Here is a list of HRN that could be used to test the implementation:

pay@dunxen.dev: Supports BOLT 12 Payment method only (CAUTION: mainnet)
send.some@satsto.me: Supports BOLT 12 payment and On Chain (CAUTION: mainnet)
Testing resolving: bdk-cli -n testnet resolve_dns_recipient testnet@bitdevsyde.org
Testing create tx: bdk-cli -n testnet wallet -w regtest_default_wallet create_tx --to "bcrt1qe497k549sgw9trzym50tdlegq3xx4apjqynjqm:1000" --to_dns "testnet@bitdevsyde.org:5000"

Changelog notice

Add top level command resolve_dns_recipient to resolve the DNS payment instructions
Add option --to_dns to create_tx command to allow sending to a Human Readable Name (HRN)

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

vadim-anfv · 2026-02-07T20:13:21Z

@sdmg15 Hi! Could you make it compile?

coveralls · 2026-02-09T14:38:13Z

Pull Request Test Coverage Report for Build 21818410493

Details

0 of 120(0.0%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.5%) to 10.237%

Changes Missing Coverage	Changed/Added Lines	%
src/utils.rs	15	0.0%
src/handlers.rs	36	0.0%
src/dns_payment_instructions.rs	69	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	12.57%

Totals
Change from base Build 21153868360:	-0.5%
Covered Lines:	268
Relevant Lines:	2618

💛 - Coveralls

sdmg15 · 2026-02-11T04:53:07Z

@va-an Thanks for taking a look. I think it should be resolved now.

vadim-anfv

Thanks for working on this @sdmg15!
I have left a few suggestions, please have a look.

sdmg15 · 2026-02-13T09:35:00Z

@va-an Applied some reviews and recommendation on 6724be5

I've also updated the PR description.

vadim-anfv

@sdmg15 thanks for update and for the detailed description!
Please take a look at the comments.

vadim-anfv

@sdmg15 thanks for update!
I've added some suggestions, pls have a look.

vadim-anfv · 2026-03-28T21:34:18Z

@sdmg15 thanks for updates!
Could you rebase?

vadim-anfv

Thanks for addressing the previous round of feedback. Before tackling more changes - branch has merge conflicts with master, would you mind rebasing first? A few more comments below.

Payment struct design

Payment is shared between two flows with different needs.

In the send flow (process_instructions -> call site in handlers.rs) only receiving_addr is read, everything else is built and discarded. .unwrap() is needed only because the field is Option<Address> despite always being Some here.

In the resolve flow (resolve_dns_recipient) the same struct is filled with the opposite subset: receiving_addr/expected_amount always None, metadata fields populated for display. So every Option<T> here is "always Some" in one flow and "always None" in the other - the type system encodes nullability that doesn't actually exist in either path.

Suggested split:

// resolve flow - display onlypubstructResolvedPaymentInfo{pubhrn:String,pubpayment_methods:Vec<PaymentMethod>,pubdescription:Option<String>,pubmin_amount:Option<Amount>,pubmax_amount:Option<Amount>,pubnotes:String,}// `display(pretty)` moves here// send flow - return what's actually usedpubasyncfnprocess_instructions(...) -> Result<(Address,Amount),Error>

This resolves several inline comments:

unwrap on receiving_addr
the FixedAmount amount mismatch
both unwraps on human_readable_name().

One small process note: please leave review threads open after addressing them - the convention in this repo is for the reviewer to resolve them once they confirm the fix.

vadim-anfv · 2026-05-15T10:52:51Z

+ let mut methods: Vec<String> = Vec::new();
+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }
+ });


Consider map + collect here - drops the mutable accumulator and lets match return the String directly. Would need PaymentMethod::* in the use block.
Suggested change
letmut methods:Vec<String> = Vec::new();
self.payment_methods.iter().for_each(|pm| match pm {
bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
methods.push(format!("Bolt 12 invoice ({})", shorten(offer,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
methods.push(format!("On chain ({})", address))
}
bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
methods.push(format!("Cashu payment ({})", shorten(csh,20,15)))
}
});
let methods:Vec<String> = self
.payment_methods
.iter()
.map(|pm| match pm {
LightningBolt11(bolt11) => {
format!("Bolt 11 invoice ({})", shorten(bolt11,20,15))
}
LightningBolt12(offer) => format!("Bolt 12 invoice ({})", shorten(offer,20,15)),
OnChain(address) => format!("On chain ({})", address),
Cashu(csh) => format!("Cashu payment ({})", shorten(csh,20,15)),
})
.collect();

vadim-anfv · 2026-05-15T11:15:57Z

+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }


shorten runs before the if pretty check, so JSON output contains truncated Bolt 11 / Bolt 12 invoices and Cashu tokens - downstream tools can't use them.
Non-pretty output:
-> % cargo run --all-features -- -n bitcoin resolve_dns_recipient send.some@satsto.me{"description": null,"expected_amount_to_send": null,"hrn":"send.some@satsto.me","max_amount": null,"min_amount": null,"notes":"This is configurable payment instructions. You must send an amount between min_amount and max_amount if set.","payment_methods":["On chain (bc1qztwy6xen3zdtt7z0vrgapmjtfz8acjkfp5fp7l)","Bolt 12 invoice (lno1zr5qyugqgskrk70k...ph4xrxtk2xc3lpq)"]}
The shorten calls must be moved into the if pretty branch only.

vadim-anfv · 2026-05-15T11:37:45Z

+ dns_recipients: Vec<(String, u64)>,
+ #[cfg(feature = "dns_payment")]
+ /// Custom resolver DNS IP to be used for resolution.
+ #[arg(long = "dns_resolver", default_value = "8.8.8.8:53")]


The default DNS resolver value is inconsistent between the two commands:
ResolveDnsRecipient::resolver -> "8.8.8.8"
CreateTx::dns_resolver -> "8.8.8.8:53"
parse_dns_instructions already appends :53 when no port is present, so the :53 suffix in CreateTx is redundant. Drop it to keep the defaults aligned.
Also worth aligning the flag names themselves - --resolver in one command and --dns_resolver in the other forces users to remember two names for the same thing.
Same value also keeps getting renamed as it travels through the call stack. For the resolve command:
commands.rs -> resolver
handlers.rs::handle_resolve_dns_recipient_command(... resolver: String ...)
dns_payment_instructions.rs::resolve_dns_recipient(... ip: String)
dns_payment_instructions.rs::parse_dns_instructions(... resolver_ip: String)
Three different names for the same value makes the flow harder to follow. Use dns_resolver everywhere - both as the CLI flag (--dns_resolver) and as the field/parameter name across all functions.

vadim-anfv · 2026-05-15T21:27:43Z

+ .map_err(|e| Error::Generic(format!("Parsing error occured {e:#?}")))?;
+ let payment = process_instructions(amount, &instructions, resolver).await?;
+
+ recipients.push((payment.receiving_addr.unwrap().into(), amount));


Two issues here:
amount is the user-supplied value, not payment.expected_amount. For FixedAmount instructions the recipient sets an exact amount, and the tx silently goes out with whatever the user typed instead.
.unwrap() relies on process_instructions always setting receiving_addr: Some(...) - an implicit invariant across two files.
Both resolved by the structural change in the top-level comment.

vadim-anfv · 2026-05-15T21:52:03Z

@sdmg15 when addressing the comments, could you split the changes into separate commits per independent fix instead of squashing everything into one? It makes the review much easier to follow (each commit maps to one review thread), and helps with bisect/revert later if needed. They can always be squashed at merge time.

vadim-anfv

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.

Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.

@tvpeter please correct me if my assumption is wrong.

tvpeter · 2026-05-22T10:01:00Z

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.
Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.
@tvpeter please correct me if my assumption is wrong.

Yes, that's correct. The --pretty flag is been removed. But you don't have to worry about removing it at your end. Once it is merged, I will clean it out.

tvpeter

@sdmg15 thank you for working on this.

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

Thank you

tvpeter · 2026-05-22T17:43:44Z

+ for recipient in dns_recipients {
+ use crate::dns_payment_instructions::{
+ parse_dns_instructions, process_instructions,
+ };
+ let amount = Amount::from_sat(recipient.1);
+ let (resolver, instructions) = parse_dns_instructions(
+ &recipient.0,
+ cli_opts.network,
+ dns_resolver.clone(),
+ )


If it is possible, can you handle this DNS resolution concurrently? Else, print the status on the terminal so that users are aware that it is resolving the DNS.

tvpeter · 2026-05-22T17:47:28Z

+ let PaymentMethod::OnChain(addr) = instructions
+ .methods()
+ .iter()
+ .find(|ix| matches!(ix, PaymentMethod::OnChain(_)))


I think you will have to reconsider prioritizing SP code for privacy concerns.

SP is still of type OnChain it's just the address derived that will change.

sdmg15 · 2026-05-25T12:42:37Z

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Yes I think this will be great but the issue here is this will require some upstream changes to the Address or PaymentMethod type in order to be able to support the sp codes formats which I'm not sure they want to support experimental features/crates.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

I've updated the examples in the description with testnet DNS address

vadim-anfv

@sdmg15 thanks for the work on this, and for applying the previous round - the Payment split into ResolvedPaymentInfo and process_instructions -> (Address, Amount) came out clean.

Left a few inline comments below, plus one bigger-picture suggestion: unifying the recipients before the send logic. Right now --to and --to_dns are handled in separate places - DNS recipients are only resolved inside the non-send_all branch, so the building logic sees two different sources at two different points. If instead all --to_dns entries were resolved to (address, amount) and merged into the single recipients list up front - before any tx-building or send_all branching - the send path would just work off one unified list and wouldn't need to know where a recipient came from.

This would naturally fix a couple of the inline issues:

--to no longer has to be required (validate "at least one recipient" on the merged list instead),
--send_all would stop silently dropping --to_dns recipients (it currently only looks at recipients[0]).

Separating recipient parsing/resolution from the payment logic keeps the tx-building code simpler and removes these edge cases by construction.

Thanks again!

vadim-anfv

tACK 2ca719b

Tested create_tx --to_dns end-to-end on regtest (with a local patch to get past the network check). Works as expected - requesting 5000 sats and 250000 sats each produced an output of exactly that amount.

Right now just pre-push does not pass:

error: variable does not need to be mutable
--> src/handlers.rs:532:13
|
532 | mut recipients,
| ----^^^^^^^^^^
| |
| help: remove this `mut`
|
= note: `-D unused-mut` implied by `-D warnings`

Other than that everything looks great.

sdmg15 · 2026-06-23T18:04:03Z

@va-an Thanks for the reviews. I've addressed that. We should be good to go.

vadim-anfv

Thanks a lot for all the work on this @sdmg15, great job - this has come together really nicely!

Just one small thing left: Cargo.lock contains changes unrelated to this PR - a stray cargo update bumped a lot of unrelated crates, including bdk_wallet 2.1.0 -> 2.4.0. Could you regenerate the lock from master so only the new dependency's entries land?

git checkout master -- Cargo.lock
cargo build --all-features

The lock also bumps bdk_wallet to 2.4.0 - this might not pass review from @tvpeter, see #225 (review).

Thanks again!

codecov · 2026-06-28T03:13:52Z

Codecov Report

❌ Patch coverage is 0% with 259 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.97%. Comparing base (2f14a36) to head (943b4b6).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/dns_payment_instructions.rs	0.00%	145 Missing ⚠️
src/handlers.rs	0.00%	106 Missing ⚠️
src/utils.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #236 +/- ##
==========================================
- Coverage 28.87% 26.97% -1.91% 
==========================================
Files 9 10 +1 Lines 3626 3882 +256 ==========================================
Hits 1047 1047 - Misses 2579 2835 +256

Flag	Coverage Δ
rust	`26.97% <0.00%> (-1.91%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tvpeter

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method. So I think it is either you consider using another library that supports SP or provides mappings to non-reusable addresses or you handling that with the available toolset. But I don't think this PR as it is fully accomplishes the purpose of BIP353.

tvpeter · 2026-06-29T16:06:35Z

 /// Adds a recipient to the transaction.
 // Clap Doesn't support complex vector parsing https://github.com/clap-rs/clap/issues/1704.
 // Address and amount parsing is done at run time in handler function.
- #[arg(env = "ADDRESS:SAT", long = "to", required = true, value_parser = parse_recipient)]


Making this optional here removes the validation that clap offers

The validation is still done but at the command implementation level

tvpeter · 2026-06-29T16:08:56Z

+ #[cfg(feature = "dns_payment")]
+ mut recipients,
+ #[cfg(not(feature = "dns_payment"))]
 recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_resolver,


I feel this introduces too many variables to combine with the existing CreateTx, so it will be better to add a new wallet command that handles DNS payment separately.

I don't know your final take on this but I tried doing it by adding CreateDNSTx or something similar which happens to just be a duplicate of CreateTx and introduce more codes. So let me know what you think should be best approach but I think they way it's done right isn't that much harmful

sdmg15 · 2026-06-30T09:29:59Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.

The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.

But I don't think this PR as it is fully accomplishes the purpose of BIP353.

I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

tvpeter · 2026-06-30T11:14:38Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.
The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.
But I don't think this PR as it is fully accomplishes the purpose of BIP353.
I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

So the whole point of my comment is that, if you can find a way around address reuse, it will serve the purpose of the BIP better. See the BIP section on it here.

Thank you.

sdmg15 · 2026-06-30T14:50:12Z

Yeah I get the point.
I think that section is more a caution targeting users setting their DNS payment wallet.

It's not meant for the application/resolver as you don't have control over that. Only the user will responsibly go and set a different wallet address in the DNS records.

That's why BIP353 combined now with BIP352 effectively solves that as the user won't need to update the dns record any more but will get a new fresh Bitcoin address at any request.

tvpeter

utACK d279097

sdmg15 · 2026-01-07T22:18:27Z

Description

This PR adds support for BIP353 payment instructions.
The following notable changes are done:

Adding of an option resolve_dns_recipient which receives a Human Readable Name (HRN) and returns the associated address
Modify the create_tx for it to support user specifying recipients as HRN.

Notes to the reviewers

Here is a list of HRN that could be used to test the implementation:

pay@dunxen.dev: Supports BOLT 12 Payment method only (CAUTION: mainnet)
send.some@satsto.me: Supports BOLT 12 payment and On Chain (CAUTION: mainnet)
Testing resolving: bdk-cli -n testnet resolve_dns_recipient testnet@bitdevsyde.org
Testing create tx: bdk-cli -n testnet wallet -w regtest_default_wallet create_tx --to "bcrt1qe497k549sgw9trzym50tdlegq3xx4apjqynjqm:1000" --to_dns "testnet@bitdevsyde.org:5000"

Changelog notice

Add top level command resolve_dns_recipient to resolve the DNS payment instructions
Add option --to_dns to create_tx command to allow sending to a Human Readable Name (HRN)

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

vadim-anfv · 2026-02-07T20:13:21Z

@sdmg15 Hi! Could you make it compile?

coveralls · 2026-02-09T14:38:13Z

Pull Request Test Coverage Report for Build 21818410493

Details

0 of 120(0.0%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.5%) to 10.237%

Changes Missing Coverage	Changed/Added Lines	%
src/utils.rs	15	0.0%
src/handlers.rs	36	0.0%
src/dns_payment_instructions.rs	69	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	12.57%

Totals
Change from base Build 21153868360:	-0.5%
Covered Lines:	268
Relevant Lines:	2618

💛 - Coveralls

sdmg15 · 2026-02-11T04:53:07Z

@va-an Thanks for taking a look. I think it should be resolved now.

vadim-anfv

Thanks for working on this @sdmg15!
I have left a few suggestions, please have a look.

sdmg15 · 2026-02-13T09:35:00Z

@va-an Applied some reviews and recommendation on 6724be5

I've also updated the PR description.

vadim-anfv

@sdmg15 thanks for update and for the detailed description!
Please take a look at the comments.

vadim-anfv

@sdmg15 thanks for update!
I've added some suggestions, pls have a look.

vadim-anfv · 2026-03-28T21:34:18Z

@sdmg15 thanks for updates!
Could you rebase?

vadim-anfv

Thanks for addressing the previous round of feedback. Before tackling more changes - branch has merge conflicts with master, would you mind rebasing first? A few more comments below.

Payment struct design

Payment is shared between two flows with different needs.

In the send flow (process_instructions -> call site in handlers.rs) only receiving_addr is read, everything else is built and discarded. .unwrap() is needed only because the field is Option<Address> despite always being Some here.

In the resolve flow (resolve_dns_recipient) the same struct is filled with the opposite subset: receiving_addr/expected_amount always None, metadata fields populated for display. So every Option<T> here is "always Some" in one flow and "always None" in the other - the type system encodes nullability that doesn't actually exist in either path.

Suggested split:

// resolve flow - display onlypubstructResolvedPaymentInfo{pubhrn:String,pubpayment_methods:Vec<PaymentMethod>,pubdescription:Option<String>,pubmin_amount:Option<Amount>,pubmax_amount:Option<Amount>,pubnotes:String,}// `display(pretty)` moves here// send flow - return what's actually usedpubasyncfnprocess_instructions(...) -> Result<(Address,Amount),Error>

This resolves several inline comments:

unwrap on receiving_addr
the FixedAmount amount mismatch
both unwraps on human_readable_name().

One small process note: please leave review threads open after addressing them - the convention in this repo is for the reviewer to resolve them once they confirm the fix.

vadim-anfv · 2026-05-15T10:52:51Z

+ let mut methods: Vec<String> = Vec::new();
+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }
+ });


Consider map + collect here - drops the mutable accumulator and lets match return the String directly. Would need PaymentMethod::* in the use block.
Suggested change
letmut methods:Vec<String> = Vec::new();
self.payment_methods.iter().for_each(|pm| match pm {
bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
methods.push(format!("Bolt 12 invoice ({})", shorten(offer,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
methods.push(format!("On chain ({})", address))
}
bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
methods.push(format!("Cashu payment ({})", shorten(csh,20,15)))
}
});
let methods:Vec<String> = self
.payment_methods
.iter()
.map(|pm| match pm {
LightningBolt11(bolt11) => {
format!("Bolt 11 invoice ({})", shorten(bolt11,20,15))
}
LightningBolt12(offer) => format!("Bolt 12 invoice ({})", shorten(offer,20,15)),
OnChain(address) => format!("On chain ({})", address),
Cashu(csh) => format!("Cashu payment ({})", shorten(csh,20,15)),
})
.collect();

vadim-anfv · 2026-05-15T11:15:57Z

+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }


shorten runs before the if pretty check, so JSON output contains truncated Bolt 11 / Bolt 12 invoices and Cashu tokens - downstream tools can't use them.
Non-pretty output:
-> % cargo run --all-features -- -n bitcoin resolve_dns_recipient send.some@satsto.me{"description": null,"expected_amount_to_send": null,"hrn":"send.some@satsto.me","max_amount": null,"min_amount": null,"notes":"This is configurable payment instructions. You must send an amount between min_amount and max_amount if set.","payment_methods":["On chain (bc1qztwy6xen3zdtt7z0vrgapmjtfz8acjkfp5fp7l)","Bolt 12 invoice (lno1zr5qyugqgskrk70k...ph4xrxtk2xc3lpq)"]}
The shorten calls must be moved into the if pretty branch only.

vadim-anfv · 2026-05-15T11:37:45Z

+ dns_recipients: Vec<(String, u64)>,
+ #[cfg(feature = "dns_payment")]
+ /// Custom resolver DNS IP to be used for resolution.
+ #[arg(long = "dns_resolver", default_value = "8.8.8.8:53")]


The default DNS resolver value is inconsistent between the two commands:
ResolveDnsRecipient::resolver -> "8.8.8.8"
CreateTx::dns_resolver -> "8.8.8.8:53"
parse_dns_instructions already appends :53 when no port is present, so the :53 suffix in CreateTx is redundant. Drop it to keep the defaults aligned.
Also worth aligning the flag names themselves - --resolver in one command and --dns_resolver in the other forces users to remember two names for the same thing.
Same value also keeps getting renamed as it travels through the call stack. For the resolve command:
commands.rs -> resolver
handlers.rs::handle_resolve_dns_recipient_command(... resolver: String ...)
dns_payment_instructions.rs::resolve_dns_recipient(... ip: String)
dns_payment_instructions.rs::parse_dns_instructions(... resolver_ip: String)
Three different names for the same value makes the flow harder to follow. Use dns_resolver everywhere - both as the CLI flag (--dns_resolver) and as the field/parameter name across all functions.

vadim-anfv · 2026-05-15T21:27:43Z

+ .map_err(|e| Error::Generic(format!("Parsing error occured {e:#?}")))?;
+ let payment = process_instructions(amount, &instructions, resolver).await?;
+
+ recipients.push((payment.receiving_addr.unwrap().into(), amount));


Two issues here:
amount is the user-supplied value, not payment.expected_amount. For FixedAmount instructions the recipient sets an exact amount, and the tx silently goes out with whatever the user typed instead.
.unwrap() relies on process_instructions always setting receiving_addr: Some(...) - an implicit invariant across two files.
Both resolved by the structural change in the top-level comment.

vadim-anfv · 2026-05-15T21:52:03Z

@sdmg15 when addressing the comments, could you split the changes into separate commits per independent fix instead of squashing everything into one? It makes the review much easier to follow (each commit maps to one review thread), and helps with bisect/revert later if needed. They can always be squashed at merge time.

vadim-anfv

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.

Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.

@tvpeter please correct me if my assumption is wrong.

tvpeter · 2026-05-22T10:01:00Z

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.
Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.
@tvpeter please correct me if my assumption is wrong.

Yes, that's correct. The --pretty flag is been removed. But you don't have to worry about removing it at your end. Once it is merged, I will clean it out.

tvpeter

@sdmg15 thank you for working on this.

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

Thank you

tvpeter · 2026-05-22T17:43:44Z

+ for recipient in dns_recipients {
+ use crate::dns_payment_instructions::{
+ parse_dns_instructions, process_instructions,
+ };
+ let amount = Amount::from_sat(recipient.1);
+ let (resolver, instructions) = parse_dns_instructions(
+ &recipient.0,
+ cli_opts.network,
+ dns_resolver.clone(),
+ )


If it is possible, can you handle this DNS resolution concurrently? Else, print the status on the terminal so that users are aware that it is resolving the DNS.

tvpeter · 2026-05-22T17:47:28Z

+ let PaymentMethod::OnChain(addr) = instructions
+ .methods()
+ .iter()
+ .find(|ix| matches!(ix, PaymentMethod::OnChain(_)))


I think you will have to reconsider prioritizing SP code for privacy concerns.

SP is still of type OnChain it's just the address derived that will change.

sdmg15 · 2026-05-25T12:42:37Z

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Yes I think this will be great but the issue here is this will require some upstream changes to the Address or PaymentMethod type in order to be able to support the sp codes formats which I'm not sure they want to support experimental features/crates.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

I've updated the examples in the description with testnet DNS address

vadim-anfv

@sdmg15 thanks for the work on this, and for applying the previous round - the Payment split into ResolvedPaymentInfo and process_instructions -> (Address, Amount) came out clean.

Left a few inline comments below, plus one bigger-picture suggestion: unifying the recipients before the send logic. Right now --to and --to_dns are handled in separate places - DNS recipients are only resolved inside the non-send_all branch, so the building logic sees two different sources at two different points. If instead all --to_dns entries were resolved to (address, amount) and merged into the single recipients list up front - before any tx-building or send_all branching - the send path would just work off one unified list and wouldn't need to know where a recipient came from.

This would naturally fix a couple of the inline issues:

--to no longer has to be required (validate "at least one recipient" on the merged list instead),
--send_all would stop silently dropping --to_dns recipients (it currently only looks at recipients[0]).

Separating recipient parsing/resolution from the payment logic keeps the tx-building code simpler and removes these edge cases by construction.

Thanks again!

vadim-anfv

tACK 2ca719b

Tested create_tx --to_dns end-to-end on regtest (with a local patch to get past the network check). Works as expected - requesting 5000 sats and 250000 sats each produced an output of exactly that amount.

Right now just pre-push does not pass:

error: variable does not need to be mutable
--> src/handlers.rs:532:13
|
532 | mut recipients,
| ----^^^^^^^^^^
| |
| help: remove this `mut`
|
= note: `-D unused-mut` implied by `-D warnings`

Other than that everything looks great.

sdmg15 · 2026-06-23T18:04:03Z

@va-an Thanks for the reviews. I've addressed that. We should be good to go.

vadim-anfv

Thanks a lot for all the work on this @sdmg15, great job - this has come together really nicely!

Just one small thing left: Cargo.lock contains changes unrelated to this PR - a stray cargo update bumped a lot of unrelated crates, including bdk_wallet 2.1.0 -> 2.4.0. Could you regenerate the lock from master so only the new dependency's entries land?

git checkout master -- Cargo.lock
cargo build --all-features

The lock also bumps bdk_wallet to 2.4.0 - this might not pass review from @tvpeter, see #225 (review).

Thanks again!

codecov · 2026-06-28T03:13:52Z

Codecov Report

❌ Patch coverage is 0% with 259 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.97%. Comparing base (2f14a36) to head (943b4b6).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/dns_payment_instructions.rs	0.00%	145 Missing ⚠️
src/handlers.rs	0.00%	106 Missing ⚠️
src/utils.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #236 +/- ##
==========================================
- Coverage 28.87% 26.97% -1.91% 
==========================================
Files 9 10 +1 Lines 3626 3882 +256 ==========================================
Hits 1047 1047 - Misses 2579 2835 +256

Flag	Coverage Δ
rust	`26.97% <0.00%> (-1.91%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tvpeter

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method. So I think it is either you consider using another library that supports SP or provides mappings to non-reusable addresses or you handling that with the available toolset. But I don't think this PR as it is fully accomplishes the purpose of BIP353.

tvpeter · 2026-06-29T16:06:35Z

 /// Adds a recipient to the transaction.
 // Clap Doesn't support complex vector parsing https://github.com/clap-rs/clap/issues/1704.
 // Address and amount parsing is done at run time in handler function.
- #[arg(env = "ADDRESS:SAT", long = "to", required = true, value_parser = parse_recipient)]


Making this optional here removes the validation that clap offers

The validation is still done but at the command implementation level

tvpeter · 2026-06-29T16:08:56Z

+ #[cfg(feature = "dns_payment")]
+ mut recipients,
+ #[cfg(not(feature = "dns_payment"))]
 recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_resolver,


I feel this introduces too many variables to combine with the existing CreateTx, so it will be better to add a new wallet command that handles DNS payment separately.

I don't know your final take on this but I tried doing it by adding CreateDNSTx or something similar which happens to just be a duplicate of CreateTx and introduce more codes. So let me know what you think should be best approach but I think they way it's done right isn't that much harmful

sdmg15 · 2026-06-30T09:29:59Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.

The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.

But I don't think this PR as it is fully accomplishes the purpose of BIP353.

I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

tvpeter · 2026-06-30T11:14:38Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.
The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.
But I don't think this PR as it is fully accomplishes the purpose of BIP353.
I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

So the whole point of my comment is that, if you can find a way around address reuse, it will serve the purpose of the BIP better. See the BIP section on it here.

Thank you.

sdmg15 · 2026-06-30T14:50:12Z

Yeah I get the point.
I think that section is more a caution targeting users setting their DNS payment wallet.

It's not meant for the application/resolver as you don't have control over that. Only the user will responsibly go and set a different wallet address in the DNS records.

That's why BIP353 combined now with BIP352 effectively solves that as the user won't need to update the dns record any more but will get a new fresh Bitcoin address at any request.

tvpeter

utACK d279097

sdmg15 · 2026-01-07T22:18:27Z

Description

This PR adds support for BIP353 payment instructions.
The following notable changes are done:

Adding of an option resolve_dns_recipient which receives a Human Readable Name (HRN) and returns the associated address
Modify the create_tx for it to support user specifying recipients as HRN.

Notes to the reviewers

Here is a list of HRN that could be used to test the implementation:

pay@dunxen.dev: Supports BOLT 12 Payment method only (CAUTION: mainnet)
send.some@satsto.me: Supports BOLT 12 payment and On Chain (CAUTION: mainnet)
Testing resolving: bdk-cli -n testnet resolve_dns_recipient testnet@bitdevsyde.org
Testing create tx: bdk-cli -n testnet wallet -w regtest_default_wallet create_tx --to "bcrt1qe497k549sgw9trzym50tdlegq3xx4apjqynjqm:1000" --to_dns "testnet@bitdevsyde.org:5000"

Changelog notice

Add top level command resolve_dns_recipient to resolve the DNS payment instructions
Add option --to_dns to create_tx command to allow sending to a Human Readable Name (HRN)

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

vadim-anfv · 2026-02-07T20:13:21Z

@sdmg15 Hi! Could you make it compile?

coveralls · 2026-02-09T14:38:13Z

Pull Request Test Coverage Report for Build 21818410493

Details

0 of 120(0.0%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.5%) to 10.237%

Changes Missing Coverage	Changed/Added Lines	%
src/utils.rs	15	0.0%
src/handlers.rs	36	0.0%
src/dns_payment_instructions.rs	69	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	12.57%

Totals
Change from base Build 21153868360:	-0.5%
Covered Lines:	268
Relevant Lines:	2618

💛 - Coveralls

sdmg15 · 2026-02-11T04:53:07Z

@va-an Thanks for taking a look. I think it should be resolved now.

vadim-anfv

Thanks for working on this @sdmg15!
I have left a few suggestions, please have a look.

sdmg15 · 2026-02-13T09:35:00Z

@va-an Applied some reviews and recommendation on 6724be5

I've also updated the PR description.

vadim-anfv

@sdmg15 thanks for update and for the detailed description!
Please take a look at the comments.

vadim-anfv

@sdmg15 thanks for update!
I've added some suggestions, pls have a look.

vadim-anfv · 2026-03-28T21:34:18Z

@sdmg15 thanks for updates!
Could you rebase?

vadim-anfv

Thanks for addressing the previous round of feedback. Before tackling more changes - branch has merge conflicts with master, would you mind rebasing first? A few more comments below.

Payment struct design

Payment is shared between two flows with different needs.

In the send flow (process_instructions -> call site in handlers.rs) only receiving_addr is read, everything else is built and discarded. .unwrap() is needed only because the field is Option<Address> despite always being Some here.

In the resolve flow (resolve_dns_recipient) the same struct is filled with the opposite subset: receiving_addr/expected_amount always None, metadata fields populated for display. So every Option<T> here is "always Some" in one flow and "always None" in the other - the type system encodes nullability that doesn't actually exist in either path.

Suggested split:

// resolve flow - display onlypubstructResolvedPaymentInfo{pubhrn:String,pubpayment_methods:Vec<PaymentMethod>,pubdescription:Option<String>,pubmin_amount:Option<Amount>,pubmax_amount:Option<Amount>,pubnotes:String,}// `display(pretty)` moves here// send flow - return what's actually usedpubasyncfnprocess_instructions(...) -> Result<(Address,Amount),Error>

This resolves several inline comments:

unwrap on receiving_addr
the FixedAmount amount mismatch
both unwraps on human_readable_name().

One small process note: please leave review threads open after addressing them - the convention in this repo is for the reviewer to resolve them once they confirm the fix.

vadim-anfv · 2026-05-15T10:52:51Z

+ let mut methods: Vec<String> = Vec::new();
+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }
+ });


Consider map + collect here - drops the mutable accumulator and lets match return the String directly. Would need PaymentMethod::* in the use block.
Suggested change
letmut methods:Vec<String> = Vec::new();
self.payment_methods.iter().for_each(|pm| match pm {
bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
methods.push(format!("Bolt 12 invoice ({})", shorten(offer,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
methods.push(format!("On chain ({})", address))
}
bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
methods.push(format!("Cashu payment ({})", shorten(csh,20,15)))
}
});
let methods:Vec<String> = self
.payment_methods
.iter()
.map(|pm| match pm {
LightningBolt11(bolt11) => {
format!("Bolt 11 invoice ({})", shorten(bolt11,20,15))
}
LightningBolt12(offer) => format!("Bolt 12 invoice ({})", shorten(offer,20,15)),
OnChain(address) => format!("On chain ({})", address),
Cashu(csh) => format!("Cashu payment ({})", shorten(csh,20,15)),
})
.collect();

vadim-anfv · 2026-05-15T11:15:57Z

+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }


shorten runs before the if pretty check, so JSON output contains truncated Bolt 11 / Bolt 12 invoices and Cashu tokens - downstream tools can't use them.
Non-pretty output:
-> % cargo run --all-features -- -n bitcoin resolve_dns_recipient send.some@satsto.me{"description": null,"expected_amount_to_send": null,"hrn":"send.some@satsto.me","max_amount": null,"min_amount": null,"notes":"This is configurable payment instructions. You must send an amount between min_amount and max_amount if set.","payment_methods":["On chain (bc1qztwy6xen3zdtt7z0vrgapmjtfz8acjkfp5fp7l)","Bolt 12 invoice (lno1zr5qyugqgskrk70k...ph4xrxtk2xc3lpq)"]}
The shorten calls must be moved into the if pretty branch only.

vadim-anfv · 2026-05-15T11:37:45Z

+ dns_recipients: Vec<(String, u64)>,
+ #[cfg(feature = "dns_payment")]
+ /// Custom resolver DNS IP to be used for resolution.
+ #[arg(long = "dns_resolver", default_value = "8.8.8.8:53")]


The default DNS resolver value is inconsistent between the two commands:
ResolveDnsRecipient::resolver -> "8.8.8.8"
CreateTx::dns_resolver -> "8.8.8.8:53"
parse_dns_instructions already appends :53 when no port is present, so the :53 suffix in CreateTx is redundant. Drop it to keep the defaults aligned.
Also worth aligning the flag names themselves - --resolver in one command and --dns_resolver in the other forces users to remember two names for the same thing.
Same value also keeps getting renamed as it travels through the call stack. For the resolve command:
commands.rs -> resolver
handlers.rs::handle_resolve_dns_recipient_command(... resolver: String ...)
dns_payment_instructions.rs::resolve_dns_recipient(... ip: String)
dns_payment_instructions.rs::parse_dns_instructions(... resolver_ip: String)
Three different names for the same value makes the flow harder to follow. Use dns_resolver everywhere - both as the CLI flag (--dns_resolver) and as the field/parameter name across all functions.

vadim-anfv · 2026-05-15T21:27:43Z

+ .map_err(|e| Error::Generic(format!("Parsing error occured {e:#?}")))?;
+ let payment = process_instructions(amount, &instructions, resolver).await?;
+
+ recipients.push((payment.receiving_addr.unwrap().into(), amount));


Two issues here:
amount is the user-supplied value, not payment.expected_amount. For FixedAmount instructions the recipient sets an exact amount, and the tx silently goes out with whatever the user typed instead.
.unwrap() relies on process_instructions always setting receiving_addr: Some(...) - an implicit invariant across two files.
Both resolved by the structural change in the top-level comment.

vadim-anfv · 2026-05-15T21:52:03Z

@sdmg15 when addressing the comments, could you split the changes into separate commits per independent fix instead of squashing everything into one? It makes the review much easier to follow (each commit maps to one review thread), and helps with bisect/revert later if needed. They can always be squashed at merge time.

vadim-anfv

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.

Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.

@tvpeter please correct me if my assumption is wrong.

tvpeter · 2026-05-22T10:01:00Z

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.
Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.
@tvpeter please correct me if my assumption is wrong.

Yes, that's correct. The --pretty flag is been removed. But you don't have to worry about removing it at your end. Once it is merged, I will clean it out.

tvpeter

@sdmg15 thank you for working on this.

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

Thank you

tvpeter · 2026-05-22T17:43:44Z

+ for recipient in dns_recipients {
+ use crate::dns_payment_instructions::{
+ parse_dns_instructions, process_instructions,
+ };
+ let amount = Amount::from_sat(recipient.1);
+ let (resolver, instructions) = parse_dns_instructions(
+ &recipient.0,
+ cli_opts.network,
+ dns_resolver.clone(),
+ )


If it is possible, can you handle this DNS resolution concurrently? Else, print the status on the terminal so that users are aware that it is resolving the DNS.

tvpeter · 2026-05-22T17:47:28Z

+ let PaymentMethod::OnChain(addr) = instructions
+ .methods()
+ .iter()
+ .find(|ix| matches!(ix, PaymentMethod::OnChain(_)))


I think you will have to reconsider prioritizing SP code for privacy concerns.

SP is still of type OnChain it's just the address derived that will change.

sdmg15 · 2026-05-25T12:42:37Z

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Yes I think this will be great but the issue here is this will require some upstream changes to the Address or PaymentMethod type in order to be able to support the sp codes formats which I'm not sure they want to support experimental features/crates.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

I've updated the examples in the description with testnet DNS address

vadim-anfv

@sdmg15 thanks for the work on this, and for applying the previous round - the Payment split into ResolvedPaymentInfo and process_instructions -> (Address, Amount) came out clean.

Left a few inline comments below, plus one bigger-picture suggestion: unifying the recipients before the send logic. Right now --to and --to_dns are handled in separate places - DNS recipients are only resolved inside the non-send_all branch, so the building logic sees two different sources at two different points. If instead all --to_dns entries were resolved to (address, amount) and merged into the single recipients list up front - before any tx-building or send_all branching - the send path would just work off one unified list and wouldn't need to know where a recipient came from.

This would naturally fix a couple of the inline issues:

--to no longer has to be required (validate "at least one recipient" on the merged list instead),
--send_all would stop silently dropping --to_dns recipients (it currently only looks at recipients[0]).

Separating recipient parsing/resolution from the payment logic keeps the tx-building code simpler and removes these edge cases by construction.

Thanks again!

vadim-anfv

tACK 2ca719b

Tested create_tx --to_dns end-to-end on regtest (with a local patch to get past the network check). Works as expected - requesting 5000 sats and 250000 sats each produced an output of exactly that amount.

Right now just pre-push does not pass:

error: variable does not need to be mutable
--> src/handlers.rs:532:13
|
532 | mut recipients,
| ----^^^^^^^^^^
| |
| help: remove this `mut`
|
= note: `-D unused-mut` implied by `-D warnings`

Other than that everything looks great.

sdmg15 · 2026-06-23T18:04:03Z

@va-an Thanks for the reviews. I've addressed that. We should be good to go.

vadim-anfv

Thanks a lot for all the work on this @sdmg15, great job - this has come together really nicely!

Just one small thing left: Cargo.lock contains changes unrelated to this PR - a stray cargo update bumped a lot of unrelated crates, including bdk_wallet 2.1.0 -> 2.4.0. Could you regenerate the lock from master so only the new dependency's entries land?

git checkout master -- Cargo.lock
cargo build --all-features

The lock also bumps bdk_wallet to 2.4.0 - this might not pass review from @tvpeter, see #225 (review).

Thanks again!

codecov · 2026-06-28T03:13:52Z

Codecov Report

❌ Patch coverage is 0% with 259 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.97%. Comparing base (2f14a36) to head (943b4b6).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/dns_payment_instructions.rs	0.00%	145 Missing ⚠️
src/handlers.rs	0.00%	106 Missing ⚠️
src/utils.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #236 +/- ##
==========================================
- Coverage 28.87% 26.97% -1.91% 
==========================================
Files 9 10 +1 Lines 3626 3882 +256 ==========================================
Hits 1047 1047 - Misses 2579 2835 +256

Flag	Coverage Δ
rust	`26.97% <0.00%> (-1.91%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tvpeter

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method. So I think it is either you consider using another library that supports SP or provides mappings to non-reusable addresses or you handling that with the available toolset. But I don't think this PR as it is fully accomplishes the purpose of BIP353.

tvpeter · 2026-06-29T16:06:35Z

 /// Adds a recipient to the transaction.
 // Clap Doesn't support complex vector parsing https://github.com/clap-rs/clap/issues/1704.
 // Address and amount parsing is done at run time in handler function.
- #[arg(env = "ADDRESS:SAT", long = "to", required = true, value_parser = parse_recipient)]


Making this optional here removes the validation that clap offers

The validation is still done but at the command implementation level

tvpeter · 2026-06-29T16:08:56Z

+ #[cfg(feature = "dns_payment")]
+ mut recipients,
+ #[cfg(not(feature = "dns_payment"))]
 recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_resolver,


I feel this introduces too many variables to combine with the existing CreateTx, so it will be better to add a new wallet command that handles DNS payment separately.

I don't know your final take on this but I tried doing it by adding CreateDNSTx or something similar which happens to just be a duplicate of CreateTx and introduce more codes. So let me know what you think should be best approach but I think they way it's done right isn't that much harmful

sdmg15 · 2026-06-30T09:29:59Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.

The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.

But I don't think this PR as it is fully accomplishes the purpose of BIP353.

I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

tvpeter · 2026-06-30T11:14:38Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.
The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.
But I don't think this PR as it is fully accomplishes the purpose of BIP353.
I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

So the whole point of my comment is that, if you can find a way around address reuse, it will serve the purpose of the BIP better. See the BIP section on it here.

Thank you.

sdmg15 · 2026-06-30T14:50:12Z

Yeah I get the point.
I think that section is more a caution targeting users setting their DNS payment wallet.

It's not meant for the application/resolver as you don't have control over that. Only the user will responsibly go and set a different wallet address in the DNS records.

That's why BIP353 combined now with BIP352 effectively solves that as the user won't need to update the dns record any more but will get a new fresh Bitcoin address at any request.

tvpeter

utACK d279097

sdmg15 · 2026-01-07T22:18:27Z

Description

This PR adds support for BIP353 payment instructions.
The following notable changes are done:

Adding of an option resolve_dns_recipient which receives a Human Readable Name (HRN) and returns the associated address
Modify the create_tx for it to support user specifying recipients as HRN.

Notes to the reviewers

Here is a list of HRN that could be used to test the implementation:

pay@dunxen.dev: Supports BOLT 12 Payment method only (CAUTION: mainnet)
send.some@satsto.me: Supports BOLT 12 payment and On Chain (CAUTION: mainnet)
Testing resolving: bdk-cli -n testnet resolve_dns_recipient testnet@bitdevsyde.org
Testing create tx: bdk-cli -n testnet wallet -w regtest_default_wallet create_tx --to "bcrt1qe497k549sgw9trzym50tdlegq3xx4apjqynjqm:1000" --to_dns "testnet@bitdevsyde.org:5000"

Changelog notice

Add top level command resolve_dns_recipient to resolve the DNS payment instructions
Add option --to_dns to create_tx command to allow sending to a Human Readable Name (HRN)

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

vadim-anfv · 2026-02-07T20:13:21Z

@sdmg15 Hi! Could you make it compile?

coveralls · 2026-02-09T14:38:13Z

Pull Request Test Coverage Report for Build 21818410493

Details

0 of 120(0.0%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.5%) to 10.237%

Changes Missing Coverage	Changed/Added Lines	%
src/utils.rs	15	0.0%
src/handlers.rs	36	0.0%
src/dns_payment_instructions.rs	69	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	12.57%

Totals
Change from base Build 21153868360:	-0.5%
Covered Lines:	268
Relevant Lines:	2618

💛 - Coveralls

sdmg15 · 2026-02-11T04:53:07Z

@va-an Thanks for taking a look. I think it should be resolved now.

vadim-anfv

Thanks for working on this @sdmg15!
I have left a few suggestions, please have a look.

sdmg15 · 2026-02-13T09:35:00Z

@va-an Applied some reviews and recommendation on 6724be5

I've also updated the PR description.

vadim-anfv

@sdmg15 thanks for update and for the detailed description!
Please take a look at the comments.

vadim-anfv

@sdmg15 thanks for update!
I've added some suggestions, pls have a look.

vadim-anfv · 2026-03-28T21:34:18Z

@sdmg15 thanks for updates!
Could you rebase?

vadim-anfv

Thanks for addressing the previous round of feedback. Before tackling more changes - branch has merge conflicts with master, would you mind rebasing first? A few more comments below.

Payment struct design

Payment is shared between two flows with different needs.

In the send flow (process_instructions -> call site in handlers.rs) only receiving_addr is read, everything else is built and discarded. .unwrap() is needed only because the field is Option<Address> despite always being Some here.

In the resolve flow (resolve_dns_recipient) the same struct is filled with the opposite subset: receiving_addr/expected_amount always None, metadata fields populated for display. So every Option<T> here is "always Some" in one flow and "always None" in the other - the type system encodes nullability that doesn't actually exist in either path.

Suggested split:

// resolve flow - display onlypubstructResolvedPaymentInfo{pubhrn:String,pubpayment_methods:Vec<PaymentMethod>,pubdescription:Option<String>,pubmin_amount:Option<Amount>,pubmax_amount:Option<Amount>,pubnotes:String,}// `display(pretty)` moves here// send flow - return what's actually usedpubasyncfnprocess_instructions(...) -> Result<(Address,Amount),Error>

This resolves several inline comments:

unwrap on receiving_addr
the FixedAmount amount mismatch
both unwraps on human_readable_name().

One small process note: please leave review threads open after addressing them - the convention in this repo is for the reviewer to resolve them once they confirm the fix.

vadim-anfv · 2026-05-15T10:52:51Z

+ let mut methods: Vec<String> = Vec::new();
+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }
+ });


Consider map + collect here - drops the mutable accumulator and lets match return the String directly. Would need PaymentMethod::* in the use block.
Suggested change
letmut methods:Vec<String> = Vec::new();
self.payment_methods.iter().for_each(|pm| match pm {
bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
methods.push(format!("Bolt 12 invoice ({})", shorten(offer,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
methods.push(format!("On chain ({})", address))
}
bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
methods.push(format!("Cashu payment ({})", shorten(csh,20,15)))
}
});
let methods:Vec<String> = self
.payment_methods
.iter()
.map(|pm| match pm {
LightningBolt11(bolt11) => {
format!("Bolt 11 invoice ({})", shorten(bolt11,20,15))
}
LightningBolt12(offer) => format!("Bolt 12 invoice ({})", shorten(offer,20,15)),
OnChain(address) => format!("On chain ({})", address),
Cashu(csh) => format!("Cashu payment ({})", shorten(csh,20,15)),
})
.collect();

vadim-anfv · 2026-05-15T11:15:57Z

+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }


shorten runs before the if pretty check, so JSON output contains truncated Bolt 11 / Bolt 12 invoices and Cashu tokens - downstream tools can't use them.
Non-pretty output:
-> % cargo run --all-features -- -n bitcoin resolve_dns_recipient send.some@satsto.me{"description": null,"expected_amount_to_send": null,"hrn":"send.some@satsto.me","max_amount": null,"min_amount": null,"notes":"This is configurable payment instructions. You must send an amount between min_amount and max_amount if set.","payment_methods":["On chain (bc1qztwy6xen3zdtt7z0vrgapmjtfz8acjkfp5fp7l)","Bolt 12 invoice (lno1zr5qyugqgskrk70k...ph4xrxtk2xc3lpq)"]}
The shorten calls must be moved into the if pretty branch only.

vadim-anfv · 2026-05-15T11:37:45Z

+ dns_recipients: Vec<(String, u64)>,
+ #[cfg(feature = "dns_payment")]
+ /// Custom resolver DNS IP to be used for resolution.
+ #[arg(long = "dns_resolver", default_value = "8.8.8.8:53")]


The default DNS resolver value is inconsistent between the two commands:
ResolveDnsRecipient::resolver -> "8.8.8.8"
CreateTx::dns_resolver -> "8.8.8.8:53"
parse_dns_instructions already appends :53 when no port is present, so the :53 suffix in CreateTx is redundant. Drop it to keep the defaults aligned.
Also worth aligning the flag names themselves - --resolver in one command and --dns_resolver in the other forces users to remember two names for the same thing.
Same value also keeps getting renamed as it travels through the call stack. For the resolve command:
commands.rs -> resolver
handlers.rs::handle_resolve_dns_recipient_command(... resolver: String ...)
dns_payment_instructions.rs::resolve_dns_recipient(... ip: String)
dns_payment_instructions.rs::parse_dns_instructions(... resolver_ip: String)
Three different names for the same value makes the flow harder to follow. Use dns_resolver everywhere - both as the CLI flag (--dns_resolver) and as the field/parameter name across all functions.

vadim-anfv · 2026-05-15T21:27:43Z

+ .map_err(|e| Error::Generic(format!("Parsing error occured {e:#?}")))?;
+ let payment = process_instructions(amount, &instructions, resolver).await?;
+
+ recipients.push((payment.receiving_addr.unwrap().into(), amount));


Two issues here:
amount is the user-supplied value, not payment.expected_amount. For FixedAmount instructions the recipient sets an exact amount, and the tx silently goes out with whatever the user typed instead.
.unwrap() relies on process_instructions always setting receiving_addr: Some(...) - an implicit invariant across two files.
Both resolved by the structural change in the top-level comment.

vadim-anfv · 2026-05-15T21:52:03Z

@sdmg15 when addressing the comments, could you split the changes into separate commits per independent fix instead of squashing everything into one? It makes the review much easier to follow (each commit maps to one review thread), and helps with bisect/revert later if needed. They can always be squashed at merge time.

vadim-anfv

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.

Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.

@tvpeter please correct me if my assumption is wrong.

tvpeter · 2026-05-22T10:01:00Z

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.
Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.
@tvpeter please correct me if my assumption is wrong.

Yes, that's correct. The --pretty flag is been removed. But you don't have to worry about removing it at your end. Once it is merged, I will clean it out.

tvpeter

@sdmg15 thank you for working on this.

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

Thank you

tvpeter · 2026-05-22T17:43:44Z

+ for recipient in dns_recipients {
+ use crate::dns_payment_instructions::{
+ parse_dns_instructions, process_instructions,
+ };
+ let amount = Amount::from_sat(recipient.1);
+ let (resolver, instructions) = parse_dns_instructions(
+ &recipient.0,
+ cli_opts.network,
+ dns_resolver.clone(),
+ )


If it is possible, can you handle this DNS resolution concurrently? Else, print the status on the terminal so that users are aware that it is resolving the DNS.

tvpeter · 2026-05-22T17:47:28Z

+ let PaymentMethod::OnChain(addr) = instructions
+ .methods()
+ .iter()
+ .find(|ix| matches!(ix, PaymentMethod::OnChain(_)))


I think you will have to reconsider prioritizing SP code for privacy concerns.

SP is still of type OnChain it's just the address derived that will change.

sdmg15 · 2026-05-25T12:42:37Z

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Yes I think this will be great but the issue here is this will require some upstream changes to the Address or PaymentMethod type in order to be able to support the sp codes formats which I'm not sure they want to support experimental features/crates.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

I've updated the examples in the description with testnet DNS address

vadim-anfv

@sdmg15 thanks for the work on this, and for applying the previous round - the Payment split into ResolvedPaymentInfo and process_instructions -> (Address, Amount) came out clean.

Left a few inline comments below, plus one bigger-picture suggestion: unifying the recipients before the send logic. Right now --to and --to_dns are handled in separate places - DNS recipients are only resolved inside the non-send_all branch, so the building logic sees two different sources at two different points. If instead all --to_dns entries were resolved to (address, amount) and merged into the single recipients list up front - before any tx-building or send_all branching - the send path would just work off one unified list and wouldn't need to know where a recipient came from.

This would naturally fix a couple of the inline issues:

--to no longer has to be required (validate "at least one recipient" on the merged list instead),
--send_all would stop silently dropping --to_dns recipients (it currently only looks at recipients[0]).

Separating recipient parsing/resolution from the payment logic keeps the tx-building code simpler and removes these edge cases by construction.

Thanks again!

vadim-anfv

tACK 2ca719b

Tested create_tx --to_dns end-to-end on regtest (with a local patch to get past the network check). Works as expected - requesting 5000 sats and 250000 sats each produced an output of exactly that amount.

Right now just pre-push does not pass:

error: variable does not need to be mutable
--> src/handlers.rs:532:13
|
532 | mut recipients,
| ----^^^^^^^^^^
| |
| help: remove this `mut`
|
= note: `-D unused-mut` implied by `-D warnings`

Other than that everything looks great.

sdmg15 · 2026-06-23T18:04:03Z

@va-an Thanks for the reviews. I've addressed that. We should be good to go.

vadim-anfv

Thanks a lot for all the work on this @sdmg15, great job - this has come together really nicely!

Just one small thing left: Cargo.lock contains changes unrelated to this PR - a stray cargo update bumped a lot of unrelated crates, including bdk_wallet 2.1.0 -> 2.4.0. Could you regenerate the lock from master so only the new dependency's entries land?

git checkout master -- Cargo.lock
cargo build --all-features

The lock also bumps bdk_wallet to 2.4.0 - this might not pass review from @tvpeter, see #225 (review).

Thanks again!

codecov · 2026-06-28T03:13:52Z

Codecov Report

❌ Patch coverage is 0% with 259 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.97%. Comparing base (2f14a36) to head (943b4b6).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/dns_payment_instructions.rs	0.00%	145 Missing ⚠️
src/handlers.rs	0.00%	106 Missing ⚠️
src/utils.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #236 +/- ##
==========================================
- Coverage 28.87% 26.97% -1.91% 
==========================================
Files 9 10 +1 Lines 3626 3882 +256 ==========================================
Hits 1047 1047 - Misses 2579 2835 +256

Flag	Coverage Δ
rust	`26.97% <0.00%> (-1.91%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tvpeter

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method. So I think it is either you consider using another library that supports SP or provides mappings to non-reusable addresses or you handling that with the available toolset. But I don't think this PR as it is fully accomplishes the purpose of BIP353.

tvpeter · 2026-06-29T16:06:35Z

 /// Adds a recipient to the transaction.
 // Clap Doesn't support complex vector parsing https://github.com/clap-rs/clap/issues/1704.
 // Address and amount parsing is done at run time in handler function.
- #[arg(env = "ADDRESS:SAT", long = "to", required = true, value_parser = parse_recipient)]


Making this optional here removes the validation that clap offers

The validation is still done but at the command implementation level

tvpeter · 2026-06-29T16:08:56Z

+ #[cfg(feature = "dns_payment")]
+ mut recipients,
+ #[cfg(not(feature = "dns_payment"))]
 recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_resolver,


I feel this introduces too many variables to combine with the existing CreateTx, so it will be better to add a new wallet command that handles DNS payment separately.

I don't know your final take on this but I tried doing it by adding CreateDNSTx or something similar which happens to just be a duplicate of CreateTx and introduce more codes. So let me know what you think should be best approach but I think they way it's done right isn't that much harmful

sdmg15 · 2026-06-30T09:29:59Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.

The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.

But I don't think this PR as it is fully accomplishes the purpose of BIP353.

I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

tvpeter · 2026-06-30T11:14:38Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.
The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.
But I don't think this PR as it is fully accomplishes the purpose of BIP353.
I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

So the whole point of my comment is that, if you can find a way around address reuse, it will serve the purpose of the BIP better. See the BIP section on it here.

Thank you.

sdmg15 · 2026-06-30T14:50:12Z

Yeah I get the point.
I think that section is more a caution targeting users setting their DNS payment wallet.

It's not meant for the application/resolver as you don't have control over that. Only the user will responsibly go and set a different wallet address in the DNS records.

That's why BIP353 combined now with BIP352 effectively solves that as the user won't need to update the dns record any more but will get a new fresh Bitcoin address at any request.

tvpeter

utACK d279097

sdmg15 · 2026-01-07T22:18:27Z

Description

This PR adds support for BIP353 payment instructions.
The following notable changes are done:

Adding of an option resolve_dns_recipient which receives a Human Readable Name (HRN) and returns the associated address
Modify the create_tx for it to support user specifying recipients as HRN.

Notes to the reviewers

Here is a list of HRN that could be used to test the implementation:

pay@dunxen.dev: Supports BOLT 12 Payment method only (CAUTION: mainnet)
send.some@satsto.me: Supports BOLT 12 payment and On Chain (CAUTION: mainnet)
Testing resolving: bdk-cli -n testnet resolve_dns_recipient testnet@bitdevsyde.org
Testing create tx: bdk-cli -n testnet wallet -w regtest_default_wallet create_tx --to "bcrt1qe497k549sgw9trzym50tdlegq3xx4apjqynjqm:1000" --to_dns "testnet@bitdevsyde.org:5000"

Changelog notice

Add top level command resolve_dns_recipient to resolve the DNS payment instructions
Add option --to_dns to create_tx command to allow sending to a Human Readable Name (HRN)

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

vadim-anfv · 2026-02-07T20:13:21Z

@sdmg15 Hi! Could you make it compile?

coveralls · 2026-02-09T14:38:13Z

Pull Request Test Coverage Report for Build 21818410493

Details

0 of 120(0.0%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.5%) to 10.237%

Changes Missing Coverage	Changed/Added Lines	%
src/utils.rs	15	0.0%
src/handlers.rs	36	0.0%
src/dns_payment_instructions.rs	69	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	12.57%

Totals
Change from base Build 21153868360:	-0.5%
Covered Lines:	268
Relevant Lines:	2618

💛 - Coveralls

sdmg15 · 2026-02-11T04:53:07Z

@va-an Thanks for taking a look. I think it should be resolved now.

vadim-anfv

Thanks for working on this @sdmg15!
I have left a few suggestions, please have a look.

sdmg15 · 2026-02-13T09:35:00Z

@va-an Applied some reviews and recommendation on 6724be5

I've also updated the PR description.

vadim-anfv

@sdmg15 thanks for update and for the detailed description!
Please take a look at the comments.

vadim-anfv

@sdmg15 thanks for update!
I've added some suggestions, pls have a look.

vadim-anfv · 2026-03-28T21:34:18Z

@sdmg15 thanks for updates!
Could you rebase?

vadim-anfv

Thanks for addressing the previous round of feedback. Before tackling more changes - branch has merge conflicts with master, would you mind rebasing first? A few more comments below.

Payment struct design

Payment is shared between two flows with different needs.

In the send flow (process_instructions -> call site in handlers.rs) only receiving_addr is read, everything else is built and discarded. .unwrap() is needed only because the field is Option<Address> despite always being Some here.

In the resolve flow (resolve_dns_recipient) the same struct is filled with the opposite subset: receiving_addr/expected_amount always None, metadata fields populated for display. So every Option<T> here is "always Some" in one flow and "always None" in the other - the type system encodes nullability that doesn't actually exist in either path.

Suggested split:

// resolve flow - display onlypubstructResolvedPaymentInfo{pubhrn:String,pubpayment_methods:Vec<PaymentMethod>,pubdescription:Option<String>,pubmin_amount:Option<Amount>,pubmax_amount:Option<Amount>,pubnotes:String,}// `display(pretty)` moves here// send flow - return what's actually usedpubasyncfnprocess_instructions(...) -> Result<(Address,Amount),Error>

This resolves several inline comments:

unwrap on receiving_addr
the FixedAmount amount mismatch
both unwraps on human_readable_name().

One small process note: please leave review threads open after addressing them - the convention in this repo is for the reviewer to resolve them once they confirm the fix.

vadim-anfv · 2026-05-15T10:52:51Z

+ let mut methods: Vec<String> = Vec::new();
+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }
+ });


Consider map + collect here - drops the mutable accumulator and lets match return the String directly. Would need PaymentMethod::* in the use block.
Suggested change
letmut methods:Vec<String> = Vec::new();
self.payment_methods.iter().for_each(|pm| match pm {
bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
methods.push(format!("Bolt 12 invoice ({})", shorten(offer,20,15)))
}
bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
methods.push(format!("On chain ({})", address))
}
bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
methods.push(format!("Cashu payment ({})", shorten(csh,20,15)))
}
});
let methods:Vec<String> = self
.payment_methods
.iter()
.map(|pm| match pm {
LightningBolt11(bolt11) => {
format!("Bolt 11 invoice ({})", shorten(bolt11,20,15))
}
LightningBolt12(offer) => format!("Bolt 12 invoice ({})", shorten(offer,20,15)),
OnChain(address) => format!("On chain ({})", address),
Cashu(csh) => format!("Cashu payment ({})", shorten(csh,20,15)),
})
.collect();

vadim-anfv · 2026-05-15T11:15:57Z

+ self.payment_methods.iter().for_each(|pm| match pm {
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt11(bolt11) => {
+ methods.push(format!("Bolt 11 invoice ({})", shorten(bolt11, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::LightningBolt12(offer) => {
+ methods.push(format!("Bolt 12 invoice ({})", shorten(offer, 20, 15)))
+ }
+ bitcoin_payment_instructions::PaymentMethod::OnChain(address) => {
+ methods.push(format!("On chain ({})", address))
+ }
+ bitcoin_payment_instructions::PaymentMethod::Cashu(csh) => {
+ methods.push(format!("Cashu payment ({})", shorten(csh, 20, 15)))
+ }


shorten runs before the if pretty check, so JSON output contains truncated Bolt 11 / Bolt 12 invoices and Cashu tokens - downstream tools can't use them.
Non-pretty output:
-> % cargo run --all-features -- -n bitcoin resolve_dns_recipient send.some@satsto.me{"description": null,"expected_amount_to_send": null,"hrn":"send.some@satsto.me","max_amount": null,"min_amount": null,"notes":"This is configurable payment instructions. You must send an amount between min_amount and max_amount if set.","payment_methods":["On chain (bc1qztwy6xen3zdtt7z0vrgapmjtfz8acjkfp5fp7l)","Bolt 12 invoice (lno1zr5qyugqgskrk70k...ph4xrxtk2xc3lpq)"]}
The shorten calls must be moved into the if pretty branch only.

vadim-anfv · 2026-05-15T11:37:45Z

+ dns_recipients: Vec<(String, u64)>,
+ #[cfg(feature = "dns_payment")]
+ /// Custom resolver DNS IP to be used for resolution.
+ #[arg(long = "dns_resolver", default_value = "8.8.8.8:53")]


The default DNS resolver value is inconsistent between the two commands:
ResolveDnsRecipient::resolver -> "8.8.8.8"
CreateTx::dns_resolver -> "8.8.8.8:53"
parse_dns_instructions already appends :53 when no port is present, so the :53 suffix in CreateTx is redundant. Drop it to keep the defaults aligned.
Also worth aligning the flag names themselves - --resolver in one command and --dns_resolver in the other forces users to remember two names for the same thing.
Same value also keeps getting renamed as it travels through the call stack. For the resolve command:
commands.rs -> resolver
handlers.rs::handle_resolve_dns_recipient_command(... resolver: String ...)
dns_payment_instructions.rs::resolve_dns_recipient(... ip: String)
dns_payment_instructions.rs::parse_dns_instructions(... resolver_ip: String)
Three different names for the same value makes the flow harder to follow. Use dns_resolver everywhere - both as the CLI flag (--dns_resolver) and as the field/parameter name across all functions.

vadim-anfv · 2026-05-15T21:27:43Z

+ .map_err(|e| Error::Generic(format!("Parsing error occured {e:#?}")))?;
+ let payment = process_instructions(amount, &instructions, resolver).await?;
+
+ recipients.push((payment.receiving_addr.unwrap().into(), amount));


Two issues here:
amount is the user-supplied value, not payment.expected_amount. For FixedAmount instructions the recipient sets an exact amount, and the tx silently goes out with whatever the user typed instead.
.unwrap() relies on process_instructions always setting receiving_addr: Some(...) - an implicit invariant across two files.
Both resolved by the structural change in the top-level comment.

vadim-anfv · 2026-05-15T21:52:03Z

@sdmg15 when addressing the comments, could you split the changes into separate commits per independent fix instead of squashing everything into one? It makes the review much easier to follow (each commit maps to one review thread), and helps with bisect/revert later if needed. They can always be squashed at merge time.

vadim-anfv

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.

Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.

@tvpeter please correct me if my assumption is wrong.

tvpeter · 2026-05-22T10:01:00Z

I just found out that we're definitely dropping the pretty flag in #278, so please remove all the code related to this flag. This will also address some of my review comments.
Upd: although removing pretty output for this feature may create inconsistency with the current codebase, I expect we won't have any releases before #278 is merged, so this shouldn't become an issue for users.
@tvpeter please correct me if my assumption is wrong.

Yes, that's correct. The --pretty flag is been removed. But you don't have to worry about removing it at your end. Once it is merged, I will clean it out.

tvpeter

@sdmg15 thank you for working on this.

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

Thank you

tvpeter · 2026-05-22T17:43:44Z

+ for recipient in dns_recipients {
+ use crate::dns_payment_instructions::{
+ parse_dns_instructions, process_instructions,
+ };
+ let amount = Amount::from_sat(recipient.1);
+ let (resolver, instructions) = parse_dns_instructions(
+ &recipient.0,
+ cli_opts.network,
+ dns_resolver.clone(),
+ )


If it is possible, can you handle this DNS resolution concurrently? Else, print the status on the terminal so that users are aware that it is resolving the DNS.

tvpeter · 2026-05-22T17:47:28Z

+ let PaymentMethod::OnChain(addr) = instructions
+ .methods()
+ .iter()
+ .find(|ix| matches!(ix, PaymentMethod::OnChain(_)))


I think you will have to reconsider prioritizing SP code for privacy concerns.

SP is still of type OnChain it's just the address derived that will change.

sdmg15 · 2026-05-25T12:42:37Z

The comment I left regarding prioritizing silent payment code, I understand that this PR was worked on at a time when the silent payment feature was not yet merged, so you may have prioritized on-chain addresses. But since SP has been merged, and BIP353 (I believe) is designed to be paired with Silent Payments to prevent privacy leaks through permanent address reuse. So you might have to reconsider that implementation aspect.

Yes I think this will be great but the issue here is this will require some upstream changes to the Address or PaymentMethod type in order to be able to support the sp codes formats which I'm not sure they want to support experimental features/crates.

Also, this feature has indicated using mainnet, and we have heavily discouraged users from using this app on mainnet. So you may also consider sharing another functional test network that users can use to play with.

I've updated the examples in the description with testnet DNS address

vadim-anfv

@sdmg15 thanks for the work on this, and for applying the previous round - the Payment split into ResolvedPaymentInfo and process_instructions -> (Address, Amount) came out clean.

Left a few inline comments below, plus one bigger-picture suggestion: unifying the recipients before the send logic. Right now --to and --to_dns are handled in separate places - DNS recipients are only resolved inside the non-send_all branch, so the building logic sees two different sources at two different points. If instead all --to_dns entries were resolved to (address, amount) and merged into the single recipients list up front - before any tx-building or send_all branching - the send path would just work off one unified list and wouldn't need to know where a recipient came from.

This would naturally fix a couple of the inline issues:

--to no longer has to be required (validate "at least one recipient" on the merged list instead),
--send_all would stop silently dropping --to_dns recipients (it currently only looks at recipients[0]).

Separating recipient parsing/resolution from the payment logic keeps the tx-building code simpler and removes these edge cases by construction.

Thanks again!

vadim-anfv

tACK 2ca719b

Tested create_tx --to_dns end-to-end on regtest (with a local patch to get past the network check). Works as expected - requesting 5000 sats and 250000 sats each produced an output of exactly that amount.

Right now just pre-push does not pass:

error: variable does not need to be mutable
--> src/handlers.rs:532:13
|
532 | mut recipients,
| ----^^^^^^^^^^
| |
| help: remove this `mut`
|
= note: `-D unused-mut` implied by `-D warnings`

Other than that everything looks great.

sdmg15 · 2026-06-23T18:04:03Z

@va-an Thanks for the reviews. I've addressed that. We should be good to go.

vadim-anfv

Thanks a lot for all the work on this @sdmg15, great job - this has come together really nicely!

Just one small thing left: Cargo.lock contains changes unrelated to this PR - a stray cargo update bumped a lot of unrelated crates, including bdk_wallet 2.1.0 -> 2.4.0. Could you regenerate the lock from master so only the new dependency's entries land?

git checkout master -- Cargo.lock
cargo build --all-features

The lock also bumps bdk_wallet to 2.4.0 - this might not pass review from @tvpeter, see #225 (review).

Thanks again!

codecov · 2026-06-28T03:13:52Z

Codecov Report

❌ Patch coverage is 0% with 259 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.97%. Comparing base (2f14a36) to head (943b4b6).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/dns_payment_instructions.rs	0.00%	145 Missing ⚠️
src/handlers.rs	0.00%	106 Missing ⚠️
src/utils.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #236 +/- ##
==========================================
- Coverage 28.87% 26.97% -1.91% 
==========================================
Files 9 10 +1 Lines 3626 3882 +256 ==========================================
Hits 1047 1047 - Misses 2579 2835 +256

Flag	Coverage Δ
rust	`26.97% <0.00%> (-1.91%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tvpeter

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method. So I think it is either you consider using another library that supports SP or provides mappings to non-reusable addresses or you handling that with the available toolset. But I don't think this PR as it is fully accomplishes the purpose of BIP353.

tvpeter · 2026-06-29T16:06:35Z

 /// Adds a recipient to the transaction.
 // Clap Doesn't support complex vector parsing https://github.com/clap-rs/clap/issues/1704.
 // Address and amount parsing is done at run time in handler function.
- #[arg(env = "ADDRESS:SAT", long = "to", required = true, value_parser = parse_recipient)]


Making this optional here removes the validation that clap offers

The validation is still done but at the command implementation level

tvpeter · 2026-06-29T16:08:56Z

+ #[cfg(feature = "dns_payment")]
+ mut recipients,
+ #[cfg(not(feature = "dns_payment"))]
 recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_recipients,
+ #[cfg(feature = "dns_payment")]
+ dns_resolver,


I feel this introduces too many variables to combine with the existing CreateTx, so it will be better to add a new wallet command that handles DNS payment separately.

I don't know your final take on this but I tried doing it by adding CreateDNSTx or something similar which happens to just be a duplicate of CreateTx and introduce more codes. So let me know what you think should be best approach but I think they way it's done right isn't that much harmful

sdmg15 · 2026-06-30T09:29:59Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.

The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.

But I don't think this PR as it is fully accomplishes the purpose of BIP353.

I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

tvpeter · 2026-06-30T11:14:38Z

Following up on on my previous review comment about prioritizing SP and your response about the library, I have checked the library and confirmed that it does not include SP payment method. This is understandable as the author is lightning first and may not consider adding SP payment method.
The library isn't "lightning first". You can check this PR rust-bitcoin/bitcoin-payment-instructions#19, there has been previous effort into adding that to the library but given that some upstream components aren't available they decided to postpone it.
But I don't think this PR as it is fully accomplishes the purpose of BIP353.
I think you're confusing the purpose of BIP353 and the one of BIP352. BIP353 for dns goal has never been about avoiding address reuse. BIP352 just came and gave more sense to that proposal as combined together you have Human Readable Name that resolves to different addresses anytime when requested.

So the whole point of my comment is that, if you can find a way around address reuse, it will serve the purpose of the BIP better. See the BIP section on it here.

Thank you.

sdmg15 · 2026-06-30T14:50:12Z

Yeah I get the point.
I think that section is more a caution targeting users setting their DNS payment wallet.

It's not meant for the application/resolver as you don't have control over that. Only the user will responsibly go and set a different wallet address in the DNS records.

That's why BIP353 combined now with BIP352 effectively solves that as the user won't need to update the dns record any more but will get a new fresh Bitcoin address at any request.

tvpeter

utACK d279097

github-project-automationBot added this to BDK-CLI Jan 7, 2026

sdmg15 force-pushed the feat/bip353-payment-instructions branch from ba3c713 to a0f3e9dCompare January 8, 2026 18:13

sdmg15 marked this pull request as ready for review January 9, 2026 08:39

sdmg15 force-pushed the feat/bip353-payment-instructions branch 2 times, most recently from 49154f3 to baf349bCompare February 9, 2026 08:51

sdmg15 force-pushed the feat/bip353-payment-instructions branch from baf349b to 0e9756eCompare February 11, 2026 04:52

vadim-anfv suggested changes Feb 11, 2026
View reviewed changes

sdmg15 force-pushed the feat/bip353-payment-instructions branch from 0e9756e to 6724be5Compare February 13, 2026 09:21

sdmg15 force-pushed the feat/bip353-payment-instructions branch from 6724be5 to 20ef094Compare February 13, 2026 11:46

vadim-anfv reviewed Feb 13, 2026
View reviewed changes

Comment threadsrc/dns_payment_instructions.rs Outdated
Comment threadsrc/handlers.rs Outdated
Comment threadsrc/handlers.rs Outdated
Comment threadsrc/handlers.rs Outdated
Comment threadsrc/handlers.rs Outdated

sdmg15 force-pushed the feat/bip353-payment-instructions branch 2 times, most recently from 977534a to 4852673Compare February 17, 2026 20:12

vadim-anfv mentioned this pull request Feb 21, 2026
Human Readable Names Support bitcoindevkit/bdk_wallet#97
Open

vadim-anfv reviewed Feb 21, 2026
View reviewed changes

Comment threadsrc/utils.rs Outdated
Comment threadsrc/dns_payment_instructions.rs Outdated
Comment threadsrc/commands.rs Outdated
Comment threadsrc/dns_payment_instructions.rs Outdated

tvpeter moved this to In Progress in BDK-CLI Feb 23, 2026

sdmg15 force-pushed the feat/bip353-payment-instructions branch from 4852673 to 58798deCompare March 16, 2026 12:44

sdmg15 force-pushed the feat/bip353-payment-instructions branch from 58798de to 72656ecCompare March 31, 2026 08:46

vadim-anfv suggested changes May 15, 2026
View reviewed changes

tvpeter assigned sdmg15 May 16, 2026

vadim-anfv suggested changes May 21, 2026
View reviewed changes

tvpeter added this to the CLI 4.0.0 milestone May 22, 2026

tvpeter requested changes May 22, 2026
View reviewed changes

sdmg15 force-pushed the feat/bip353-payment-instructions branch from 72656ec to 5b8d1b1Compare May 26, 2026 11:14

vadim-anfv suggested changes Jun 16, 2026
View reviewed changes

Comment threadsrc/commands.rs Outdated
Comment threadsrc/dns_payment_instructions.rs Outdated
Comment threadsrc/handlers.rs Outdated
Comment threadsrc/dns_payment_instructions.rs Outdated

sdmg15 force-pushed the feat/bip353-payment-instructions branch from b2ec8d9 to 2ca719bCompare June 17, 2026 17:15

vadim-anfv suggested changes Jun 23, 2026
View reviewed changes

sdmg15 force-pushed the feat/bip353-payment-instructions branch from 2ca719b to f56f72cCompare June 23, 2026 18:01

sdmg15 requested a review from tvpeter June 23, 2026 18:04

vadim-anfv suggested changes Jun 24, 2026
View reviewed changes

sdmg15 force-pushed the feat/bip353-payment-instructions branch from f56f72c to f3f78f3Compare June 28, 2026 13:25

tvpeter moved this from In Progress to Ready to Review in BDK-CLI Jun 28, 2026

sdmg15 force-pushed the feat/bip353-payment-instructions branch from f3f78f3 to 0680744Compare June 29, 2026 06:56

tvpeter requested changes Jun 29, 2026
View reviewed changes

sdmg15 force-pushed the feat/bip353-payment-instructions branch from 0680744 to 5c496b1Compare July 2, 2026 05:28

tvpeter approved these changes Jul 8, 2026
View reviewed changes

sdmg15 added 5 commits July 8, 2026 09:13

feat: integrate BIP 353 dns payment instructions
7a2c5df

discard pretty flag and apply reviews
e3c8efc

fix: add log when resolving
6ea1b21

fix: unifying recipients before send
550918c

fix: separate CreateTx from CreateDnsTx
943b4b6

tvpeter force-pushed the feat/bip353-payment-instructions branch from cb5424d to 943b4b6Compare July 8, 2026 08:19

tvpeter merged commit d8e1801 into bitcoindevkit:masterJul 8, 2026
7 of 9 checks passed

github-project-automationBot moved this from Ready to Review to Done in BDK-CLI Jul 8, 2026

tvpeter mentioned this pull request Jul 20, 2026
Release 4.0.0 #304
Closed
14 tasks