Skip to content

fix: accept tsp recipients on Signet and Testnet4 - #70

Merged
nymius merged 2 commits into
bitcoindevkit:masterfrom
kkdao:fix/tsp-network-group-compat
Sep 1, 2026
Merged

fix: accept tsp recipients on Signet and Testnet4#70
nymius merged 2 commits into
bitcoindevkit:masterfrom
kkdao:fix/tsp-network-group-compat

Conversation

@kkdao

@kkdao kkdao commented Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Fixes #68

Every test network encodes to the same tsp prefix, so parsing always returns Network::Testnet. sp-cli2 new-tx --to-sp compared that to the wallet's network with !=, so Signet and Testnet4 wallets rejected valid codes, including their own.

Adds SilentPaymentCode::is_valid_for_network, which compares prefixes instead, and uses it in sp-cli2. The empty bail!("") next to it gets a message.

Notes to the reviewers

Encoding is unchanged — this only changes how compatibility is judged. prefix_assignment_is_unchanged pins that.

The prefix match already in Display::fmt is lifted into hrp_for_network and shared, so the two can't drift.

Groups follow this crate's encoder, not bitcoin::NetworkKind, which groups regtest with the other test networks while this crate gives regtest its own sprt.

Tests are in silentpayments rather than next to the CLI because cli/v2 isn't in default-members and just check/just test exclude it, so a test there would never run. I built and linted cli/v2 directly to check this change.

Changelog notice

Added: SilentPaymentCode::is_valid_for_network and encoding::hrp_for_network.

Fixed: sp-cli2 new-tx --to-sp rejected valid tsp recipients on Signet and Testnet4.

Checklists

All Submissions:

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@kkdao
kkdao force-pushed the fix/tsp-network-group-compat branch from 01408a9 to ea4cd0c Compare August 21, 2026 21:36
@kkdao
kkdao marked this pull request as draft August 21, 2026 21:48
@kkdao
kkdao marked this pull request as ready for review August 21, 2026 21:56
nymius
nymius requested changes Aug 25, 2026
View reviewed changes

@nymius nymius left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing this project. The issue is legit.

Comment thread silentpayments/src/encoding/mod.rs Outdated
@kkdao
kkdao force-pushed the fix/tsp-network-group-compat branch from bb6d7c1 to 754c920 Compare August 26, 2026 03:50
nymius
nymius requested changes Aug 26, 2026
View reviewed changes

@nymius nymius left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squash 754c920 into 5d7192e please.

@kkdao
kkdao force-pushed the fix/tsp-network-group-compat branch from 754c920 to e33251f Compare August 26, 2026 16:53
@kkdao

kkdao commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Squashed, thanks. 754c920 is folded into 5d7192e, so the branch is two commits now. The resulting tree is identical to before the rebase.

@kkdao

kkdao commented Aug 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Heads up on the red Incremental mutants test lane: it does not come from this PR. It fails at the unmutated baseline build, not on any mutant, and the same failure reproduces on a clean master worktree, where cargo check -p bdk_sp --tests dies with E0432 on serde. The last mutations.yml run on master (783c63c) also concluded failure.

The cause looks like arg placement: mutations.yml passes --all-features after --, so it reaches the test phase rather than the baseline build. --cargo-arg=--all-features would cover the build too. That is CI config on master rather than anything on this branch, so I left it alone. Happy to open a separate PR for it if you want.

Update: fixed by #74, which makes cargo mutants build the baseline with the features it needs. That PR is green on this same lane. Once it merges I will rebase this branch and the check should pass here too.

@kkdao kkdao mentioned this pull request Aug 26, 2026
Merged
nymius
nymius requested changes Aug 31, 2026
View reviewed changes

@nymius nymius left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, sign the commits like in #71 .

@kkdao
kkdao force-pushed the fix/tsp-network-group-compat branch from e33251f to e8e1b08 Compare August 31, 2026 21:33
kkdao added a commit to kkdao/bdk-sp that referenced this pull request Aug 31, 2026
@kkdao
ci(mutants): build the baseline with --all-features
14e6cbe 
The incremental mutants lane has been failing at the unmutated baseline, so no
mutants are ever tested. Args after `--` are handed to the test phase only, so
the baseline build ran with default features. `bdk_sp` gates serde behind an
optional feature while its tests derive Deserialize, which leaves the test
targets uncompilable without it and fails the build before mutation starts.

