Silent Payments: Implement BIP352 #28122

pull josibake wants to merge 10 commits into bitcoin:master from josibake:implement-bip352 changing 52 files +13818 −266
  1. 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/1698

    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.

  2. 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 & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28122.

    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:

    • #32579 (headerssync: Preempt unrealistic unit test behavior by hodlinator)
    • #31974 (Drop testnet3 by Sjors)
    • #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.

  3. josibake force-pushed on Jul 21, 2023
  4. DrahtBot added the label CI failed on Jul 21, 2023
  5. josibake force-pushed on Jul 21, 2023
  6. in src/kernel/chainparams.cpp:633 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)
  7. josibake force-pushed on Jul 23, 2023
  8. 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!
  9. josibake force-pushed on Jul 26, 2023
  10. DrahtBot removed the label CI failed on Jul 26, 2023
  11. 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
  12. 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
  13. 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.
  14. 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
  15. 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
  16. 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
  17. 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) {
  18. 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
  19. aureleoules commented at 8:55 am on July 28, 2023: contributor
    Left some comments. I will review further later.
  20. DrahtBot added the label CI failed on Jul 29, 2023
  21. josibake force-pushed on Aug 1, 2023
  22. josibake commented at 3:48 pm on August 1, 2023: member

    Needs rebase, if still relevant

    thanks, fixed!

  23. DrahtBot removed the label CI failed on Aug 1, 2023
  24. josibake force-pushed on Aug 2, 2023
  25. 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.
  26. 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.
  27. 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
  28. 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.
  29. 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”
  30. 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.
  31. 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.
  32. 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.
  33. 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.
  34. 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
  35. in src/wallet/wallet.h:288 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
  36. 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.

  37. 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
  38. 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.
  39. 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)
  40. 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
  41. 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
  42. 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)
  43. josibake force-pushed on Aug 10, 2023
  44. DrahtBot added the label CI failed on Aug 10, 2023
  45. josibake force-pushed on Aug 13, 2023
  46. 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?
  47. DrahtBot added the label Needs rebase on Aug 17, 2023
  48. Sjors commented at 11:53 am on August 21, 2023: member
    #28244 was merged
  49. josibake force-pushed on Aug 22, 2023
  50. 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.

  51. DrahtBot removed the label Needs rebase on Aug 22, 2023
  52. josibake force-pushed on Aug 22, 2023
  53. DrahtBot removed the label CI failed on Aug 22, 2023
  54. DrahtBot added the label CI failed on Aug 22, 2023
  55. josibake force-pushed on Aug 28, 2023
  56. josibake force-pushed on Aug 28, 2023
  57. 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
  58. josibake force-pushed on Aug 29, 2023
  59. 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)

  60. josibake commented at 9:07 am on August 29, 2023: member
    Fixed CI and fuzzing errors, will be responding to unaddressed review comments
  61. josibake force-pushed on Aug 30, 2023
  62. DrahtBot removed the label CI failed on Aug 30, 2023
  63. josibake force-pushed on Aug 30, 2023
  64. josibake force-pushed on Aug 31, 2023
  65. 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

  66. josibake commented at 3:03 pm on August 31, 2023: member
    @hebasto can you rerun the Win64 CI task?
  67. fanquake referenced this in commit 238d29aff9 on Sep 7, 2023
  68. manjaneqx approved
  69. josibake force-pushed on Sep 8, 2023
  70. Frank-GER referenced this in commit 086142fb64 on Sep 8, 2023
  71. josibake force-pushed on Sep 11, 2023
  72. josibake force-pushed on Sep 11, 2023
  73. josibake force-pushed on Sep 11, 2023
  74. 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!
  75. 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.

  76. josibake force-pushed on Sep 11, 2023
  77. 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
  78. 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
  79. 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
  80. 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
  81. 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.

  82. 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
  83. 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
  84. DrahtBot added the label CI failed on Sep 11, 2023
  85. 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
  86. 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);
  87. 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?
  88. 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
  89. 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.
  90. 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)
  91. in src/addresstype.h:172 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 😅
  92. 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
  93. 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
  94. 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
  95. josibake force-pushed on Sep 12, 2023
  96. josibake force-pushed on Sep 12, 2023
  97. josibake force-pushed on Sep 12, 2023
  98. josibake force-pushed on Sep 12, 2023
  99. 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.

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

  101. josibake commented at 9:08 am on September 12, 2023: member
    Next step: add documentation for the new functions, requested in #28122 (review)
  102. DrahtBot removed the label CI failed on Sep 12, 2023
  103. josibake force-pushed on Sep 12, 2023
  104. 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.

  105. DrahtBot added the label CI failed on Sep 12, 2023
  106. josibake force-pushed on Sep 13, 2023
  107. 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
  108. DrahtBot removed the label CI failed on Sep 13, 2023
  109. josibake force-pushed on Sep 14, 2023
  110. 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!

  111. DrahtBot added the label CI failed on Sep 15, 2023
  112. DrahtBot added the label Needs rebase on Sep 19, 2023
  113. josibake force-pushed on Sep 20, 2023
  114. DrahtBot removed the label Needs rebase on Sep 20, 2023
  115. DrahtBot removed the label CI failed on Sep 20, 2023
  116. josibake force-pushed on Sep 21, 2023
  117. josibake force-pushed on Sep 21, 2023
  118. DrahtBot added the label CI failed on Sep 21, 2023
  119. DrahtBot removed the label CI failed on Sep 21, 2023
  120. Sjors commented at 8:45 am on September 28, 2023: member

    @josibake can you add the following helper functions?

    Maybe also make a type alias for std::map<COutPoint, Coin>

  121. josibake force-pushed on Oct 3, 2023
  122. josibake commented at 9:13 am on October 3, 2023: member
    force-pushed to fix a silent merge conflict with #28500
  123. 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.
  124. 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
  125. DrahtBot added the label CI failed on Nov 25, 2023
  126. josibake force-pushed on Dec 8, 2023
  127. josibake force-pushed on Dec 8, 2023
  128. 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.
  129. DrahtBot removed the label CI failed on Dec 8, 2023
  130. 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.

  131. DrahtBot added the label CI failed on Jan 12, 2024
  132. josibake force-pushed on Jan 15, 2024
  133. josibake force-pushed on Jan 15, 2024
  134. DrahtBot removed the label CI failed on Jan 15, 2024
  135. josibake force-pushed on Jan 15, 2024
  136. josibake force-pushed on Jan 16, 2024
  137. DrahtBot added the label CI failed on Jan 18, 2024
  138. 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

  139. josibake force-pushed on Jan 19, 2024
  140. 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
  141. josibake force-pushed on Jan 19, 2024
  142. josibake force-pushed on Jan 19, 2024
  143. 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.

  144. DrahtBot removed the label CI failed on Jan 19, 2024
  145. 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
  146. josibake force-pushed on Jan 24, 2024
  147. willcl-ark added the label Wallet on Jan 24, 2024
  148. willcl-ark added the label Privacy on Jan 24, 2024
  149. DrahtBot added the label Needs rebase on Jan 26, 2024
  150. josibake force-pushed on Jan 26, 2024
  152. josibake force-pushed on Jan 26, 2024
  153. 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
  154. DrahtBot removed the label Needs rebase on Jan 26, 2024
  155. 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.

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

    1. Immediately, as (b_scan * input_hash) * sum_input_pubkeys. This is one scalar multiplication and one EC multiplication.
    2. 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)

  157. 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?
  158. 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.

  159. josibake force-pushed on Feb 19, 2024
  160. josibake force-pushed on Feb 19, 2024
  161. DrahtBot added the label CI failed on Feb 19, 2024
  162. 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

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

  164. josibake marked this as a draft on Feb 19, 2024
  165. 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.
  166. josibake force-pushed on Feb 20, 2024
  167. josibake force-pushed on Feb 20, 2024
  168. josibake force-pushed on Feb 21, 2024
  169. josibake force-pushed on Feb 21, 2024
  170. josibake force-pushed on Feb 23, 2024
  171. DrahtBot removed the label CI failed on Feb 23, 2024
  172. DrahtBot added the label Needs rebase on Apr 6, 2024
  173. josibake force-pushed on Apr 7, 2024
  174. josibake force-pushed on Apr 22, 2024
  175. josibake force-pushed on Apr 22, 2024
  176. DrahtBot added the label CI failed on Apr 22, 2024
  177. 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

  178. DrahtBot removed the label Needs rebase on Apr 22, 2024
  179. josibake force-pushed on Apr 26, 2024
  180. josibake force-pushed on Apr 26, 2024
  181. 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:
  182. josibake force-pushed on Apr 27, 2024
  183. 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.
  184. josibake force-pushed on May 5, 2024
  185. josibake force-pushed on May 5, 2024
  186. josibake force-pushed on May 5, 2024
  187. DrahtBot removed the label CI failed on May 5, 2024
  188. achow101 referenced this in commit 4877fcdb42 on May 17, 2024
  189. DrahtBot added the label Needs rebase on May 20, 2024
  190. achow101 referenced this in commit 55cf34a5c3 on Jun 5, 2024
  191. 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.
  192. josibake force-pushed on Jul 15, 2024
  193. DrahtBot removed the label Needs rebase on Jul 15, 2024
  194. in src/common/bip352.cpp:347 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.

  195. 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?
  196. DrahtBot added the label Needs rebase on Aug 2, 2024
  197. josibake commented at 1:30 pm on November 7, 2024: member
    Regarding the @DrahtBot comment, I am still working on this PR and will be rebasing this week, once I’ve finished responding to review feedback in https://github.com/bitcoin-core/secp256k1/pull/1519
  198. josibake force-pushed on Apr 3, 2025
  199. DrahtBot removed the label Needs rebase on Apr 3, 2025
  200. josibake force-pushed on Apr 4, 2025
  201. josibake force-pushed on Apr 4, 2025
  202. josibake force-pushed on Apr 4, 2025
  203. DrahtBot added the label CI failed on Apr 4, 2025
  204. DrahtBot commented at 10:51 am on April 4, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/39980345983

    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.

  205. DrahtBot removed the label CI failed on Apr 4, 2025
  206. josibake force-pushed on Apr 7, 2025
  207. josibake force-pushed on May 12, 2025
  208. josibake force-pushed on May 13, 2025
  209. DrahtBot added the label Needs rebase on May 13, 2025
  210. josibake force-pushed on May 14, 2025
  211. 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
  212. Merge commit '497f1536ef9774ce0dd7d2225a5d79cc3811c0f8' into refresh-secp256k1 7cf15ce8fd
  213. josibake force-pushed on May 14, 2025
  214. DrahtBot added the label CI failed on May 14, 2025
  215. 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.

  216. DrahtBot removed the label Needs rebase on May 14, 2025
  217. DrahtBot removed the label CI failed on May 14, 2025
  218. Squashed 'src/secp256k1/' changes from c0db6509bd..6264c3d093 
    6264c3d093 docs: update README
