Skip to content

Conversation

@josibake
Copy link
Member

@josibake josibake commented Aug 2, 2023

This PR is part of integrating silent payments into Bitcoin Core. Status and tracking for the project is managed in #28536

This PR depends on #28122 and is marked as a draft until it is merged. If interested in those commits, please review on #28122

Sending

Silent Payments logic

The main focus of this PR is:

  • Applying the Taptweak to a taproot internal private key (this is a copy-paste of the code for applying the taptweak in the signing process)
  • Getting a private key from a given scriptPubKey
  • Creating silent payment outputs
  • Applying the created scriptPubKeys back to the vector of CRecipients

The functions are then used together to create silent payment outputs during CreateTransactionInternal.

Final steps

The last commits ensure that:

  • Coin selection is silent payments aware and knows to exclude taproot script path spends and inputs with unknown witness when funding a transaction which pays to a silent payment address
  • The change output type is correctly chosen when paying to a silent payment address
  • Functional tests

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 2, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28201.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #33625 (Update secp256k1 subtree to latest master by fanquake)
  • #33034 (wallet: Store transactions in a separate sqlite table by achow101)
  • #32763 (wallet: Replace CWalletTx::mapValue and vOrderForm with explicit class members by achow101)
  • #32579 (p2p: Correct unrealistic headerssync unit test behavior by hodlinator)
  • #31974 (Drop testnet3 by Sjors)
  • #28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
  • #27260 (Enhanced error messages for invalid network prefix during address parsing. by portlandhodl)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

  • exctracting -> extracting [typo in comment: "before exctracting the public key" -> "before extracting the public key"]
  • desintaions -> destinations [typo in doc: "The silent payment desintaions are passed in map..." -> "The silent payment destinations are passed in a map..."]
  • in map -> in a map [missing article in doc: "passed in map" -> "passed in a map"]
  • std::<CPubKey, uint156> -> std::pair<CPubKey, uint256> [incorrect/garbled return type in doc causing confusion about the return value]
  • intendended -> intended [typo in comment: "intendended for the recipient" -> "intended for the recipient"]
  • _recipent_prevouts_summary_create -> _recipient_prevouts_summary_create [typo in doc reference to function name]
  • recipent -> recipient [typo in comment: "when creating the shared secret, i.e., (recipient_scan_key * input_hash) * prevout_pubkey_sum" and similar uses]
  • A explanation -> An explanation [grammar in comment: "A explanation..." -> "An explanation..."]
  • faily -> fail [typo in comment: "This will only faily if the inputs..." -> "This will only fail if the inputs..."]

If you want, I can produce a patch snippet with these exact replacements.

drahtbot_id_5_m

@josibake josibake mentioned this pull request Aug 2, 2023
3 tasks
@josibake josibake changed the title Silent Payments: implement sending Silent Payments: sending Aug 3, 2023
@josibake josibake force-pushed the implement-bip352-sending branch from abd8210 to ec1c579 Compare August 3, 2023 06:45
@josibake josibake force-pushed the implement-bip352-sending branch from ec1c579 to 37724f8 Compare August 3, 2023 06:54
@josibake josibake force-pushed the implement-bip352-sending branch from 37724f8 to e6f7458 Compare August 3, 2023 07:24
@DrahtBot DrahtBot removed the CI failed label Aug 3, 2023
@josibake
Copy link
Member Author

josibake commented Aug 3, 2023

Maybe add a quick summary in the description with the main implementation differences relative to #24897. It seems a big one is that this doesn't require an index!

updated! I added the summary in #27827 and added links back to the parent PR in each of the child PRs.

)
assert txid

