[Testing] additional silent payments tests - descriptors + spend(send) - Replaces PR #1#2
Draft
macgyver13 wants to merge 43 commits intoEunovo:2025-implement-bip352-receivingfrom
Draft
Conversation
Eunovo
force-pushed
the
2025-implement-bip352-receiving
branch
2 times, most recently
from
d543c87 to
50fafd9
Compare
2b57d2ad89 docs: update README 5d2d4adabd ci: enable silentpayments module d6788a888c tests: add constant time tests 4c32ba9613 tests: add BIP-352 test vectors 183e5414f7 silentpayments: add benchmarks for scanning 288390446a silentpayments: add examples/silentpayments.c 250beff5d2 silentpayments: receiving f90d7a76b0 silentpayments: recipient label support 1a5f53f2cf silentpayments: sending ca2538a878 build: add skeleton for new silentpayments (BIP352) module git-subtree-dir: src/secp256k1 git-subtree-split: 2b57d2ad8964e536508fae0b6ab1331396fe0308
Eunovo
force-pushed
the
2025-implement-bip352-receiving
branch
from
50fafd9 to
01e275b
Compare
macgyver13
force-pushed
the
bip352-receiving-testing
branch
2 times, most recently
from
a615ae7 to
0e7fbf9
Compare
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 <37949128+eunovo@users.noreply.github.com>
While the scankey looks like a private key, and is represented as a `CKey` in the code, it is actually much closer to a public key and it needs to be treated as a public key, so it can be added to watch-only wallets.
sp() expects a scan private key and a spend key which could be a private key or a public key. The scan key is really a "scan entropy" and will not be treated as an actual private key
This method makes it easier to tweak silent payment spend keys in later commits
The spent_coins data will be used to scan for silent payments in later commits Co-authored-by: Ava Chow <github@achow101.com>
This undo data will be used to scan for silent payments in later commits Co-authored-by: Ava Chow <github@achow101.com>
Co-authored-by: Ava Chow <github@achow101.com>
Co-authored-by: Ava Chow <github@achow101.com>
silent_payments wallet needs to scan entire blocks since it doesn't know ahead of time which scriptPubKeys to use in it's fast rescan filter.
Co-authored-by: Ava Chow <github@achow101.com>
traditionally, the receiving scripts are known to the core wallet because they are added to the addressbook at the time they are requested. in silent payments, the outputs are not known until found. its important, however, to have the found scripts in the addressbook so all of the change accounting can be down properly. this commits adds found outputs to the address book if they are not change. the way change is determined is a bit hacky in that we just check if the found output was found with a label (since this is the only label currently implemented). in the future, we should specifically check for a change specific label.
Check if a send is a self transfer when the transaction is created. This ensures our cache and address book is properly updated since the wallet will not recheck this transaction fully if it detects change.
The wallet tries to match change OutputTpe to other outputs to preserve privacy. Use silent payment change address when sending coins to: - any silent payment address - any taproot address
`CreateTransaction` fails if change_type is set to OutputType::SILENT_PAYMENTS and there are no eligible inputs for the transaction. This commit modifies `CreateTransaction` logic to make a second attempt to create a tx without OutputType::SILENT_PAYMENTS change_type
This allows wallets with only sp descs to use sp change destination for transactions.
Eunovo
force-pushed
the
2025-implement-bip352-receiving
branch
from
01e275b to
09277cb
Compare
(cherry picked from commit 4ad6f20)
macgyver13
force-pushed
the
bip352-receiving-testing
branch
from
0e7fbf9 to
1981a1c
Compare
Eunovo
force-pushed
the
2025-implement-bip352-receiving
branch
3 times, most recently
from
f86b9d2 to
7150948
Compare
Eunovo
force-pushed
the
2025-implement-bip352-receiving
branch
2 times, most recently
from
891b8dd to
cc0b97b
Compare
Eunovo
force-pushed
the
2025-implement-bip352-receiving
branch
from
cc0b97b to
55485a6
Compare
Eunovo
force-pushed
the
2025-implement-bip352-receiving
branch
2 times, most recently
from
fcc8b88 to
3bfab00
Compare
Eunovo
pushed a commit
that referenced
this pull request
Jan 19, 2026
… corruption check in fees.dat fa1d17d refactor: Use uint64_t over size_t for serialize corruption check in fees.dat (MarcoFalke) Pull request description: Serialization should not behave differently on different architectures. See also the related commit 3789215. However, on fees.dat file corruption, 32-bit builds may run into an unsigned integer overflow and report the wrong corruption reason, or may even silently continue after the corruption. This is a bit hard to reproduce, because 32-bit platforms are rare and most of them don't support running the unsigned integer overflow sanitizer. So the possible options to reproduce are: * Run on armhf and manually annotate the code to detect the overflow * Run on i386 with the integer sanitizer (possibly via `podman run -it --rm --platform linux/i386 'debian:trixie'`) * Run the integer sanitizer on any 64-bit platform and manually replace type in the affected line by `uint32_t` Afterwards, the steps to reproduce are: ``` export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./b-c && cd b-c && apt install build-essential cmake pkg-config python3-zmq libzmq3-dev libevent-dev libboost-dev libsqlite3-dev systemtap-sdt-dev libcapnp-dev capnproto libqrencode-dev qt6-tools-dev qt6-l10n-tools qt6-base-dev clang llvm libc++-dev libc++abi-dev -y cmake -B ./bld-cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=undefined,integer,float-divide-by-zero --preset=dev-mode cmake --build ./bld-cmake --parallel $(nproc) curl -fLO 'https://github.com/bitcoin-core/qa-assets/raw/b5ad78e070e4cf36beb415d7b490d948d70ba73f/fuzz_corpora/policy_estimator_io/607473137013139e3676e30ec4b29639e673fa9b' UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=policy_estimator_io ./bld-cmake/bin/fuzz ./607473137013139e3676e30ec4b29639e673fa9b ``` The output will be something like: ``` /b-c/src/policy/fees/block_policy_estimator.cpp:448:25: runtime error: unsigned integer overflow: 346685954 * 219 cannot be represented in type 'unsigned int' #0 0x5b0b1bbe in TxConfirmStats::Read(AutoFile&, unsigned int) /b-c/bld-cmake/src/./policy/fees/block_policy_estimator.cpp:448:25 #1 0x5b0b7d3f in CBlockPolicyEstimator::Read(AutoFile&) /b-c/bld-cmake/src/./policy/fees/block_policy_estimator.cpp:1037:29 #2 0x592a9783 in policy_estimator_io_fuzz_target(std::span<unsigned char const, 4294967295u>) /b-c/bld-cmake/src/test/fuzz/./test/fuzz/policy_estimator_io.cpp:32:32 #3 0x5896ba8e in void std::__invoke_impl<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>(std::__invoke_other, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/invoke.h:61:14 bitcoin#4 0x5896b8eb in std::enable_if<is_invocable_r_v<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>, void>::type std::__invoke_r<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>(void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/invoke.h:111:2 bitcoin#5 0x5896b44b in std::_Function_handler<void (std::span<unsigned char const, 4294967295u>), void (*)(std::span<unsigned char const, 4294967295u>)>::_M_invoke(std::_Any_data const&, std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/std_function.h:290:9 bitcoin#6 0x59845c95 in std::function<void (std::span<unsigned char const, 4294967295u>)>::operator()(std::span<unsigned char const, 4294967295u>) const /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/std_function.h:591:9 bitcoin#7 0x5983a0da in test_one_input(std::span<unsigned char const, 4294967295u>) /b-c/bld-cmake/src/test/fuzz/util/./test/fuzz/fuzz.cpp:88:5 bitcoin#8 0x5983cb80 in main /b-c/bld-cmake/src/test/fuzz/util/./test/fuzz/fuzz.cpp:271:13 bitcoin#9 0xf75aecc2 (/lib/i386-linux-gnu/libc.so.6+0x24cc2) (BuildId: 2dc5f2945fad35c1b07d1a5a32520b3c41afaa75) bitcoin#10 0xf75aed87 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x24d87) (BuildId: 2dc5f2945fad35c1b07d1a5a32520b3c41afaa75) bitcoin#11 0x58932db6 in _start (/b-c/bld-cmake/bin/fuzz+0x235ddb6) (BuildId: 7d8d83a77923f14e99c0de64acbc5f5bfc2cce9b) SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /b-c/src/policy/fees/block_policy_estimator.cpp:448:25 ``` Note: This is marked a "refactor", because the code change does not affect 64-bit builds, and on the still remaining rare 32-bit builds today it is extremely unlikely to happen in production. ACKs for top commit: bensig: ACK fa1d17d ismaelsadeeq: utACK fa1d17d luke-jr: Also, utACK fa1d17d as an improvement. Tree-SHA512: 696bf8e0dbe4777c84cb90e313c7f8f9ee90d4b3e64de1222f8472b2d9d0f3a0f6f027fda743dd6ca8c6aab94f404db7a65bb562a76000d9c33a8a39de28d8d4
Eunovo
force-pushed
the
2025-implement-bip352-receiving
branch
from
3bfab00 to
9ecf265
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 includes additional testing that exposes a few issues with silent payments.
1 of the working tests was already resolved last week 👏
The 2 broken tests highlight issues that need attention. Let me know if you need any additional details.