#35302 Silent Payments: Sending (take 2)
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35302 Silent Payments: Sending (take 2) | Ready | 0 | Blocked | 1,096 + 1,604 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Adds wallet and RPC support for sending transactions to BIP 352 Silent Payment addresses. It introduces a PaymentDestination abstraction to accommodate dynamic, input-dependent destinations, adjusts coin selection to require derivation-eligible inputs whose private keys are accessible, derives taproot output scripts during transaction creation, and persists silent payment recipient metadata to allow RBF fee bumping.
Problem: Bitcoin Core cannot fund, construct, or fee-bump transactions paying to BIP 352 silent payment addresses. Users wanting receiver privacy and address reuse prevention cannot pay silent payment recipients using Bitcoin Core wallet RPCs or the GUI.
Evidence of importance:
- Tracked under the silent payments project tracking issue #28536 and serves as the second iteration of PR #28201.
- PR description highlights that 'recipient silent payments destinations are now persisted to the wallet to make it possible to RBF' and restricts unsafe operations such as creating PSBTs or sending unadded transactions with silent payment destinations.
Scope: Touches core wallet spending routines, coin selection, wallet database records, fee bumping, and RPC endpoints (send, sendtoaddress, sendmany, sendall, validateaddress). Also includes a standalone BIP 352 scanner in the Python functional test framework.
Risk of a mistake: high (wallet, wallet_storage, rpc_api). Defects in output key derivation, input private key summation, or recipient persistence during fee bumps could create unspendable outputs or permanently burn user funds.
Changed since the previous assessment: Replaced the node-level scantxforsilentpayments RPC with a BIP 352 scanning utility inside the Python test framework, refactored CreateRateBumpTransaction to return util::Expected, and made ConstructTransaction output parsing optional.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- author: address Yudis-bit's objection regarding anchor filtering with m_subtract_fee_outputs 2026-11-01
Agreement
Blocked
objection that needs a change, nobody has replied (Yudis-bit)
Thread notes: Added Yudis-bit's objection regarding anchor filtering with m_subtract_fee_outputs in spend.cpp.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| theStack (member) | objection, maintenance | the condition would require manual updates whenever TxoutType is extended, risking silent omissions | fixed · fix pushed · author implemented explicit script type check | answered | yes | 2026-05-23: 'I think it would be more robust to express this condition using the explicit list of relevant output script types as specified in the BIP' Status from: 2026-06-07: Eunovo: 'Done.' |
| theStack (member) | question, maintenance | whether the fake-use line with a single underscore identifier is needed | fixed · fix pushed · author removed the fake-use line | answered | yes | 2026-05-23: 'nit: is this line needed?' Status from: 2026-06-07: Eunovo: 'Removed.' |
| theStack (member) | suggestion, maintenance | use std::any_of instead to simplify the check | fixed · fix pushed · author simplified using std::any_of | answered | yes | 2026-05-23: 'could slightly simplify by using `std::any_of` instead (returns a bool instead of an iterator)' Status from: 2026-06-07: Eunovo: 'Done.' |
| theStack (member) | question, approach | why the coin_control instance was moved up | fixed · fix pushed · author reverted the move | answered | yes | 2026-05-23: 'any reason why the coin_control instance got moved up?' Status from: 2026-06-07: Eunovo: 'Reverted.' |
| rkrux (member) | suggestion, maintenance | remove unnecessary createwallet argument from functional tests | fixed · fix pushed · author removed the argument | answered | yes | 2026-06-24: 'Please remove all occurrences of this createwallet argument, it's not required post disallowing loading legacy wallets.' Status from: 2026-06-26: Eunovo: 'Done' |
| rkrux (member) | suggestion, maintenance | remove deprecated rbf argument occurrences in transaction and psbt RPCs in tests | fixed · fix pushed · author removed the rbf argument | answered | yes | 2026-06-24: 'Please remove all occurrences of this in transaction/psbt creation/fund/send RPCs that are being used in this test.' Status from: 2026-06-26: Eunovo: 'Done' |
| rkrux (member) | suggestion, scope | add test case covering bumpfee when outputs argument is passed replacing a silent payment transaction | fixed · fix pushed · author added test cases for bumpfee with the outputs replaced | answered | yes | 2026-06-24: 'Please add a test case covering the behaviour when this argument is passed while replacing a silent payment transaction.' Status from: 2026-06-26: Eunovo: 'Added new tests for bumpfee with the outputs replaced.' |
| rkrux (member) | suggestion, scope | add test case covering rbf-cancel of a silent payment transaction by sending funds to self | fixed · fix pushed · author added test coverage and reviewer confirmed previous suggestions addressed | answered | yes | 2026-06-24: 'Please also add a test case covering the scenario where the user is able to rbf-cancel the SP transaction by sending the funds to themselves.' Status from: 2026-06-27: rkrux: 'Thanks for addressing the previous suggestions in both the source code and the tests' |
| rkrux (member) | suggestion, maintenance | generate xprvs and xpubs dynamically using ExtendedPrivateKey.generate() instead of hardcoding them | fixed · fix pushed · author updated dynamically generated keys upon rebase | answered | yes | 2026-06-27: 'After PR #35543 is merged, we don't need to (manually find and) hardcode xprvs and xpubs in the functional tests anymore.' Status from: 2026-07-24: Eunovo: 'Fixed.' |
| rkrux (member) | suggestion, maintenance | remove dead code copied from wallet_taproot.py | fixed · fix pushed · author removed the dead code | answered | yes | 2026-06-27: 'It's dead code and should be removed.' Status from: 2026-06-29: Eunovo: 'Done.' |
| rkrux (member) | suggestion, maintenance | use a local context manager for setup and cleanup in functional tests | fixed · fix pushed · author adopted the generator context manager pattern | answered | yes | 2026-06-27: 'For all these cases where the testing functionality is preceded by setup() and succeeded by cleanup(), a local context manager can be used for readability.' Status from: 2026-07-01: Eunovo: 'Changed the code to use the generator context manager pattern.' |
| Yudis-bit (none) | objection, correctness | MSVC fails to compile walletmodel.cpp due to referencing a deleted move constructor in a ternary expression | open | changed | no | 2026-10-04: 'The `Windows native, VS` failure in CI is reproducible on MSVC and directly related to `src/qt/walletmodel.cpp:457`' |
| Yudis-bit (none) | objection, correctness | IsInputForSharedSecretDerivation erroneously marks leaf-only Taproot inputs as eligible, leading to failures in CreateSilentPaymentsOutputs, and creates an inconsistency between send and sendall | fixed · fix pushed · author updated CheckSilentPaymentsInput to exclude eligible coins with missing private keys | changed | yes | 2026-10-07: 'For descriptors where the wallet only owns a leaf key (`tr(unspendable_key, pk(leaf))`), this returns `true`, but `CreateSilentPaymentsOutputs` subsequently fails' Status from: 2026-10-21: Eunovo: 'I have updated the rules in `CheckSilentPaymentsInput` to exclude eligible coins with missing private keys.' |
| Yudis-bit (none) | objection, correctness | EraseSpRecipients erases the original transaction recipient metadata on bumpfee, permanently losing recipient records if the original transaction is mined after replacement eviction | answered · author replied that persisted SP recipients info is only useful for fee bumping before confirmation | changed | yes | 2026-10-07: 'If the replacement is evicted from the mempool or dropped while an older block template mines the original transaction, the record of who it paid to is permanently lost.' Status from: 2026-10-13: Eunovo: 'The persisted SP recipients information is only useful for bumping the transaction. This can only happen before it is co' |
| Yudis-bit (none) | objection, correctness | multiplexing m_is_sp_tx into replaced_by_txid causes replaced transactions to lose their silent payments identity across wallet restarts | answered · author replied that m_is_sp_tx is only needed prior to bumping the transaction | changed | yes | 2026-10-07: 'Combined with `EraseSpRecipients`, a replaced transaction loses its silent payments identity entirely across restarts.' Status from: 2026-10-13: Eunovo: '`m_is_sp_tx` is only useful before the transaction is bumped.' |
| Yudis-bit (none) | objection, correctness | restricting anchor candidates to positive_group causes coin selection to fail with INSUFFICIENT_FUNDS at high fee rates when eligible inputs have non-positive effective value | fixed · fix pushed · author updated AttemptSelectionSP to use the negative group and sort by GetSelectionValue | changed | yes | 2026-10-07: 'In `AttemptSelectionSP` (`spend.cpp:1058`), anchor candidates are restricted to `positive_group`. At high fee rates, a small eligible coin (such as a 1,000 sat P2WPKH UTXO) can have `effective_value <' Status from: 2026-10-13: Eunovo: '`AttemptSelectionSP` has been updated to use the "negative" group.' |
| rustaceanrob (member) | suggestion, interface | use a boolean argument and return PaymentDestination instead of templated ParseOutputsWith functions | fixed · fix pushed · author confirmed change implemented | answered | yes | 2026-10-09: 'why not just use a boolean argument and return a `PaymentDestination`? I think this makes it more explicit as to what this function is doing.' Status from: 2026-10-13: Eunovo: 'Done.' |
| rustaceanrob (member) | suggestion, approach | allow duplicate silent payment destinations in ParseOutputs and sendall to support multiple outputs to the same recipient as allowed by BIP 352 | fixed · fix pushed · author confirmed change implemented | answered | yes | 2026-10-09: 'Can we remove this and only check for duplicate `CTxDestination` in `ParseOutputs`?' Status from: 2026-10-21: Eunovo: 'Done.' |
| rustaceanrob (member) | suggestion, interface | remove the default constructor and IsValid method on PaymentDestination and make destChange an optional in coin control to ensure correctness by construction | fixed · fix pushed · author confirmed change implemented | answered | yes | 2026-10-09: 'The type can be correct by construction and not rely on the doc comment for the convention. If the default constructor is deleted we can also remove the `IsValid` method as well.' Status from: 2026-10-13: Eunovo: 'Done' |
| rustaceanrob (member) | objection, interface | making silent payments transactions non-replaceable on older software leaves the wallet UI in an opaque and confusing state during fee bumping; older versions should fail loudly with a wallet flag instead | accepted · reviewer dropped the wallet flag objection after learning it prevents wallet downgrade, recommending a placeholder txid instead | answered | no | 2026-10-10: 'When bumping a transaction the user likely has some urgency, and this would leave the UI in an opaque/hard to understand state.' Status from: 2026-10-14: rustaceanrob: 'preventing a wallet open on downgrade is not desirable in basically any circumstance. I can empathize with that approach' |
| rustaceanrob (member) | suggestion, maintenance | simplify the comparison operator implementation using std::tie | fixed · fix pushed · author simplified comparison using std::tie | answered | yes | 2026-10-13: 'And if we do go this route, this can be simpler with:
```
return std::tie(a.m_version, a.m_scan_pubkey, a.m_spend_pubkey, a.m_extension_data)
< std::tie(b.m_version, b.m_scan_pubkey, b.m_spend_pu' Status from: 2026-10-13: Eunovo: 'Changed to use `std::tie`' |
| rustaceanrob (member) | suggestion, interface | use a dummy placeholder replacement txid for silent payments transactions so the behavior can be documented and shared as expected behavior | fixed · fix pushed · author confirmed change implemented | answered | yes | 2026-10-14: 'A dummy replacement value could make this easier to share as expected behavior (`00` + silent payments in hex):' Status from: 2026-10-21: Eunovo: 'Done.' |
| rustaceanrob (member) | suggestion, scope | consider introducing an RPC for BIP 352 scanning or adding a Python BIP 352 test scanner in a separate PR | open | answered | no | 2026-10-14: 'We may want to introduce an RPC for this, but not sure if it should be introduced as part of this PR. A scanner in python that passes the test vectors is relatively straightforward, and we can open th' |
| rustaceanrob (member) | suggestion, safety | inline GetDummyTxOut and make scriptChange an optional to ensure dummy taproot scripts are not used or signed as real outputs | answered · author rewrote scriptChange logic but declined inlining GetDummyTxOut to prevent duplication | answered | yes | 2026-10-18: 'This function seems like it could introduce surface for bugs. In the case of `scriptChange` for instance, the `scriptChange` variable is used in the actual transaction later. I would prefer if these c' Status from: 2026-10-21: Eunovo: 'I rewrote the `scriptChange` logic, but left the other code as is.' |
| Yudis-bit (none) | objection, correctness | filtering anchor outputs by GetEffectiveValue() <= 0 prematurely drops valid anchors under m_subtract_fee_outputs at high feerates and causes transaction creation to abort with no eligible inputs found | open | changed | no | 2026-11-01: 'Checking `GetEffectiveValue() <= 0` prematurely drops valid anchors and aborts with "No silent payment eligible inputs were found" even when the wallet has sufficient funds to cover the send.' |
Support:
- rkrux (member) (Concept ACK, 2026-05-20): no reason given [not substantive]
- w0xlt (contributor) (Concept ACK, 2026-10-10): no reason given [not substantive]
- rustaceanrob (member) (Concept ACK, 2026-10-10): no reason given [not substantive]
Participants:
- rkrux (member, support, 12 statements 2026-05-20 to 2026-06-30): Concept ACK; reviewed functional tests and suggested cleanups and additional test coverage
- theStack (member, objection, 5 statements 2026-05-23 to 2026-05-23): objected to TxoutType checking condition and suggested code cleanups
- Yudis-bit (none, objection, 6 statements 2026-10-04 to 2026-11-01): reported MSVC compilation failure, anchor filtering defect under subtract fee outputs, and edge cases in key derivation and RBF state persistence
- rustaceanrob (member, objection, 11 statements 2026-10-09 to 2026-10-18): Concept ACK; dropped wallet flag objection in favor of a placeholder txid suggestion, and suggested inlining GetDummyTxOut and using optional for scriptChange
- w0xlt (contributor, support, 1 statements 2026-10-10 to 2026-10-10): Concept ACK
Dependencies
Files
1674 lines under test/bench/ci.
- test/functional/wallet_silentpayments_sending.py +823/-0
- src/wallet/spend.cpp +275/-17
- src/wallet/test/coinselector_tests.cpp +243/-0
- src/wallet/rpc/spend.cpp +181/-58
- test/functional/wallet_sendall.py +136/-62
- src/wallet/feebumper.cpp +112/-36
- test/functional/test_framework/bip352.py +101/-0
- src/test/paymentdestination_tests.cpp +94/-0
- src/common/paymentdestination.h +69/-0
- test/functional/wallet_backwards_compatibility.py +67/-0
- src/wallet/test/spend_tests.cpp +53/-2
- src/wallet/spend.h +51/-0
- src/common/paymentdestination.cpp +49/-0
- src/wallet/feebumper.h +33/-12
- src/wallet/scriptpubkeyman.cpp +41/-0
- src/common/bip352.cpp +32/-8
- src/wallet/walletdb.cpp +38/-2
- src/wallet/test/wallet_transaction_tests.cpp +38/-0
- src/rpc/output_script.cpp +24/-13
- src/wallet/transaction.h +34/-2
- test/functional/rpc_validateaddress.py +36/-0
- src/rpc/rawtransaction_util.cpp +20/-14
- src/wallet/wallet.cpp +24/-5
- src/qt/walletmodel.cpp +24/-4
- src/common/bip352.h +26/-0
- src/wallet/interfaces.cpp +17/-5
- src/interfaces/wallet.h +11/-3
- src/rpc/rawtransaction_util.h +5/-5
- src/wallet/wallet.h +8/-2
- src/qt/sendcoinsdialog.cpp +4/-4
- src/wallet/scriptpubkeyman.h +8/-0
- src/wallet/coincontrol.h +4/-1
- src/wallet/test/fuzz/spend.cpp +3/-2
- src/wallet/walletdb.h +5/-0
- src/wallet/test/wallet_tests.cpp +2/-2
- src/bench/wallet_create_tx.cpp +2/-1
- src/qt/test/wallettests.cpp +1/-1
- src/CMakeLists.txt +1/-0
- src/test/CMakeLists.txt +1/-0
- src/test/bip352_tests.cpp +1/-0
- test/CMakeLists.txt +1/-0
- test/functional/feature_framework_unit_tests.py +1/-0
- test/functional/test_runner.py +1/-0
Card
The short text the ranking pass compares across PRs.
Adds wallet and RPC support for sending to BIP 352 Silent Payment addresses across send, sendtoaddress, sendmany, and sendall. Currently, Bitcoin Core wallet users cannot pay silent payment addresses, preventing the use of receiver privacy protocols in Core. The PR introduces PaymentDestination to handle dynamic output derivation, updates coin selection to ensure derivation-eligible keys are available, and stores recipient destinations in the wallet database to support RBF fee bumping. This work is tracked under project issue #28536, depends on prerequisite PR #35301, and helps unblock receiving support in PR #32966.
Data
dossier JSON · extract JSON · model ledger, generated 2026-11-02T01:11, confidence high, input hash 862e0306ba42b1ef
thread read through 2026-11-01 (61 statements), head 3e55df3748
labels: none
thread read through 2026-11-01 (61 statements), head 3e55df3748
labels: none
Categories
Wallet
P2 · new feature
Adds wallet and RPC support for sending transactions to BIP 352 Silent Payment addresses, providing substantial user-facing value for receiver privacy and address reuse prevention. Tracked under project issue #28536 and unblocks follow-on work #32966, making it a strategically important wallet milestone deserving deliberate review.
Membership: Significantly modifies wallet spending routines in src/wallet/spend.cpp, coin selection, fee bumping, wallet database records, and wallet RPCs to support BIP 352 destinations.
Factors: security/stability 1, bug 0, performance 0, user value 3, leverage 2