def test_deterministic_send(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorporating logs within the test is crucial for offering transparent insight into the test's progression, simplifying the identification of problems, and enhancing comprehension of the test's overall behavior.

else:
assert False

def test_address_reuse(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Also consider adding logs to this test as the ones above

@josibake josibake force-pushed the implement-bip352-sending branch from a96aa7c to 9be4755 Compare August 14, 2025 08:01
@josibake josibake force-pushed the implement-bip352-sending branch from 9be4755 to 6986e50 Compare August 15, 2025 14:53
}
const auto* spk_manager = *spk_managers.begin();
const auto& key = spk_manager->GetPrivKeyForSilentPayment(input->txout.scriptPubKey);
if (std::holds_alternative<KeyPair>(key)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

7c1093b: We get a segfault later if the keys are not valid. We should check that keys are valid before adding them to taproot_keys or plain_keys

Copy link
Contributor

Choose a reason for hiding this comment

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

GetPrivKeyForSilentPayment returns {} which is a default initialized object if the coin's keys are not available. std::holds_alternative<CKey> will pass for the retuen object and add the default intialized CKey to the plain_keys array.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for finding this! I've rebased on master and pulled in the latest libsecp256k1 changes; I'll push a fix for this tomorrow.

josibake and others added 16 commits September 5, 2025 15:51
aa85bfb530 docs: update README
9f42a30b82 ci: enable silentpayments module
d504e48145 tests: add sha256 tag test
124750d580 tests: add constant time tests
b35ffa2e30 tests: add BIP-352 test vectors
038c5b9c9d silentpayments: add benchmarks for scanning
88eb3d4545 silentpayments: add examples/silentpayments.c
22b20fd617 silentpayments: receiving
df1de93765 silentpayments: recipient label support
76a0451c76 silentpayments: sending
3cd3a93bff build: add skeleton for new silentpayments (BIP352) module
f36afb8b3d Merge bitcoin-core/secp256k1#1725: tests: refactor tagged hash verification
5153cf1c91 tests: refactor tagged hash tests
d2dcf52091 Merge bitcoin-core/secp256k1#1726: docs: fix broken link to Tromer's cache.pdf paper
489a43d1bf docs: fix broken link to eprint cache.pdf paper
d599714147 Merge bitcoin-core/secp256k1#1722: docs: Exclude modules' `bench_impl.h` headers from coverage report
0458def51e doc: Add `--gcov-ignore-parse-errors=all` option to `gcovr` invocations
1aecce5936 doc: Add `--merge-mode-functions=separate` option to `gcovr` invocations
106a7cbf41 doc: Exclude modules' `bench_impl.h` headers from coverage report
a9e955d3ea autotools, docs: Adjust help string for `--enable-coverage` option
e523e4f90e Merge bitcoin-core/secp256k1#1720: chore(ci): Fix typo in Dockerfile comment
24ba8ff168 chore(ci): Fix typo in Dockerfile comment
74b8068c5d Merge bitcoin-core/secp256k1#1717: test: update wycheproof test vectors
c25c3c8a88 test: update wycheproof test vectors
20e3b44746 Merge bitcoin-core/secp256k1#1688: cmake: Avoid contaminating parent project's cache with `BUILD_SHARED_LIBS`
2c076d907a Merge bitcoin-core/secp256k1#1711: tests: update Wycheproof
7b07b22957 cmake: Avoid contaminating parent project's cache with BUILD_SHARED_LIBS
5433648ca0 Fix typos and spellings
9ea54c69b7 tests: update Wycheproof files

git-subtree-dir: src/secp256k1
git-subtree-split: aa85bfb530b9ffc3dde6eaa7a976e129b8bd2f58
Add a method for passing a KeyPair object to secp256k1 functions expecting a secp256k1_keypair.
This allows for passing a KeyPair directly to a secp256k1 function without needing to create a
temporary secp256k1_keypair object.
Wrap the silentpayments module from libsecp256k1. This is placed in
common as it is intended to be used by:

  * RPCs: for parsing addresses
  * Wallet: for sending, receiving, spending silent payment outputs
  * Node: for creating silent payment indexes for light clients
Have `IsValidDestination` return false for silent payment destinations
and set an error string when decoding a silent payment address.

This prevents anyone from sending to a silent payment address before
sending is implemented in the wallet, but also allows the functions to
be used in the unit testing famework.
Use the test vectors to test sending and receiving. A few cases are not
covered here, namely anything that requires testing specific to the
wallet. For example:

* Taproot script path spending is not tested, as that is better tested in
  a wallets coin selection / signing logic
* Re-computing outputs during RBF is not tested, as that is better
  tested in a wallets RBF logic

The unit tests are written in such a way that adding new test cases is
as easy as updating the JSON file
BIP352 v0 specifies that a silent payment output is a taproot output.
Taproot scriptPubKeys are a fixed size, so when calculating the
serialized size for a CRecipient with a V0SilentPayments destination,
use WitnessV1Taproot for the serialized txout size.
Add a method for retreiving a private key for a given scriptPubKey.
If the scriptPubKey is a taproot output, tweak the private key with the
merkle root or hash of the public key, if applicable.
Add a flag to the `CoinControl` object if silent payment destinations
are provided. Before adding the flag, call a function which checks if:

* The wallet has private keys
* The wallet is unlocked

Without both of the above being true, we cannot send to a silent payment
address.

During coin selection, if this flag is set, skip taproot inputs when
script spend data is available. This is based on the assumption that if
a user provides script spend data, they don't have access to the key
path spend. As future improvement, we could instead check to see if we
have access to the key path spend, and only exclude the output when we
don't regardless of whether or not the user provides script spend data.

Also skip UTXOs of type `WITNESS_UNKNOWN`, although it is very unlikely
our wallet would ever try to spend a witness unknown output.
`CreateSilentPaymentsOutputs` gets the correct private keys, adds them
together, groups the silent payment destinations and then generates the
taproot script pubkeys. These are then passed back to
CreateTransactionInternal, which uses these scriptPubKeys to update
vecSend before adding them to the transaction outputs.
If sending to a silent payment destination, the change type should be taproot
Treat silent payment addresses as valid destination. Also disable using
silent payment addresses with the `addr()` descriptor, as this
descriptor expects an encoding of a scriptPubKey, whereas a silent
payment address consists of instructions on how to generate a
scriptPubKey for the recipient.

Co-authored-by: Oghenovo Usiwoma <[email protected]>
@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants