Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BIP352 silentpayments module #1519

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 82 additions & 0 deletions include/secp256k1_silentpayments.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#define SECP256K1_SILENTPAYMENTS_H

#include "secp256k1.h"
#include "secp256k1_extrakeys.h"

#ifdef __cplusplus
extern "C" {
Expand All @@ -25,6 +26,87 @@ extern "C" {
* any further elliptic-curve operations from the wallet.
*/

/* This struct serves as an In param for passing the silent payment address
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* This struct serves as an In param for passing the silent payment address
/* This struct serves as an input argument for passing the silent payment address

would be more consistent with secp256k1.h

* data. The index field is for when more than one address is being sent to in
* a transaction. Index is set based on the original ordering of the addresses
* and used to return the generated outputs matching the original ordering.
* When more than one recipient is used the recipient array will be sorted in
* place as part of generating the outputs, but the generated outputs will be
* returned in the original ordering specified by the index to ensure the
* caller is able to match up the generated outputs to the correct silent
* payment address (e.g. to be able to assign the correct amounts to the
Comment on lines +33 to +37
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* When more than one recipient is used the recipient array will be sorted in
* place as part of generating the outputs, but the generated outputs will be
* returned in the original ordering specified by the index to ensure the
* caller is able to match up the generated outputs to the correct silent
* payment address (e.g. to be able to assign the correct amounts to the
* When more than one recipient is used, the recipient array will be sorted in
* place as part of generating the outputs, but the generated outputs will be
* returned in the original ordering specified by the index to ensure the
* caller is able to match up the generated outputs to the correct silent
* payment address (e.g., to be able to assign the correct amounts to the

* correct generated outputs in the final transaction).
*/
typedef struct {
secp256k1_pubkey scan_pubkey;
secp256k1_pubkey spend_pubkey;
size_t index;
} secp256k1_silentpayments_recipient;

/** Create Silent Payment outputs for recipient(s).
*
* Given a list of n private keys a_1...a_n (one for each silent payment
* eligible input to spend), a serialized outpoint, and a list of recipients,
* create the taproot outputs.
*
* `outpoint_smallest36` refers to the smallest outpoint lexicographically
* from the transaction inputs (both silent payments eligible and non-eligible
* inputs). This value MUST be the smallest outpoint out of all of the
* transaction inputs, otherwise the recipient will be unable to find the
* payment.
*
* If necessary, the private keys are negated to enforce the right y-parity.
* For that reason, the private keys have to be passed in via two different
* parameter pairs, depending on whether the seckeys correspond to x-only
* outputs or not.
*
* Returns: 1 if creation of outputs was successful. 0 if an error occured.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: typo nit: s/occured/occurred here and in few more places in the file

* Args: ctx: pointer to a context object
* Out: generated_outputs: pointer to an array of pointers to xonly pubkeys,
* one per recipient.
* The order of outputs here matches the original
* ordering of the recipients array.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: the words "matches the original ordering" had me thoroughly confused, because I thought it referred to the array order. Try instead:

The outputs here are sorted by the index value provided in recipients. 

* In: recipients: pointer to an array of pointers to silent payment
* recipients, where each recipient is a scan public
* key, a spend public key, and an index indicating
* its position in the original ordering. The
* recipient array will be sorted in place, but
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: grouped by scan and spend key,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an implementation detail the caller does not need to be aware of, since we handle the grouping internally. From a callers perspective, all they need to do is pass an array of recipients, in any order.

* generated outputs are saved in the
* `generated_outputs` array to match the ordering
* from the index field. This ensures the caller is
* able to match the generated outputs to the
* correct silent payment addresses. The same
* recipient can be passed multiple times to create
* multiple outputs for the same recipient.
* n_recipients: the number of recipients. This is equal to the
* total number of outputs to be generated as each
* recipient may passed multiple times to generate
* multiple outputs for the same recipient
* outpoint_smallest36: serialized smallest outpoint (lexicographically)
* from the transaction inputs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: the 36 suffix is weird. outpoint36_smallest would be slightly more clear.

Suggest adding to the doc (instead): (36 bytes)

Given how many times this has gone wrong, the following warning seems justified:

Lexicographical sorting applies to the concatenated outpoint hash and
index (vout). The latter is little-endian encoded, so must not be
sorted in its integer representation.

The test vector in this PR doesn't prevent this, because sorting is left to the implementer.

Test vectors in the BIP don't necessary prevent this either, because implementers will assume it's all covered in libsecp.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a separate discussion on the Musig2 PR that I need to follow up on regarding this, where there might be a more clever way to enforce the array length without these suffix hints. For now, I think I will remove the 36 and add it to the docs, as you suggest.

Agree that we can't enforce they are passing the correct outpoint out of a list of outpoints here, but I do think we can make it clear in the header file that this is not handled by libsecp and implementations MUST ensure they are following the test vectors from the BIP. I'll add something as such to the header documentation.

* taproot_seckeys: pointer to an array of pointers to 32-byte
* private keys of taproot inputs (can be NULL if no
* private keys of taproot inputs are used)
* n_taproot_seckeys: the number of sender's taproot input private keys
* plain_seckeys: pointer to an array of pointers to 32-byte
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: why can't this be secp256k1_keypair as well? If not documented in the API, at least a clarifying code comment would be useful.

The PR description implies the logic is the other way around:

taproot_seckeys are passed as keypairs to avoid the function needing to compute the public key to determine parity.

So you want to save the caller from having to use secp256k1_keypair? But only for legacy inputs, so that doesn't seem that useful towards the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, this is outdated documentation. The function does expect taproot_seckeys to be secp256k1_keypairs.

* private keys of non-taproot inputs (can be NULL
* if no private keys of non-taproot inputs are
* used)
* n_plain_seckeys: the number of sender's non-taproot input private
* keys
*/
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_sender_create_outputs(
const secp256k1_context *ctx,
secp256k1_xonly_pubkey **generated_outputs,
const secp256k1_silentpayments_recipient **recipients,
Copy link

@jlest01 jlest01 Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason not to make the outer pointer constant since it is not modified in this function?

Suggested change
const secp256k1_silentpayments_recipient **recipients,
const secp256k1_silentpayments_recipient * const *recipients,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recipients array is passed to secp256k1_hsort, where the array is sorted in place.

size_t n_recipients,
const unsigned char *outpoint_smallest36,
const secp256k1_keypair * const *taproot_seckeys,
size_t n_taproot_seckeys,
const unsigned char * const *plain_seckeys,
size_t n_plain_seckeys
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(5);

#ifdef __cplusplus
}
#endif
Expand Down
243 changes: 241 additions & 2 deletions src/modules/silentpayments/main_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,249 @@
#define SECP256K1_MODULE_SILENTPAYMENTS_MAIN_H

#include "../../../include/secp256k1.h"
#include "../../../include/secp256k1_extrakeys.h"
#include "../../../include/secp256k1_silentpayments.h"

/* TODO: implement functions for sender side. */
/** Sort an array of silent payment recipients. This is used to group recipients by scan pubkey to
* ensure the correct values of k are used when creating multiple outputs for a recipient. */
static int secp256k1_silentpayments_recipient_sort_cmp(const void* pk1, const void* pk2, void *ctx) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: shouldn't this also sort by spend key, and use index as a tie breaker? It seems BIP352 is ambiguous here. IIUC it doesn't matter in the end, because the recipient is expected to reconsider all remaining outputs for each increment of k.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we have multiple spend keys for the same scan key, the order of the spend keys doesn't matter. We purposely did not specify a sorting order for the spend keys in BIP352 in order to avoid making clients do more work than necessary.

As you mention, the algorithm for scanning ensures it does not rely on any order dependence of the outputs to be able to find all outputs.

return secp256k1_ec_pubkey_cmp((secp256k1_context *)ctx,
&(*(const secp256k1_silentpayments_recipient **)pk1)->scan_pubkey,
&(*(const secp256k1_silentpayments_recipient **)pk2)->scan_pubkey
josibake marked this conversation as resolved.
Show resolved Hide resolved
);
}

/* TODO: implement functions for receiver side. */
static void secp256k1_silentpayments_recipient_sort(const secp256k1_context* ctx, const secp256k1_silentpayments_recipient **recipients, size_t n_recipients) {

/* Suppress wrong warning (fixed in MSVC 19.33) */
#if defined(_MSC_VER) && (_MSC_VER < 1933)
#pragma warning(push)
#pragma warning(disable: 4090)
#endif

secp256k1_hsort(recipients, n_recipients, sizeof(*recipients), secp256k1_silentpayments_recipient_sort_cmp, (void *)ctx);

#if defined(_MSC_VER) && (_MSC_VER < 1933)
#pragma warning(pop)
#endif
}

/** Set hash state to the BIP340 tagged hash midstate for "BIP0352/Inputs". */
static void secp256k1_silentpayments_sha256_init_inputs(secp256k1_sha256* hash) {
secp256k1_sha256_initialize(hash);
hash->s[0] = 0xd4143ffcul;
hash->s[1] = 0x012ea4b5ul;
hash->s[2] = 0x36e21c8ful;
hash->s[3] = 0xf7ec7b54ul;
hash->s[4] = 0x4dd4e2acul;
hash->s[5] = 0x9bcaa0a4ul;
hash->s[6] = 0xe244899bul;
hash->s[7] = 0xcd06903eul;

hash->bytes = 64;
}

static void secp256k1_silentpayments_calculate_input_hash(unsigned char *input_hash, const unsigned char *outpoint_smallest36, secp256k1_ge *pubkey_sum) {
secp256k1_sha256 hash;
unsigned char pubkey_sum_ser[33];
size_t len;
int ret;

secp256k1_silentpayments_sha256_init_inputs(&hash);
secp256k1_sha256_write(&hash, outpoint_smallest36, 36);
ret = secp256k1_eckey_pubkey_serialize(pubkey_sum, pubkey_sum_ser, &len, 1);
VERIFY_CHECK(ret && len == sizeof(pubkey_sum_ser));
(void)ret;
secp256k1_sha256_write(&hash, pubkey_sum_ser, sizeof(pubkey_sum_ser));
secp256k1_sha256_finalize(&hash, input_hash);
}

static void secp256k1_silentpayments_create_shared_secret(unsigned char *shared_secret33, const secp256k1_scalar *secret_component, const secp256k1_ge *public_component) {
secp256k1_gej ss_j;
secp256k1_ge ss;
size_t len;
int ret;

/* Compute shared_secret = tweaked_secret_component * Public_component */
secp256k1_ecmult_const(&ss_j, public_component, secret_component);
secp256k1_ge_set_gej(&ss, &ss_j);
/* This can only fail if the shared secret is the point at infinity, which should be
* impossible at this point, considering we have already validated the public key and
* the secret key being used
*/
ret = secp256k1_eckey_pubkey_serialize(&ss, shared_secret33, &len, 1);
VERIFY_CHECK(ret && len == 33);
(void)ret;
/* While not technically "secret" data, explicitly clear the shared secret since leaking this would allow an attacker
* to identify the resulting transaction as a silent payments transaction and potentially link the transaction
* back to the silent payment address
*/
secp256k1_ge_clear(&ss);
secp256k1_gej_clear(&ss_j);
}

/** Set hash state to the BIP340 tagged hash midstate for "BIP0352/SharedSecret". */
static void secp256k1_silentpayments_sha256_init_sharedsecret(secp256k1_sha256* hash) {
secp256k1_sha256_initialize(hash);
hash->s[0] = 0x88831537ul;
hash->s[1] = 0x5127079bul;
hash->s[2] = 0x69c2137bul;
hash->s[3] = 0xab0303e6ul;
hash->s[4] = 0x98fa21faul;
hash->s[5] = 0x4a888523ul;
hash->s[6] = 0xbd99daabul;
hash->s[7] = 0xf25e5e0aul;

hash->bytes = 64;
}

static void secp256k1_silentpayments_create_t_k(secp256k1_scalar *t_k_scalar, const unsigned char *shared_secret33, unsigned int k) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: micro nit: might make sense to add const qualifier to the int too. also for the int in other functions - secp256k1_silentpayments_recipient_create_output_pubkey, secp256k1_silentpayments_recipient_create_label_tweak, secp256k1_silentpayments_create_output_pubkey

secp256k1_sha256 hash;
unsigned char hash_ser[32];
josibake marked this conversation as resolved.
Show resolved Hide resolved
unsigned char k_serialized[4];

/* Compute t_k = hash(shared_secret || ser_32(k)) [sha256 with tag "BIP0352/SharedSecret"] */
secp256k1_silentpayments_sha256_init_sharedsecret(&hash);
secp256k1_sha256_write(&hash, shared_secret33, 33);
secp256k1_write_be32(k_serialized, k);
secp256k1_sha256_write(&hash, k_serialized, sizeof(k_serialized));
secp256k1_sha256_finalize(&hash, hash_ser);
secp256k1_scalar_set_b32(t_k_scalar, hash_ser, NULL);
Copy link
Contributor

@stratospher stratospher Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: BIP says to fail If t_k is not valid tweak - https://github.com/bitcoin/bips/blob/master/bip-0352.mediawiki#creating-outputs
here, we're ignoring the overflow. we could keep it consistent with the BIP?

(have a similar comment in call site of secp256k1_silentpayments_create_t_k)

/* While not technically "secret" data, explicitly clear hash_ser since leaking this would allow an attacker
* to identify the resulting transaction as a silent payments transaction and potentially link the transaction
* back to the silent payment address
*/
memset(hash_ser, 0, sizeof(hash_ser));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this will probably be a noop. See "notes" here: https://en.cppreference.com/w/c/string/byte/memset

secp has secure_erase in examples. Maybe that could be brought out? Surely there are other places where a cleanse is necessary?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A memory cleanse function secp256k1_memclear is available since the recently merged #1579 (using the same memory-barrier-approach as in Bitcoin Core) and should be used for that purpose.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, perfect, thanks! I was grepping on an old branch and missed this.

}

