This PR focuses strictly on the BIP logic and attempts to separate it from the wallet and transaction implementation details. This is accomplished by working directly with public and private keys, instead of needing a wallet backend and transactions for testing. Labels for the receiver are optional and thus deferred for a later PR.
Test vectors from the BIP are included as unit tests.
#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.
josibake force-pushed
on Jul 21, 2023
DrahtBot added the label
CI failed
on Jul 21, 2023
josibake force-pushed
on Jul 21, 2023
in
src/kernel/chainparams.cpp:633
in
6d11e3c435outdated
Didn’t verify my theory in practice yet, but according to my calculations having a HRP of size 4 would exceed the current 117 character limit (see https://github.com/bitcoin/bips/pull/1458/files#r1271312933). I assume the regtest-HRP should just also be set to “tsp” here (as it’s currently stated in the BIP)? Otherweise the limit has to be raised by one.
TLDR; at the time regtest was created, it was assumed that using a separate HRP wouldn’t cost anything.
If each of the four had a different one, I’d get it, but why does regtest have a different one than the other two testing networks?
This actually makes sense to me. Testnet and Signet are “global” networks, so everyone has to agree on the address format. Regtest is always used in a private context, and is specific to Bitcoin Core. Having a separate HRP helps differentiate the local vs networked usage IMO.
Unless I’m forgetting about some good reason for that to be there, I might recommend that you just use tsp for all three
If we decided to make the HRP for regtest throughout Bitcoin Core to be the same as signet and testnet, then I’m fine with this. What I don’t want is for silent payment addresses to be a special case in the functional test framework.
I’ve updated the BIP to recommend upping the character limit to 1023, as having a limit of 117 would cause issues with forward compatibility with future silent payment addresses. I’ve also removed regtest from the BIP, as I don’t think it’s correct to define Bitcoin Core specific conventions in BIPs. Regarding “tsp” vs “sprt”, I don’t really have a strong opinion here, but would prefer to follow the convention in Bitcoin Core. If we end up moving Bitcoin Core to use the same HRP for all networks, then we should update the silent payments HRP to do the same.
As long as testnet and signet are the same, and different from mainnet, I’m happy. When testing with e.g. hardware wallets it lets you use the testnet app / firmware instead of signet-specific stuff.
murchandamus
commented at 7:48 pm on August 16, 2023:
@josibake: It’s rather that bech32(m) addresses are the odd ones out. Before native segwit outputs all addresses across testnet and regtest were the same. After rereading #12314, I would still recommend that you use the same hrp for all test-networks.
josibake
commented at 2:44 pm on September 11, 2023:
In the interest of keeping this PR well scoped, I think we should go for consistency. Since silent payment addresses are bech32m encoded, we should follow the convention currently used for bech32(m), and if we do decided to change the convention for bech32(m) hrp’s in the future, then it should change for all of them (including silent payments)
josibake force-pushed
on Jul 23, 2023
in
src/wallet/silentpayments.h:2
in
1d75df2aaeoutdated
c75d9de0683a91151eb4a508cb64a8937ca92 s/PAYMENT_H/PAYMENTS_H/ in ifndef/define/endif or rename the files to silentpayment.{h,cpp}
0$ test/lint/lint-include-guards.py
1src/wallet/silentpayments.h seems to be missing the expected include guard:
2#ifndef BITCOIN_WALLET_SILENTPAYMENTS_H3#define BITCOIN_WALLET_SILENTPAYMENTS_H4 ...
5#endif // BITCOIN_WALLET_SILENTPAYMENTS_H
In 56882622faf469b6f948f79a69c3c8ddbde92ff8 “crypto: update CKey, CPubkey for silent payments”
We shouldn’t be copying secret data out of a secure structure into an insecure one. When tweaked_seckey is destroyed, the underlying data may still linger in memory and end up being read by something else.
If this function must return a new CKey, then it should instantiate it here and perform the operation on it’s data() rather than doing this copy.
However, I would prefer if this function mirrored the libsecp one where the tweaking occurs on and modified this CKey itself.
aureleoules
commented at 10:36 pm on July 27, 2023:
65b2dec1e39ea4efb327457563d2b18d49b4bc4c: Use an initializer list for data members
in
src/wallet/silentpayments.cpp:71
in
dc18efa78doutdated
65+ std::vector<CKey> raw_tr_keys;
66+ do {
67+ // We haven't removed anything yet on this pass and if we don't remove anything, we didn't find
68+ // any silent payment outputs and should stop checking
69+ removed = false;
70+ std::pair<CKey, CPubKey> tweakResult = CreateOutput(output_index);
aureleoules
commented at 10:39 pm on July 27, 2023:
65b2dec1e39ea4efb327457563d2b18d49b4bc4c: Could use structured binding
in
src/wallet/silentpayments.cpp:259
in
dc18efa78doutdated
Wrap this function in a more generic “Decode” function that doesn’t have size restrictions, possibly use an ENUM for indicating what sort of error guarantees you want.
ismaelsadeeq
commented at 9:29 pm on August 15, 2023:
nit:
I know you describe why the max_size is 1024 in commit message.
But will it be nice if you add a comment about the 1024 / 1023max_size.
josibake
commented at 2:39 pm on September 11, 2023:
This has been re-written to use a character limit enum.
To improve maintainability and support forward compatibility, consider abstracting the handling of different versions into a separate helper function like ‘evalCheckSig’ does. This approach will allow for easier modification and extension for new versions of Silent addresses, and would also be more clear how version handling is done
Thanks for the suggestion, I’ll take a look at EvalCheckSig!
josibake
commented at 2:26 pm on September 11, 2023:
Took a look at EvalCheckSig and while I think this is a good idea in general, I’d prefer to leave it as a follow-up for now. My reasoning here is we currently only have a v0 silent payments version and I’d like to keep the code simple and focused on the V0 rules. As a follow-up, or when we add support for a future v1 version is likely a better time to introduce more complex code for handling multiple versions.
rename to TweakAdd to more closely mirror the secp function
in
src/key.cpp:211
in
376a9b9ad4outdated
206+ return new_seckey;
207+}
208+
209+// we dont want to hash the output of ECDH yet, so we provide a custom hash function to secp256k1_ecdh which
210+// returns the serialized public key without hashing it
211+int custom_ecdh_hash_function(unsigned char *output, const unsigned char *x32, const unsigned char *y32, void *data) {
ismaelsadeeq
commented at 9:24 pm on August 15, 2023:
Coming from review club:
Should also be moved to secp256k1 right
josibake
commented at 2:39 pm on September 11, 2023:
This has been re-written so there is no longer a stand-alone function, but also yes, I think this could be upstreamed into libsecp256k1 at some point.
in
src/key.cpp:234
in
376a9b9ad4outdated
229+
230+// WARNING: DO NOT USE THIS FUNCTION OUTSIDE OF SILENT PAYMENTS
231+// This function returns the un-hashed ECDH result and is specific to silent payments.
232+// If ECDH is required for a different use-case, a separate function should be used which does not use
233+// the `custom_ecdh_hash_function` and instead returns the hashed ECDH result (as is standard practice)
234+CPubKey CKey::SilentPaymentECDH(const CPubKey& pubkey) const
This shouldn’t return private keys, it should only return the tweak portion of the private key. This also means Recipient doesn’t need to have the spend private key.
this should be its own commit, might not be needed if we instead V0SilentPaymentDestination as a CTxDestination type
Sjors
commented at 3:06 pm on August 8, 2023:
member
Maybe point out in the description how the first commit 8283ca76e4feb37d3514fbd8dc491b8c4965dff1 relates to BIP 324, which doesn’t use the ecdh module, but ElligatorSwift instead.
8be91169396a560bfc4a4e8b34a242111704f8e9: maybe split this up a bit more: e.g. one commit to introduce a dummy SilentPaymentRecipient / Sender class, one that adds ComputeECDHSharedSecret, CreateOutput, etc. And then build up the tests across these commits.
37b0ca1c3d753cc61e0debf8ea59160bdf4127ec: alternatively you could scan for the exact pubkey length. That would tolerate future versions to add extra stuff to the address that’s safe to ignore for older versions.
josibake
commented at 2:26 pm on September 11, 2023:
37b0ca1c3d753cc61e0debf8ea59160bdf4127ec: could use version == 0 && silent_payment_data.size() < SILENT_PAYMENT_V0_DATA_SIZE (see comment above)
josibake
commented at 2:29 pm on September 11, 2023:
I think this should fail if an address says it is v0 but has a data part larger than what is defined for v0. If an address has a version > v0 and data part > V0_SIZE, then only the first 66 bytes of the data part are read, which is what we want for the forward compatibility bit.
37b0ca1c3d753cc61e0debf8ea59160bdf4127ecassert data.size() >= SILENT_PAYMENT_V0_DATA_SIZE (or early return if you don’t expect the caller to have already checked this)
328951d056e939b6db573043fc71d8bd6ae68dff: if cheap, could assert/assume both CPubKey are valid.
josibake
commented at 2:35 pm on September 11, 2023:
This now uses the src/key_io.cpp::Encode function, which takes a V0SilentPaymentDestination object, so should be good. If we do want an assert, we can/should do it when creating the V0SilentPaymentDestination object
328951d056e939b6db573043fc71d8bd6ae68dff: this should be in some bech32 helper function.
josibake
commented at 2:37 pm on September 11, 2023:
Not totally convinced we do want to wrap this in a helper function, but perhaps that can be a follow-up looking at how we handle bech32 across WitnessV0, WitnessV1 and V0SilentPaymentsDestinations
Sjors
commented at 9:01 am on August 9, 2023:
member
I think this PR should also contain IsInputForSharedSecretDerivation that’s currently in the send PR, as well as SumInputPubKeys (not sure where that’s introduced). More generally: anything that’s critical in deriving the correct key from a transaction, given its inputs. (this would also make it easier to build an index based on just this PR, see #28241)
josibake force-pushed
on Aug 10, 2023
DrahtBot added the label
CI failed
on Aug 10, 2023
josibake force-pushed
on Aug 13, 2023
maflcko
commented at 9:21 am on August 17, 2023:
member
Maybe mark as draft while CI is red and this is based on other pulls?
DrahtBot added the label
Needs rebase
on Aug 17, 2023
Sjors
commented at 11:53 am on August 21, 2023:
member
Sjors
commented at 9:06 am on August 29, 2023:
member
In order to build the index PR #28241 directly on top of this and, more importantly, have more complete coverage of BIP 352 here, it would be useful to move SumInputPubKeys and HashOutpoints from the send / receive PR’s here. These define which types of inputs are considered and how outpoints must be sorted and hashed.
(update: mmm, looking at the latest test vectors, maybe this functionality is already in here, just named differently)
josibake
commented at 9:07 am on August 29, 2023:
member
Fixed CI and fuzzing errors, will be responding to unaddressed review comments
josibake force-pushed
on Aug 30, 2023
DrahtBot removed the label
CI failed
on Aug 30, 2023
josibake force-pushed
on Aug 30, 2023
josibake force-pushed
on Aug 31, 2023
josibake
commented at 1:08 pm on August 31, 2023:
member
In order to build the index PR #28241 directly on top of this and, more importantly, have more complete coverage of BIP 352 here, it would be useful to move SumInputPubKeys and HashOutpoints from the send / receive PR’s here. These define which types of inputs are considered and how outpoints must be sorted and hashed.
(update: mmm, looking at the latest test vectors, maybe this functionality is already in here, just named differently)
I think the only thing missing now is IsInputForSharedSecretDerivation; I’ll work on pulling that in next
josibake
commented at 3:03 pm on August 31, 2023:
member
fanquake referenced this in commit
238d29aff9
on Sep 7, 2023
manjaneqx approved
josibake force-pushed
on Sep 8, 2023
Frank-GER referenced this in commit
086142fb64
on Sep 8, 2023
josibake force-pushed
on Sep 11, 2023
josibake force-pushed
on Sep 11, 2023
josibake force-pushed
on Sep 11, 2023
josibake
commented at 2:55 pm on September 11, 2023:
member
I think I’ve managed to address all the outstanding review comments, ready for another round of review!
josibake
commented at 3:12 pm on September 11, 2023:
member
In order to build the index PR #28241 directly on top of this and, more importantly, have more complete coverage of BIP 352 here, it would be useful to move SumInputPubKeys and HashOutpoints from the send / receive PR’s here. These define which types of inputs are considered and how outpoints must be sorted and hashed.
(update: mmm, looking at the latest test vectors, maybe this functionality is already in here, just named differently)
This PR should have everything you need now. I’ve added a function GetSilentPaymentTweakDataFromTxInputs, which iterates over the transaction inputs, determines if they are eligible for silent payments and returns the sum of the input pubkeys, along with the hash of the tx outpoints.
The reason it returns the pubkey sum and outpoint hash separately is because if you are using this function to scan as a receiver, you want to calculate outpoint_hash * scan_priv_key first, and then do the ECDH step. If you’re using this function to create an index, however, you’d want sum_pubkeys * outpoint_hash before storing it in the index.
achow101
commented at 6:36 pm on September 11, 2023:
In 7b9f48450738c42d934280881d9ff2dfb5419725 “crypto: update CKey, CPubkey for silent payments”
Currently CKey::TweakAdd modifies in place, while CPubKey::TweakAdd returns a new CPubKey. This can be confusing as I would expect these functions to do the same things to their respective objects rather than behave slightly differently.
josibake
commented at 9:00 am on September 12, 2023:
Fixed, and also updated CKey::TweakAdd,TweakMultiply to return a bool (instead of an assert + void), as this better matches the CKey/CPubKey api
in
src/key_io.cpp:174
in
3235eb8c63outdated
163+ }
164+ auto version = dec.data[0]; // retrieve the version
165+ if (version == 0 && data.size() != SILENT_PAYMENT_V0_DATA_SIZE) {
166+ error_str = strprintf("Silent payment version is v0 but data is not the correct size (expected %d, got %d).", SILENT_PAYMENT_V0_DATA_SIZE, data.size());
167+ return CNoDestination();
168+ }
achow101
commented at 6:43 pm on September 11, 2023:
In 3235eb8c63ea055c68fdf59bb20afd6f06f294e4 “Add V0SilentPaymentDestination address type”
What about unknown versions?
josibake
commented at 6:19 am on September 12, 2023:
Since SP addresses are forward-compatible, we don’t care: we always read the first 66 bytes as two pubkeys and ignore the rest. However, we do need a check here for v31 or greater, as this indicates a backward incompatible change
josibake
commented at 8:59 am on September 12, 2023:
Added a check for version >= 31
in
src/key_io.cpp:76
in
3235eb8c63outdated
71+ // The data_in is scan_pubkey + spend_pubkey
72+ std::vector<unsigned char> data_in = {};
73+ data_in.reserve(66);
74+ // Set 0 as the silent payments version
75+ std::vector<unsigned char> data_out = {0};
76+ data_out.reserve(67);
achow101
commented at 6:46 pm on September 11, 2023:
In 3235eb8c63ea055c68fdf59bb20afd6f06f294e4 “Add V0SilentPaymentDestination address type”
data_out contains result of ConvertBits which expands each 8-bit byte into 5-bit chunks. So the expected size is 67 * 8 / 5 = 107.2. So data_out should be reserving 108 bytes rather than 67.
josibake
commented at 8:51 am on September 12, 2023:
Fixed and added a comment
in
src/wallet/silentpayments.cpp:95
in
644ff77be8outdated
achow101
commented at 7:04 pm on September 11, 2023:
In 644ff77be819a2dea15be0daad10fca8524a18f4 “Implement BIP352: Silent Payments”
Since we aren’t modifying txin.scriptWitness.stack after checking for the annex, this may not actually be the control block. We need to remove the annex if it was there.
0const std::vector<unsigned char>& control = txin.scriptWitness.stack.at(post_annex_size -1);
josibake
commented at 8:51 am on September 12, 2023:
Done
in
src/wallet/silentpayments.h:22
in
644ff77be8outdated
josibake
commented at 8:50 am on September 12, 2023:
Done
in
src/wallet/test/silentpayment_tests.cpp:140
in
78d1426faboutdated
135+ CPubKey sum_input_pub_keys = CPubKey::Combine(input_pub_keys);
136+
137+ const auto expected_addresses = expected["addresses"].getValues();
138+ // We know there is only one address, but if we support labels, this could be multiple addresses
139+ CPubKey ecdh_pubkey = ComputeECDHSharedSecret(scan_priv_key, sum_input_pub_keys, HashOutpoints(outpoints));
140+ std::vector<uint256> found_tweaks = GetTxOutputTweaks(spend_priv_key.GetPubKey(), ecdh_pubkey, output_pub_keys);
achow101
commented at 7:34 pm on September 11, 2023:
In 78d1426fabd7b5b6cdd28de508a1692fce291174 “Add BIP352 test vectors as unit tests”
josibake
commented at 6:27 am on September 12, 2023:
Maybe? If we want to know if the actual objects themselves are equal, then yes, but if we wan’t a more conceptual “are these the same transaction output,” then I’d argue no?
in
src/wallet/test/silentpayment_tests.cpp:31
in
78d1426faboutdated
26+ key_parent.SetSeed(seed);
27+ for (auto index : path) {
28+ BOOST_CHECK(key_parent.Derive(key_child, index));
29+ std::swap(key_parent, key_child);
30+ }
31+ return key_parent.key;
achow101
commented at 7:47 pm on September 11, 2023:
In 78d1426fabd7b5b6cdd28de508a1692fce291174 “Add BIP352 test vectors as unit tests”
No need to have two CExtKeys and swapping them, you can just derive a key into itself.
0 CExtKey key;
1 key.SetSeed(seed);
2 for (auto index : path) {
3 BOOST_CHECK(key.Derive(key, index));
4 }
5 return key.key;
josibake
commented at 8:50 am on September 12, 2023:
Done
achow101
commented at 8:32 pm on September 11, 2023:
member
Currently, this PR is dangerous by itself as it will accept silent payment addresses for sending, but fail to actually send the coins to the correct script. It just uses an empty script. We should disallow sending to silent payment addresses in this PR, and enable it in the sending PR.
theStack
commented at 9:20 pm on September 11, 2023:
in commit c14ad74f8eeaa744d831a77654164c856a86fdee: this change in MessageVerify seems unrelated to the WitnessUnknown changes (and AFAICT, even after the later changes in CTxDestination it’s not needed, but just another way to express the same logic).
josibake
commented at 6:32 am on September 12, 2023:
This is a fix for an -Wuninitialized warning, see #28246 (review)
in
src/addresstype.h:172
in
621b1ca6fboutdated
122- * P2PKH, P2SH, P2WPKH, and P2WSH scripts.
123+ * Parse a scriptPubKey for the destination.
124+ *
125+ * For standard scripts that have addresses (and P2PK as an exception), a corresponding CTxDestination
126+ * is assigned to addressRet.
127+ * For all other scripts. addressRet is assigned as a CNoDestination containing the scriptPubKey.
theStack
commented at 9:22 pm on September 11, 2023:
in commit 621b1ca6fbdf66ccf9da84c38fcc3b18de9e95fe: punctuation nit:
0 * For all other scripts, addressRet is assigned as a CNoDestination containing the scriptPubKey.
josibake
commented at 6:33 am on September 12, 2023:
These commits belong to #28246 , might be better to leave a review there
theStack
commented at 9:42 am on September 12, 2023:
Oops, indeed! It might be a good idea to read PR descriptions before starting a review 😅
in
src/key.cpp:200
in
7b9f484507outdated
196+// If ECDH is required for a different use-case, use a function which hashes the result (as is standard practice).
197+CPubKey CKey::UnhashedECDH(const CPubKey& pubkey) const
198+{
199+ unsigned char shared_secret[33];
200+
201+ secp256k1_pubkey ecdh_pubkey;
theStack
commented at 9:48 pm on September 11, 2023:
in commit 7b9f48450738c42d934280881d9ff2dfb5419725: maybe assert here that both the private key and the passed pubkey are valid?
(using IsFullyValid for the pubkey would be a more thorough check, but that’s more expensive and could hurt scanning performance I guess)
josibake
commented at 6:39 am on September 12, 2023:
Hm, it seems a bit odd to assert that the passed pubkey is valid? I think it’s safe to assume that validation was done upstream by the caller. Also, I agree that we want to avoid doing expensive checks here as this function is called during scanning.
I’ll add the assert for the private key, though, as this seems in keeping with what the other code with private keys is doing.
josibake
commented at 8:49 am on September 12, 2023:
Added assert(fValid) check for CKey and also made the functions consistent with the rest of the CPubKey/CKey functions by returning a bool instead of an assert inside a void function
theStack
commented at 9:53 pm on September 11, 2023:
is this method used anywhere?
josibake
commented at 6:40 am on September 12, 2023:
Good catch! This is dead code from an old version of the PR, will remove it
josibake
commented at 8:36 am on September 12, 2023:
Fixed
in
src/bech32.h:36
in
6c19662156outdated
27@@ -28,6 +28,17 @@ enum class Encoding {
28 BECH32M, //!< Bech32m encoding as defined in BIP350
29 };
3031+/** Character limits for bech32(m) encoded strings. Character limits are how we provide error location guarantees.
32+ * These values should never exceed 2^31 - 1 (max value for a 32-bit int), since there are places where we may need to
33+ * convert the CharLimit::VALUE to an int. In practice, this should never happen since this CharLimit applies to an address encoding
34+ * and we would never encode an address with such a massive value */
35+enum CharLimit : size_t {
36+ NO_LIMIT = 0, //!< Allows for Bech32(m) encoded strings of arbitruary size. No guarantees on the number of errors detected
theStack
commented at 9:55 pm on September 11, 2023:
typo nit:
0 NO_LIMIT = 0, //!< Allows for Bech32(m) encoded strings of arbitrary size. No guarantees on the number of errors detected
josibake
commented at 8:37 am on September 12, 2023:
Fixed
josibake force-pushed
on Sep 12, 2023
josibake force-pushed
on Sep 12, 2023
josibake force-pushed
on Sep 12, 2023
josibake force-pushed
on Sep 12, 2023
josibake
commented at 9:06 am on September 12, 2023:
member
git range-diff 0cf23d0...d053c3a
Addressed review comments from @theStack and @achow101 and added a commit to disable sending to silent payment addresses, while still allowing the functions to be used in the unit tests.
josibake
commented at 9:06 am on September 12, 2023:
member
Currently, this PR is dangerous by itself as it will accept silent payment addresses for sending, but fail to actually send the coins to the correct script. It just uses an empty script. We should disallow sending to silent payment addresses in this PR, and enable it in the sending PR.
josibake
commented at 9:08 am on September 12, 2023:
member
Next step: add documentation for the new functions, requested in #28122 (review)
DrahtBot removed the label
CI failed
on Sep 12, 2023
josibake force-pushed
on Sep 12, 2023
in
src/wallet/silentpayments.cpp:92
in
93ff4ce117outdated
87+
88+std::vector<uint256> GetTxOutputTweaks(const CPubKey& spend_pubkey, const CPubKey& ecdh_pubkey, std::vector<XOnlyPubKey> output_pub_keys, const std::pair<uint256, CPubKey> change_label)
89+{
90+ // Scan first for any outputs to us, and then check if there is any change
91+ std::vector<uint256> tweaks = GetTxOutputTweaks(spend_pubkey, ecdh_pubkey, output_pub_keys);
92+ std::vector<uint256> labeled_tweaks = GetTxOutputTweaks(change_label.second, ecdh_pubkey, output_pub_keys);
achow101
commented at 6:29 pm on September 12, 2023:
In 93ff4ce117d4b85128dd55630d2dc9f8efbfb473 “Implement BIP352: Silent Payments”
In the current receiving PR, change_label is a pair of the change label and it’s corresponding curve point. However this is expecting it as the pair of the change label and the resulting spend pubkey.
DrahtBot added the label
CI failed
on Sep 12, 2023
josibake force-pushed
on Sep 13, 2023
josibake
commented at 9:56 am on September 13, 2023:
member
git range-diff c9dff7e...5829940 changes /*compressed=*/ to /*fCompressedIn=*/ to fix a tidy warning
DrahtBot removed the label
CI failed
on Sep 13, 2023
josibake force-pushed
on Sep 14, 2023
josibake
commented at 3:18 pm on September 14, 2023:
member
Pulled in support for scanning with labels (h/t @achow101) and updated the unit tests. Major changes:
Add new functions for generating a change label and labeled silent payments address, per BIP352
Update GetTxOutputTweaks to scan with labels
Refactor CPubKey to get rid of Combine in favor of operator+
Ready for another round of review!
DrahtBot added the label
CI failed
on Sep 15, 2023
DrahtBot added the label
Needs rebase
on Sep 19, 2023
josibake force-pushed
on Sep 20, 2023
DrahtBot removed the label
Needs rebase
on Sep 20, 2023
DrahtBot removed the label
CI failed
on Sep 20, 2023
josibake force-pushed
on Sep 21, 2023
josibake force-pushed
on Sep 21, 2023
DrahtBot added the label
CI failed
on Sep 21, 2023
DrahtBot removed the label
CI failed
on Sep 21, 2023
Sjors
commented at 8:45 am on September 28, 2023:
member
@josibake can you add the following helper functions?
CouldBeSilentPayment(CTransactionRef tx): checks that it’s not a coinbase and that at least one of the outputs is taproot
See #28241 (review)
With current implementation, we can’t have test vectors for sending that include non-eligible sp inputs. This is because we just sum all the private keys without checking if the input is eligible. I propose to extract eligibility checking as a function from GetSilentPaymentsTweakDataFromTxInputs so we can use it here in the unit test.
in
src/wallet/silentpayments.cpp:233
in
699577c911outdated
According to the BIP we should skip the whole tx if there’s unknown witness input. As currently, implemented it will silently skip just that input and attempt to proceed. I added a test vector for unknown witness version
DrahtBot added the label
CI failed
on Nov 25, 2023
josibake force-pushed
on Dec 8, 2023
josibake force-pushed
on Dec 8, 2023
josibake
commented at 6:43 pm on December 8, 2023:
member
rebased on master and updated to fix a silent merge conflict in the tests, related to the new Txid type.
DrahtBot removed the label
CI failed
on Dec 8, 2023
josibake
commented at 1:11 pm on December 11, 2023:
member
@josibake can you add the following helper functions?
* [ ] `CouldBeSilentPayment(CTransactionRef tx)`: checks that it's not a coinbase and that at least one of the outputs is taproot
See [Silent payment index (for light wallets and consistency check) [#28241](/bitcoin-bitcoin/28241/) (comment)](https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1331881346)
* [ ] `bool GetSilentPaymentsTweakedPubKeySum(std::vector<CTxIn>, std::map<COutPoint, Coin>, CPubKey& tweaked_key) nodiscard` (with clear documentation on what to do when it returns `false`). Although relatively straight-forward, my current index implementation does look a bit brittle, and I'm not sure how to handle failure cases: https://github.com/bitcoin/bitcoin/pull/28241/files#diff-708eeba963ec57793dbec2309d27f32869d4ccefa14fbeb575d3a1e36e5e1a03R74-R85
Maybe also make a type alias for std::map<COutPoint, Coin>
the second function you’re asking for exists, just need to expose it in the header file. it also returns pair<uint256, CPubKey>, so you would need to multiply those together (using the TweakMultiply method on CPubKey) before storing in the index.
For the first function, we can probably roll that into the second one? Need to look at the code again to see if that makes sense.
Regarding the alias, I’m somewhat ambivalent. Since we use this same struct in other places unrelated to silent payments, it might make sense as a follow-up refactor. Less inclined to do it here.
DrahtBot added the label
CI failed
on Jan 12, 2024
josibake force-pushed
on Jan 15, 2024
josibake force-pushed
on Jan 15, 2024
DrahtBot removed the label
CI failed
on Jan 15, 2024
josibake force-pushed
on Jan 15, 2024
josibake force-pushed
on Jan 16, 2024
DrahtBot added the label
CI failed
on Jan 18, 2024
DrahtBot
commented at 0:20 am on January 18, 2024:
contributor
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
Adds a test case for taproot inputs with H as the internal key (with and without the optional annex) (h/t @Eunovo)
Splits out the pubkey extraction logic from GetSilentPaymentTweakDataFromTxInputs function into its own function: ExtractPubKeyFromInput
Updates the sending tests to check if the input is a silent payments input before using the secret key, using the ExtractPubKeyFromInput function
Going forward, it might make more sense to have a function that tests if the input is eligible (bool IsInputForSharedSecretDerivation, and a separate function to extract the pubkey. Both sending and receiving need the bool IsInputForSharedSecretDerivation function, but only the receiver would need the pubkey extraction function. Right now, it’s using the extract function for both by checking if a pubkey was extracted or not, which seems kinda clunky but works for now.
DrahtBot removed the label
CI failed
on Jan 19, 2024
in
src/wallet/silentpayments.cpp:281
in
5e04b72b4coutdated
276+ input_pubkeys.emplace_back(stack.back());
277+ } else if (type == TxoutType::SCRIPTHASH) {
278+ // Check if the redeemScript is P2WPKH
279+ CScript redeem_script{stack.back().begin(), stack.back().end()};
280+ TxoutType rs_type = Solver(redeem_script, solutions);
281+ if (rs_type == TxoutType::WITNESS_V0_KEYHASH) {
@josibake p2sh-p2pkh or p2sh-p2pk are not addressed in the BIP. While I understand they are not commonly used scripts but Solver(redeem_script, solutions) can still detect p2pkh or p2pk in the redeem_script, so, they are technically “Standard”. Is there anything against adding support for these scripts? assuming that we ignore non-standard or malleated p2pkh in the redeem script
josibake
commented at 5:08 pm on January 24, 2024:
The goal is not to support every possible script. Rather, we want to limit it to a sane subset to avoid technical debt and keep the implementation simple. In fact, the only reason for including P2PKH at all is due to the fact it is still widely used and makes up a significant percentage of the UTXO set, otherwise we would have only allowed Segwit script templates as this gives us non-malleability guarantees.
Add doxygen style comments to src/wallet/silentpayments.h
Make GetTxOutputTweaks return a std::optional to cover the case where no payments to us are found
DrahtBot removed the label
Needs rebase
on Jan 26, 2024
in
src/wallet/silentpayments.h:30
in
de7dd3aaf6outdated
86+/**
87+ * @brief Generate silent payment taproot destinations.
88+ *
89+ * Given a set of silent payment destinations, generate the requested number of outputs. If a silent payment
90+ * destination is repeated, this indicates multiple outputs are requested for the same recipient. The silent payment
91+ * desintaions are passed in map where the key indicates their desired position in the final tx.vout array.
Not sure protocol implementation needs to now about the position in tx.vout array. AFAIK, output calculation doesn’t depend on the position. Have you considered passing destinations one by one together with the counter if a destination is repeated?
josibake
commented at 12:50 pm on January 31, 2024:
Output calculation doesn’t depend on the position, but using output position makes the implementation much simpler. I did try something similar to what you suggested but it ended up being really complicated to make sure I was matching up all the right amounts and was always implicitly relying on output position. @achow101 pointed out it would be better and simpler to just use the output index explicitly.
This also doesn’t create any dependence on output position. The user can still order the outputs however they want after generating the silent payment scriptPubKeys.
in
src/wallet/silentpayments.h:49
in
de7dd3aaf6outdated
106+ * @brief Get silent payment tweak data from transaction inputs.
107+ *
108+ * Get the necessary data from the transaction inputs to be able to scan the transaction outputs for silent payment outputs.
109+ * This requires knowledge of the prevout scriptPubKey, which is passed via `coins`.
110+ *
111+ * If the transaction has silent payment eligible inputs, the public keys from these inputs are returned as a sum along with
I’m too dumb to understand why do we need to return sum on input pubkeys together with the input hash. Does it have something to do with index and light clients?
josibake
commented at 10:53 am on January 31, 2024:
Yep! There are two ways to use this data:
Immediately, as (b_scan * input_hash) * sum_input_pubkeys. This is one scalar multiplication and one EC multiplication.
Store it for later (either for wallet re-scans or to serve to light clients), as input_hash * sum_input_pubkeys. This is one EC multiplication and allows us to store all of the input data needed as 33 bytes.
If the function instead returned input_hash * sum_input_pubkeys, we would end up doing two EC multiplications for case 1 (i.e. (input_hash * sum_input_pubkeys) * b_scan)
S3RK
commented at 8:36 am on January 31, 2024:
contributor
Started reviewing. Basic question first. Which RPCs are supposed to work at this stage? I was under impression, that none RPCs should understand SP addresses yet and the support come with send and receiving PRs. But since you’ve registered SP destination in CTXDestination variant, the SP addresses would be recognised by some RPCs. Is that intended? Should we verify that they return sane results?
josibake
commented at 12:58 pm on January 31, 2024:
member
Started reviewing. Basic question first. Which RPCs are supposed to work at this stage? I was under impression, that none RPCs should understand SP addresses yet and the support come with send and receiving PRs.
Thanks for the review @S3RK ! The goal isn’t that none of the RPCs should understand SP addresses, rather the goal is that full RPC support is deferred to the send and receiving PRs.
But since you’ve registered SP destination in CTXDestination variant, the SP addresses would be recognised by some RPCs. Is that intended? Should we verify that they return sane results?
In “wallet: disable sending to silent payment address” https://github.com/bitcoin/bitcoin/pull/28122/commits/ed75ce5362ab69c5968b85e42c45683519325626 SP addresses are marked as invalid by the ValidDestinationVisitor (which should cover all of the RPCs where an address can be passed) and validateaddress returns a message that this is a silent payment address, but support is not implemented yet.
josibake force-pushed
on Feb 19, 2024
josibake force-pushed
on Feb 19, 2024
DrahtBot added the label
CI failed
on Feb 19, 2024
DrahtBot
commented at 4:32 pm on February 19, 2024:
contributor
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
josibake
commented at 4:47 pm on February 19, 2024:
member
I’ve updated this to use https://github.com/bitcoin-core/secp256k1/pull/1471 for the cryptography needed for silent payments. This way, we don’t need make any changes to CKey/CPubKey and keeps all of the low-level cryptography in libsecp256k1. This simplifies this PR to creating the necessary wrappers for the libsecp silent payments module and implementing the non-cryptography parts of the protocol (i.e. the client code).
Assuming I still need this for the index #28241, I still prefer if this wasn’t part of the wallet. Afaik nothing in this file has a dependency on the wallet, so the easiest change would to be move the file to src/common.
josibake
commented at 10:17 am on February 20, 2024:
Now that we have src/bip352.h, it might make more sense to get rid of src/wallet/silentpayents.h and put everything in src/bip352.h. In the future, anything that is wallet specific can go in src/wallet/silentpayments.h and you’d have everything you need for an index in src/bip352.h. Thoughts?
DrahtBot removed the label
CI failed
on Feb 23, 2024
DrahtBot added the label
Needs rebase
on Apr 6, 2024
josibake force-pushed
on Apr 7, 2024
josibake force-pushed
on Apr 22, 2024
josibake force-pushed
on Apr 22, 2024
DrahtBot added the label
CI failed
on Apr 22, 2024
DrahtBot
commented at 4:14 pm on April 22, 2024:
contributor
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
7a27533332d14cf98f7e165f464142132e9f9d85: if you’re going to change this here instead of adding a custom sort for BIP352, then you should add a note that it’s critical for BIP352.
However I’m not sure if we should sort this way by default. If I wanted to list transaction outputs, I’d want to sorted by their integer output position.
That said, I don’t know if we currently have any RPC where this sort order is exposed.
From what I could tell, we don’t expose this anywhere. I’m in favor of making this the default since we only ever really talk about outpoints in terms of their serialised encoding. Gonna give it some more thought, but I’m considering opening this commit as its own PR. It seems preferable to me to have one way of sorting outpoints vs creating a special case for silent payments, especially when there doesn’t seem to be any other “sort outpoints” use case (e.g. none of the tests failed when I made this change).
DrahtBot removed the label
CI failed
on May 5, 2024
achow101 referenced this in commit
4877fcdb42
on May 17, 2024
DrahtBot added the label
Needs rebase
on May 20, 2024
achow101 referenced this in commit
55cf34a5c3
on Jun 5, 2024
Sjors
commented at 11:44 am on July 9, 2024:
member
Now might be a good time to rebase this and the send and receive branches? The “pre-work” commits mentioned in the description are merged and the libsecp PR seems to be in reasonable shape.
josibake force-pushed
on Jul 15, 2024
DrahtBot removed the label
Needs rebase
on Jul 15, 2024
in
src/common/bip352.cpp:347
in
667e71e456outdated
AFAICT this assert would lead to a crash if a tx is scanned with a taproot output where the x-only-pubkey is not on the curve (note that such outputs adhere to standardness rules, i.e. it’s trivial to create such a tx, and probably there are already a good amount of them on mainnet). Proposed fix, not tested yet:
0 if (!secp256k1_xonly_pubkey_parse(secp256k1_context_static, &tx_output_obj, txoutputs[i].data())) {
1 continue;
2 }
For consistency reasons, the extension of the found_output_{objs,ptrs} vectors should happen after this part, so that all vectors end up with the same size.
Sjors
commented at 4:54 pm on July 16, 2024:
member
Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
DrahtBot removed the label
CI failed
on Apr 4, 2025
josibake force-pushed
on Apr 7, 2025
josibake force-pushed
on May 12, 2025
josibake force-pushed
on May 13, 2025
DrahtBot added the label
Needs rebase
on May 13, 2025
josibake force-pushed
on May 14, 2025
Squashed 'src/secp256k1/' changes from 4187a46649..c0db6509bd
c0db6509bd docs: update README
8339232b7e ci: enable silentpayments module
635745fc3a tests: add constant time tests
b1de2ee2f7 tests: add BIP-352 test vectors
aea372837f silentpayments: add benchmarks for scanning
1ec7857aed silentpayments: add examples/silentpayments.c
c9bec084eb silentpayments: receiving
28fd17d7c4 silentpayments: recipient label support
065e8b7793 silentpayments: sending
a6d8b11754 build: add skeleton for new silentpayments (BIP352) module
6274359346 bench: add ellswift to bench help output
0258186573 configure: Show exhaustive tests in summary
53b578d10b include: remove WARN_UNUSED_RESULT for functions always returning 1
f75c985604 ci: Fix exiting from ci.sh on error
947761b842 tests: remove unused uncounting_illegal_callback_fn
5d01f375c6 build: Drop no longer needed `-fvisibility=hidden` compiler option
dbf1e95d2a ci: Run `tools/symbol-check.py`
8174c88f47 test: Add `tools/symbol-check.py`
8a287f9a32 Introduce `SECP256K1_LOCAL_VAR` macro
7106544a16 Remove deprecated _ec_privkey_{negate,tweak_add,tweak_mul} aliases
1e2da62eff gha: Print all *.log files, in a separate action
REVERT: 4187a46649 Merge bitcoin-core/secp256k1#1492: tests: Add Wycheproof ECDH vectors
REVERT: e266ba11ae tests: Add Wycheproof ECDH vectors
REVERT: 13906b7154 Merge bitcoin-core/secp256k1#1669: gitignore: Add Python cache files
REVERT: c1bcb03276 gitignore: Add Python cache files
REVERT: 70f149b9a1 Merge bitcoin-core/secp256k1#1662: bench: add ellswift to bench help output
REVERT: 6b3fe51fb6 bench: add ellswift to bench help output
REVERT: d84bb83e26 Merge bitcoin-core/secp256k1#1661: configure: Show exhaustive tests in summary
REVERT: 3f54ed8c1b Merge bitcoin-core/secp256k1#1659: include: remove WARN_UNUSED_RESULT for functions always returning 1
REVERT: 20b05c9d3f configure: Show exhaustive tests in summary
REVERT: e56716a3bc Merge bitcoin-core/secp256k1#1660: ci: Fix exiting from ci.sh on error
REVERT: d87c3bc58f ci: Fix exiting from ci.sh on error
REVERT: 1b6e081538 include: remove WARN_UNUSED_RESULT for functions always returning 1
REVERT: 2abb35b034 Merge bitcoin-core/secp256k1#1657: tests: remove unused uncounting_illegal_callback_fn
REVERT: 51907fa918 tests: remove unused uncounting_illegal_callback_fn
REVERT: a7a5117144 Merge bitcoin-core/secp256k1#1359: Fix symbol visibility issues, add test for it
REVERT: 13ed6f65dc Merge bitcoin-core/secp256k1#1593: Remove deprecated `_ec_privkey_{negate,tweak_add,tweak_mul}` aliases from API
REVERT: d1478763a5 build: Drop no longer needed `-fvisibility=hidden` compiler option
REVERT: 8ed1d83d92 ci: Run `tools/symbol-check.py`
REVERT: 41d32ab2de test: Add `tools/symbol-check.py`
REVERT: 88548058b3 Introduce `SECP256K1_LOCAL_VAR` macro
REVERT: 03bbe8c615 Merge bitcoin-core/secp256k1#1655: gha: Print all *.log files, in a separate action
REVERT: 59860bcc24 gha: Print all *.log files, in a separate action
REVERT: 37d2c60bec Remove deprecated _ec_privkey_{negate,tweak_add,tweak_mul} aliases
git-subtree-dir: src/secp256k1
git-subtree-split: c0db6509bd2cb0777ce0d335e2582f74364fb8ec
497f1536ef
Merge commit '497f1536ef9774ce0dd7d2225a5d79cc3811c0f8' into refresh-secp256k17cf15ce8fd
crypto: add read-only method to KeyPair
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
84cc673356
wallet: disable sending to silent payment address
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.
d766390f8d
tests: add BIP352 test vectors as unit tests
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
51b6e003d2
josibake force-pushed
on May 14, 2025
DrahtBot added the label
CI failed
on May 14, 2025
DrahtBot
commented at 11:28 am on May 14, 2025:
contributor
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/42203729137
LLM reason (✨ experimental): The CI failure is due to a lint check failure related to subtree merging.
Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
DrahtBot removed the label
Needs rebase
on May 14, 2025
DrahtBot removed the label
CI failed
on May 14, 2025
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-06-07 18:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
