Silent Payments: Implement BIP352

josibake commented at 6:36 pm on July 21, 2023: member

This PR is part of integrating silent payments into Bitcoin Core. The project is tracked in #28536

Based on https://github.com/bitcoin-core/secp256k1/pull/1519

Pre-work / Refactors

The first three commits are pre-work / refactors needed for this PR. They are broken out into the following PRS:

BIP352

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.

Before reviewing, it is strongly recommended you read https://github.com/bitcoin/bips/pull/1458 and take a look at the reference python implementation on the BIP.

DrahtBot commented at 6:36 pm on July 21, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30562 (PayToAnchor(P2A) followups by instagibbs)
#30352 (policy: Add PayToAnchor(P2A), OP_1 <0x4e73> as a standard output script for spending by instagibbs)
#30051 (crypto, refactor: add new KeyPair class by josibake)
#29432 (Stratum v2 Template Provider (take 3) by Sjors)
#29295 (CKey: add Serialize and Unserialize by Sjors)
#28201 (Silent Payments: sending by josibake)

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:536 in 6d11e3c435 outdated

510@@ -508,6 +511,7 @@ class CRegTestParams : public CChainParams
511         base58Prefixes[EXT_SECRET_KEY] = {0x04, 0x35, 0x83, 0x94};
512 
513         bech32_hrp = "bcrt";
514+        silent_payment_hrp = "sprt";

theStack commented at 4:10 pm on July 22, 2023:

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.

Fun fact: in #27827 the “sprt” HRP works, but this is because the pubkeys are encoded as x-only (see commit https://github.com/bitcoin/bitcoin/pull/27827/commits/3a0d081ee07f3c65220a45855c96bf35ac2643fd), i.e. the payload of the address is only 64 bytes instead of 66 bytes and hence the limit is not exceeded.

murchandamus commented at 3:43 pm on August 2, 2023:

Can someone remind me wtf regtest has a different hrp than testnet and signet?

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?

Unless I’m forgetting about some good reason for that to be there, I might recommend that you just use tsp for all three

josibake commented at 8:41 am on August 3, 2023:

This issue has some helpful context: #12314 (comment)

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.

josibake commented at 9:56 am on August 3, 2023:

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.

Sjors commented at 4:40 pm on August 8, 2023:

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 1d75df2aae outdated

0@@ -0,0 +1,56 @@
1+#ifndef BITCOIN_WALLET_SILENTPAYMENT_H
2+#define BITCOIN_WALLET_SILENTPAYMENT_H

jonatack commented at 5:27 pm on July 23, 2023:

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_H
3  #define BITCOIN_WALLET_SILENTPAYMENTS_H
4  ...
5  #endif // BITCOIN_WALLET_SILENTPAYMENTS_H

josibake commented at 9:11 am on July 26, 2023:

Fixed, thanks!

josibake force-pushed on Jul 26, 2023

DrahtBot removed the label CI failed on Jul 26, 2023

in src/key.cpp:182 in 56882622fa outdated

173@@ -172,6 +174,79 @@ bool CKey::Negate()
174     return secp256k1_ec_seckey_negate(secp256k1_context_sign, keydata.data());
175 }
176 
177+CKey CKey::AddTweak(const unsigned char *tweak32) const
178+{
179+    assert(fValid);
180+
181+    unsigned char tweaked_seckey[32];
182+    memcpy(tweaked_seckey, data(), 32);

achow101 commented at 8:16 pm on July 26, 2023:

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.

josibake commented at 1:04 pm on August 31, 2023:

Now operates on keydata.data() in place

in src/key.cpp:198 in 56882622fa outdated

193+CKey CKey::MultiplyTweak(const unsigned char *tweak32) const
194+{
195+    assert(fValid);
196+
197+    unsigned char tweaked_seckey[32];
198+    memcpy(tweaked_seckey, data(), 32);

achow101 commented at 8:17 pm on July 26, 2023:

In 56882622faf469b6f948f79a69c3c8ddbde92ff8 “crypto: update CKey, CPubkey for silent payments”

Same data copying concerns as AddTweak.

josibake commented at 1:04 pm on August 31, 2023:

Fixed

in src/pubkey.h:215 in dc18efa78d outdated

210@@ -211,6 +211,12 @@ class CPubKey
211      */
212     static bool CheckLowS(const std::vector<unsigned char>& vchSig);
213 
214+    //! Add a number of public keys together.
215+    static CPubKey Combine(std::vector<CPubKey> pubkeys);

aureleoules commented at 10:33 pm on July 27, 2023:

56882622faf469b6f948f79a69c3c8ddbde92ff8

0    static CPubKey Combine(const std::vector<CPubKey> &pubkeys);

josibake commented at 11:32 am on September 11, 2023:

Fixed, and made pass by reference.

in src/wallet/silentpayments.cpp:70 in dc18efa78d outdated

13+{
14+    m_scan_seckey = scan_seckey;
15+    m_scan_pubkey = CPubKey{m_scan_seckey.GetPubKey()};
16+    m_spend_seckey = spend_seckey;
17+    m_spend_pubkey = CPubKey{m_spend_seckey.GetPubKey()};
18+}

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 dc18efa78d outdated

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 dc18efa78d outdated

87+
88+Sender::Sender(const CKey& scalar_ecdh_input, const SilentPaymentRecipient& recipient)
89+{
90+    m_ecdh_pubkey = scalar_ecdh_input.SilentPaymentECDH(recipient.m_scan_pubkey);
91+    m_recipient = recipient;
92+}

aureleoules commented at 10:39 pm on July 27, 2023:

65b2dec1e39ea4efb327457563d2b18d49b4bc4c: Could use an initializer list

in src/wallet/silentpayments.cpp:89 in dc18efa78d outdated

104+
105+std::vector<wallet::CRecipient> Sender::GenerateRecipientScriptPubKeys() const
106+{
107+    int n = 0;
108+    std::vector<wallet::CRecipient> spks;
109+    for (const auto& pair : m_recipient.m_outputs) {

aureleoules commented at 10:42 pm on July 27, 2023:

65b2dec1e39ea4efb327457563d2b18d49b4bc4c: Could use structured binding

0    for (const auto& [pubkey, amount]: m_recipient.m_outputs) {

in src/wallet/silentpayments.cpp:153 in dc18efa78d outdated

148+    std::map<CPubKey, std::vector<std::pair<CPubKey, CAmount>>> recipient_groups;
149+    std::vector<SilentPaymentRecipient> recipients;
150+    for (const auto& destination : silent_payment_destinations) {
151+        recipient_groups[destination.m_scan_pubkey].emplace_back(destination.m_spend_pubkey, destination.m_amount);
152+    }
153+    for (const auto& pair : recipient_groups) {

aureleoules commented at 10:43 pm on July 27, 2023:

65b2dec1e39ea4efb327457563d2b18d49b4bc4c: Could use structured binding

aureleoules commented at 8:55 am on July 28, 2023: member

Left some comments. I will review further later.

DrahtBot added the label CI failed on Jul 29, 2023

josibake force-pushed on Aug 1, 2023

josibake commented at 3:48 pm on August 1, 2023: member

Needs rebase, if still relevant

thanks, fixed!

DrahtBot removed the label CI failed on Aug 1, 2023

josibake force-pushed on Aug 2, 2023

in src/bech32.cpp:374 in a469cb1e55 outdated

369@@ -370,11 +370,13 @@ std::string Encode(Encoding encoding, const std::string& hrp, const data& values
370 }
371 
372 /** Decode a Bech32 or Bech32m string. */
373-DecodeResult Decode(const std::string& str) {
374+DecodeResult Decode(const std::string& str, bool silent) {
375+    std::size_t max_size = silent ? 1024 : 90;

josibake commented at 10:38 am on August 3, 2023:

Note to self, this should be 1023

josibake commented at 9:41 am on August 8, 2023:

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 / 1023 max_size.

josibake commented at 2:39 pm on September 11, 2023:

This has been re-written to use a character limit enum.

in src/key_io.cpp:341 in 8ee791e7b3 outdated

336+    std::vector<unsigned char> silent_payment_data;
337+    silent_payment_data.reserve(((dec.data.size() - 1) * 5) / 8);
338+    if (!ConvertBits<5, 8, false>([&](unsigned char c) { silent_payment_data.push_back(c); }, dec.data.begin() + 1, dec.data.end())) {
339+        return {};
340+    }
341+    if ((version == 0 && silent_payment_data.size() != SILENT_PAYMENT_V0_DATA_SIZE) || silent_payment_data.size() < SILENT_PAYMENT_V0_DATA_SIZE) return {};

BrandonOdiwuor commented at 5:41 am on August 4, 2023:

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

josibake commented at 3:26 pm on August 6, 2023:

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.

in src/key.cpp:177 in 376a9b9ad4 outdated

173@@ -172,6 +174,79 @@ bool CKey::Negate()
174     return secp256k1_ec_seckey_negate(secp256k1_context_sign, keydata.data());
175 }
176 
177+CKey CKey::AddTweak(const unsigned char *tweak32) const

josibake commented at 9:15 am on August 8, 2023:

rename to TweakAdd to more closely mirror the secp function

in src/key.cpp:211 in 376a9b9ad4 outdated

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) {

josibake commented at 9:16 am on August 8, 2023:

this should also be rename since we aren’t hashing

josibake commented at 9:27 am on August 8, 2023:

move this to the UnhashedECDH function

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 376a9b9ad4 outdated

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

josibake commented at 9:19 am on August 8, 2023:

Should probably rename this to just “UnhashedECDH”

in src/key_io.cpp:327 in 37b0ca1c3d outdated

322+    CPubKey spend_pubkey{spend_pubkey_data};
323+
324+    return {scan_pubkey, spend_pubkey};
325+}
326+
327+std::vector<unsigned char> DecodeSilentAddress(const std::string& str)

josibake commented at 10:08 am on August 8, 2023:

Similar to “Decode”, there should be a higher-level function that first decodes the string and then passes it to the relevant decoding function

josibake commented at 10:15 am on August 8, 2023:

Even better, should probably first add a new destination type to CTxDestination

josibake commented at 1:06 pm on August 31, 2023:

Resolved in #28246 , which adds a new CTxDestination and lets us use the existing encoder and decoder.

in src/wallet/silentpayments.cpp:13 in 8be9116939 outdated

 7+#include <undo.h>
 8+#include <logging.h>
 9+
10+namespace wallet {
11+
12+Recipient::Recipient(const CKey& scan_seckey, const CKey& spend_seckey)

josibake commented at 10:34 am on August 8, 2023:

This doesn’t need to be a struct, could just be functions which the DescScriptPubKeyMan calls as needed

achow101 commented at 8:57 pm on August 14, 2023:

https://github.com/achow101/bitcoin/commit/57d72b52ca5a442982907cb09c8b26e1ab5e9d59 can be squashed in here to remove the Recipient class.

in src/wallet/silentpayments.cpp:51 in 8be9116939 outdated

45+std::pair<CPubKey,CPubKey> Recipient::GetAddress() const
46+{
47+    return {m_scan_pubkey, m_spend_pubkey};
48+}
49+
50+std::vector<CKey> Recipient::ScanTxOutputs(std::vector<XOnlyPubKey> output_pub_keys)

josibake commented at 10:38 am on August 8, 2023:

this function should be “label aware,” meaning if the wallet implements labels later on, this function does not need to change.

josibake commented at 1:10 pm on August 31, 2023:

Leaving this alone for now. This can be easily updated to add label support when it makes sense.

in src/wallet/silentpayments.cpp:86 in 8be9116939 outdated

80+                return true;
81+            }
82+            return false;
83+        }), output_pub_keys.end());
84+    } while (!output_pub_keys.empty() && removed);
85+    return raw_tr_keys;

josibake commented at 10:44 am on August 8, 2023:

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.

in src/wallet/types.h:74 in 8be9116939 outdated

69+    CScript scriptPubKey;
70+    CAmount nAmount;
71+    bool fSubtractFeeFromAmount;
72+};
73+
74+struct V0SilentPaymentDestination

josibake commented at 10:45 am on August 8, 2023:

this should be its own commit

in src/wallet/wallet.h:291 in 8be9116939 outdated

261@@ -262,13 +262,6 @@ inline std::optional<AddressPurpose> PurposeFromString(std::string_view s)
262     return {};
263 }
264 
265-struct CRecipient