static int secp256k1_silentpayments_create_output_pubkey(const secp256k1_context *ctx, secp256k1_xonly_pubkey *P_output_xonly, const unsigned char *shared_secret33, const secp256k1_pubkey *recipient_spend_pubkey, unsigned int k) {
secp256k1_ge P_output_ge;
secp256k1_scalar t_k_scalar;
int ret;

/* Calculate and return P_output_xonly = B_spend + t_k * G
* This will fail if B_spend is the point at infinity or if
* B_spend + t_k*G is the point at infinity.
*/
secp256k1_silentpayments_create_t_k(&t_k_scalar, shared_secret33, k);
ret = secp256k1_pubkey_load(ctx, &P_output_ge, recipient_spend_pubkey);
ret &= secp256k1_eckey_pubkey_tweak_add(&P_output_ge, &t_k_scalar);
secp256k1_xonly_pubkey_save(P_output_xonly, &P_output_ge);

/* While not technically "secret" data, explicitly clear t_k since leaking this would allow an attacker
* to identify the resulting transaction as a silent payments transaction and potentially link the transaction
* back to the silent payment address
*/
secp256k1_scalar_clear(&t_k_scalar);
return ret;
}

int secp256k1_silentpayments_sender_create_outputs(
const secp256k1_context *ctx,
secp256k1_xonly_pubkey **generated_outputs,
const secp256k1_silentpayments_recipient **recipients,
size_t n_recipients,
const unsigned char *outpoint_smallest36,
const secp256k1_keypair * const *taproot_seckeys,
size_t n_taproot_seckeys,
const unsigned char * const *plain_seckeys,
size_t n_plain_seckeys
) {
size_t i, k;
secp256k1_scalar a_sum_scalar, addend, input_hash_scalar;
secp256k1_ge A_sum_ge;
secp256k1_gej A_sum_gej;
unsigned char input_hash[32];
unsigned char shared_secret[33];
secp256k1_silentpayments_recipient last_recipient;
int overflow = 0;
int ret = 1;

/* Sanity check inputs. */
VERIFY_CHECK(ctx != NULL);
ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));
ARG_CHECK(generated_outputs != NULL);
ARG_CHECK(recipients != NULL);
josibake marked this conversation as resolved.
Show resolved Hide resolved
ARG_CHECK(n_recipients > 0);
ARG_CHECK((plain_seckeys != NULL) || (taproot_seckeys != NULL));
if (taproot_seckeys != NULL) {
ARG_CHECK(n_taproot_seckeys > 0);
} else {
ARG_CHECK(n_taproot_seckeys == 0);
}
if (plain_seckeys != NULL) {
ARG_CHECK(n_plain_seckeys > 0);
} else {
ARG_CHECK(n_plain_seckeys == 0);
}
ARG_CHECK(outpoint_smallest36 != NULL);
/* ensure the index field is set correctly */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: a wallet might want to use the output index here. So maybe just enforce that each number is strictly higher than the previous? Or even just uniqueness. The documentation should clarify what is expected, because libsecp errors are no fun to debug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear: this is not the output index from the transaction. This is an index going from 0 - r-1 where r represents the total number of silent payment outputs in the final transaction. There may be other non silent payment outputs in the transaction, hence this has no relation to vout index.

for (i = 0; i < n_recipients; i++) {
ARG_CHECK(recipients[i]->index == i);
}

/* Compute input private keys sum: a_sum = a_1 + a_2 + ... + a_n */
a_sum_scalar = secp256k1_scalar_zero;
for (i = 0; i < n_plain_seckeys; i++) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How should we handle secret keys that add up to SECP256k1_N?
I tested this locally with ALICE_SECKKEY = 0xeadc78165ff1f8ea94ad7cfdc54990738a4c53f6e0507b42154201b8e5dff3b1 and another secret key I generated with SECP256K1_N - ALICE_SECKEY = 0x152387e9a00e07156b5283023ab66f8b306288efcef824f9aa905cd3ea564d90. Passing these two plain secret keys causes secp256k1_silentpayments_sender_create_outputs to fail

/* TODO: in other places where _set_b32_seckey is called, its normally followed by a _cmov call
* Do we need that here and if so, is it better to call it after the loop is finished?
*/
ret &= secp256k1_scalar_set_b32_seckey(&addend, plain_seckeys[i]);
secp256k1_scalar_add(&a_sum_scalar, &a_sum_scalar, &addend);
}
/* private keys used for taproot outputs have to be negated if they resulted in an odd point */
for (i = 0; i < n_taproot_seckeys; i++) {
secp256k1_ge addend_point;
/* TODO: why don't we need _cmov here after calling keypair_load? Because the ret is declassified? */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: My impression is that _cmov is used to clear addend in case _load fails. But since we don't have an early return inside the loop, might as well do it at the end of the loop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offline discussion: key pair load should never fail, because key pair object has already been validated. So constant time is not an issue here.

ret &= secp256k1_keypair_load(ctx, &addend, &addend_point, taproot_seckeys[i]);
if (secp256k1_fe_is_odd(&addend_point.y)) {
secp256k1_scalar_negate(&addend, &addend);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could simplify the early return below by:

9d6769f: ret &=

}
secp256k1_scalar_add(&a_sum_scalar, &a_sum_scalar, &addend);
}
/* If there are any failures in loading/summing up the secret keys, fail early */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f

const int zero = 0;
secp256k1_int_cmov(addend, &zero, !ret);
secp256k1_int_cmov(a_sum_scalar, &zero, !ret);

/* If there are any failures ... */
if (!ret) return 0;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josibake says to clear the a_sum_scalar in early returns. Probably addend too.

cmove vs memset: the former is constant time, but that's not always necessary.

In general the library is not always consistent when it comes to early returns. Those are often not constant time (invalid secret behaves different from valid secret).

cc @jonasnick please brainstorm a bit more about this code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw, we have this:

secp256k1/src/util.h

Lines 209 to 210 in f0868a9

/* Zero memory if flag == 1. Flag must be 0 or 1. Constant time. */
static SECP256K1_INLINE void secp256k1_memczero(void *s, size_t len, int flag) {

But AFAIU, this is "just" about clearing secrets from memory, and then memset is the right answer. (The compiler will optimize those memsets, but this PR will address this: #1579) No need to be constant-time in an error branch.

if (!ret || secp256k1_scalar_is_zero(&a_sum_scalar)) {
return 0;
}
/* Compute input_hash = hash(outpoint_L || (a_sum * G)) */
secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &A_sum_gej, &a_sum_scalar);
secp256k1_ge_set_gej(&A_sum_ge, &A_sum_gej);

/* Calculate the input hash and tweak a_sum, i.e., a_sum_tweaked = a_sum * input_hash */
secp256k1_silentpayments_calculate_input_hash(input_hash, outpoint_smallest36, &A_sum_ge);
secp256k1_scalar_set_b32(&input_hash_scalar, input_hash, &overflow);
ret &= !overflow;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f: BIP sounds like it ignores overflow in https://github.com/bitcoin/bips/blob/master/bip-0352.mediawiki#creating-outputs. though everywhere in the code input_hash overflow would lead to failure. It's a very unlikely scenario and not sure which option is preferable, but would be nice to keep BIP and code consistent.

secp256k1_scalar_mul(&a_sum_scalar, &a_sum_scalar, &input_hash_scalar);
secp256k1_silentpayments_recipient_sort(ctx, recipients, n_recipients);
last_recipient = *recipients[0];
k = 0;
for (i = 0; i < n_recipients; i++) {
if ((i == 0) || (secp256k1_ec_pubkey_cmp(ctx, &last_recipient.scan_pubkey, &recipients[i]->scan_pubkey) != 0)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit worried about that part, because it seems that if we have 3 recipients with 2 scan_pubkeys that are not contiguous, i.e. ABA instead of AAB, we would reset k to 0 when hitting A for the second time instead of correctly incrementing k to 1, resulting in generating the same pubkey twice (assuming identical spend_key of course).
I must be missing something though because I can't make the tests fail with this pattern, it seems that the same (correct) outputs are being generated regardless of the order.

Copy link
Contributor

@theStack theStack Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that a few lines above before the loop, the recipients are sorted by scan pubkey (see secp256k1_silentpayments_recipient_sort) in order to avoid the problem you described. The outputs are still returned in the original order, by taking use of the index field of the recipient structure that a user has to set in ascending order (0, 1, 2, ...).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, nothing to see here then

/* If we are on a different scan pubkey, its time to recreate the the shared secret and reset k to 0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d6769f:

Suggested change
/* If we are on a different scan pubkey, its time to recreate the the shared secret and reset k to 0.
/* If we are on a different scan pubkey, its time to recreate the shared secret and reset k to 0.

* It's very unlikely tha the scan public key is invalid by this point, since this means the caller would
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* It's very unlikely tha the scan public key is invalid by this point, since this means the caller would
* It's very unlikely that the scan public key is invalid by this point, since this means the caller would

* have created the _silentpayments_recipient object incorrectly, but just to be sure we still check that
* the public key is valid.
*/
secp256k1_ge pk;
ret &= secp256k1_pubkey_load(ctx, &pk, &recipients[i]->scan_pubkey);
if (!ret) break;
secp256k1_silentpayments_create_shared_secret(shared_secret, &a_sum_scalar, &pk);
k = 0;
}
ret &= secp256k1_silentpayments_create_output_pubkey(ctx, generated_outputs[recipients[i]->index], shared_secret, &recipients[i]->spend_pubkey, k);
k++;
last_recipient = *recipients[i];
}
/* Explicitly clear variables containing secret data */
secp256k1_scalar_clear(&addend);
secp256k1_scalar_clear(&a_sum_scalar);

/* While technically not "secret data," explicitly clear the shared secret since leaking this
* could result in a third party being able to identify the transaction as a silent payments transaction
* and potentially link the transaction back to a silent payment address
*/
memset(&shared_secret, 0, sizeof(shared_secret));
return ret;
}

#endif
Loading