Add silentpayments (BIP352) module #1471

pull theStack wants to merge 13 commits into bitcoin-core:master from theStack:silentpayments-module-demo changing 16 files +3560 −11
  1. theStack commented at 5:39 pm on December 23, 2023: contributor

    This PR adds a new Silent Payments (BIP352) module to secp256k1. The following routines are provided ($a_i$ are input private keys, $A_i$ are input public keys, $b$ and $B$ denote recipient privkeys/pubkeys that would be encoded in silent payment addresses, $d$ and $P$ the keypair for the actual transaction taproot x-only output):

    Side Function Inputs Outputs
    Sender _create_private_tweak_data $a_1…a_n$, $outpoint_L$ $a_{sum} = (a_1 + a_2 + … + a_n)$$inputhash = hash_I(outpoint_L || (a_{sum} * G))$
    Receiver _create_public_tweak_data $A_1…A_n$, $outpoint_L$ $A_{sum} = (A_1 + A_2 + … + A_n)$$inputhash = hash_I(outpoint_L || A_{sum})$
    Receiver _create_tweaked_pubkey $A_{sum}, inputhash$ $A_{tweaked} = inputhash * A_{sum}$
    Both _create_shared_secret $Pub$, $sec$(Sender: $B_{scan}, a_{sum}$Receiver: $A_{sum}, b_{scan}$Lightclient: $A_{tweaked}, b_{scan}$) $SS = (inputhash * sec) * Pub$ (ECDH)
    Receiver _create_label_tweak $b_{scan}, m$ $labeltweak = hash_L(b_{scan} || m)$$label = labeltweak * G$
    Receiver _create_address_spend_pubkey $B_{spend}, label$ $B_m = B_{spend} + label$
    Sender _sender_create_output_pubkey $SS, B_m, k$ $P_{xonly} = B_m + hash_S(SS || k) * G$
    Receiver _receiver_scan_output $SS, B_m, k, tx_{output}$ $t_k = hash_S(SS || k)$$P_{xonly} = B_m + t_k * G$ [not returned]$directmatch = P_{xonly} == tx_{output}$if $directmatch == 0$:$\quad label1 = tx_{output} - P$$\quad label2 = -tx_{output} - P$
    Receiver _create_output_seckey $b_{spend}, t_k, (labeltweak)$ $d = (b_{spend} + labeltweak) + t_k$

    where

    • $hash_I$ denotes a SHA256 tagged hash with tag “BIP0352/Inputs”
    • $hash_L$ denotes a SHA256 tagged hash with tag “BIP0352/Label”
    • $hash_S$ denotes a SHA256 tagged hash with tag “BIP0352/SharedSecret”

    For ending up with output key material used for sending to / scanning for / spending from, both sides would follow the chain of tweak_data -> shared_secret -> output key. The public tweak data can be useful for faster scanning of output transactions by storing them in an index, see e.g. Bitcoin Core PR https://github.com/bitcoin/bitcoin/pull/28241. Private tweak data is arguably less useful, so in theory one could collapse the tweak data and shared secret creation functions into a single one, but IMHO it’s nicer if the API is symmetric.

    As discussed in #1427 (comment), the approach of passing in two separate key pairs for taproot and non-taproot inputs is followed here. This may seem a bit confusing at first, but has the advantage that the caller doesn’t have to deal with enforcing even y-parity for key material manually (e.g. negating private keys of taproot outputs if they would end up in an odd point), which seems error-prone.

    The last commit contains the BIP352 test vectors, converted to C code with a Python script. An earlier version of the tests, directly written in Python (by calling in to the secp256k1 shared library using ctypes) can still be found here: https://github.com/theStack/secp256k1/tree/silentpayments-module-demo_old5

  2. theStack marked this as a draft on Dec 23, 2023
  3. josibake commented at 10:42 pm on January 10, 2024: member

    Thanks for starting this! Will review. Just wanted to comment that I’ve made a round of edits to the BIP and it is now updated with the new hashing method (along with the tests).

    The PR assumes that the outpoint hash is provided by the caller, but this could change in the future if its calculation involves elliptic-curve operations that would be a good fit for being done within the module as well.

    The hash is calculated as hash(outpointL || Asum). Since it commits to the sum of input public keys used, seems like it might make sense to move it into the module? Otherwise, we’d have to do something like calculate the sum of the public keys, return that to the caller so that they can calculate an input hash and then send it back to us.

  4. real-or-random added the label feature on Jan 11, 2024
  5. theStack commented at 11:48 am on January 12, 2024: contributor

    Thanks for starting this! Will review. Just wanted to comment that I’ve made a round of edits to the BIP and it is now updated with the new hashing method (along with the tests).

    👍

    The PR assumes that the outpoint hash is provided by the caller, but this could change in the future if its calculation involves elliptic-curve operations that would be a good fit for being done within the module as well.

    The hash is calculated as hash(outpointL || Asum). Since it commits to the sum of input public keys used, seems like it might make sense to move it into the module?

    Yes, I agree. A naive solution based on the current PR state would be to provide another function for calculating the input hash and leave the others as they are, but then we would need to pass the input pubkeys and calculate the pubkey sum twice (once for the input hash, and once for the pubkey tweak data), which should be avoided. I guess we want to introduce a function for calculating $A_{sum}$ to reuse that result. Will try to figure out an interface. Suggestions would be greatly appreciated :)

  6. josibake commented at 5:07 pm on January 16, 2024: member

    but then we would need to pass the input pubkeys and calculate the pubkey sum twice

    (I need to actually review your implementation, but..) I don’t think we need to do it twice? On the sender side, you pass in the private keys, add those up (call it priv_key_sum) and then you can generate the pubkey sum from the private keys (e.g. priv_key_sum.GetPubKey(). On the receiving side, you pass in the pubkeys, sum them, use them in the hash and then use them in the shared secret derivation.

  7. theStack commented at 6:43 pm on January 17, 2024: contributor

    but then we would need to pass the input pubkeys and calculate the pubkey sum twice

    (I need to actually review your implementation, but..) I don’t think we need to do it twice? On the sender side, you pass in the private keys, add those up (call it priv_key_sum) and then you can generate the pubkey sum from the private keys (e.g. priv_key_sum.GetPubKey(). On the receiving side, you pass in the pubkeys, sum them, use them in the hash and then use them in the shared secret derivation.

    Oh indeed, I missed that after calculating the sum of private keys, summing up the pubkeys is not necessary anymore.

    So if I’m not overlooking something, the interface change should be as simple as replacing the “outpointhash” parameter in the tweak functions with an “outpoint_L” parameter. Will tackle that in a bit.

  8. theStack force-pushed on Jan 24, 2024
  9. build: add skeleton for new silentpayments (BIP352) module a9a5fe8e28
  10. doc: add module description for secp256k1-silentpayments 6e3ed2d5df
  11. theStack force-pushed on Jan 24, 2024
  12. theStack commented at 5:17 pm on January 24, 2024: contributor

    I’ve force-pushed with a version that is now up to date with the latest state of the BIP, and updated the PR description accordingly. Labels support is also included, and all the test vectors from the BIP pass. The tests are still run from a Python script that directly interacts with the shared library via ctypes, I’ll hopefully have something ready soon to create the actual tests in tests_impl.h from that automatically. The code was changed in many places to operate internal data types and functions in the routines (e.g. secp256k1_ge instead of secp256k1_pubkey, secp256k1_scalar instead of uchar-pointers to 32-byte chunks), which seems to make more sense and have less overhead.

    (The previous version of the PR is still available here: https://github.com/theStack/secp256k1/tree/silentpayments-module-demo_old)

  13. in include/secp256k1_silentpayments.h:33 in 0b3ecd8f83 outdated
    27@@ -28,7 +28,41 @@ extern "C" {
    28  * operations.
    29  */
    30 
    31-/* TODO: add function API for sender side. */
    32+/** Create Silent Payment tweak data from input private keys.
    33+ *
    34+ * Given a list of n private keys a_1...a_n (one for each input to spend)
    


    josibake commented at 4:43 pm on January 26, 2024:

    I think it’s more correct to say something like:

    0 * Given a list of n private keys a_1...a_n (one for each silent payment eligible input to spend)
    

    since these are only the private keys of inputs that meet the silent payments inputs criteria.

  14. in include/secp256k1_silentpayments.h:34 in 0b3ecd8f83 outdated
    27@@ -28,7 +28,41 @@ extern "C" {
    28  * operations.
    29  */
    30 
    31-/* TODO: add function API for sender side. */
    32+/** Create Silent Payment tweak data from input private keys.
    33+ *
    34+ * Given a list of n private keys a_1...a_n (one for each input to spend)
    35+ * and a serialized outpoint_lowest, compute the corresponding input
    


    josibake commented at 4:45 pm on January 26, 2024:
    0 * and a serialized outpoint_smallest, compute the corresponding input
    1Comment
    
  15. in include/secp256k1_silentpayments.h:55 in 0b3ecd8f83 outdated
    51+ *                              (can be NULL if no private keys of non-taproot inputs are used)
    52+ *             n_plain_seckeys: the number of sender's non-taproot input private keys
    53+ *             taproot_seckeys: pointer to an array of 32-byte private keys of taproot inputs
    54+ *                              (can be NULL if no private keys of taproot inputs are used)
    55+ *           n_taproot_seckeys: the number of sender's taproot input private keys
    56+ *           outpoint_lowest36: serialized lowest outpoint
    


    josibake commented at 4:47 pm on January 26, 2024:
    0 *           outpoint_smallest36: serialized smallest outpoint
    
  16. in include/secp256k1_silentpayments.h:94 in 0acde7b470 outdated
    88@@ -89,7 +89,39 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_send_cre
    89     const secp256k1_pubkey *receiver_scan_pubkey
    90 ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);
    91 
    92-/* TODO: add function API for receiver side. */
    93+/** Create Silent Payment tweak data from input public keys.
    94+ *
    95+ * Given a list of n public keys A_1...A_n (one for each input to spend)
    


    josibake commented at 4:57 pm on January 26, 2024:
    0 * Given a list of n public keys A_1...A_n (one for each silent payment eligible input to spend)
    
  17. in include/secp256k1_silentpayments.h:95 in 0acde7b470 outdated
    88@@ -89,7 +89,39 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_send_cre
    89     const secp256k1_pubkey *receiver_scan_pubkey
    90 ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);
    91 
    92-/* TODO: add function API for receiver side. */
    93+/** Create Silent Payment tweak data from input public keys.
    94+ *
    95+ * Given a list of n public keys A_1...A_n (one for each input to spend)
    96+ * and a serialized outpoint_lowest, compute the corresponding input
    


    josibake commented at 4:57 pm on January 26, 2024:
    0 * and a serialized outpoint_smallest, compute the corresponding input
    
  18. in include/secp256k1_silentpayments.h:100 in 0acde7b470 outdated
    94+ *
    95+ * Given a list of n public keys A_1...A_n (one for each input to spend)
    96+ * and a serialized outpoint_lowest, compute the corresponding input
    97+ * public keys tweak data:
    98+ *
    99+ * A_tweaked = (A_1 + A_2 + ... + A_n) * hash(outpoint_lowest || A)
    


    josibake commented at 4:58 pm on January 26, 2024:
    0 * A_tweaked = (A_1 + A_2 + ... + A_n) * hash(outpoint_smallest || A)
    
  19. in include/secp256k1_silentpayments.h:83 in 0acde7b470 outdated
     99+ * A_tweaked = (A_1 + A_2 + ... + A_n) * hash(outpoint_lowest || A)
    100+ *
    101+ * If necessary, the public keys are negated to enforce the right y-parity.
    102+ * For that reason, the public keys have to be passed in via two different parameter
    103+ * pairs, depending on whether they were used for creating taproot outputs or not.
    104+ * The resulting data is needed to create a shared secret for the receiver's side.
    


    josibake commented at 5:05 pm on January 26, 2024:

    Do we need this? By definition, taproot public keys have even y-parity. My understanding is that this only an issue when dealing with private keys, since for a given x-only public key there are two possible private keys (d, and n - d and we need to make sure we pick the correct private key (the one that produces a point with even parity).

    We could just have the caller pass in one list of public keys (which means they would have already converted them to compressed keys by prefixing them with 0x02.


    theStack commented at 6:02 pm on January 26, 2024:

    Oops, you’re totally right! Not sure what I was thinking when writing this (“Lord forgive them, for they do not know what they are doing”…) 🙈 that’s actually good news, as it results in less complex code.

    We could just have the caller pass in one list of public keys (which means they would have already converted them to compressed keys by prefixing them with 0x02.

    Not sure about that part. For the module it would simplify the interface a lot, but do we want users needing to fiddle around manually with pubkey data (even if its only prepending a single byte)? See also the discussion in #1427#issue-1920495384

    In contrast to private keys which are always just 32-bytes long for our purposes, public keys come in different sizes (33, 65 and 32 bytes) and formats (“full”, x-only). The question arises how a user would pass in those different types in a single function call. Should we

    1. Pass in two lists, one of the type secp256k1_pubkey, another one of the type secp256k1_xonly_pubkey? (The user would need to call the corresponding parse functions before, obviously)
    2. Provide a function that let’s the user convert xonly-pubkeys to pubkeys first (in this context, this should be simple by just prepending a 0x02 byte, IIUC) and then only take a single list of secp256k1_pubkey elements?
    3. Something else?

    and @jonasnick’s answer below:

    Passing two lists seems like an okay approach. Conversion functions may just lead to additional confusion. Without them, we maintain this relatively straightforward model:

    0xonly pubkey encoding -> use xonly_parse -> use in functions that accept xonly_pubkeys
    1compressed pubkey encoding -> use ec_pubkey_parse -> use in functions that accept ec_pubkeys
    

    josibake commented at 9:52 am on January 27, 2024:
    Thanks for the link to the discussion! Reading through that again, I agree it’s probably better to just have two lists: one for public keys and another for x-only public keys. Despite a more complicated API, it does make the module more user friendly in that users can pass arguments directly without needing to do any preprocessing.
  20. in src/modules/silentpayments/main_impl.h:174 in 0acde7b470 outdated
    169+
    170+    /* X-only public keys have to be converted to regular public keys (assuming even parity) */
    171+    for (i = 0; i < n_xonly_pubkeys; i++) {
    172+        secp256k1_ge addend;
    173+        secp256k1_xonly_pubkey_load(ctx, &addend, &xonly_pubkeys[i]);
    174+        if (secp256k1_fe_is_odd(&addend.y)) {
    


    josibake commented at 5:08 pm on January 26, 2024:
    I might be missing something here, but I think this will always be true, right? AFAIU, when we “load” an x-only pubkey, we choose the even y point.

    theStack commented at 6:03 pm on January 26, 2024:
    Yeah, it’s the other way round: as x-only-pubkeys always have an even y-value, the condition is never true, hence the negation in the body is dead code. Will adapt in a bit and remove the corresponding text in the API comments.
  21. in include/secp256k1_silentpayments.h:180 in ac0f2b3387 outdated
    175+ *  B_m = B_spend + label_tweak * G
    176+ *
    177+ *  The result is used by the receiver to create a Silent Payment address, consisting
    178+ *  of the serialized and concatenated scan public key and (labelled) spend public key each.
    179+ *
    180+ *  Returns: 1 if labellend spend public key creation was successful. 0 if an error occured.
    


    josibake commented at 5:13 pm on January 26, 2024:
    0 *  Returns: 1 if labelled spend public key creation was successful. 0 if an error occurred.
    
  22. josibake commented at 5:21 pm on January 26, 2024: member

    Looks great! Did a quick pass, mostly spelling and wording nits.

    Another thought I had (need to actually re-review more carefully to confirm) is we might be able to minimize the de-serializations of keys (moving from bytes -> point). From my understanding, this is one of the “expensive” operations as it involves calculating the y value. This would mean either combining things into larger routines, or having the functions return points instead of serialized pubkeys. Curious what you think @theStack

  23. theStack commented at 6:14 pm on January 26, 2024: contributor

    @josibake: Thanks for the initial review, very much appreciated!

    Another thought I had (need to actually re-review more carefully to confirm) is we might be able to minimize the de-serializations of keys (moving from bytes -> point). From my understanding, this is one of the “expensive” operations as it involves calculating the y value. This would mean either combining things into larger routines, or having the functions return points instead of serialized pubkeys. Curious what you think @theStack

    Good point, that’s also something I’ve been asking myself. I think the expensive part of calculating the y-value only applies for loading secp256k1_xonly_pubkey objects though (not for regular secp256k1_pubkeys), and since those are not used for any intermediate results, it (hopefully) shouldn’t be too bad. Directly using the group element type is not possible I think, as that datatype (secp265k1_ge) is internal and not exposed by the API. Would be great to get input from experienced long-term contributors in that subject. // EDIT: oh, you probably meant the raw 33-byte results of the shared secret creation and public tweak data. Maybe those should be changed to secp256k1_pubkey instead.

  24. theStack commented at 3:07 am on January 27, 2024: contributor

    Force-pushed with the following changes:

    • parameter naming: s/outpoint_lowest/outpoint_smallest/, s/tweak_data32/private_tweak_data32/, s/tweak_data33/public_tweak_data/ (the data type for the last one was changed, see below)
    • API description: s/for each input/for each silent payment eligible input/
    • changed the result type of the public tweak data routine to secp256k1_pubkey to avoid expensive deserialization after in the shared secret creation routine (in light of comment #1471#pullrequestreview-1846116671); AFAICT, this was the only deserialization of public keys that happened for intermediate results?
    • removed unnecessary parity handling of x-only-pubkeys, as they always have an even y (https://github.com/bitcoin-core/secp256k1/pull/1471#discussion_r1467904182) and adapted the API description accordingly
    • added explicit checks for invalid sum results in the tweak creation routines (zero for the private keys sum, point of infinity for public keys sum) with an open TODO whether we want to have a special return code in this case. IIUC, for the public keys sum case, this case could occur during scanning if someone crafted a transaction where the input pubkeys cancel each other out (trivially reachable with two pubkeys P and -P). It might make sense to have a return code signalling “ignore this tx, it’s not suitable for silent payments” to differentiate it from an actual error that happened.
  25. theStack force-pushed on Jan 27, 2024
  26. josibake commented at 10:01 am on January 27, 2024: member

    EDIT: oh, you probably meant the raw 33-byte results of the shared secret creation and public tweak data. Maybe those should be changed to secp256k1_pubkey instead.

    Yep! Sorry, my original comment wasn’t very clear. But to be precise, what I’m referring to is anytime we have to de-serialize a public key encoding into a point (be it compressed or x-only), we need to calculate a sqrt to get the y-value, which is the “expensive” part. So if we only de-serialize once (taking the inputs from the user) and then for the rest of the process use points (i.e. (x, y) pairs) until the final step where we return the generated x-only pubkeys to the user, this should save us some work!

    At a quick glance, it looks like you addressed this in your most recent push!

  27. josibake commented at 10:46 am on January 27, 2024: member

    It might make sense to have a return code signalling “ignore this tx, it’s not suitable for silent payments” to differentiate it from an actual error that happened.

    Thinking about this a bit more, the errors that can happen are:

    1. De-serialization errors: I pass some bytes that cannot be de-serialized into a valid public key
    2. Hash returns something greater than the curve order: extremely unlikely to ever happen
    3. Pubkey / private key sum is point at infinity / 0: extremely unlikely or malicious

    I think in all cases the expected user behavior is to move on. For the sender, if they run into any of these errors they would need to pick a different set of inputs (i.e. make a new transaction). For the receiver, their only recourse is to skip the transaction.

    I’m not sure what the added value to the user is here if we return different error codes.

  28. in include/secp256k1_silentpayments.h:97 in 2290e80067 outdated
    112+ *               xonly_pubkeys: pointer to an array of taproot x-only public keys
    113+ *                              (can be NULL if no taproot input public keys are used)
    114+ *             n_xonly_pubkeys: the number of taproot input public keys
    115+ *         outpoint_smallest36: serialized smallest outpoint
    116+ */
    117+SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_create_public_tweak_data(
    


    josibake commented at 11:52 am on January 28, 2024:

    in https://github.com/bitcoin-core/secp256k1/pull/1471/commits/2290e80067b9127ca7931ed4677f688070ec3c5a:

    This function covers a very important use case: preparing public transaction input data so that it can be served to a light client (a light client in this scenario is any client that does not have access to the blockchain but does have access to their b_scan private key).

    However, if used by a full node client, this function would cause the receiver to do two ECC multiplications:

    1. A_sum * input_hash_scalar
    2. (A_sum * input_hash_Scalar) * b_scan

    In https://github.com/bitcoin-core/secp256k1/pull/1471/commits/d7250503aca9d0cfc0c2df12d3f98c02945bcf33 the sender only does one ECC multiplication by first doing the less expensive scalar multiplication of a_sum * input_hash_scalar.

    What if we had a single function that is used by both the sender and receiver, e.g.silentpayments_create_shared_secret, which takes the input_hash_scalar as an input and a private key (can be b_scan or a_sum) as inputs along with a public key (can be A_sum or B_scan). This function would multiply the private key and scalar and then do the ECDH step. We would then need separate function for the light client receiver only which takes b_scan and A_tweaked and does the ECDH step.

    Just brainstorming, open to other suggestions! Also might be prematurely optimizing but from what I understand ECC Multiplication is expensive, so keeping it to one for both sender and receiver seems worth it even at this stage.


    theStack commented at 6:48 pm on January 28, 2024:

    Good point! I assumed that full node clients using silent payments would usually also be interested in the public tweak data to maintain a tweak index (like e.g. https://github.com/bitcoin/bitcoin/pull/28241), both for the purpose of serving the data to light clients and for faster silent payment rescans, but didn’t consider that this might not be the case for all full nodes.

    What if we had a single function that is used by both the sender and receiver, e.g.silentpayments_create_shared_secret, which takes the input_hash_scalar as an input and a private key (can be b_scan or a_sum) as inputs along with a public key (can be A_sum or B_scan). This function would multiply the private key and scalar and then do the ECDH step. We would then need separate function for the light client receiver only which takes b_scan and A_tweaked and does the ECDH step.

    Seems reasonable, though currently we don’t expose input_hash_scalar, so we’d need extra routines to also calculate that. Have to think more about it, open for all suggestions. Glad that the interface discussion is unleashed!

    Just brainstorming, open to other suggestions! Also might be prematurely optimizing but from what I understand ECC Multiplication is expensive, so keeping it to one for both sender and receiver seems worth it even at this stage.

    Agree that we should avoid these extra multiplications if possible.


    josibake commented at 7:05 pm on January 28, 2024:

    Have to think more about it, open for all suggestions. Glad that the interface discussion is unleashed!

    Yeah, what I suggested is pretty half baked! I’ll also give this a more thorough think and share my thoughts. Happy to keep the discussion here (easier to reference things concretely), or we can move it to the linked issue if you prefer.


    theStack commented at 4:57 pm on January 30, 2024:

    Have to think more about it, open for all suggestions. Glad that the interface discussion is unleashed!

    Yeah, what I suggested is pretty half baked! I’ll also give this a more thorough think and share my thoughts. Happy to keep the discussion here (easier to reference things concretely), or we can move it to the linked issue if you prefer.

    No strong preference for issue or PR as discussion platform either, it’s fine to keep it here for me as well!

    I still haven’t come up with something concrete yet, but am planning to study BIP327 and it’s secp256k1 module PR (https://github.com/bitcoin-core/secp256k1/pull/1479), as it might give some ideas, both regarding interface and concrete implementation. Haven’t looked deeper, but I see that a dedicated caching data type is introduced there to avoid recomputations, maybe something like that could be useful for a BIP352 interface too.


    theStack commented at 1:12 am on February 5, 2024:

    One easy possibility to avoid the extra point multiplication on the receiver side for full node clients:

    • change _create_public_tweak_data to only calculate and return a tuple (A_sum, input_hash), without doing the multiplication. E.g. with a new struct data type:
    0typedef struct {
    1    secp256k1_pubkey pubkey_sum;
    2    unsigned char input_hash[32];
    3} secp256k1_silentpayments_pubkey_tweak_data;
    
    • change _receive_create_shared_secret to take an instance of this struct (instead of $A_{tweaked}$) accordingly, e.g.:
    0int secp256k1_silentpayments_receive_create_shared_secret(
    1    const secp256k1_context *ctx,
    2    unsigned char *shared_secret33,
    3    const secp256k1_silentpayments_pubkey_tweak_data *public_tweak_data,
    4    const unsigned char *receiver_scan_seckey
    5)
    

    In that function, the shared secret would then be calculated via $SS = (b_{scan} * inputhash) * A_{sum}$

    That would be a straightforward change. The only thing needed then for light clients (or nodes that want to create a silent payments tweak index) is an additional routine to calculate $A_{tweaked} = inputhash * A_{sum}$, given a _pubkey_tweak_data instance, and a possibility to calculate the shared secret from that $A_{tweaked}$. Should we have an extra shared secret creation routine for the receiver side that takes $A_{tweaked}$ (instead of the pubkey_tweak_data instance) and $b_{scan}$ (basically exactly what we have right now in the current PR state), or somehow abuse the pubkey_tweak_data structure to be also able to calculate and hold $A_{tweaked}$ already, e.g. with a flag (0=pubkey is sum, 1=pubkey is already tweaked with hash)?

    Just some ideas and mostly thinking out loud, happy to receive further input.


    josibake commented at 12:07 pm on February 5, 2024:

    change _create_public_tweak_data to only calculate and return a tuple (A_sum, input_hash)

    This is how I did it in #28122 and this seemed to work well. This would also allow us to simplify things a bit and use a single routine for shared secret creation for both sender and receiver:

     0typedef struct {
     1    secp256k1_pubkey pubkey;
     2    unsigned char input_hash[32];
     3} secp256k1_silentpayments_pubkey_tweak_data;
     4
     5int secp256k1_silentpayments_create_shared_secret(
     6    const secp256k1_context *ctx,
     7    unsigned char *shared_secret33,
     8    const secp256k1_silentpayments_pubkey_tweak_data *public_tweak_data,
     9    const unsigned char *seckey
    10)
    

    where pubkey represents either the pubkey sum or the receivers scan public key and seckey represents either the senders secret key sum or the receivers scan private key. I think this would require modifying your _sender routines a bit, e.g. have the sender routine first sum the secret keys and then call the shared routine _silentpayments_create_shared_secret.


    For the receiver, yes, I think we would need the two routines you mentioned:

    • one for creating $A_{tweaked}$ (i.e. $A_{tweaked} = inputhash * A_{sum}$)
    • one for creating a shared secret from $A_{tweaked}$ (i.e. $SS = b_{scan} * A_{tweaked}$)

    These could also be the same routine since they are essentially doing the same thing ($d * P$) and both return unsigned char data[33], either to be written to an index/sent to light clients or hashed with $k$ to create an output. What do you think?


    theStack commented at 4:50 pm on February 5, 2024:

    This is how I did it in https://github.com/bitcoin/bitcoin/pull/28122 and this seemed to work well. This would also allow us to simplify things a bit and use a single routine for shared secret creation for both sender and receiver: …

    Consolidating the API to a single shared secret creation function for both directions would be nice indeed. One drawback could be though that it is likely more confusing for the user and it’s a bit easier to use it wrong; since the paramters can’t be named exactly after what is expected anymore (e.g. receiver_scan_pubkey), they have to have more generic names (e.g. pubkey_part, privkey_part or sth alike), as it now depends on the direction. But that can (hopefully) be compensated by good API documentation? Not sure yet, but I think we should give it a try.

    Even the shared secret creation for light clients (passing $A_{tweaked}$) case could be done by that same routine, by making one parameter optional (i.e. if input_hash is NULL, that signals that the tweak is already included in the passed pubkey). Another suggestions based on that, where the newly introduced struct from the previous comment doesn’t exist anymore:

    0Tweak data creation:
    1  _create_private_tweak_data -> returns (a_sum, input_hash)
    2  _create_public_tweak_data  -> returns (A_sum, input_hash)
    3
    4Shared secret creation:
    5  Sender:                  _create_shared_secret(..., ..., B_scan, a_sum, input_hash)
    6  Receiver (Full node):    _create_shared_secret(..., ..., A_sum, b_scan, input_hash)
    7  Receiver (Light client): _create_shared_secret(..., ..., A_tweaked, b_scan, NULL)
    

    For the receiver, yes, I think we would need the two routines you mentioned:

    • one for creating Atweaked (i.e. Atweaked=inputhash∗Asum)
    • one for creating a shared secret from Atweaked (i.e. SS=bscan∗Atweaked)

    These could also be the same routine since they are essentially doing the same thing (d∗P) and both return unsigned char data[33], either to be written to an index/sent to light clients or hashed with k to create an output. What do you think?

    Those two calculations are different in the sense that the shared secret creation one does a full ECDH including the call of the ECDH hash function, while the other one is just a normal point multiplication (less critical, as there is no secret key material involved, IIUC). So I think a dedicated routine for creating A_tweaked from (input_hash, A_sum) is still needed. For that one, it probably makes sense to include the serialization to the 33-bytes already, as the user would need to do that for storing it in an index or sending it to the light client anyway.


    josibake commented at 4:33 pm on February 6, 2024:

    One drawback could be though that it is likely more confusing for the user and it’s a bit easier to use it wrong; since the paramters can’t be named exactly after what is expected anymore

    True, but as you said, I think we can address this with good documentation. Another for using the same routine for both sender and receiver is we ensure that sender and receiver will arrive at the same shared secret (provided they give correct inputs), since they are using the same routine. If we use separate routines, there is a small chance of introducing a bug in one of the routines that would cause the sender and receiver to arrive at different shared secrets despite giving the correct inputs.

    Even the shared secret creation for light clients (passing ) case could be done by that same routine, by making one parameter optional (i.e. if input_hash is NULL, that signals that the tweak is already included in the passed pubkey)

    Also not a bad idea! I’d say we can go with this for now and always revert to multiple routines if there are objections.

    Those two calculations are different in the sense that the shared secret creation one does a full ECDH including the call of the ECDH hash function, while the other one is just a normal point multiplication

    Good point, I forgot about that. I think the main difference here is that ECDH is done in constant time whereas point multiplication is not. Regardless, you’re correct that these should remain separate routines and I agree we should just return the 33 byte serialized pubkey for the routine creating the light client/index data.

  29. theStack commented at 6:30 pm on January 28, 2024: contributor

    It might make sense to have a return code signalling “ignore this tx, it’s not suitable for silent payments” to differentiate it from an actual error that happened.

    Thinking about this a bit more, the errors that can happen are:

    1. De-serialization errors: I pass some bytes that cannot be de-serialized into a valid public key
    2. Hash returns something greater than the curve order: extremely unlikely to ever happen
    3. Pubkey / private key sum is point at infinity / 0: extremely unlikely or malicious

    Leaving 2. aside (from what I understand, it’s fine to just ignore those unlikely hash-not-within-curve-order cases), I think the difference between 1. and 3. is that the first suggests that the user is using the API in a wrong way (i.e. shouldn’t ever happen in a correct implementation), while 3. can be triggered externally for the pubkey sum case, see below.

    I think in all cases the expected user behavior is to move on. For the sender, if they run into any of these errors they would need to pick a different set of inputs (i.e. make a new transaction). For the receiver, their only recourse is to skip the transaction.

    I’m not sure what the added value to the user is here if we return different error codes.

    Yeah, I tend to agree. The rationale behind introducing those TODOs was to consider differentiating between the cases “invalid input data is passed”, indicating that the user did something wrong (case 1 above) and “the individual input data passed is fine, but we still can’t continue” (case 3 above). Since the transactions appearing in the mempool / block chain are not in the control of the user, such a “point of infinity” case could be triggered externally in the course of scanning for silent payments.

    In Bitcoin Core, at some places we call secp256k1 functions with an additional assert(ret), as we are confident that the input data passed is fine and the function always succeeds, e.g. several times in CKey::SignCompact: https://github.com/bitcoin/bitcoin/blob/5fbcc8f0560cce36abafb8467339276b7c0d62b6/src/key.cpp#L255

    In case of the pubkey tweak data creation routine, that would be a mistake as an external transaction could make a node crash in the course of e.g. the silent payment index creation. I guess we can avoid this though by just properly documenting in the API that the return code 0 also could mean “tx is not eligible for silent payments”? Maybe I’m overthinking here and users would hopefully always check for return values for more complex routines.

  30. josibake commented at 7:17 pm on January 28, 2024: member

    “invalid input data is passed”, indicating that the user did something wrong (case 1 above) and “the individual input data passed is fine, but we still can’t continue” (case 3 above)

    This is a good point. In theory, a user could recover from case 1, which at that point I’d agree we need two error codes: one to indicate that the user needs to do something different with the current transaction in order to proceed, and another to indicated the can’t do anything with the current transaction and it needs to be skipped.

    In practice, I don’t think case 1 is likely to happen often since the inputs already exist in validated transactions, it’s just a matter of correctly extracting them. That being said, I am slightly leaning towards two error codes but I’d also prefer to be consistent with the other modules if we need a tie breaker. I’m curious if there is a general pattern in the library that we can take inspiration from.

  31. theStack referenced this in commit b702461a8d on Feb 1, 2024
  32. in include/secp256k1_silentpayments.h:60 in d7250503ac outdated
    56+ *         outpoint_smallest36: serialized smallest outpoint
    57+ */
    58+SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_create_private_tweak_data(
    59+    const secp256k1_context *ctx,
    60+    unsigned char *private_tweak_data32,
    61+    const unsigned char *plain_seckeys,
    


    josibake commented at 7:21 pm on February 1, 2024:
    Started trying to use the module in #28122 and noticed this is an array of concatenated bytes, where n determines the number of items in the “list.” Any particular reason for choosing this method of passing lists of pubkey/seckeys? Is this a convention in libsecp?

    theStack commented at 11:32 pm on February 1, 2024:

    That’s actually also one of the interface questions where I’m not sure what’s the right way to do it. As we don’t have a fancy dynamic list type in C (like e.g. std::vector in C++), we will anyhow have to pass two arguments per list: one pointing to the data, the other one for the number of elements, but the question is how to structure the data. I chose flat-concatenated-bytes/pubkey-objects in a contiguous memory area, but on a second thought it’s a pretty bad idea to demand from the user to concatenate all the keys before calling, involving a lot of copying. It seems a list of pointers (ending up in a double-pointer) makes more sense?

    That’s also how other public secp256k1 routines do it, e.g. secp256k1_ec_pubkey_combine, where a list of pubkeys is passed in:

    0    const secp256k1_pubkey * const *ins,
    1    size_t n
    

    For a list of private keys, I presume this would look like

    0    const unsigned char * const *ins,
    1    size_t,
    

    (note that there is no dedicated “seckey” data type in the secp256k1 API, it’s just an unsigned char pointer to memory location where the 32 bytes key data is expected).

    I think the users would then use that somehow like this in C++ (untested):

    0void foobar(const std::vector<CKey>& keys)
    1{    
    2    std::vector<unsigned char*> privkey_pointers;
    3    for (auto& key : keys) privkey_pointers.push_back(key.data());
    4    secp256k1_create_private_tweak_data(..., ..., privkey_pointers.data(), privkey_pointers.size(), ...);
    5}
    

    Should I change the interface to list of pointers? That seems strictly better than the current interface. Or is there another method of passing in key lists that you could think of?


    theStack commented at 11:38 pm on February 1, 2024:
    Oh btw, I found an old Bitcoin Core branch from December where I already tried to use this module in #28122. It felt too early back then to publish it, as it was based on a not-even-draft module and I didn’t want to trigger another interface discussion on top of an interface discussion, but it might be helpful for giving an idea how the module is used: https://github.com/theStack/bitcoin/tree/pr28122_using_secp256k1-silentpayments-outdated (obviously, it’s outdated and the interface has changed a bit now).

    josibake commented at 9:32 am on February 2, 2024:

    It seems a list of pointers (ending up in a double-pointer) makes more sense?

    This makes more sense to me. If we have to concatenate the bytes for the secret keys, it seems like we would have to copy them out of secure memory just to pass them in to the function, whereas if we have a list of pointers the keys can be read directly from their secure memory.

    Should I change the interface to list of pointers? That seems strictly better than the current interface. Or is there another method of passing in key lists that you could think of?

    list of pointers was going to be my suggestion :smile:

    Oh btw, I found an old Bitcoin Core branch from December

    Awesome, I’ll take a look!

  33. theStack referenced this in commit 5f73bf36ec on Feb 1, 2024
  34. theStack force-pushed on Feb 4, 2024
  35. theStack commented at 9:57 pm on February 4, 2024: contributor
    Changed the tweak data creation interfaces to take lists of pointers to seckeys/pubkeys (instead of expecting the data to come in concatenated form), as suggested in the discussion #1471 (review). Took me a bit to figure out how to properly create the array of pointers with ctypes in the secp256k1 glue module for the Python test-suite, but now everything passes again.
  36. in include/secp256k1_silentpayments.h:207 in e8cb4e74ff outdated
    202+ *  (if no label tweak is used, then B_m = B_spend)
    203+ *  P_output = B_m + hash(shared_secret || ser_32(k)) * G
    204+ *
    205+ *  Returns: 1 if outputs creation was successful. 0 if an error occured.
    206+ *  Args:                  ctx: pointer to a context object
    207+ *  Out:   output_xonly_pubkey: pointer to the resulting output x-only pubkey
    


    josibake commented at 4:42 pm on February 9, 2024:

    in “silentpayments: implement output pubkey creation (both sender and receiver)” (https://github.com/bitcoin-core/secp256k1/pull/1471/commits/e8cb4e74ff586d332eb023e157b69e38660668ce):

    I think we will need sender and receiver specific routines for creating outputs. For the sender, what you have here is perfect. For the receiver, GetTxOutputTweaks (from #28122) returns t_k instead of the full output, which allows the wallet to scan without ever needing the private key b_spend. The idea here is t_k can be stored in the wallet and then added to b_spend at signing time. Additionally, the receiver needs to perform the following steps when scanning for labels:

    • calculate tx_output - P_k
    • calculate - tx_output - P_k

    What if we renamed _create_output_pubkey to _sender_create_output_pubkey and added a new function for the receiver, something like:

     0typedef struct {
     1    unsigned char t_k[32];
     2    secp256k1_xonly_pubkey xonly_output;
     3    secp256k1_pubkey label;
     4    secp256k1_pubkey label_negated;
     5} secp256k1_silentpayments_scanning_data;
     6
     7int secp256k1_silentpayments_receiver_create_scanning_data(
     8    const secp256k1_context *ctx,
     9    secp256k1_silentpayments_scanning_data *scanning_data,
    10    const unsigned char *shared_secret33,
    11    const secp256k1_pubkey *receiver_spend_pubkey,
    12    unsigned int k
    13)
    

    Where _receiver_create_scanning_data creates the output same as before and also calculates label_pubkey = tx_output + P_k_negated and label_pubkey_negated = tx_output_negated + P_k_negated.

    This way, if xonly_output matches one of the transaction outputs, we add t_k to the wallet. If xonly_output doesn’t match, we check if label_pubkey or label_pubkey_negated is in our labels cache and if there is a match we add t_k + label_tweak to the wallet.

    If the scanning client is not using labels, they can just ignore the label data and move on if they don’t find a match for xonly_pubkey. Creating the label data only involves a few negations and EC additions so I don’t think it hurts to always calculate it. The alternative would be even moar routines: one for receiving clients that use labels and one for receiving clients that don’t.

    WDYT?


    josibake commented at 2:24 pm on February 10, 2024:

    theStack commented at 3:44 pm on February 11, 2024:

    Thanks for clarifying, this helped me a lot in understanding. IIUC, trying to summarize:

    1. calculating $P_k$ (from $SS$, $B_{spend}$ and $k$) is always needed as bare minimum for both sides, i.e. either for output creation (sender) or as first step for scanning (receiver)
    2. for the receiver, the tweak $t_k$ is also relevant (to be stored in the wallet for spending later, without the need to involve $b_{spend}$ yet)
    3. only receivers with labels support have to process $P_k$ further (together with the actual output of the scanned tx) and calculate label and label_negated

    To add another interface possibility, would it work to have steps 1. and 2. done in one routine (basically the one we have already now, with the only change that it also optionally returns $t_k$), and step 3. in a separate one?

    The interface would look like

    0int secp256k1_silentpayments_create_output_pubkey(
    1    const secp256k1_context *ctx,
    2    secp256k1_xonly_pubkey *output_xonly_pubkey,
    3    unsigned char *t_k,
    4    const unsigned char *shared_secret33,
    5    const secp256k1_pubkey *receiver_spend_pubkey,
    6    unsigned int k
    7);
    

    (senders can pass in NULL for t_k as they don’t have any use for it), and

    0int secp256k1_silentpayments_create_scan_labels(
    1    const secp256k1_context *ctx,
    2    secp256k1_pubkey *label,
    3    secp256k1_pubkey *label_negated,
    4    const secp256k1_xonly_pubkey *P_output,
    5    const secp256k1_xonly_pubkey *tx_output
    6);
    

    Depending on the role and labels support, the following needs to be done:

    • Sender: call secp256k1_silentpayments_create_output_pubkey (with t_k set to NULL to avoid having to store dummy data), create tx with output $P_{output}$
    • Receiver:
      • call secp256k1_silentpayments_create_output_pubkey (with t_k set to non-NULL)
      • if $P_{output}$ equals $tx_{output}$, add $t_k$ to wallet and proceed with next k value
      • if labels are supported
        • call secp256k1_silentpayments_create_scan_labels with $P_{output}$ and $tx_{output}$
        • if any of the results match the labels cache, add $t_k + labeltweak$ to the wallet
      • proceed with next k value

    That way we would only have to calculate the scan label data if it’s needed / supported. What do you prefer, especially w.r.t. using it in #28122? Grouping by dedicated sender_ and receiver_ routines also has its advantages though in terms of readability for the user. I’m fine with either approach.


    josibake commented at 11:52 am on February 12, 2024:

    senders can pass in NULL for t_k as they don’t have any use for it

    Nice, I like this.

    only receivers with labels support have to process further

    Small correction: per the BIP, it is recommended the receiver always check for the change label. That being said, I somewhat prefer what you’re proposing here with a separate routine for generating the label data. Initially, my thinking was its better to keep it all in one routine because we avoid needing to save P_output as a secp256k1_pubkey only to load it back into a secp256k1_ge/gej group element for label processing, but I suspect this conversion is trivial enough.

    Let’s go with the interface you proposed and keep the functions separate for now.


    theStack commented at 3:52 am on February 13, 2024:

    Small correction: per the BIP, it is recommended the receiver always check for the change label. That being said, I somewhat prefer what you’re proposing here with a separate routine for generating the label data. Initially, my thinking was its better to keep it all in one routine because we avoid needing to save P_output as a secp256k1_pubkey only to load it back into a secp256k1_ge/gej group element for label processing, but I suspect this conversion is trivial enough.

    Fair point with the pubkey <-> ge/gej conversions, didn’t think about that. I also presume that these are not hurting the performance in a significant way though.

    Let’s go with the interface you proposed and keep the functions separate for now.

    Updated the PR with the proposed interface changes, with one caveat: my suggestion of returning P_output only as x-only pubkey in _create_output_pubkey doesn’t work, as the parity is needed for the scan label calculation for the receiver. The x-only format OTOH is handy both for the sender (for creating the taproot output) and the receiver (for the first scanning step, where the tx’s taproot output is directly compared to the calculated pubkey). So I’ve went now with returning P both in x-only and plain pubkey format, where the second is optional.

    The alternative would be to only return the plain pubkey and document properly that users have to convert that to an x-only-pubkey via secp256k1_xonly_pubkey_from_pubkey after, for the purpose of output creation / the first scanning step. I feel that it’s better if the users don’t have to do any manual plain -> xonly pubkey conversions and having an interface with one parameter more is the lesser evil here, if the API documentation is clear enough. Thoughts?


    josibake commented at 11:46 am on February 13, 2024:

    Hm, I completely forgot about the parity of P_output being relevant. I’m not crazy about returning two encodings of the same public key, one of which is only relevant if you’re the receiver and scanning for labels. This means the function now has an optional t_k parameter and an optional P_output_pubkey that are only used if you are a receiver, so it seems like it might be better to just have _sender/_receiver specific functions.

    Apologies for the back and forth on this, but what do you think about the following:

    For the sender, same as before:

    0int secp256k1_silentpayments_sender_create_output_pubkey(
    1    const secp256k1_context *ctx,
    2    secp256k1_xonly_pubkey *output_xonly_pubkey,
    3    const unsigned char *shared_secret33,
    4    const secp256k1_pubkey *receiver_spend_pubkey,
    5    unsigned int k
    6);
    

    For the receiver:

     0typedef struct {
     1    unsigned char t_k[32];
     2    secp256k1_xonly_pubkey xonly_output;
     3} secp256k1_silentpayments_output_data;
     4
     5typedef struct {
     6    secp256k1_pubkey label;
     7    secp256k1_pubkey label_negated;
     8} secp256k1_silentpayments_label_data;
     9
    10int secp256k1_silentpayments_receiver_create_scanning_data(
    11    const secp256k1_context *ctx,
    12    secp256k1_silentpayments_output_data *output_data,
    13    secp256k1_silentpayments_label_data *label_data,
    14    const unsigned char *shared_secret33,
    15    const secp256k1_pubkey *receiver_spend_pubkey,
    16    unsigned int k
    17)
    

    This way, the receiver can pass NULL for label_data if they don’t support labels, otherwise we can compute the label data while we still have P_output as a group element and avoid all of the pubkey encoding woes.


    theStack commented at 1:09 pm on February 13, 2024:

    Hm, I completely forgot about the parity of P_output being relevant.

    Yeah, same here. I only noticed the importance of the P_output parity at the very end of doing the changes, when about half of the receiving tests with labels were failing. As a quick-fix, I came up with this minimum-effort interface change of outputting the pubkey in two formats.

    I’m not crazy about returning two encodings of the same public key, one of which is only relevant if you’re the receiver and scanning for labels. This means the function now has an optional t_k parameter and an optional P_output_pubkey that are only used if you are a receiver, so it seems like it might be better to just have _sender/_receiver specific functions.

    Agree, dedicated sender_ and receiver_ routines make even more sense now! Considering that wallets following the BIP recommendation should always scan for the change label anyways, I think it’s also simpler to have a single routine doing both the output and labels calculation (still with the option to skip the labels calculation though, as you suggested above).

    Apologies for the back and forth on this, but what do you think about the following: ….

    Your proposed interface SGTM, will adapt in a bit.


    theStack commented at 6:51 pm on February 13, 2024:
    Force-pushed with the latest interface proposal in #1471 (review) and updated the table in the PR description accordingly. One small annoyance of the current interface is that one of the calculated labels is invalid if the calculated and scanned output match (i.e. $P_{output} == tx_{output}$), as this results in the point of infinity. If the users behave correctly, they should only ever look at the labels if there’s a mismatch between scanned and calculated outputs, so this is probably not a problem.

    josibake commented at 7:20 pm on February 13, 2024:
    Yeah, I was thinking about this today. One thought was have the _create_scanning_data check if P_output == tx_output before calculating the label data (since it has both), and then just skip creating the label data if they match.. but it felt a bit weird to have the routine doing checks for the caller. Curious what you think.

    theStack commented at 10:02 pm on February 13, 2024:

    Heh, then we had similar thoughts, that’s what is actually done right now:

    https://github.com/theStack/secp256k1/blob/c541071488c8132e5897e3eb74b010fc80a74b85/src/modules/silentpayments/main_impl.h#L340-L344

    But I’m also not sure about whether doing this check in the function is something we want. If we decide to do it, then it seems redundant to return the output x-only pubkey and we could just return a boolean (e.g. int *direct_match, set to 0 or 1) instead? Otherwise the P_output == tx_output comparison happens twice, once inside the function, and another time on the caller. We could state in the API description that the label data is only relevant out if no direct match was found and has to be ignored otherwise. Have to think more about it, happy to hear more suggestions.


    josibake commented at 6:19 pm on February 14, 2024:

    Hah, I don’t know if there is a good answer here. Our options are:

    • Don’t do the check and always compute the label data - this means in the event of finding an output, we are doing a few unnecessary EC negations / additions and one of the labels we return is the point at infinity. Not too big a deal, since this would be unusable by the caller anyways
    • Do the check to avoid computing label data when we don’t need it - this means the caller will end up doing the check again
    • Return a boolean (which is a separate in param, instead of the functions normal int return) which is checked by the caller. If false, look at the label data (if using)

    Using the boolean approach seems like the most efficient of the options and avoids calculating / returning unnecessary data, at the expense of a slightly awkward API. Personally, I’m leaning toward performance given that scanning is a resource intensive operation.


    theStack commented at 11:12 pm on February 14, 2024:

    Had very similar thoughts today with the same conclusion. What is indeed a bit unclean about the return-a-boolean API is the fact that the responsibilities of “creating scan data” and “scanning” are not strictly separated anymore, as the latter would now be done also partly by the secp module (for the direct match), and partly by the caller (for the labels). As an upside though, it’s more efficient and also leads to less code for the caller, so I’d think it’s maybe not a conceptually perfect, but practically still the most reasonable choice from the variants we considered so far.

    Will update that tomorrow, shouldn’t take too long. Do you think we should rename the function, now that it’s not only creating scan data, but doing also the first part of the scan? (e.g. simply _receiver_scan_output?)


    theStack commented at 2:24 am on February 15, 2024:
    Updated the interface to the return-bool-as-out-param suggestion, went with my renaming idea of _receiver_scan_output for now.
  37. in bip352-testsuite/run_bip352_tests.py:160 in 3deeebd1b5 outdated
    154+        while True:
    155+            if len(outputs_to_check) == 0:
    156+                break
    157+            for output_to_check in outputs_to_check:
    158+                found_sth = False
    159+                for label_m in [None] + receive_data_given['labels']:
    


    josibake commented at 4:52 pm on February 9, 2024:

    in “[DEMO-ONLY] add bip352 test vectors running suite using python and ctypes” (https://github.com/bitcoin-core/secp256k1/pull/1471/commits/3deeebd1b5d7f3684e6425be749c643504f7c471):

    This is kinda cheating in that you are iterating through all of your labels for every output in the transaction that you’re scanning. So lets say I have 100k labels and I’m checking a tx with 100 outputs.. I’d doing 2 * 100k * 100 hashes, along with 100k * 100 EC additions, etc.

    Since the receiver already has to generate the label to use it, the idea is they would store the label (something like {(B_spend + label_tweak * G) : label_tweak} so that when scanning they can calculate label = tx_output - P_k and label = tx_output_negated - P_k. This means that regardless of the number of labels used, they only have to do at most two look ups in their label cache when scanning.


    theStack commented at 6:40 pm on February 11, 2024:
    True, good point. Will adapt that in the next force-push, when the output pubkey creation / scanning routines are updated (according to the discussion above: #1471 (review)).

    theStack commented at 3:58 am on February 13, 2024:
    Updated that as well by introducing a label cache and using the new routine for label scanning. The test should now more resemble what an actual wallet would do and the hacky constructs for getting the privkey tweak are also gone. One of the next steps is to also test against the “signature” field in the test vectors, I ignored that so far.
  38. josibake commented at 4:54 pm on February 9, 2024: member
    Working on using this branch in #28122 and had some thoughts regarding labels
  39. silentpayments: add private tweak data creation routine 81d13038d5
  40. silentpayments: add public tweak data creation routine 98f5ba4aa6
  41. theStack force-pushed on Feb 10, 2024
  42. theStack commented at 10:56 pm on February 10, 2024: contributor

    Force-pushed with interface changes in the tweak data and shared secret creation routines, following the latest suggestion in #1471 (review) / #1471 (review). The tweak data creation routines now return both the key sums and input hash, without doing any tweaks yet, in order to avoid extra point multiplication for the full node receiver case (see discussion #1471 (review)). Also, there is now only one shared secret creation routine left, with the parameters named public_component, private_component and input_hash. I tried documenting in the API description on what has to be passed depending on the scenario; unsure if this is sufficient yet, maybe there is a better, more clear way of presenting the expected parameters to the user (e.g. summarizing it in a table).

    One open question is if we really need a dedicated routine to calculate A_tweaked = input_hash * A_sum (for light clients / silent payment indexes…). That routine would essentially be a copy of secp256k1_ec_pubkey_tweak_mul. Is it okay for the user to demand calling this routine, if it’s properly documented, or do we still want do have a dedicated routine secp256k1_silentpayments_create_tweaked_pubkey that does the same, making it clear in the API what has to be passed?

    Thanks for your review comments regarding labels and your POC branch @josibake, I will look at this closer in a bit.

  43. theStack force-pushed on Feb 13, 2024
  44. josibake commented at 11:52 am on February 13, 2024: member

    One open question is if we really need a dedicated routine to calculate A_tweaked = input_hash * A_sum (for light clients / silent payment indexes…). That routine would essentially be a copy of secp256k1_ec_pubkey_tweak_mul. Is it okay for the user to demand calling this routine, if it’s properly documented, or do we still want do have a dedicated routine secp256k1_silentpayments_create_tweaked_pubkey that does the same, making it clear in the API what has to be passed?

    I have a slight preference for wrapping secp256k1_ec_pubkey_tweak_mul in a dedicated secp256k1_silentpayments_created_tweaked_pubkeys routine. This way, you can do everything you need to do with just the silent payments module and don’t really need to worry about the rest of libsecp256k1. In general, I think the goal for libsecp256k1 is to further restrict the API and stop exposing routines for low level operations, so always wrapping in our own silent payments specific routines seems better.

  45. theStack force-pushed on Feb 13, 2024
  46. josibake commented at 6:07 pm on February 14, 2024: member
    I’ve rewritten #28122 to use this module here: https://github.com/josibake/bitcoin/tree/implement-bip352-secp256k1-module. So far, so good! I’ll take a fresh look here tomorrow with some feedback, and then update #28122 once I’ve cleaned up the branch a bit and we are ready to move this PR out of draft. @theStack can you remind me what your criteria for considering this PR “not a draft” were?
  47. theStack commented at 11:37 pm on February 14, 2024: contributor

    I’ve rewritten #28122 to use this module here: https://github.com/josibake/bitcoin/tree/implement-bip352-secp256k1-module. So far, so good! I’ll take a fresh look here tomorrow with some feedback, and then update #28122 once I’ve cleaned up the branch a bit and we are ready to move this PR out of draft. @theStack can you remind me what your criteria for considering this PR “not a draft” were?

    Nice, only skimmed over the branch so far but it’s good to see the module in full action. As for the out-of-draft criteria, I think we are pretty close, in my view the only two points missing are:

  48. theStack force-pushed on Feb 15, 2024
  49. silentpayments: add tweaked pubkey creation routine (for light clients / sp index) 842e5bf427
  50. silentpayments: add shared secret creation routine (a*B == A*b) b0e37968b0
  51. silentpayments: add label tweak calculation routine 2a00e12e58
  52. silentpayments: add routine for creating labelled spend pubkeys (for addresses) dbcccbb7cc
  53. silentpayments: implement output pubkey creation (for sender) 8460be58cc
  54. silentpayments: add routine for tx output scanning (for receiver)
    Co-authored-by: josibake <josibake@protonmail.com>
    d6c9856bde
  55. silentpayments: implement output spending privkey creation (for receiver) 26bdb5f195
  56. theStack force-pushed on Feb 16, 2024
  57. theStack commented at 1:34 pm on February 16, 2024: contributor
    @josibake: I’ve add the missing _create_tweaked_pubkey function to the API. As far as I’m aware, a user would have all the necessary routines now that are needed for a full silent payments implementation, without the need to do any manual ECC calculations (see also the summary table with formulas in the PR description). If there are no further objections, I’ll move the PR out of draft within the next days, after increasing the test coverage a bit more.
  58. josibake commented at 4:39 pm on February 19, 2024: member

    @josibake: I’ve add the missing _create_tweaked_pubkey function to the API. As far as I’m aware, a user would have all the necessary routines now that are needed for a full silent payments implementation, without the need to do any manual ECC calculations (see also the summary table with formulas in the PR description). If there are no further objections, I’ll move the PR out of draft within the next days, after increasing the test coverage a bit more.

    Looks great! I’ve updated #28122 to use this branch. I think we can take this out of draft whenever you’re ready. While I’m sure the API will change with further review, this seems like a pretty solid starting point!

  59. Sjors commented at 9:54 am on February 20, 2024: member

    What is the input k in _sender_create_output_pubkey?

    I find the label stuff fairly confusing, both in the BIP and in the description here. But no concrete suggestion on how to improve that…

  60. josibake commented at 9:58 am on February 20, 2024: member

    What is the input k in _sender_create_output_pubkey?

    k is a counter used by the receiver to create multiple outputs for the sender. Using the variable k here to match the notation in the BIP (see BIP352#creating-outputs)

  61. in src/modules/silentpayments/Makefile.am.include:2 in a9a5fe8e28 outdated
    0@@ -0,0 +1,2 @@
    1+include_HEADERS += include/secp256k1_silentpayments.h
    2+noinst_HEADERS += src/modules/silentpayments/main_impl.h
    


    josibake commented at 11:58 am on February 20, 2024:

    in “build: add skeleton for new silentpayments (BIP352) module” (https://github.com/bitcoin-core/secp256k1/pull/1471/commits/a9a5fe8e28f2f4dd53fdfb4ce797b23b4452f0a6):

    Needed for Bitcoin Core’s CI:

    0noinst_HEADERS += src/modules/silentpayments/tests_impl.h
    1noinst_HEADERS += src/modules/silentpayments/main_impl.h
    

    theStack commented at 0:46 am on February 23, 2024:
    Done in the latest force-push, where also the generated vectors.h is included.
  62. theStack force-pushed on Feb 22, 2024
  63. theStack force-pushed on Feb 22, 2024
  64. tests: add BIP-352 test vectors
    The vectors are generated with a Python script that converts the .json
    file from the BIP to C code:
    
    $ ./tools/tests_silentpayments_generate.py test_vectors.json > ./src/modules/silentpayments/vectors.h
    59488c4dd8
  65. ci: enable silentpayments module e4ffa036d1
  66. theStack force-pushed on Feb 22, 2024
  67. theStack commented at 0:40 am on February 23, 2024: contributor

    Force-pushed with changes regarding tests and CI. The demo-only python test runner script has now been replaced by one that converts the BIP352 test vector data in JSON format to C code (./src/modules/silentpayments/vectors.h), that is in turn included and evaluated in the main test file (./src/modules/silentpayments/tests_impl.h). The last test vector is skipped in the generation script, as is it doesn’t contain any silent payment eligible inputs; since the tweak data creation APIs require that at least one input is passed, a user would detect that and not call into secp256k1-silentpayments in such a case in the first place.

    Looks great! I’ve updated #28122 to use this branch. I think we can take this out of draft whenever you’re ready. While I’m sure the API will change with further review, this seems like a pretty solid starting point!

    Agree. I think there is still a long list of things that could/should be added (some manual tests e.g. for edge-cases, constant-time tests, benchmarks, examples, a proper doc/silentpayments.md document… anything else?), so the PR still feels early, but hopefully good enough as starting point. CI just showed green (enabled for silentpayments in the latest commit), so will move out of draft now.

    Happy to also take suggestions regarding ease-of-review. Should I squash the implementation commits? Personally I tend to prefer a higher number of small review chunks rather than the other way round, but I also understand that too many commits can trigger a “the PR is too big to even look into” sensation for potential reviewers.

  68. theStack renamed this:
    [DRAFT] Add silentpayments (BIP352) module
    Add silentpayments (BIP352) module
    on Feb 23, 2024
  69. theStack marked this as ready for review on Feb 23, 2024
  70. in include/secp256k1_silentpayments.h:47 in 81d13038d5 outdated
    43+ * pairs, depending on whether they were used for creating taproot outputs or not.
    44+ * The resulting data is needed to create a shared secret for the sender side.
    45+ *
    46+ *  Returns: 1 if shared secret creation was successful. 0 if an error occured.
    47+ *  Args:                  ctx: pointer to a context object
    48+ *  Out:                 a_sum: pointer to the resulting 32-byte private key sum
    


    josibake commented at 12:45 pm on February 23, 2024:

    in “silentpayments: add private tweak data creation routine” (https://github.com/bitcoin-core/secp256k1/pull/1471/commits/81d13038d51fbab400cc247512fbaf939db08d49):

    Thinking about this more, is there a usecase where the sender might need a_sum and not need the shared secret? If not, seems like it would be better to have the API accept private keys as an input and return the final shared secret. Otherwise, callers need to be really careful with a_sum as it could be a single private key.

    What about renaming this function to something like secp256k1_silentpayments_create_shared_secret_from_private_data and it returns unsigned char shared_secret33. This way, we can pass pointers to the privkey data and get back the shared secret without needing to worry about handling a potentially dangerous a_sum in between.


    theStack commented at 3:04 pm on February 23, 2024:

    Thinking about this more, is there a usecase where the sender might need a_sum and not need the shared secret? If not, seems like it would be better to have the API accept private keys as an input and return the final shared secret. Otherwise, callers need to be really careful with a_sum as it could be a single private key.

    The only a_sum/input_hash reuse scenario I could think of is one where the sender wants to create a transaction with more than one silent payments recipient (and with that, different scan pubkeys, i.e. the same recipient with different labels but same scan pubkey wouldn’t count as “different” in that sense). Given that sending is an infrequent scenario and calculating a_sum and input_hash are comparably cheap operations, that’s probably not a big deal though. The receiving/scanning part is potentially more of a problem, see comment below.


    josibake commented at 3:27 pm on February 23, 2024:

    Great point. This could be a fairly common usecase (e.g. an exchange processing withdrawals to $n$ silent payment addresses), but it is still a bounded problem in that you can’t send to more than $N$ silent payment addresses in a single transaction, where $N$ is the number of outputs you can fit in a single block.

    I’m leaning toward making this so the caller doesn’t need to handle intermediary secret data. The other option is we expose both routines in the API?


    theStack commented at 5:54 pm on February 27, 2024:

    Great point. This could be a fairly common usecase (e.g. an exchange processing withdrawals to n silent payment addresses), but it is still a bounded problem in that you can’t send to more than N silent payment addresses in a single transaction, where N is the number of outputs you can fit in a single block.

    I’m leaning toward making this so the caller doesn’t need to handle intermediary secret data. The other option is we expose both routines in the API?

    Oops, I missed this comment last week. I agree that a secp256k1_silentpayments_create_shared_secret_from_private_data routine would be useful for the reasons you mentioned (i.e. avoid having to handle intermediate secret key data), will add one in a bit. I tend to think that exposing both routines still makes sense, in cases where performance for sending to multiple receivers is really a concern? Will keep the current one around for discussion, if it’s not considered useful it can still be removed later.

  71. in include/secp256k1_silentpayments.h:96 in 98f5ba4aa6 outdated
    92+ *               xonly_pubkeys: pointer to an array of pointers to taproot x-only
    93+ *                              public keys (can be NULL if no taproot inputs are used)
    94+ *             n_xonly_pubkeys: the number of taproot input public keys
    95+ *         outpoint_smallest36: serialized smallest outpoint
    96+ */
    97+SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_create_public_tweak_data(
    


    josibake commented at 12:55 pm on February 23, 2024:

    in “silentpayments: add public tweak data creation routine” (https://github.com/bitcoin-core/secp256k1/pull/1471/commits/98f5ba4aa69e2b86d32fab649e65730a0a998a12):

    Similarly (see previous comment), we could rename this function to something like _create_shared_secret_from_public_data where it takes all the arguments and returns unsigned char *shared_secret33. The advantages here are:

    1. We can avoid returning needing to convert ge -> secp256k1_pubkey -> ge (the caller also doesn’t need to know anything about secp256k1_pubkey)
    2. Less confusing for full node sender and receiver since they now only need one function call each and don’t need to worry about the ambiguity with _create_shared_secret’s arguments

    theStack commented at 3:04 pm on February 23, 2024:

    in “silentpayments: add public tweak data creation routine” (98f5ba4):

    Similarly (see previous comment), we could rename this function to something like _create_shared_secret_from_public_data where it takes all the arguments and returns unsigned char *shared_secret33. The advantages here are:

    1. We can avoid returning needing to convert ge -> secp256k1_pubkey -> ge (the caller also doesn’t need to know anything about secp256k1_pubkey)
    2. Less confusing for full node sender and receiver since they now only need one function call each and don’t need to worry about the ambiguity with _create_shared_secret’s arguments

    Nice idea, I think that interface would indeed be easier for users. The drawback I see here is that if a wallet scans for multiple silent payments addresses (ones that differ in the scan pubkey part), it is more costly, as the intermediate results of A_sum/input_hash can’t be reused. E.g. deriving the shared secrets when scanning for both B1_scan/B1_spend and B2_scan/B2_spend:

    0Current interface:
    1    A_sum, input_hash = _create_public_tweak_data(input_pubkeys, input_xonly_pubkeys, outpoint_smallest)
    2    SS1 = _create_shared_secret(A_sum, b1_scan, input_hash)
    3    SS2 = _create_shared_secret(A_sum, b2_scan, input_hash)
    4    -> A_sum and input_hash only calculated once
    5
    6Proposed interface:
    7    SS1 = _create_shared_secret_from_public_data(inputs_plain, inputs_taproot, outpoint_smallest, b1_scan)
    8    SS2 = _create_shared_secret_from_public_data(inputs_plain, inputs_taproot, outpoint_smallest, b2_scan)
    9    -> A_sum and input_hash need to be calculated twice
    

    Can we live with that? Would that be common or is it much more likely that the scan keypair is constant and a user would only work with labels on the same scan pubkey anyway? Not sure what the typical usecase would be here in the future, interested about your thoughts about that. Without having numbers, I could imagine that summing up pubkeys is insignifcant compared to the point multiplication in the ECDH step, but it’s still a bit ugly that for scanning $n$ different addresses, the pubkey summing and input hashing has to be repeated $n$ times.


    josibake commented at 3:18 pm on February 23, 2024:
    That’s a great point, scanning the same transaction for multiple receivers will be a likely use case, so probably better to keep it as is.

    josibake commented at 3:29 pm on February 23, 2024:
    Also worth pointing out this is an unbounded problem (unlike the sending case): I could be scanning for 30M scan keys for a single transaction. In this case, we would want to avoid recalculating A_sum and the inputs_hash 30M times.

    theStack commented at 6:15 pm on February 27, 2024:

    Also worth pointing out this is an unbounded problem (unlike the sending case): I could be scanning for 30M scan keys for a single transaction. In this case, we would want to avoid recalculating A_sum and the inputs_hash 30M times.

    True! Do you think it’s worth it to still introduce a _create_shared_secret_from_public_data helper like you proposed for the simple-case (with a mentioning hint like “don’t use this routine repeatedly when scanning for multiple scan keys”), in addition to the current one? I’m a bit torn between “both routines could give more flexibility” and “the API should stay small and clean”. :thinking:

  72. in include/secp256k1_silentpayments.h:122 in 842e5bf427 outdated
    117+ *  Args:              ctx: pointer to a context object
    118+ *  Out:         A_tweaked: pointer to the resulting tweaked public key
    119+ *  In:              A_sum: pointer to the public keys sum
    120+ *              input_hash: pointer to the 32-byte input hash
    121+ */
    122+SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_create_tweaked_pubkey(
    


    josibake commented at 1:17 pm on February 23, 2024:

    in " silentpayments: add tweaked pubkey creation routine (for light clients / sp index)" (https://github.com/bitcoin-core/secp256k1/pull/1471/commits/842e5bf427d63b86d55aef3c234c05b42c2a6ad7):

    If we end up using the _create_shared_secret_from_public_data routine, we could also have this be _create_tweaked_pubkey_from_public_data where it takes all the same inputs as _create_shared_secret_from_public_data and returns unsigned char *A_tweaked33. This function would be used by anyone intending to store this data in an index or serve directly to a light client.

  73. in include/secp256k1_silentpayments.h:166 in b0e37968b0 outdated
    161+ *  In:       public_component: pointer to the public component
    162+ *           private_component: pointer to 32-byte private component
    163+ *                  input_hash: pointer to 32-byte input hash (can be NULL if the
    164+ *                              public component is already tweaked with the input hash)
    165+ */
    166+SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_create_shared_secret(
    


    josibake commented at 1:20 pm on February 23, 2024:

    in “silentpayments: add shared secret creation routine (aB == Ab)” (https://github.com/bitcoin-core/secp256k1/pull/1471/commits/b0e37968b09b830b422bafb60c0e870d611ef6ed):

    Building off the last couple suggestions, this function is now exclusively for light clients and there is no ambiguity about the arguments:

    • public_component is always A_tweaked
    • private_component is always b_scan

    we can also drop the input_hash argument and change public_component to unsigned char *A_tweaked

  74. in include/secp256k1_silentpayments.h:190 in 2a00e12e58 outdated
    185+ *   In:  receiver_scan_seckey: pointer to the receiver's scan private key
    186+ *                           m: label integer (0 is used for change outputs)
    187+ */
    188+SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_create_label_tweak(
    189+    const secp256k1_context *ctx,
    190+    secp256k1_pubkey *label,
    


    josibake commented at 1:22 pm on February 23, 2024:

    in “silentpayments: add label tweak calculation routine” (https://github.com/bitcoin-core/secp256k1/pull/1471/commits/2a00e12e58b6afd2b9ea9b3999dedde91f8ff82f):

    I was trying to see if we could completely get rid of secp256k1_pubkey as a return value, but here it makes sense since this output is likely to be used in the next commit to create the labeled spend pubkey.

    Alternatively, we could combine this and the next function into one that takes scan_seckey, spend_pubkey, and m as arguments and returns label_tweak, label33, labeled_spend_pubkey33 as arguments. Not convinced this is strictly better, but curious to hear your thoughts.


    theStack commented at 3:39 pm on February 23, 2024:

    Alternatively, we could combine this and the next function into one that takes scan_seckey, spend_pubkey, and m as arguments and returns label_tweak, label33, labeled_spend_pubkey33 as arguments. Not convinced this is strictly better, but curious to hear your thoughts.

    Seems to be a good idea to create all the label-related data directly in one shot. I imagine that a wallet would typically also want to create the labelled address and put its corresponding data into the labels cache at the same instant. Are there use-cases for creating labelled addresses given a labelled pubkey without having access to private key data? If yes, then the current interface is probably better though. If not, I’d prefer your suggested interface (one routine less, yay!).


    josibake commented at 3:45 pm on February 23, 2024:

    Are there use-cases for creating labelled addresses given a labelled pubkey without having access to private key data

    None that I can think of, but as I thought about this more I realized:

    • Its likely a scanner might want to generate a labels cache when recovering a wallet without needing the labeled_receiver_pubkey
    • A receiver might want to use a custom label scheme (e.g. hashing a customerId for a static deposit address)

    For both of those cases, it seems having the two separate routines is best.

  75. in include/secp256k1_silentpayments.h:239 in 8460be58cc outdated
    234+ */
    235+SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_sender_create_output_pubkey(
    236+    const secp256k1_context *ctx,
    237+    secp256k1_xonly_pubkey *P_output_xonly,
    238+    const unsigned char *shared_secret33,
    239+    const secp256k1_pubkey *receiver_spend_pubkey,
    


    josibake commented at 1:41 pm on February 23, 2024:

    in “silentpayments: implement output pubkey creation (for sender)” (https://github.com/bitcoin-core/secp256k1/pull/1471/commits/8460be58ccad9ca98e27bc60e838633f8abe2ed3):

    I think the sender will always be getting the receiver spend key externally, so we could just have this be unsigned char *receiver_spend_pubkey and handle the conversion to secp256k1_pubkey for the caller.


    theStack commented at 6:27 pm on February 27, 2024:

    in “silentpayments: implement output pubkey creation (for sender)” (8460be5):

    I think the sender will always be getting the receiver spend key externally, so we could just have this be unsigned char *receiver_spend_pubkey and handle the conversion to secp256k1_pubkey for the caller.

    SGTM, I’m all for minimizing necessary conversions at the caller-side. One slight drawback might be that the routine has now one more reason to fail (if the passed byte-string is not representing a valid public key), but I think that’s acceptable.

  76. in include/secp256k1_silentpayments.h:313 in 26bdb5f195 outdated
    308+SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_create_output_seckey(
    309+    const secp256k1_context *ctx,
    310+    unsigned char *output_seckey,
    311+    const unsigned char *receiver_spend_seckey,
    312+    const unsigned char *t_k,
    313+    const unsigned char *label_tweak
    


    josibake commented at 1:50 pm on February 23, 2024:

    in “silentpayments: implement output spending privkey creation (for receiver)” (https://github.com/bitcoin-core/secp256k1/pull/1471/commits/26bdb5f195ea5e9ebcd96919709c0a8f50156f80#):

    In #28122 I ended up abusing this function to first combine t_k and label_tweak into a single tweak, and then later called it again to combine receiver_spend_seckey with my combined tweak for the final key.

    One option is keep this function as is and then we store the t_k and the label_tweak (presumably already stored since we the curve point for scanning) and use them both at signing time. The other option would be to add another routine for combining the tweaks. The only advantage I can think of here is for HWW, where it might be more convenient to pass the combined tweak (32 bytes) instead of t_k and label (64 bytes) when signing. In all other cases, the receiver would have already calculated label or can cheaply recalculate it with scan_seckey and m, so leaning toward keeping this as is.

  77. josibake commented at 1:52 pm on February 23, 2024: member
    I know I just said the API looks good, but here’s a bunch of API feedback :see_no_evil:
  78. theStack referenced this in commit 11420a7a28 on Feb 27, 2024
  79. real-or-random commented at 0:17 am on February 28, 2024: contributor

    I ended up abusing this function [secp256k1_silentpayments_create_output_seckey]

    Yeah, this is basically scalar_add, with some high-level docs.

    This is something we need to think about. A goal of the library is to provide safe high-level APIs. I guess we often assume that this is equivalent to avoiding access to low-level operations in the API. But as this functions shows, it’s not equivalent. This function is part of a high-level API, but it happens to expose a simple low-level operation… Now, there’s nothing wrong with this per se, but people will abuse this for all kinds of things.

    And let’s be honest. If you need a scalar_add, and the library gives you a (disguised) scalar_add, it’s just pragmatic to use it. The API docs won’t prevent anyone from doing so. Is it their responsibility? Probably yes.


    I think the entire module, as proposed, is a bit unusual for libsecp256k1 because it only wraps some ECC operations of a higher-level protocol (as explained in the header). This design makes sense, but it’s different from what we usually have in libsecp256k1.

    You could say that BIP340 signatures are also just some ECC parts of a higher-level protocol “Bitcoin”, but the difference is that they implement a common interface of a signature scheme, what people call a cryptographic primitive (like public-key encryption, key exchange, hashing, etc.). But this here is merely the “cryptographic core of silent payments”, and it’s tightly coupled to it.

    Again, there’s per se nothing wrong with a more low-level module, but I think we’ll need to spend some thoughts on how we make it fit the library best. Sometimes the potential for abuse can be reduced by having opaque data types that you can only pass from API function to API function. We do something similar in the proposed musig2 API for example. I think this would be closest to the current spirit of the library.

    Another design pattern is to have “stage” functions that perform all the different computation that should be done at a specific stage of the protocol. This doesn’t seem elegant, but it prevents mistakes where the caller combines the API functions wrongly, e.g., calls them in the wrong order.


    Off-topic ramblings about the library:

    So if we expose the silentpayments_create_output_seckey function, we might as well go ahead and expose scalar_add and possibly some more scalar and group functions as part of a mid-level, or “hazmat API” as it’s called in some libraries (ignoring for a moment that this is work and raises more questions). We anyway have ec_pubkey_combine left as a relic. Then the caller at least won’t have to abuse the silent payment API.

    And if we had hazmat API, then one could alternatively simply build some libsilentpayments around it, and other applications could do the same. This would certainly help some applications. But it’s not obvious that this is the ideal solution. There are multiple related but different questions, e.g., in should stuff be a module, in which repo should it live, who should maintain it, how should it be linked, etc… (And lol, please don’t be discouraged by my comments here. I see value in this as a libsecp256k1 module, it’s just that the PR makes me think about the bigger picture again…)

  80. josibake commented at 6:14 pm on February 28, 2024: member

    Thanks for chiming in @real-or-random , this is exactly the kind of review we are looking for at this stage! As much as possible, we’d like to keep this module in the spirit of the library. I’ll take a look at the Musig2 API for inspiration and also spend some more time thinking about the “stages” concept. That feels somewhat closer to what we have now.

    And if we had hazmat API, then one could alternatively simply build some libsilentpayments around it

    This also makes a lot of sense to me as an approach. For example, I’ve been chatting with the developers of rust-silentpayments about how their library might evolve if there is a libsecp256k1 silentpayments module. Seems like having a rust silent payments library that consumes a “hazmat” secp256k1 API could be a reasonable solution?

    And lol, please don’t be discouraged by my comments here

    The only thing that discourages me is no comments at all :smile:

  81. theStack commented at 11:42 pm on February 28, 2024: contributor

    Thanks for the valuable feedback @real-or-random, glad that the interface discussion is in full swing! After thinking about it for a while, I sympathize with the “libsilentpayments” idea more and more.

    So if we expose the silentpayments_create_output_seckey function, we might as well go ahead and expose scalar_add and possibly some more scalar and group functions as part of a mid-level, or “hazmat API” as it’s called in some libraries (ignoring for a moment that this is work and raises more questions). We anyway have ec_pubkey_combine left as a relic. Then the caller at least won’t have to abuse the silent payment API.

    Doesn’t secp256k1 already have a good amount of exposed routines that potentially fall into the “hazmat API” category, especially w.r.t. the tweaking routines? For example, even without a silent payments module, one could say that the disguised scalar_add you mentioned is right now available as secp256k1_ec_seckey_tweak_add (that’s btw the exact routine called within secp256k1_silentpayments_create_output_seckey, so it’s currently more like an alias/wrapper around an already exposed function and hence wouldn’t enable something new to the user that’s not possible yet). Or would the idea be to go one step further and then also expose the internal scalar/ge(j) data types to such a hazmat API, in order to avoid unnecessary conversions between public and internal data types (and avoid error checks) that potentially hurt performance?

    To my understanding, secp256k1 as of now provides all the necessary routines for a full BIP352 implementation (Bitcoin Core PR https://github.com/bitcoin/bitcoin/pull/28122 followed that approach before it used this module). The two main reasons for a secp256k1 module (or library using secp256k1, if the libsilentpayments approach is followed) would be AFAICT 1) efficiency and 2) hiding cryptographic complexity from the user.

    It’s kind of embarrassing to only come up with this doubts weeks after starting this implementation, but I only realize now that the potential of point 1) to warrant a module was actually never verified. Assuming we would have a libsilentpayments C library that uses secp256k1 as it is now (i.e. even without any extra hazmat API). Would that even be significantly slower than using a dedicated optimized silent payments module using low-level scalar/ge(j) primitives? Without having numbers, I’d assume that the most expensive operations are by far always the involved point multiplications (e.g. the shared secret creation via ECDH), and for those having a hazmat API wouldn’t change much. @josibake: Do you think it makes sense to benchmark the Bitcoin Core PR #28122 before and after it uses this module to compare what the gains are? Happy to investigate this a bit further, if the old version (based on w0xlt’s work, AFAIR) is still available somewhere. Trying Sjor’s branch for building the index would probably also be interesting to run once with the “pure secp256k1” and once with the “secp256k1-silentpayments” module approach. Even if my simplified performance assumptions above are completely wrong (I wouldn’t be surprised if they are :D), I think it would be good to have some benchmark numbers anyway.

  82. in include/secp256k1_silentpayments.h:255 in d6c9856bde outdated
    250+ *  Given a shared_secret, a recipient's spend public key B_spend,
    251+ *  an output counter k, and a scanned tx's output x-only public key tx_output,
    252+ *  calculate the corresponding scanning data:
    253+ *
    254+ *  t_k = hash(shared_secret || ser_32(k))
    255+ *  P_output = B_spend + t_k * G  [not returned]
    


    theStack commented at 2:08 am on March 1, 2024:
    In the course of writing benchmarks, I just noticed that this scanning API is flawed from a performance perspective, as we currently recalculate $t_k$ and $P_{output}$ on every scanned output instead of only after the output counter $k$ is increased (that is, if a match was found). It seems that this routine should be split up into one calculating $P_{output}$ (only to be called once per $(B_{spend}, k)$ pair) and another one for calculating label candidates, given $P_{output}$ and $tx_{output}$ (to be called on each scanned output, if there’s no match). So the direct_match boolean out-parameter would go away (it was a bit weird anyway), as the caller would do the comparison between $P_{output}$ and $tx_{output}$.
  83. josibake commented at 7:43 pm on March 7, 2024: member

    @real-or-random spent some time digesting your comment and wanted to summarize some thoughts:

    I think the entire module, as proposed, is a bit unusual for libsecp256k1 because it only wraps some ECC operations of a higher-level protocol (as explained in the header). This design makes sense, but it’s different from what we usually have in libsecp256k1.

    In hindsight, I think BIP352 could (should) have been three BIPS:

    • BIP-silent-payments-non-interactive-multikey-ecdh (SPNIMKECDH)
    • BIP-silent-payments-v0 (the part that defines which inputs to use)
    • BIP-silent-payments-address-encoding

    This way, if we need change how the inputs are chosen in the future, we’d write a small BIP-silent-payments-v1, and nothing about the SPNIMKECDH part would need to change. Conceptually, when talking about a libsecp256k1 module I’m only talking about SPNIMKECDH (i.e. we wouldn’t expect a libsecp256k1module to filter transaction inputs to the silent-payments-v0 eligible inputs before performing the SPNIMKECDH protocol). If we start from this assumption and forget about the labels part of silent payments for a second, we could build a high-level API like so:

    Sender

     0typedef struct {
     1    secp256k1_pubkey recipient_scan_pubkey,
     2    secp256k1_pubkey recipient_spend_pubkey,
     3    size_t n_desired_outputs
     4} secp256k1_silentpayments_recipient;
     5
     6/**
     7 * Takes plain and taproot seckeys as inputs, and a list of the desired recipients
     8 * and returns a list of generated xonly outputs.
     9 * In this example, a recipient is a scan/spend pubkey pair along with an int to indicate
    10 * how many outputs should be created for this particular recipient
    11 * /
    12int secp256k1_silentpayments_sender_create_outputs(
    13    const secp256k1_context *ctx,
    14    secp256k1_xonly_pubkeys **outputs,
    15    const unsigned char * const *plain_seckeys,
    16    size_t n_plain_seckeys,
    17    const unsigned char * const *taproot_seckeys,
    18    size_t n_taproot_seckeys,
    19    const secp256k1_silentpayments_recipient * const *recipients,
    20    size_t n_recipients,
    21    const unsigned char *outpoint_smallest36
    22)
    

    Receiver

     0typedef struct {
     1    secp256k1_xonly_pubkey pubkey,
     2    unsigned char output_tweak[32],
     3} secp256k1_silentpayments_output;
     4
     5/**
     6 * Takes tx input pubkeys, the receivers spend pubkey and scan private key, and tx outputs as inputs
     7 * and returns a list of found outputs, where the found output consists of the xonly pubkey and the tweak data
     8 * needed to spend it
     9 * /
    10int secp256k1_silentpayments_receiver_scan_outputs(
    11    const secp256k1_context *ctx,
    12    secp256k1_silentpayments_output **found_outputs,
    13    const secp256k1_pubkey **input_pubkeys,
    14    size_t n_input_pubkeys,
    15    const secp256k1_pubkey *receiver_spend_pubkey,
    16    const unsigned char *receiver_scan_seckey,
    17    const secp256k1_xonly_pubkey * const *tx_outputs,
    18    size_t n_tx_outputs
    19)
    20/** 
    21 * Takes tx input pubkeys and the smallest outpoint and returns a pubkey
    22 * that can be stored and distributed to clients who need to scan but dont have
    23 * access to block data/tx data (index for wallet rescanning, light clients, etc).
    24 * /
    25int secp256k1_silentpayments_receiver_create_index_data(
    26    const secp256k1_context *ctx,
    27    secp256k1_pubkey *A_sum_tweaked,
    28    const secp256k1_pubkey **input_pubkeys,
    29    size_t n_input_pubkeys,
    30    const unsigned char *outpoint_smallest36
    31)
    32/**
    33 * takes the index data (A_sum*input_hash), tx outputs, receivers spend pubkey and scan priv key
    34 * and returns found outputs. Intended for light clients/wallet rescanning.
    35 * /
    36int secp256k1_silentpayments_scan_from_index_data(
    37    const secp256k1_context *ctx,
    38    secp256k1_silentpayments_outputs **found_outputs,
    39    const secp256k1_pubkey *receiver_spend_pubkey,
    40    const unsigned char *receiver_scan_seckey,
    41    const secp256k1_pubkey *A_sum_tweaked,
    42    const secp256k1_xonly_pubkey *tx_outputs,
    43)
    44/** 
    45 * produces a signature for a silent payments taproot output
    46 * this function is just a wrapper function around schnorrsign which first
    47 * tweaks the receivers spend key with the tweak data found during scanning.
    48 * this way, we avoid needing to expose any `scalar_add` functions to the caller.
    49 * /
    50int secp256k1_silentpayments_sign32(
    51    const secp256k1_context *ctx,
    52    unsigned char *sig64,
    53    const unsigned char *msg32,
    54    const secp256k1_keypair *keypair,
    55    const unsigned char *aux_rand32
    56    const unsigned char *tweak32
    57)
    

    This is more of an example than a proposal to demonstrate a few simple function calls and not exposing things that could be abused or used unsafely. The main question here is “would something like this fit as a module for libsecp256k1,” considering its not really a generic cryptographic primitive but also not quite the a full silent payments v0 implementation in that the sender and receiver still need to filter the inputs. If the answer is yes, I think we can start from here and figure out how to add labels in sane way (I have a few ideas for labels, both with opaque data types or stages). If the answer is no, then I think looking into other approaches, like a hazmat API is better.

    I also spent some time thinking about whether or not this could be a more generic “non-interactive-multikey-ecdh” module that is used by a libsilentpayments or something, but this didn’t seem super promising. Mainly because it wasn’t clear that the hashing with a counter k would be generically useful, and without that we’d need to return an unhashed ECDH result. Also similar questions about whether the spend and scan key are generically useful or silent payments specific. Furthermore, we’d still need to do low-level operations even after the ECDH step (adding the spend pubkey, scanning with labels, etc), so it felt like just kicking the can down the road a bit.

    To my understanding, secp256k1 as of now provides all the necessary routines for a full BIP352 implementation (Bitcoin Core PR https://github.com/bitcoin/bitcoin/pull/28122 followed that approach before it used this module). The two main reasons for a secp256k1 module (or library using secp256k1, if the libsilentpayments approach is followed) would be AFAICT 1) efficiency and 2) hiding cryptographic complexity from the user.

    I think this was more an example of “could” rather than “should.” From my understanding, bitcoin core is also a bit of a “privileged” user of secp256k1 and the goal of a module would be something generally useful for wallets outside of bitcoin core. Regarding efficiency, I’d be very surprised if this approach is less efficient than what we were originally doing in core, although IIRC bitcoin core is faster when it comes to hashing? The main advantages of doing it all here w.r.t efficiency is not need to serialize/deserialize all the time from compressed keys to group elements. Regarding benchmarking, I do still have the old version not using this module, if you want to compare with that! Would be nice to see some numbers!

  84. josibake commented at 6:44 pm on March 25, 2024: member

    @theStack I started refactoring the API based on #1471 (comment). I’ve only completed the sender side, but will continue working on the receiver side.

    The main changes are:

    1. Provide a single function for the sender: this means the sender doesn’t need to worry about grouping addresses and ensuring they pick the right values for k
    2. Remove the tweaked_pubkey routine (for light clients): instead, if the caller does not pass a pointer for input_hash, then include input_hash with the returned A_sum

    The branch is still a bit rough so some of the comments / commit messages are likely correct. Mostly just looking for conceptual feedback and curious if you have any objections to this approach. The branch is https://github.com/josibake/secp256k1/tree/bip352-api-refactor . I’ve tried to keep the edits in the relevant commits so that if the changes make sense we can easily incorporate them into your branch.

  85. theStack commented at 0:07 am on March 26, 2024: contributor

    @theStack I started refactoring the API based on #1471 (comment). I’ve only completed the sender side, but will continue working on the receiver side.

    The branch is still a bit rough so some of the comments / commit messages are likely correct. Mostly just looking for conceptual feedback and curious if you have any objections to this approach. The branch is https://github.com/josibake/secp256k1/tree/bip352-api-refactor . I’ve tried to keep the edits in the relevant commits so that if the changes make sense we can easily incorporate them into your branch.

    Thanks for pushing. A single-function API for sending that takes care of everything (even the scan-pubkey grouping, sort ftw!) seems a good idea to me. What’s currently a problem, I think, is that the user doesn’t have a way to determine which of the created outputs belong to which recipient. E.g. if the recipients are passed in in the order $(B1_{scan}, B1_{spend})$ and $(B2_{scan}, B2_{spend})$, the outputs could be created in the (unexpected) order $output_{B2}, output_{B1}$, if $B2_{scan}$ is lexographically smaller than $B1_{scan}$.

    One solution for this might be to extend the recipient structure by the resulting x-only output key and let the function fill that out, i.e. passing the recipients as in/out-parameter?

  86. josibake commented at 5:35 pm on March 27, 2024: member

    A single-function API for sending that takes care of everything (even the scan-pubkey grouping, sort ftw!) seems a good idea to me.

    yeah, the more I thought about it, we are already requiring the user to pass all of the keys at once so it didn’t seem like we are gaining anything with separate function calls. In the future, it might make sense to have functions to enable sending where the sender does not have access to all of the private keys at once, but that will require a lot more thought and seems out of scope for now.

    the user doesn’t have a way to determine which of the created outputs belong to which recipient

    Good catch, I overlooked this! I took your suggestion of extending the recipient struct and updated the branch. This has the added benefit of simplifying the function signature (i.e. no longer need to pass arguments for outputs and n_outputs).


    Thinking about the receiver API a bit more, I realized we can’t assume the receiver has access to the tx_outputs when scanning. For example, if the client is using BIP158 block filters, they would generate the scriptPubKey and then check if it is in the filter before downloading the block, or in the case of an electrum style client, they might generate the taproot output and then call the electrum backend to see if the scriptPubKey exists in the UTXO set.

    Given this, what do you think about using a callback function for checking if the generated output exists / checking if the label exists? It seems libsecp256k1 does use this pattern already in the case of the ECDH module where the caller can provide their own hash function and using callbacks could allow for a much simpler, easy to use API but I’m not what the cost in performance would be or if this would work with language bindings (e.g. a javascript library using libsecp256k1). Curious to hear your thoughts.

  87. theStack commented at 2:11 am on March 28, 2024: contributor

    the user doesn’t have a way to determine which of the created outputs belong to which recipient

    Good catch, I overlooked this! I took your suggestion of extending the recipient struct and updated the branch. This has the added benefit of simplifying the function signature (i.e. no longer need to pass arguments for outputs and n_outputs).

    Ah indeed, smaller function signature is a nice benefit, especially since the number of parameters was quite high already. :+1:

    As for the current version: I didn’t think about this when I made the suggestion, but now having the generated outputs as pointers within the _silentpayments_recipient struct is a bit of tedious/weird interface for users, as they would have to allocate memory also for those (first, for the array of pointers, and then also for the _xonly_pubkey objects as well!), in addition to the struct itself. Do you think it’s an acceptable choice if we just drop n_outputs from the struct and store the output directly as instance, i.e.

    0typedef struct {
    1    secp256k1_pubkey scan_pubkey;
    2    secp256k1_pubkey spend_pubkey;
    3    secp256k1_xonly_pubkey generated_output;
    4} secp256k1_silentpayments_recipient;
    

    This should make things much easier imho, as the user only has to allocate memory for a single struct instance per recipient. It should also make the implementation slightly simpler (one loop less). I’m assuming here that creating multiple outputs to the same SP address is rather the exception than the rule. Even if it is used more often, I think repeatedly filling out the same scan/spend keypairs is not a problem for wallets.

    Thinking about the receiver API a bit more, I realized we can’t assume the receiver has access to the tx_outputs when scanning. For example, if the client is using BIP158 block filters, they would generate the scriptPubKey and then check if it is in the filter before downloading the block, or in the case of an electrum style client, they might generate the taproot output and then call the electrum backend to see if the scriptPubKey exists in the UTXO set.

    Oh, wasn’t aware of that. The two examples you state here are only applicable for the non-label case though, right (not even the change address label)? To my understanding, when scanning for labels, we always need all outputs of a tx. Is it thinkable to have two scanning interfaces, one for the simple non-label case where simply the output is returned, and one for labels, where all outputs are passed and the labels scanning is done?

    Given this, what do you think about using a callback function for checking if the generated output exists / checking if the label exists? It seems libsecp256k1 does use this pattern already in the case of the ECDH module where the caller can provide their own hash function and using callbacks could allow for a much simpler, easy to use API but I’m not what the cost in performance would be or if this would work with language bindings (e.g. a javascript library using libsecp256k1). Curious to hear your thoughts.

    Interesting idea, I’d intuitively assume it’s neither a huge for problem for performance nor for other language bindings, but would have to check deeper.

  88. josibake commented at 1:56 pm on April 2, 2024: member

    Do you think it’s an acceptable choice if we just drop n_outputs from the struct and store the output directly as instance

    This is much simpler. Also agree that cases where a sender is creating multiple outputs for the same recipient will likely be rare, and even in those instances it might still make sense to pass the same address multiple times (e.g. to easily match up the generated address back to the requested amount). This ended up being a bit tedious due to the fact that the generated outputs depend on the order of the recipients, but rather than hack around it here, I updated the test vectors to include all possible output sets.

    Oh, wasn’t aware of that. The two examples you state here are only applicable for the non-label case though, right (not even the change address label)? To my understanding, when scanning for labels, we always need all outputs of a tx.

    Correct, you do need the tx outputs to scan for labels using the (more efficient) subtraction technique. Worth mentioning you can scan for labels without the tx outputs (e.g. the change label) by just adding the label to each output, but this is really inefficient as the number of labels grows.

    one for the simple non-label case where simply the output is returned, and one for labels, where all outputs are passed and the labels scanning is done?

    I think this makes sense. I’d imagine light clients will not support labels, whereas full nodes will, so it seems reasonable to provide two interfaces.

    Still mulling over the callbacks idea, will try to have a concrete proposal over the next few days. I’ve updated the branch in the meanwhile with the sending changes.

  89. in include/secp256k1_silentpayments.h:230 in e4ffa036d1
    225+ *
    226+ *  Returns: 1 if output creation was successful. 0 if an error occured.
    227+ *  Args:               ctx: pointer to a context object
    228+ *  Out:     P_output_xonly: pointer to the resulting output x-only pubkey
    229+ *  In:     shared_secret33: shared secret, derived from either sender's
    230+ *                           or receiver's perspective with routines from above
    


    jonasnick commented at 7:51 pm on April 2, 2024:
    Only from the sender’s perspective since only the sender calls this (as per the first sentence in the doc), right?

    josibake commented at 2:29 pm on April 4, 2024:
    I removed _sender_ from the name in my refactor branch since this function can be used by the sender when creating outputs and also by the receiver when scanning without access to the transaction outputs (e.g. light clients).
  90. jonasnick commented at 8:21 pm on April 2, 2024: contributor

    @josibake I had a quick look at the API in your branch, a few minor comments:

    • The In/Out parameter secp256k1_silentpayments_recipient seems a bit strange. Why not just group by index? secp256k1_silentpayments_recipient could just contain the scan and spend pks, and the i-th generated output would correspond to the i-th provided secp256k1_silentpayments_recipient?
    • Instead of taking a const unsigned char * const *taproot_seckeys argument and multiplying each seckey with base point G to see if it needs to be negated, you could accept an array of secp256k1_xonly_keypair, which holds both the seckey and the pubkey. Often the caller will already have the pubkey and would, if the function wouldn’t accept xonly_keypair, do a costly recomputation of the public key unnecessarily. If the caller doesn’t have an xonly_keypair, they can just create one.
    • I can imagine that others disagree, but I wouldn’t add another sign32 function if the spend key can just be tweaked via some keypair_tweak function and then given to the regular schnorrsig_sign32 function. Then this would be the same workflow for a silent payments derived key to sign via schnorrsig_custom. And a similar workflow for the musig module (where tweaking happens via secp256k1_musig_pubkey_tweak_add.
  91. josibake commented at 11:20 am on April 3, 2024: member

    Thanks for the review, @jonasnick !

    * The In/Out parameter `secp256k1_silentpayments_recipient` seems a bit strange. Why not just group by index? `secp256k1_silentpayments_recipient` could just contain the scan and spend pks, and the i-th generated output would correspond to the i-th provided `secp256k1_silentpayments_recipient`?
    

    IIUC, this means bringing back the out param taproot_outputs and n_taproot_outputs (or just reuse n_recipients), correct? We could use the index in the taproot_outputs array to match to the recipient (this is because the heapsort does the sort in place, so the recipient order after sorting would match the order in the taproot_outputs array, right?). This seems more complex for the caller in that the need to initialize the taproot output array and match up the indices and I don’t see any clear advantage, but perhaps I’m missing something? Also fine to go with the proposed change even if the reason is just that this is a more common pattern

    * Instead of taking a `const unsigned char * const *taproot_seckeys` argument and multiplying each seckey with base point G to see if it needs to be negated, you could accept an array of `secp256k1_xonly_keypair`, which holds both the seckey and the pubkey. Often the caller will already have the pubkey and would, if the function wouldn't accept `xonly_keypair`, do a costly recomputation of the public key unnecessarily. If the caller doesn't have an `xonly_keypair`, they can just create one.
    

    Great point, this was on my list of things to change but I forgot to include it!

    * I can imagine that others disagree, but I wouldn't add another `sign32` function if the spend key can just be tweaked via some `keypair_tweak` function and then given to the regular `schnorrsig_sign32` function. Then this would be the same workflow for a silent payments derived key to sign via `schnorrsig_custom`. And a similar workflow for the musig module (where tweaking happens via `secp256k1_musig_pubkey_tweak_add`.
    

    I agree. The silent payments protocol doesn’t really have anything to do with signing because once the spend key is tweaked, it is just a regular taproot output. My initial thinking was not wanting to expose any low level tweaking functions in the module, but based on the recent IRC discussion regarding a hazmat API, this seems less of a concern.

  92. jonasnick commented at 12:16 pm on April 4, 2024: contributor

    We could use the index in the taproot_outputs array to match to the recipient (this is because the heapsort does the sort in place, so the recipient order after sorting would match the order in the taproot_outputs array, right?). @josibake I missed that the function sorts internally. I had only looked at the API, but the BIP clearly states “Group receiver silent payment addresses by B_scan”. So it seems like the purpose of sorting is to make sure that any two tuples (B_scan, B_m), (B_scan', B_m') with B_scan = B_scan' are next to each other in the array. Isn’t that something that can be ensured by the caller? I suppose one problem is that if the caller doesn’t do that correctly, the resulting outputs are garbage and are either burned or require some manual procedure to recover.

    So I guess an alternative would be:

    1. Change the API as I had suggested (remove generated_output from recipient, change recipient to be an input-only parameter, and add taproot_outputs and n_taproot_outputs as output parameters).
    2. Check that the recipients array is sorted to make sure that grouping was done correctly. If it wasn’t, return 0.

    Just thinking aloud. I don’t think this is clearly better than having an In/Out argument as you had designed it.

    EDIT: Actually, I think there’s a case to be made that the caller organizing the recipient array correctly before calling the function (i.e., my suggested alternative) is cleaner than having the caller scan through the output of the function to find the correct scanning key and the corresponding generated_outputs.

  93. josibake commented at 12:57 pm on April 4, 2024: member

    So it seems like the purpose of sorting is to make sure that any two tuples (B_scan, B_m), (B_scan’, B_m’) with B_scan = B_scan’ are next to each other in the array.

    Yep! This is because the counter k is incremented over a single B_scan for each value of B_m (included repeated values of B_m. As you mentioned, if the sender does not do this grouping correctly the recipient will be unable to find the outputs, so I think we will always need the sorting step. My thinking was if we are going to do the sorting regardless, it seemed better to abstract the grouping step away from the caller completely.

    1. Change the API as I had suggested (remove generated_output from recipient, change recipient to be an input-only parameter, and add taproot_outputs and n_taproot_outputs as output parameters).
    2. Sort inside the function to make sure that grouping was done correctly. If it wasn’t, return 0.

    IIUC, heapsort is unstable which means if the caller groups and passes in the tuples [(B_scan, B_m), (B_scan, B_m')...], we might end up reordering them to [(B_scan, B_m'), (B_scan, B_m) ...]. If the caller meant to send 1 BTC to (B_scan, B_m) and 2 BTC to (B_scan, B_m'), it seems there’s a risk the values might get switched using the index approach. So I think we’d have to also reconsider how we are verifying the grouping (e.g. use a different sorting algorithm) if we wanted to go with the approach you’re suggesting.

  94. josibake commented at 2:23 pm on April 4, 2024: member

    @theStack I pushed a refactor of the recipient API to https://github.com/josibake/secp256k1/tree/bip352-api-refactor, let me know what you think. Most notably, it allows for the receiver to scan the transaction in a single call (vs scanning each output individually and managing values of k). This simplifies things for the receiver and I think gets us a bit more speed by reducing some overhead related to serializing/deserializing pubkeys.

    I also removed the _seckey function: using the label_lookup callback, I am able to get the label tweak and apply it directly while scanning, so the end result is one single sp_tweak. Given that, it seems we can just use the _ec_seckey_add function directly before signing.

    For clients that do not have access to the transaction outputs (e.g. a BIP158 client), they are able to scan using the create_public_tweak_data, create_shared_secret, and create_output functions. They would do so by creating an output, checking if it exists in the UTXO set, and proceeding to create the next output if a match is found. Once they have all of the outputs, they can get the necessary transaction data via their preferred source (e.g. downloading the whole block, downloading the transaction, downloading the outpoints, etc). Another option would be to create a single output, check if it exits in a block, download the full block if it does and continue scanning with the _receiver_scan_outputs function since they would now have all of the transaction outputs.


    @jonasnick (since you’ve been looking at the API), one thing that’s kinda gross about this approach is the found_objects struct. I couldn’t really think of a better way to do this since we don’t know how many found_objects there will be before scanning and I wanted to avoid having the scanner go over each output individually (the old approach). Curious if you have any ideas on how we could improve this.

  95. jonasnick commented at 3:30 pm on April 4, 2024: contributor

    @josibake

    My thinking was if we are going to do the sorting regardless, it seemed better to abstract the grouping step away from the caller completely.

    It’s not abstracted away completely though because, as far as I understand, the caller needs to search through the In/Out recipients array to find the right (B_scan, B_m) tuple.

    Anyway, my suggestion is flawed because it would actually require the caller to sort instead of just grouping the inputs recipients correctly.

    @jonasnick (since you’ve been looking at the API), one thing that’s kinda gross about this approach is the found_objects struct.

    You mean found_outputs? Unfortunately I don’t have an idea right now for a cleaner approach.

    Speaking of the API, what’s the reason for exposing create_shared_secret to the user instead of just calling it internally in sender_create_outputs and receiver_scan_outputs?

  96. josibake commented at 4:07 pm on April 4, 2024: member

    the caller needs to search through the In/Out recipients array to find the right (B_scan, B_m) tuple.

    You mean to match up the original sp1qxxx: amount address to the correct generated output? Good point, which is where being able to use the index from the original array would be nice. Will give this some thought. When I say “abstracted” from the caller, I was more referring to the caller not needing to worry about ordering the recipients in any particular way before calling the function.

    Speaking of the API, what’s the reason for exposing create_shared_secret to the user instead of just calling it internally in sender_create_outputs and receiver_scan_outputs?

    For the sender, no reason. create_shared_secret and create_output_pubkey are called internally in sender_create_outputs. For the receiver, there are three clients:

    • Index: indexes transactions for wallet rescanning and for serving light clients (uses create_public_tweak_data)
    • Fullnode: scans the transaction using receiver_scan_outputs or is doing a wallet rescan using the index data
      • A Fullnode might also be scanning for multiple scan secret keys, which is why it made sense to me to separate create_shared_secret from receiver_scan_outputs. This would allow a node to first do the ECDH step (creating multiple shared secrets) and then scan the transaction with each generated shared secret. You could also have the scan seckeys not on the scanning application and instead in an HSM or some separate application
    • Light clients: likely does not have access to the transaction, so generates an output using create_output_pubkey and checks if it exists in the UTXO set
      • In this model, it makes sense to separate create_shared_secret and create_output_pubkey, since create shared secret is a fairly expensive operation, so we’d want a light client to be able to reuse the shared_secret when creating multiple outputs

    That being said, open to suggestions on how we could lock down the API even further!

  97. theStack commented at 4:28 pm on April 4, 2024: contributor

    @theStack I pushed a refactor of the recipient API to https://github.com/josibake/secp256k1/tree/bip352-api-refactor, let me know what you think.

    Thanks, looks good at a first glance, will go deeper in a bit. One thing I wondered: if an output is found with a direct match, what is the label field in secp256k1_silentpayments_found_output set to? Do we need some flag to differ between “found via direct match” and “found via label” (assuming we don’t want to set label to point of infinity, which seems hacky)? Currently, it seems that label is not set at all, I assume the idea is to set it within the if-branch that follows the label_lookup callback invocation.

    I agree that the way the result is returned via found_outputs is a bit cumbersome. One alternative approach could be to also pass an in-out parameter here with a list of struct instances that contains both the tx-output to scan for and the found results (label + tweak), together with a boolean flag found, e.g. something like:

    0typedef struct {
    1    /* input (in-param part) */
    2    secp256k1_xonly_pubkey tx_output;
    3
    4    /* result part (out-param part), only relevant if found is set to 1 */
    5    int found;
    6    secp256k1_pubkey label;
    7    unsigned char tweak[32];
    8} secp256k1_silentpayments_scanned_output;
    

    That would save the user the headache of allocating memory twice and lead to an interface with less parameters (e.g. the n_found_outputs out-param is not strictly needed anymore, as it’s implicit the by the number of instances that have found set to 1). On the other hand, having to iterate through the whole in-out structure array again to collect the found results is admiteddly also quite ugly and presumably slow for transactions with lots of x-only outputs, so absolutely not sure. Just wanted to drop the idea, as it kind of picks up the same interface idea used for the sender side API. Curious on what you think.

    // EDIT: the “too slow” problem could be mitigated by having an out-param that returns if any output was found at all. If it is set to zero, the caller doesn’t have to bother with inspecting the result and can just continue scanning the next tx.

    From an organizational point of view, do you think it makes sense to open a new PR for the refactored branch soon? This PR’s description doesn’t match the new high-level module approach anymore and with soon reaching >100 comments, it also will start to suffer from the typical github UI problems soon. Would also be nice being able to directly leave code review comments for refactored API. The drawback is that we’d lose some of the previous discussion (or, at least it’s harder to find).

  98. josibake commented at 4:48 pm on April 4, 2024: member

    Currently, it seems that label is not set at all, I assume the idea is to set it within the if-branch that follows the label_lookup callback invocation

    Oops! That was an oversight. It would only be set if the output found contained a label, not set in the case of a direct match. The idea was to make it easy for the caller to match up the output to the correct label, but something else I considered is making that the responsibility of the callback function.

    From an organizational point of view, do you think it makes sense to open a new PR for the refactored branch soon

    I’ll defer to the libsecp maintainers on what they prefer. For me, I don’t mind merging the branch into this PR and updating the description, but I’m also happy to open an entirely new PR. Whichever is easiest!

    Will read the rest of your comment more carefully and respond later.

  99. josibake commented at 11:47 am on April 5, 2024: member

    One alternative approach could be to also pass an in-out parameter here with a list of struct instances that contains both the tx-output to scan for and the found results (label + tweak), together with a boolean flag found

    I think this makes more sense than what I have now. Currently, the caller already needs to allocate found_outputs to be the same size as tx_outputs (in the event every output is a payment to the recipient). Seems easy enough to have them iterate through the list again after scanning is finished and grab all the outputs for them (by checking the boolean). They would have the scriptPubKey, the tweak, and the label if used all in one place.

  100. josibake commented at 1:36 pm on April 5, 2024: member

    @jonasnick

    EDIT: Actually, I think there’s a case to be made that the caller organizing the recipient array correctly before calling the function (i.e., my suggested alternative) is cleaner than having the caller scan through the output of the function to find the correct scanning key and the corresponding generated_outputs.

    I’m starting to agree, my only concern is I’d still want the sending function to verify that the addresses are in the correct groupings and fail otherwise. Sorting seemed like the easiest way to do this, but another idea is something like the following:

     0    if (n_recipients < 2) return true;
     1    for (i = 1; i < n_recipeints; i++) {
     2        if (secp256k1_ec_pubkey_cmp(ctx, recipients[i-1].scan_pubkey, recipients[i].scan_pubkey) != 0) {
     3            for (j = i + 1; j < size; j++) {
     4                if (secp256k1_ec_pubkey_cmp(ctx, recipients[i-1].scan_pubkey, recipients[j].scan_pubkey) == 0) {
     5                    return 0;
     6                }
     7            }
     8        }
     9    }
    10    return 1;
    

    Essentially, anytime there is a scan key change, check the remainder of the list to see if the last seen scan key shows up again. This wouldn’t change ordering of the list, but the downside is that the check is O(n^2) in the worst case. But in the context of recipient addresses we kinda know the upper bound in that you can’t really request more outputs than you can fit in a single transaction (roughly 9k outputs, see https://bitcoin.stackexchange.com/questions/116958/what-is-the-maximum-number-of-taproot-transactions-that-can-be-mined-in-a-single), so maybe O(n^2) is okay? In the context of a signing device, probably not, but just spitballing some ideas.

  101. jonasnick commented at 8:52 am on April 7, 2024: contributor

    @josibake

    You mean to match up the original sp1qxxx: amount address to the correct generated output?

    Yes.

    This wouldn’t change ordering of the list, but the downside is that the check is O(n^2) in the worst case.

    I would try to avoid adding a function with O(n^2) complexity.

    Here’s yet another alternative, which adds an index field to the recipient list, sorts the recipients inside the sender_create_outputs but returns the generated_output at the “right” position:

     0/* In param */
     1typedef struct {
     2    secp256k1_pubkey scan_pubkey;
     3    secp256k1_pubkey spend_pubkey;
     4    size_t idx;
     5} secp256k1_silentpayments_recipient;
     6
     7int secp256k1_silentpayments_sender_create_outputs(secp256k1_xonly_pubkey *generated_output, secp256k1_silentpayments_recipient **recipients) {
     8  /* sort based on (scan_pubkey, spend_pubkey) */
     9  recipients = sort(recipients)
    10  for rec in recipient {
    11    generated_output[rec->idx] = create_output(...)
    12  }
    13}
    

    This modifies the recipient array which may still be annoying for the caller. Of course, the caller can just copy the array to avoid that. Or we could sort it back based on the idx field, but that seems to be overkill. Would the Bitcoin Core implementation require the order of the recipient array unmodified?

    The caller would be responsible for setting idx to the right value, but this could be easily checked in sender_create_outputs. @josibake @theStack

    From an organizational point of view, do you think it makes sense to open a new PR for the refactored branch soon? I’ll defer to the libsecp maintainers on what they prefer.

    I have a slight preference for opening a new PR since josi’s branch is more a rewrite than a refactor and a lot of the discussion in this PR doesn’t apply.

  102. jonasnick commented at 8:52 am on April 7, 2024: contributor

    @josibake

    For the receiver, there are three clients:

    This would allow a node to first do the ECDH step (creating multiple shared secrets) and then scan the transaction with each generated shared secret.

    I don’t see advantage of this. Wouldn’t it be an equal amount of work to scan the transaction with each scan secret key and generate the shared secret internally?

    You could also have the scan seckeys not on the scanning application and instead in an HSM or some separate application

    If you can run create_shared_secret on the HSM, why couldn’t you run receiver_scan_outputs?

    In this model, it makes sense to separate create_shared_secret and create_output_pubkey

    I see. Or have create_output_pubkeys function that derives the shared_secret and creates one or more pubkeys.

  103. josibake commented at 10:20 am on April 7, 2024: member

    Wouldn’t it be an equal amount of work to scan the transaction with each scan secret key and generate the shared secret internally?

    I misspoke, it’s not the shared secret part, but the summing of the input public keys that would duplicated work when scanning with multiple scan keys. So it seems we could have a receiver_scan_outputs function that takes the summed input pubkeys as an input, a scan secret key, and an optional smallest_outpoint (null if the input hash is already included with the summed input pubkeys) and have the share secret be created internally.

    Regarding an HSM, I suppose there’s nothing stopping it from taking everything it needs to scan the transaction since the data needed will always be less than the maximum transaction size.

    Or have create_output_pubkeys function that derives the shared_secret and creates one or more pubkeys.

    I think for this to work the client would need to know the total number of outputs in the transaction to avoid needing to redo ECDH in the event they don’t generate enough pubkeys the first time. However, if we assume the client knows the total number of outputs, we could make this function more closely match the receiver_scan_outputs function by using a output_lookup callback (instead of a label lookup). This way we could completely remove create_shared_secret from the API

    (apologies for any typos, doing this on a phone)

  104. josibake commented at 2:25 pm on April 19, 2024: member

    I have a slight preference for opening a new PR since josi’s branch is more a rewrite than a refactor and a lot of the discussion in this PR doesn’t apply.

    I’ve opened #1519 as a new PR, which attempts to incorporate all of the outstanding feedback here and includes a few new ideas.

  105. real-or-random commented at 4:17 pm on April 19, 2024: contributor

    I’ve opened #1519 as a new PR, which attempts to incorporate all of the outstanding feedback here and includes a few new ideas.

    Let’s close this one then.

  106. real-or-random closed this on Apr 19, 2024


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-31 23:15 UTC

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