josibake commented at 10:45 am on August 8, 2023:

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.

in src/key_io.cpp:321 in 37b0ca1c3d outdated

316+{
317+
318+    std::vector<unsigned char> scan_pubkey_data(data.begin(), data.begin() + 33);
319+    CPubKey scan_pubkey{scan_pubkey_data};
320+
321+    std::vector<unsigned char> spend_pubkey_data(data.begin() + 33, data.end());

Sjors commented at 4:44 pm on August 8, 2023:

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:

Done

in src/key_io.cpp:341 in 37b0ca1c3d outdated

336+    std::vector<unsigned char> silent_payment_data;
337+    silent_payment_data.reserve(((dec.data.size() - 1) * 5) / 8);
338+    if (!ConvertBits<5, 8, false>([&](unsigned char c) { silent_payment_data.push_back(c); }, dec.data.begin() + 1, dec.data.end())) {
339+        return {};
340+    }
341+    if ((version == 0 && silent_payment_data.size() != SILENT_PAYMENT_V0_DATA_SIZE) || silent_payment_data.size() < SILENT_PAYMENT_V0_DATA_SIZE) return {};

Sjors commented at 4:46 pm on August 8, 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.

in src/key_io.cpp:315 in 37b0ca1c3d outdated

310@@ -309,3 +311,33 @@ bool IsValidDestinationString(const std::string& str)
311 {
312     return IsValidDestinationString(str, Params());
313 }
314+
315+std::pair<CPubKey, CPubKey> DecodeSilentData(const std::vector<unsigned char>& data)

Sjors commented at 4:47 pm on August 8, 2023:

37b0ca1c3d753cc61e0debf8ea59160bdf4127ec assert data.size() >= SILENT_PAYMENT_V0_DATA_SIZE (or early return if you don’t expect the caller to have already checked this)

in src/key_io.cpp:346 in 328951d056 outdated

340@@ -341,3 +341,21 @@ std::vector<unsigned char> DecodeSilentAddress(const std::string& str)
341     if ((version == 0 && silent_payment_data.size() != SILENT_PAYMENT_V0_DATA_SIZE) || silent_payment_data.size() < SILENT_PAYMENT_V0_DATA_SIZE) return {};
342     return silent_payment_data;
343 }
344+
345+std::string EncodeSilentDestination(const CPubKey& scan_pubkey, const CPubKey& spend_pubkey)
346+{

Sjors commented at 4:49 pm on August 8, 2023:

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

in src/key_io.cpp:355 in 328951d056 outdated

350+    std::vector<unsigned char> data_out = {0};
351+
352+    data_in.insert(data_in.end(), scan_pubkey.begin(), scan_pubkey.end());
353+    data_in.insert(data_in.end(), spend_pubkey.begin(), spend_pubkey.end());
354+
355+    ConvertBits<8, 5, true>([&](unsigned char c) { data_out.push_back(c); }, data_in.begin(), data_in.end());

Sjors commented at 4:50 pm on August 8, 2023:

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

#28244 was merged

josibake force-pushed on Aug 22, 2023

josibake commented at 6:46 am on August 22, 2023: member

#28244 was merged

Awesome! I rebased on #28246, will respond to outstanding review comments today/tomorrow.

DrahtBot removed the label Needs rebase on Aug 22, 2023

josibake force-pushed on Aug 22, 2023

DrahtBot removed the label CI failed on Aug 22, 2023

DrahtBot added the label CI failed on Aug 22, 2023

josibake force-pushed on Aug 28, 2023

in src/wallet/silentpayments.h:30 in e3f9217ffb outdated

37+        SilentPaymentRecipient m_recipient;
38+        CPubKey CreateOutput(const CPubKey& spend_pubkey, const uint32_t output_index) const;
39+
40+    public:
41+        Sender(const CKey& scalar_ecdh_input, const SilentPaymentRecipient& recipient);
42+        std::vector<wallet::CRecipient> GenerateRecipientScriptPubKeys() const;

achow101 commented at 9:42 pm on August 28, 2023:

Please don’t use CRecipient here, it causes a circular dependency.

josibake commented at 1:06 pm on August 31, 2023:

Fixed

josibake force-pushed on Aug 29, 2023

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

@hebasto can you rerun the Win64 CI task?

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 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.

josibake force-pushed on Sep 11, 2023

in src/pubkey.cpp:276 in 7b9f484507 outdated

271+    unsigned char pubkey_bytes[COMPRESSED_SIZE];
272+    size_t publen = COMPRESSED_SIZE;
273+    return_val = secp256k1_ec_pubkey_serialize(secp256k1_context_static, pubkey_bytes, &publen, &original_pubkey, SECP256K1_EC_COMPRESSED);
274+    assert(return_val);
275+
276+    return CPubKey(pubkey_bytes);

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 3235eb8c63 outdated

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 3235eb8c63 outdated

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 644ff77be8 outdated

90+                // Check for annex
91+                bool has_annex = txin.scriptWitness.stack.back()[0] == ANNEX_TAG;
92+                size_t post_annex_size = txin.scriptWitness.stack.size() - (has_annex ? 1 : 0);
93+                if (post_annex_size > 1) {
94+                    // Actually a script path spend
95+                    const std::vector<unsigned char>& control = txin.scriptWitness.stack.back();

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.

0                    const 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 644ff77be8 outdated

13+std::optional<std::pair<uint256, CPubKey>> GetSilentPaymentsTweakDataFromTxInputs(const std::vector<CTxIn>& vin, const std::map<COutPoint, Coin>& coins);
14+
15+CPubKey CreateOutput(const CKey& ecdh_scalar, const CPubKey& scan_pubkey, const CPubKey& spend_pubkey, const uint32_t output_index);
16+uint256 HashOutpoints(const std::vector<COutPoint>& tx_outpoints);
17+std::map<size_t, WitnessV1Taproot> GenerateSilentPaymentTaprootDestinations(const CKey& ecdh_scalar, const std::map<size_t, V0SilentPaymentDestination>& sp_dests);
18+CKey PrepareScalarECDHInput(const std::vector<std::pair<CKey, bool>>& sender_secret_keys, const std::vector<COutPoint>& tx_outpoints);

achow101 commented at 7:07 pm on September 11, 2023:

In 644ff77be819a2dea15be0daad10fca8524a18f4 “Implement BIP352: Silent Payments”

It would be nice to have some documentation for these functions.

in src/wallet/silentpayments.cpp:14 in 644ff77be8 outdated

 9+#include <logging.h>
10+#include <pubkey.h>
11+#include <policy/policy.h>
12+#include <script/solver.h>
13+#include <util/check.h>
14+#include <wallet/wallet.h>

achow101 commented at 7:26 pm on September 11, 2023:

In 644ff77be819a2dea15be0daad10fca8524a18f4 “Implement BIP352: Silent Payments”

wallet/wallet.h does not need to be included.

josibake commented at 8:51 am on September 12, 2023:

Done

in src/wallet/test/silentpayment_tests.cpp:47 in 78d1426fab outdated

42+        for (const auto& sender : vec["sending"].getValues()) {
43+            const auto& given = sender["given"];
44+            const auto& expected = sender["expected"];
45+
46+            std::vector<COutPoint> outpoints;
47+            for (const auto& outpoint : sender["given"]["outpoints"].getValues()) {

achow101 commented at 7:29 pm on September 11, 2023:

In 78d1426fabd7b5b6cdd28de508a1692fce291174 “Add BIP352 test vectors as unit tests”

nit: s/sender["given"]/given

0            for (const auto& outpoint : given["outpoints"].getValues()) {

josibake commented at 8:50 am on September 12, 2023:

Done

DrahtBot added the label CI failed on Sep 11, 2023

in src/wallet/test/silentpayment_tests.cpp:105 in 78d1426fab outdated

100+
101+            const auto& given = recipient["given"];
102+            const auto& expected = recipient["expected"];
103+
104+            std::vector<COutPoint> outpoints;
105+            for (const auto& outpoint : recipient["given"]["outpoints"].getValues()) {

achow101 commented at 7:32 pm on September 11, 2023:

In 78d1426fabd7b5b6cdd28de508a1692fce291174 “Add BIP352 test vectors as unit tests”

nit: s/recipient["given"]/given

0            for (const auto& outpoint : given["outpoints"].getValues()) {

josibake commented at 8:50 am on September 12, 2023:

Done

in src/wallet/test/silentpayment_tests.cpp:140 in 78d1426fab outdated

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”

nit: s/spend_priv_key.GetPubKey()/spend_pubkey

0            std::vector<uint256> found_tweaks = GetTxOutputTweaks(spend_pubkey, ecdh_pubkey, output_pub_keys);

in src/wallet/wallet.h:297 in 78d1426fab outdated

290@@ -291,6 +291,11 @@ struct CRecipient
291     CTxDestination dest;
292     CAmount nAmount;
293     bool fSubtractFeeFromAmount;
294+
295+    friend bool operator==(const CRecipient& a, const CRecipient& b)
296+    {
297+        return a.dest == b.dest && a.nAmount == b.nAmount;

achow101 commented at 7:37 pm on September 11, 2023:

In 78d1426fabd7b5b6cdd28de508a1692fce291174 “Add BIP352 test vectors as unit tests”

Shouldn’t this also check sffo too?

0        return a.dest == b.dest && a.nAmount == b.nAmount && a.fSubtractFeeFromAmount == b.SubtractFeeFromAmount;

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 78d1426fab outdated

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.

in src/util/message.cpp:50 in c14ad74f8e outdated

46@@ -47,7 +47,7 @@ MessageVerificationResult MessageVerify(
47         return MessageVerificationResult::ERR_PUBKEY_NOT_RECOVERED;
48     }
49 
50-    if (!(CTxDestination(PKHash(pubkey)) == destination)) {
51+    if (!(PKHash(pubkey) == *std::get_if<PKHash>(&destination))) {

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:160 in 621b1ca6fb outdated

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 7b9f484507 outdated

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?

0    assert(fValid);
1    assert(pubkey.IsValid());
2    secp256k1_pubkey ecdh_pubkey;

(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

in src/pubkey.h:306 in 7b9f484507 outdated

293@@ -287,10 +294,19 @@ class XOnlyPubKey
294     bool operator!=(const XOnlyPubKey& other) const { return m_keydata != other.m_keydata; }
295     bool operator<(const XOnlyPubKey& other) const { return m_keydata < other.m_keydata; }
296 
297+    CPubKey ConvertToCompressedPubKey(bool even = true) const

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 6c19662156 outdated

27@@ -28,6 +28,17 @@ enum class Encoding {
28     BECH32M, //!< Bech32m encoding as defined in BIP350
29 };
30 
31+/** 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 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.

Fixed in https://github.com/bitcoin/bitcoin/pull/28122/commits/d053c3af90324df4fddc57c8301fcf414019d726

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 93ff4ce117 outdated

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

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)
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>

josibake force-pushed on Oct 3, 2023

josibake commented at 9:13 am on October 3, 2023: member

force-pushed to fix a silent merge conflict with #28500

in src/wallet/test/silentpayment_tests.cpp:52 in 30bbeb3804 outdated

47+            for (const auto& outpoint : given["outpoints"].getValues()) {
48+                outpoints.emplace_back(uint256S(outpoint[0].get_str()), outpoint[1].getInt<uint32_t>());
49+            }
50+
51+            std::vector<std::pair<CKey, bool>> sender_secret_keys;
52+            for (const auto& key : given["input_priv_keys"].getValues()) {

S3RK commented at 3:16 pm on October 10, 2023:

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 699577c911 outdated

228+        }), output_pub_keys.end());
229+    } while (!output_pub_keys.empty() && keep_going);
230+    return tweaks;
231+}
232+
233+std::optional<std::pair<uint256, CPubKey>> GetSilentPaymentsTweakDataFromTxInputs(const std::vector<CTxIn>& vin, const std::map<COutPoint, Coin>& coins)

S3RK commented at 10:20 am on October 19, 2023:

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 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

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.

Debug: https://github.com/bitcoin/bitcoin/runs/20533630775

josibake force-pushed on Jan 19, 2024

josibake commented at 10:21 am on January 19, 2024: member

Rebased https://github.com/bitcoin/bitcoin/commit/0299dfb0607b85813c7226690772ab3be198b0df -> https://github.com/bitcoin/bitcoin/commit/6a9ec4771d691bf4143f3574dd129da641c6ee74

Fixes a silent merge conflict introduced by changing CKey to use std::byte* instead of unsigned char

josibake force-pushed on Jan 19, 2024

josibake commented at 10:57 am on January 19, 2024: member

Updated https://github.com/bitcoin/bitcoin/commit/6a9ec4771d691bf4143f3574dd129da641c6ee74 -> https://github.com/bitcoin/bitcoin/pull/28122/commits/6e7c29528eb4a7c39bf8290b7c622e30bf07b45d (implement-bip352-00 -> implement-bip352-01, compare)

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 5e04b72b4c outdated

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) {

Eunovo commented at 4:51 am on January 24, 2024:

@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.

Eunovo commented at 6:20 am on January 25, 2024:

Understood. Thanks for clearing that up @josibake

josibake force-pushed on Jan 24, 2024

willcl-ark added the label Wallet on Jan 24, 2024

willcl-ark added the label Privacy on Jan 24, 2024

DrahtBot added the label Needs rebase on Jan 26, 2024

josibake force-pushed on Jan 26, 2024

josibake commented at 1:54 pm on January 26, 2024: member

Rebased https://github.com/bitcoin/bitcoin/commit/e0ea52da97b5bb18b813e862455aafd56a7809dd -> https://github.com/bitcoin/bitcoin/commit/08b62764190a907c0d7d329f4df07f9a2b6dcdeb

Fix small configure.ac merge conflict

josibake force-pushed on Jan 26, 2024

josibake commented at 2:06 pm on January 26, 2024: member

Updated https://github.com/bitcoin/bitcoin/commit/08b62764190a907c0d7d329f4df07f9a2b6dcdeb -> https://github.com/bitcoin/bitcoin/commit/cbb8d3a6574e10841c52716bbcd27e0c60752e1c (implement-bip352-01-rebase -> implement-bip352-02 , compare)

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 de7dd3aaf6 outdated

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.

S3RK commented at 8:30 am on January 31, 2024:

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 de7dd3aaf6 outdated

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

S3RK commented at 8:32 am on January 31, 2024:

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

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.

Debug: https://github.com/bitcoin/bitcoin/runs/21735671954

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).

Putting this PR in draft for now, until https://github.com/bitcoin-core/secp256k1/pull/1471 merges.

josibake marked this as a draft on Feb 19, 2024

in src/wallet/silentpayments.h:57 in b885ac0ddd outdated

52+ *
53+ * @param vin                                              The transaction inputs.
54+ * @param coins                                            The coins (potentially) spent in this transaction.
55+ * @return std::optional<std::pair<uint256, PubTweakData>> The silent payment tweak data, or nullopt if not found.
56+ */
57+std::optional<BIP352::PubTweakData> GetSilentPaymentTweakDataFromTxInputs(const std::vector<CTxIn>& vin, const std::map<COutPoint, Coin>& coins);

Sjors commented at 9:30 am on February 20, 2024:

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?

Sjors commented at 2:15 pm on February 20, 2024:

Moving to bip352.h sounds good too.

josibake force-pushed on Feb 20, 2024

josibake force-pushed on Feb 21, 2024

josibake force-pushed on Feb 23, 2024

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

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.

Debug: https://github.com/bitcoin/bitcoin/runs/24112092548

DrahtBot removed the label Needs rebase on Apr 22, 2024

josibake force-pushed on Apr 26, 2024

in src/primitives/transaction.h:50 in 7a27533332 outdated

43@@ -43,7 +44,10 @@ class COutPoint
44 
45     friend bool operator<(const COutPoint& a, const COutPoint& b)
46     {
47-        return std::tie(a.hash, a.n) < std::tie(b.hash, b.n);
48+        DataStream ser_a, ser_b;
49+        ser_a << a;
50+        ser_b << b;
51+        return Span{ser_a} < Span{ser_b};

Sjors commented at 10:34 am on April 26, 2024:

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.

josibake commented at 10:38 am on April 26, 2024:

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).

josibake commented at 10:42 am on April 26, 2024:

But still need to verify re: displayed in RPCs. At that point tho, we could just sort by vout and ignore the txid in the sort

Sjors commented at 11:34 am on April 26, 2024:

Separate PR sounds good to get more eyes on it. Meanwhile this is fine a temporary fix for this PR.

josibake commented at 9:57 am on May 6, 2024:

opened in https://github.com/bitcoin/bitcoin/pull/30046

josibake commented at 4:22 pm on May 6, 2024:

Closed #30046 :smile:

josibake force-pushed on Apr 27, 2024

Sjors commented at 8:35 am on May 2, 2024: member

@josibake can you add a link to https://github.com/bitcoin-core/secp256k1/pull/1519 in the PR description? It currently takes a few “load more” clicks and searching to see what this PR depends on.

josibake force-pushed on May 5, 2024

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.

Squashed 'src/secp256k1/' changes from 4af241b320..00b0cb19a9

00b0cb19a9 docs: update README
54b8bc8ec6 ci: enable silentpayments module
96bd71fb8a tests: add BIP-352 test vectors
c30bc013fe silentpayments: add benchmark for `scan_outputs`
91b1b3365b silentpayments: add examples/silentpayments.c
b4475ea80c silentpayments: receiving
23c7aead63 silentpayments: recipient label support
79562d0cd1 silentpayments: sending
35f91359b8 build: add skeleton for new silentpayments (BIP352) module
0055b86780 Merge bitcoin-core/secp256k1#1551: Add ellswift usage example
ea2d5f0f17 Merge bitcoin-core/secp256k1#1563: doc: Add convention for defaults
ca06e58b2c Merge bitcoin-core/secp256k1#1564: build, ci: Adjust the default size of the precomputed table for signing
e2af491263 ci: Switch to the new default value of the precomputed table for signing
d94a9273f8 build: Adjust the default size of the precomputed table for signing
fcc5d7381b Merge bitcoin-core/secp256k1#1565: cmake: Bump CMake minimum required version up to 3.16
9420eece24 cmake: Bump CMake minimum required version up to 3.16
16685649d2 doc: Add convention for defaults
a5269373fa Merge bitcoin-core/secp256k1#1555: Fixed O3 replacement
b8fe33332b cmake: Fixed O3 replacement
31f84595c4 Add ellswift usage example
fe4fbaa7f3 examples: fix case typos in secret clearing paragraphs (s/, Or/, or/)

git-subtree-dir: src/secp256k1
git-subtree-split: 00b0cb19a97718dfaab70aa7505ff157f22a31bd

5a0b27cf3c

Merge commit '5a0b27cf3c122ef9b9caea5727beaea2a9172442' into refresh-secp256k1 6ddbfefcee

crypto: Add a KeyPair wrapper class

The wallet returns an untweaked internal key for taproot outputs. If the
output commits to a tree of scripts, this key needs to be tweaked with
the merkle root. Even if the output does not commit to a tree of
scripts, BIP341/342 recommend commiting to a hash of the public key.

Previously, this logic for applying the taptweak was implemented in the
::SignSchnorr method.

Move this tweaking and signing logic to a new class, KeyPair, and add
a method to CKey for computing a KeyPair, CKey::ComputeKeyPair. This
class is a wrapper for the `secp256k1_keypair` type.

The motivation is to be able to use the the tweaked internal key outside
of signing, e.g. in silent payments when retreiving the private key before
ECDH. Having the KeyPair class is also a general improvement in that we
almost always convert to `secp256k1_keypair` objects when using taproot
private keys with libsecp256k1.

Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>

bf8d718db8

tests: add test for KeyPair

Reuse existing bip340 test vectors.

7ea05d0502

conf: add ECDH,SILENTPAYMENTS secp256k1 modules 1ca9ce13da

Add "sp" HRP 92a4c8a401

Add V0SilentPaymentDestination address type 10fb7e0def

common: add bip352.{h,cpp} secp256k1 module

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

667e71e456

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.

21b6093017

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

211dca1190

josibake force-pushed on Jul 15, 2024

DrahtBot removed the label Needs rebase on Jul 15, 2024

in src/common/bip352.cpp:321 in 667e71e456 outdated

316+        secp256k1_silentpayments_found_output found_output{};
317+        secp256k1_xonly_pubkey tx_output_obj;
318+        found_output_objs.push_back(found_output);
319+        found_output_ptrs.push_back(&found_output_objs[i]);
320+        ret = secp256k1_xonly_pubkey_parse(secp256k1_context_static, &tx_output_obj, tx_outputs[i].data());
321+        assert(ret);

theStack commented at 3:35 pm on July 16, 2024:

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

I think you lost https://github.com/bitcoin/bitcoin/pull/30046/commits/e11d764a6ce72cd571ad9cf818c6918dd16b02f6 in the rebase?

DrahtBot added the label Needs rebase on Aug 2, 2024

DrahtBot commented at 5:59 pm on August 2, 2024: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Silent Payments: Implement BIP352 #28122

Pre-work / Refactors

BIP352

Code Coverage

Reviews

Conflicts