Pass the feature through `--cargo-arg` instead, which applies to every cargo
invocation including the build. It also has to come out of the trailing args:
`--cargo-arg` already adds it to the test command, and cargo rejects
`--all-features` given twice.

Verified against the diff from bitcoindevkit#70, which previously died at the baseline:
7 mutants found, 6 caught, 1 unviable, none missed.
@kkdao
kkdao requested a review from nymius August 31, 2026 21:40
nymius added a commit that referenced this pull request Sep 1, 2026
@nymius
Merge #74: ci(mutants): build the baseline with --all-features
de7c1ee 
14e6cbe ci(mutants): build the baseline with --all-features (kkdao)

Pull request description:

  The `Incremental mutants test` lane is currently dead: it fails at the unmutated baseline on every PR, so no mutants are ever tested. The last run on master, 783c63c, is red for the same reason.

  Args after `--` are handed to the test phase only, so the baseline build runs with default features. `bdk_sp` gates `serde` behind an optional feature while its test scaffolding derives `Deserialize`, so the test targets do not compile without it and the build fails before mutation starts. Reproduces on a clean master worktree with `cargo check -p bdk_sp --tests`.

  Passing the feature through `--cargo-arg` fixes it, since that applies to every cargo invocation including the build. It also has to come out of the trailing args: `--cargo-arg` already adds it to the test command, and cargo rejects `--all-features` twice.

  Verified locally against the diff from #70, which previously died at the baseline:

  ```
  7 mutants tested in 83s: 6 caught, 1 unviable
  ```

  Unrelated, noted while here and left alone: this workflow has `on: push: branches: [main]` while the default branch is `master`, so that trigger never fires. The job is gated on `github.event_name == 'pull_request' anyway, so it changes nothing today.

ACKs for top commit:
  nymius:
    ACK 14e6cbe

Tree-SHA512: f72ac39a6fc09b426c5f2eab96388cec0c32345d8ca8162a652ccb5f8e19888cde61e02dfedcb9fb5c93d94da9cb6d1737c541ac58617aee0f5fa4021b212f81
nymius
nymius requested changes Sep 1, 2026
View reviewed changes

@nymius nymius left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs rebase.

kkdao added 2 commits September 1, 2026 08:11
@kkdao
feat(encoding): add SilentPaymentCode::is_valid_for_network
b2dc834 
BIP 352 assigns one human readable prefix to every test network, so codes
written for Testnet, Testnet4 and Signet are indistinguishable once encoded and
parsing reports `Network::Testnet` for all three. Callers that check a parsed
code against a wallet with `code.network == wallet.network()` consequently
reject valid recipients on Signet and Testnet4.

What the encoding carries is the prefix, not the exact chain, so expose a
predicate that compares prefixes and leave the encoding itself untouched.
`Display` now shares that mapping through `hrp_for_network` instead of repeating
the match, so the two cannot drift apart.

The groups follow this crate's encoder rather than `bitcoin::NetworkKind`:
regtest has its own `sprt` prefix here, while `NetworkKind` groups it with the
other test networks.
@kkdao
fix(cli): accept tsp recipients on Signet and Testnet4
bca79c5 
`new-tx --to-sp` compared the parsed code's network to the wallet's with `!=`.
Because every test network encodes to the same `tsp` prefix and parsing reports
`Network::Testnet` for all of them, a Signet or Testnet4 wallet rejected codes
that were written for it, including its own.

Compare prefixes through `is_valid_for_network` instead, which is the
distinction the encoding carries. The bail also gains a message; it was empty,
so the rejection surfaced as an error with no text.
@kkdao
kkdao force-pushed the fix/tsp-network-group-compat branch from e8e1b08 to bca79c5 Compare September 1, 2026 12:14
nymius
nymius approved these changes Sep 1, 2026
View reviewed changes

@nymius nymius left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK bca79c5

@nymius
nymius merged commit e3c3207 into bitcoindevkit:master Sep 1, 2026
5 checks passed
@kkdao
kkdao deleted the fix/tsp-network-group-compat branch September 2, 2026 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nymius nymius nymius approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

sp-cli2 --to-sp rejects tsp recipients on Signet and Testnet4

2 participants

@kkdao @nymius