Implement bip352 full #1
Conversation
Eunovo
commented
Jul 3, 2024
Eunovo
commented
- Moved Silent Payments SPKM code to new DescriptorScriptPubKeyMan subclass
- Fixed errors
- Repositioned commits that fail to compile because they use code defined in later commits
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
This is a temporary measure and might not be what we want long term.
this is a temporary measure. The alternative to having this method is to store the full spk + tweak in the DB (34 + 32 bytes), vs only storing the tweak (32 bytes) and recreating the spks every time we load the tweaks. Another option is to add this method directly to V0SilentPayments so that its not generally available for any CPubKey.
* Exclude OutputType::SILENT_PAYMENT from wallet notifications fuzz test because wallet cannot create a V0SilentPaymentDestination yet
* Add base58 prefixes for Silent Payment Keys * Encode and Decode SP Keys using Base58Check. This will changed to Bech32M later
This allows DescriptorImpl.Extract to add spkeys to FlatSigningProvider
Parse sp(sppub or spprv) or sp(scan_priv_key,spend_pub_key or spend_priv_key)
Update GenerateWalletDescriptor to generate sp descriptor for SP and return scan and spend private keys to caller Add function to extranct SP tweak data from tx and spent_coins Add function to extract SpPubKey from an sp desc All of these will be used for the SP DescriptorSPKMan Impl
Optionally allow users to create a wallet that supports silent payments. This is signaled through an option in createwallet and a new wallet flag.
Call IsMineSilentPayment when sending to a SP address if our wallet has SPs enabled. In `CreateSilentPaymentOutputs`, we create a change output using silent payments. The presence of the change output will cause us to not fully check the transaction and thus never create the silent payment tweaks, which is why we check for self-transfers here. This feels a bit hacky, but works for now.
* Allow same Silent Payments DSPKM to be loaded as both internal and external spkm * Update AddWalletDescriptor to handle silent payment descr
|
Couple of things I noticed on
|
|
Also, using the GUI to send to a silent payment address results in some sort of malformed non-standard output. So you may want to disable or fix that :-) Tried with both |
|
It would be useful if |
Come to think of it, it's nice if you can specify how many labels have been used so far. Otherwise you have to generate enough labels and then do another rescan. We could shoehorn this number into range For the same reason it would be useful if |
|
I managed to hit Recreating the wallet, generating new addresses for each label, and then rescanning the existing transactions works. Once I restart QT and load the wallet again it crashes. If I skip the rescan the transactions don't show up (as expected). But that does avoid the crash after a restart. But then rescanwallet doesn't reveal the earlier transactions. This behavior seems quite consistent. Here's the branch I used: https://github.com/Sjors/bitcoin/tree/2024/08/sp (same as |
|
One more thing: I tried to make a watch-only version of the wallet. I first obtained the descriptor with only the scan key using Disabling that check triggers |
@Sjors SGTM but https://github.com/bitcoin/bips/blob/master/bip-0352.mediawiki#scanning specifies that implementations scan for labels until no new payments are found. Adding a label range to |
@Sjors Will look into these |
Taken care of in #3
@Sjors They are derived but current |
It's possible that I used labels in the "wrong" order - and so the one unused label caused the import to not find my transaction? But this strategy seems problematic, because you can't guarantee that people will pay to your label. Oh hi gap problem! :-) cc @josibake.
Generally when you specify a range for a regular descriptor, it will top up the key pool every time it finds a used address. So it will go past the specified range.
I think it should. I'll try out #3. |
|
We need to wait to hear from @josibake Which will probably be next week. However, this PR does not need to remain open as the changes from this PR are already on the josibake/implement-bip352-full branch, so, I am closing this. |
The previous commit added a test which would fail the
unsigned-integer-overflow sanitizer. The warning is harmless and can be
triggered on any commit, since the code was introduced.
For reference, the warning would happen when the separator `-` was not
present.
For example:
GET /rest/getutxos/6a297bfa5cb8dd976ab0207a767d6cbfaa5e876f30081127ec8674c8c52b16c0_+1.json
would result in:
rest.cpp:792:77: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'size_type' (aka 'unsigned long')
#0 0x55ad42c16931 in rest_getutxos(std::any const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) src/rest.cpp:792:77
#1 0x55ad4319e3c0 in std::function<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&)>::operator()(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
#2 0x55ad4319e3c0 in HTTPWorkItem::operator()() src/httpserver.cpp:59:9
#3 0x55ad431a3eea in WorkQueue<HTTPClosure>::Run() src/httpserver.cpp:114:13
#4 0x55ad4318f961 in HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) src/httpserver.cpp:403:12
#5 0x7f078ebcbbb3 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xeabb3) (BuildId: 40b9b0d17fdeebfb57331304da2b7f85e1396ef2)
#6 0x55ad4277e01c in asan_thread_start(void*) asan_interceptors.cpp.o
#7 0x7f078e840a93 (/lib/x86_64-linux-gnu/libc.so.6+0x9ca93) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3)
bitcoin#8 0x7f078e8cdc3b (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow rest.cpp:792:77