f825d34260 ci: enable silentpayments module
b821a467e2 tests: add constant time tests
b5b73bcd99 tests: add BIP-352 test vectors
eabeedb752 silentpayments: add benchmarks for scanning
1de8b7e854 silentpayments: add examples/silentpayments.c
ed3a44b10a silentpayments: receiving
3c9362dd6a silentpayments: recipient label support
70e20b7145 silentpayments: sending
cf44324b5e build: add skeleton for new silentpayments (BIP352) module
ad60ef7ea7 Merge bitcoin-core/secp256k1#1689: ci: Convert `arm64` Cirrus tasks to GHA jobs
c498779096 Merge bitcoin-core/secp256k1#1687: cmake: support the use of launchers in ctest -S scripts
0dfe387dbe cmake: support the use of launchers in ctest -S scripts
89096c234d Merge bitcoin-core/secp256k1#1692: cmake: configure libsecp256k1.pc during install
7106dce6fd cmake: configure libsecp256k1.pc during install
29e73f4ba5 Merge bitcoin-core/secp256k1#1685: cmake: Emulate Libtool's behavior on FreeBSD
746e36b141 Merge bitcoin-core/secp256k1#1678: cmake: add a helper for linking into static libs
a28c2ffa5c Merge bitcoin-core/secp256k1#1683: README: add link to musig example
2a9d374735 Merge bitcoin-core/secp256k1#1690: ci: Bump GCC snapshot major version to 16
add146e101 ci: Bump GCC snapshot major version to 16
004f57fcd8 ci: Move Valgrind build for `arm64` from Cirrus to GHA
5fafdfc30f ci: Move `gcc-snapshot` build for `arm64` from Cirrus to GHA
e814b79a8b ci: Switch `arm64_debian` from QEMU to native `arm64` Docker image
bcf77346b9 ci: Add `arm64` architecture to `docker_cache` job
b77aae9226 ci: Rename Docker image tag to reflect architecture
145ae3e28d cmake: add a helper for linking into static libs
819210974b README: add link to musig example, generalize module enabling hint
95db29b144 Merge bitcoin-core/secp256k1#1679: cmake: Use `PUBLIC_HEADER` target property in installation logic
37dd422b5c cmake: Emulate Libtool's behavior on FreeBSD
f24b838bed Merge bitcoin-core/secp256k1#1680: doc: Promote "Building with CMake" to standard procedure
3f31ac43e0 doc: Promote "Building with CMake" to standard procedure
6f67151ee2 cmake: Use `PUBLIC_HEADER` target property
c32715b2a0 cmake, move-only: Move module option processing to `src/CMakeLists.txt`
201b2b8f06 Merge bitcoin-core/secp256k1#1675: cmake: Bump minimum required CMake version to 3.22
3af71987a8 cmake: Bump minimum required CMake version to 3.22
92394476e9 Merge bitcoin-core/secp256k1#1673: Assert field magnitude at control-flow join
3a4f448cb4 Assert field magnitude at control-flow join
9fab425256 Merge bitcoin-core/secp256k1#1668: bench_ecmult: add benchmark for ecmult_const_xonly
05445377f4 bench_ecmult: add benchmark for ecmult_const_xonly
bb597b3d39 Merge bitcoin-core/secp256k1#1670: tests: update wycheproof files
d73ed99479 tests: update wycheproof files
4187a46649 Merge bitcoin-core/secp256k1#1492: tests: Add Wycheproof ECDH vectors
e266ba11ae tests: Add Wycheproof ECDH vectors
13906b7154 Merge bitcoin-core/secp256k1#1669: gitignore: Add Python cache files
c1bcb03276 gitignore: Add Python cache files
70f149b9a1 Merge bitcoin-core/secp256k1#1662: bench: add ellswift to bench help output
6b3fe51fb6 bench: add ellswift to bench help output
d84bb83e26 Merge bitcoin-core/secp256k1#1661: configure: Show exhaustive tests in summary
3f54ed8c1b Merge bitcoin-core/secp256k1#1659: include: remove WARN_UNUSED_RESULT for functions always returning 1
20b05c9d3f configure: Show exhaustive tests in summary
e56716a3bc Merge bitcoin-core/secp256k1#1660: ci: Fix exiting from ci.sh on error
d87c3bc58f ci: Fix exiting from ci.sh on error
1b6e081538 include: remove WARN_UNUSED_RESULT for functions always returning 1
2abb35b034 Merge bitcoin-core/secp256k1#1657: tests: remove unused uncounting_illegal_callback_fn
51907fa918 tests: remove unused uncounting_illegal_callback_fn
a7a5117144 Merge bitcoin-core/secp256k1#1359: Fix symbol visibility issues, add test for it
13ed6f65dc Merge bitcoin-core/secp256k1#1593: Remove deprecated `_ec_privkey_{negate,tweak_add,tweak_mul}` aliases from API
d1478763a5 build: Drop no longer needed  `-fvisibility=hidden` compiler option
8ed1d83d92 ci: Run `tools/symbol-check.py`
41d32ab2de test: Add `tools/symbol-check.py`
88548058b3 Introduce `SECP256K1_LOCAL_VAR` macro
03bbe8c615 Merge bitcoin-core/secp256k1#1655: gha: Print all *.log files, in a separate action
59860bcc24 gha: Print all *.log files, in a separate action
37d2c60bec Remove deprecated _ec_privkey_{negate,tweak_add,tweak_mul} aliases
REVERT: c0db6509bd docs: update README
REVERT: 8339232b7e ci: enable silentpayments module
REVERT: 635745fc3a tests: add constant time tests
REVERT: b1de2ee2f7 tests: add BIP-352 test vectors
REVERT: aea372837f silentpayments: add benchmarks for scanning
REVERT: 1ec7857aed silentpayments: add examples/silentpayments.c
REVERT: c9bec084eb silentpayments: receiving
REVERT: 28fd17d7c4 silentpayments: recipient label support
REVERT: 065e8b7793 silentpayments: sending
REVERT: a6d8b11754 build: add skeleton for new silentpayments (BIP352) module
REVERT: 6274359346 bench: add ellswift to bench help output
REVERT: 0258186573 configure: Show exhaustive tests in summary
REVERT: 53b578d10b include: remove WARN_UNUSED_RESULT for functions always returning 1
REVERT: f75c985604 ci: Fix exiting from ci.sh on error
REVERT: 947761b842 tests: remove unused uncounting_illegal_callback_fn
REVERT: 5d01f375c6 build: Drop no longer needed  `-fvisibility=hidden` compiler option
REVERT: dbf1e95d2a ci: Run `tools/symbol-check.py`
REVERT: 8174c88f47 test: Add `tools/symbol-check.py`
REVERT: 8a287f9a32 Introduce `SECP256K1_LOCAL_VAR` macro
REVERT: 7106544a16 Remove deprecated _ec_privkey_{negate,tweak_add,tweak_mul} aliases
REVERT: 1e2da62eff gha: Print all *.log files, in a separate action

git-subtree-dir: src/secp256k1
git-subtree-split: 6264c3d0939f2ab11ba8c92f3cb521f9c89c8596
    bb0dc6d75c
  219. Merge commit 'bb0dc6d75cae0f4dcb6ef83a2f08922cd2e58f03' into refresh-secp256k1 8b726c4e6c
  220. 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.
    da3494063f
  221. Add "sp" HRP 687eb1c44f
  222. Add V0SilentPaymentDestination address type ebc062ba2a
  223. 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
    8d07a4e097
  224. 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.
    b1848aa0c9
  225. 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
    8c073407aa
  226. josibake force-pushed on Jul 10, 2025


josibake DrahtBot theStack murchandamus Sjors jonatack achow101 aureleoules ismaelsadeeq BrandonOdiwuor maflcko manjaneqx S3RK Eunovo

Labels
Wallet Privacy

github-metadata-mirror

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-07-10 21:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me