#35301 Silent Payments: Implement bip352 (take 2)
https://github.com/bitcoin/bitcoin/pull/35301 · · +6946/-0 in 10 files, 4 commits · labels: none
Goal
- Implement BIP 352 silent payments logic to improve receiver privacy without address reuse
- Establish shared protocol primitives needed before the wallet can send or receive silent payments
This pull request implements the BIP352 Silent Payments protocol logic in a dedicated common library module (`common/bip352.{h,cpp}`). It wraps libsecp256k1's silentpayments module, providing destination decoding, output generation, prevout summary calculation, and scanning routines for silent payment recipients. BIP352 official test vectors are added as unit tests.
Problem: Bitcoin Core lacks support for BIP352 silent payments, preventing users from receiving reusable private payments without address reuse. Integrating silent payments requires a well-isolated cryptographic foundation that can be used independently across wallet, RPC, and future indexing infrastructure.
Category: Utilities (logging, arguments, libraries) (#3 of 66)
P2 · new feature
- P2 because it supplies the foundational protocol primitives for silent payments in Bitcoin Core
- High leverage work that unblocks subsequent wallet sending, receiving, and descriptor tracking PRs
P2 because it implements the core cryptographic protocol layer for BIP352 Silent Payments in src/common. This is the prerequisite foundation that unblocks downstream wallet sending and receiving PRs as well as indexing tools tracked in issue #28536.
Membership: Adds common/bip352.cpp, common/bip352.h, and updates src/key.h to wrap the libsecp256k1 silent payments module.
Factors: security/stability 1, bug 0, performance 1, user value 2, leverage 3
Reviewability: Ready
- Ready for review
- All previous feedback is addressed and tests pass with scope narrowed to standalone protocol logic
All reviewer feedback has been addressed, and CI is clean following the recent scope reduction.
Author status: active, recently pushed a scoped-down version addressing reviewer feedback
Resolved concerns:
- w0xlt identified several protocol edge cases: preserving output vout indexes when mixed with regular outputs, skipping transactions spending unknown witness versions, enforcing Bech32m checksums, and handling invalid x-only pubkeys without out-of-bounds access.
- theStack pointed out missing preconditions and potential crashes when parsing consensus-invalid transaction inputs or adversarially crafted keys.
- josibake and rustaceanrob recommended decoupling the PR from CTxDestination and narrowing scope strictly to the BIP352 module and test vectors, which the author implemented.
Agreement: Strong
- Strong consensus on architecture and scope from multiple domain experts
- Concept approval from multiple contributors (w0xlt, rkrux, josibake)
- Extensive line-by-line review across multiple revisions (theStack)
- Agreed to narrow scope to standalone protocol primitives instead of touching wallet types (rustaceanrob, josibake)
Strong consensus on introducing the BIP352 module; all technical objections resolved and scope cleanly reduced.
Multiple maintainers and contributors support integrating the BIP352 module. All initial review concerns regarding correctness and scope have been addressed.
- 2026-05-19: w0xlt: 'Concept ACK'
- 2026-05-19: rkrux: 'Concept ACK'
- 2026-08-13: josibake: 'Concept ACK'
- 2026-09-09: rustaceanrob: 'I think the current API is very solid and the next stage should determine how it is integrated.'
- 2026-09-16: Eunovo: 'I have reduced the scope of this PR. It no longer adds the SilentPayments destination to CTxDestination...'
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| w0xlt | correctness | Output vout positions were re-indexed to 0..n-1 rather than preserving requested recipient positions | resolved | no | yes | 2026-05-28: 'If SP outputs are mixed with regular outputs, callers would assign them to the wrong positions; the original map keys should be preserved.' Settled: 2026-05-29: Eunovo pushed a fix preserving map keys. |
| rustaceanrob | scope | Coupling the PR to CTxDestination created unnecessary churn in fuzz, unit, and functional tests before wallet decisions were settled | resolved | no | yes | 2026-09-09: 'I am wondering if the scope here can be reduced to introducing the module and passing the BIP test vectors so we can determine choices like is this a CTxDestination? during the send PR.' Settled: 2026-09-16: Eunovo pushed an update removing CTxDestination integration and reducing scope. |
| w0xlt | correctness | Output destinations would be assigned to contiguous indexes rather than preserving requested tx.vout positions when mixed with regular outputs | resolved | no | yes | 2026-05-27: "If SP outputs are mixed with regular outputs, callers would assign them to the wrong positions; the original map keys should be preserved." Settled: 2026-05-29: Eunovo replied "Done." after pushing a fix preserving positions. |
| w0xlt | correctness | Calling CreateOutputs with empty input key sets triggered libsecp illegal-argument abort | resolved | no | yes | 2026-08-11: "CreateOutputs can return an empty output vector when both input-key sets are empty, before calling secp... secp's illegal-argument callback can abort the process." Settled: 2026-08-12: Eunovo replied "Done." and pushed the guard check. |
| theStack | safety | Adversarially crafted spend keys could lead to point-at-infinity failures triggering assert crashes in recipient output pubkey creation | resolved | no | yes | 2026-08-12: "I think we should check the return value here rather than assert to avoid a potential crash... worthwhile to avoid crashes that could be provoked by e.g. 'example RPCs' or 'example wallets'" Settled: 2026-08-15: Eunovo replied "Fixed." |
Support:
- w0xlt: Expressed Concept ACK and contributed detailed review and test cases.
- rkrux: Expressed Concept ACK. [not substantive]
- josibake: Concept ACK, noted the API design is clean and simpler with reduced footguns.
- rustaceanrob: Endorsed the revised API design and recommended the minimal scope approach.
- theStack: Expressed support for the API design and conducted comprehensive code reviews across all commits.
Participants: theStack (objection), w0xlt (objection), rkrux (support), sedited (neutral), josibake (objection), rustaceanrob (objection)
State derived from the lists: substantive support, no open objection (w0xlt, josibake, rustaceanrob, theStack)
Review verdicts (DrahtBot): 0
Dependencies
Files
6118 lines under test/bench/ci.
- src/test/data/bip352_send_and_receive_vectors.json +5729/-0
- src/common/bip352.cpp +504/-0
- src/test/bip352_tests.cpp +387/-0
- src/common/bip352.h +308/-0
- src/key.h +7/-0
- src/kernel/chainparams.cpp +5/-0
- src/kernel/chainparams.h +2/-0
- src/test/CMakeLists.txt +2/-0
- src/CMakeLists.txt +1/-0
- src/bech32.h +1/-0
Card
This PR implements the core BIP352 Silent Payments protocol logic and secp256k1 wrapper in src/common/bip352. It provides address decoding, output generation, prevout summary extraction, and scanning functions tested against official BIP test vectors. It resolves the problem of isolating complex cryptographic silent payments logic from wallet mechanics, unblocking the silent payments milestone tracked in issue #28536. Review consensus is strong, and previous scope concerns regarding CTxDestination integration were resolved in a recent update.