Silent Payments: receiving #28202
Closed
Silent Payments: receiving #28202
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Add methods tweaking a CKey by adding a scalar value or by multiplying * Add a method for combining multiple CPubKeys via addition * Add a method for doing ECDH with a CPubKey and scalar These are the CKey,CPubkey primitives we need to implement the silent payments protocol.
Bech32m imposes a 90 character limit when decoding strings. Since a silent payment address is the concatenation of two public keys, raise this limit to 1024 when decoding a silent payment address. The new limit is much higher than needed to account for forward compatibility: new silent payment versions may add new data to the data part of the address.
Add a function for decoding the string address and a second function for decoding the data part of the silent payment address.
Implement sending and receiving, per BIP352. This is done without requiring a full wallet, in order to simplify unit testing and to create a more clear boundary as to what pertains to the BIP and what is left to the wallet to decided as an implementation. To correctly hash the public keys, per the BIP, update the HashWriter.write method to accept a Span<unsigned char>. This allows us to easily pass a public key in for hashing, where only the data from the public key is hashed. This commit also moves the `CRecipient` struct to wallet/types.h and adds a new struct, `V0SilentPaymentDestination`. These are added here to simplify implementing sending and receiving in the future.
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. Also adds a comparator to `CRecipient`, to make testing easier.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
3 tasks
This was referenced Aug 2, 2023
Closed
josibake
changed the title
Co-authored-by: Aurèle Oulès <aurele@oules.com>
Co-authored-by: josibake <josibake@protonmail.com>
josibake
force-pushed
the
implement-bip352-receiving
branch
from
839b1a6
to
e2d9e23
Compare
josibake
force-pushed
the
implement-bip352-receiving
branch
from
e2d9e23
to
3f1b810
Compare
This was referenced Aug 3, 2023
This was referenced Aug 3, 2023
This was referenced Aug 3, 2023
Merged
2 tasks
achow101
reviewed
Aug 9, 2023
| pubkeys_for_shared_secret.push_back(pubkey); | ||
| continue; | ||
| } | ||
| case TxoutType::SCRIPTHASH: |
There was a problem hiding this comment.
Should recurse on SCRIPTHASH instead of assuming.
| // Find the UTXO being spent in UTXO Set to learn the transaction type | ||
| std::map<COutPoint, Coin> coins; | ||
| coins[txin.prevout]; | ||
| chain().findCoins(coins); |
There was a problem hiding this comment.
This looks in the UTXO set but is called on a transaction that has already been verified and so therefore it's inputs are no longer in the UTXO set. This results in the Coin being uninitialized which causes later Solver calls to fail unexpectedly.
There was a problem hiding this comment.
As discussed offline, we shouldn't use any view of the UTXO set. Rather we should use the undo data when receiving a blockConnected, and modify TransactionAddedToMempool to include the previous outputs. This lets us avoid any race conditions with the state of the UTXO set and guarantees that the previous output data will always be available for the silent payments calculations.
There was a problem hiding this comment.
Tested ACK with some comments at the end
Was able to successfully run unit and functional tests on 'pr/28202' branch and verified that the tests fail against the master branch
Manually tested on regtest as described below:
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="sp-wallet" silent_payment=true$ ./src/bitcoin-cli -regtest getwalletinfo # showed that a wallet with silent payment enabled was created successfully
{
"walletname": "sp-wallet",
"walletversion": 169900,
"format": "sqlite",
"balance": 0.00000000,
"unconfirmed_balance": 0.00000000,
"immature_balance": 0.00000000,
"txcount": 0,
"keypoolsize": 4001,
"keypoolsize_hd_internal": 4000,
"paytxfee": 0.00000000,
"private_keys_enabled": true,
"avoid_reuse": false,
"scanning": false,
"descriptors": true,
"external_signer": false,
"blank": false,
"silent_payment": true,
"lastprocessedblock": {
"hash": "2fbd2f41ecd9e553cc8d129cff71846e0f26da7584eb8362e7cc70c0913b8e98",
"height": 212
}
}$ ./src/bitcoin-cli -regtest listdescriptors # was able to list a silent payment descriptor
{
…
{
"desc": "sp([f3a37e3d/352h/1h/0h/1h]tpubDFMFwrp8wbV1voiEY3dy6ba5mv3nzbzRQmBHYfu6c3gGnf49QM9PB6Uy4P3Rpb5YcgXHT4vcXCpWiDt8rSoQXceJenU2WkvxcV6rYAxVxm6/0,[f3a37e3d/352h/1h/0h/0h]tpubDFMFwrp8wbV1tkGrdjqb8VSRp7srJQGvtqxmBSU3LVySKFPzCH1sP9cuLtUHA2Cz6qFSXdi2F52gAGNzqMrZrGrNwmf7EzbxebVEzBKugvm/0)#qzhxnezt",
"timestamp": 1692181466,
"active": true,
"internal": false,
"next": 0
},
…
}$ ./src/bitcoin-cli -regtest getsilentaddress # was able to generate a silent payment address
{
"address": "sprt1qq0yr8yv2dzrrvkyde8lex6qjy2jklgpxud260yvepgdfh5rh2ldj2qket9cwqj9xt5j7mseqk4appaqzung4xhwknaxcchlf54vutvv4mcper7ca"
}$ ./src/bitcoin-cli -regtest decodesilentaddress sprt1qq0yr8yv2dzrrvkyde8lex6qjy2jklgpxud260yvepgdfh5rh2ldj2qket9cwqj9xt5j7mseqk4appaqzung4xhwknaxcchlf54vutvv4mcper7ca
# was able to correctly decode the silent payment address above into scan and spen pubkeys
{
"address": "sprt1qq0yr8yv2dzrrvkyde8lex6qjy2jklgpxud260yvepgdfh5rh2ldj2qket9cwqj9xt5j7mseqk4appaqzung4xhwknaxcchlf54vutvv4mcper7ca",
"scan_pubkey": "03c833918a688636588dc9ff93681222a56fa026e355a791990a1a9bd07757db25",
"spend_pubkey": "02d95970e048a65d25edc320b57a10f402e4d1535dd69f4d8c5fe9a559c5b195de"
}comments
- I noticed that
./src/bitcoin-cli -regtest listdescriptorsonly listed one descriptor for silent payments - Also that
./src/bitcoin-cli -regtest getsilentaddressalways return the same one address every time
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
Closing in favor of #28453 Originally, the idea was to implement sending and receiving separately, but it makes more sense to have them together so the receiving PR can use silent payments for generating change. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a child of #27827 and only implements the receiving logic. For the rest:
This PR depends on #28122 and is marked as a draft until it is merged. Commits up to 8ee791e belong to #28122; please review those commits on #28122
Receiving
Description coming soon