-
Notifications
You must be signed in to change notification settings - Fork 38.6k
Silent Payments: sending #28201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Silent Payments: sending #28201
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28201. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
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:
If you want, I can produce a patch snippet with these exact replacements. drahtbot_id_5_m |
abd8210 to
ec1c579
Compare
ec1c579 to
37724f8
Compare
37724f8 to
e6f7458
Compare
| ) | ||
| assert txid | ||
|
|
||
| def test_deterministic_send(self): |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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
a96aa7c to
9be4755
Compare
9be4755 to
6986e50
Compare
| } | ||
| const auto* spk_manager = *spk_managers.begin(); | ||
| const auto& key = spk_manager->GetPrivKeyForSilentPayment(input->txout.scriptPubKey); | ||
| if (std::holds_alternative<KeyPair>(key)) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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]>
6986e50 to
db58cb7
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. |
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:
scriptPubKeyCRecipientsThe functions are then used together to create silent payment outputs during
CreateTransactionInternal.Final steps
The last commits ensure that: