Silent Payments: Implement BIP352 #28122

pull josibake wants to merge 10 commits into bitcoin:master from josibake:implement-bip352 changing 51 files +13393 −60
  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/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.

  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

    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.

  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: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)
  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: member
    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: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
  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: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 😅
  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
  151. josibake force-pushed on Jan 26, 2024
  152. 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
  153. DrahtBot removed the label Needs rebase on Jan 26, 2024
  154. 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.

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

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

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

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

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

  177. DrahtBot removed the label Needs rebase on Apr 22, 2024
  178. josibake force-pushed on Apr 26, 2024
  179. josibake force-pushed on Apr 26, 2024
  180. 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:

    josibake commented at 4:22 pm on May 6, 2024:
    Closed #30046 :smile:
  181. josibake force-pushed on Apr 27, 2024
  182. 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.
  183. josibake force-pushed on May 5, 2024
  184. josibake force-pushed on May 5, 2024
  185. josibake force-pushed on May 5, 2024
  186. DrahtBot removed the label CI failed on May 5, 2024
  187. achow101 referenced this in commit 4877fcdb42 on May 17, 2024
  188. DrahtBot added the label Needs rebase on May 20, 2024
  189. achow101 referenced this in commit 55cf34a5c3 on Jun 5, 2024
  190. 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.
  191. 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
  192. Merge commit '5a0b27cf3c122ef9b9caea5727beaea2a9172442' into refresh-secp256k1 6ddbfefcee
  193. 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
  194. tests: add test for KeyPair
    Reuse existing bip340 test vectors.
    7ea05d0502
  195. conf: add ECDH,SILENTPAYMENTS secp256k1 modules 1ca9ce13da
  196. Add "sp" HRP 92a4c8a401
  197. Add V0SilentPaymentDestination address type 10fb7e0def
  198. 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
  199. 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
  200. 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
  201. josibake force-pushed on Jul 15, 2024
  202. DrahtBot removed the label Needs rebase on Jul 15, 2024
  203. 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.

  204. Sjors commented at 4:54 pm on July 16, 2024: member
  205. DrahtBot added the label Needs rebase on Aug 2, 2024
  206. DrahtBot commented at 5:59 pm on August 2, 2024: contributor

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


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: 2024-10-04 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me