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 10 commits into
base: master
Choose a base branch
from
87 changes: 87 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,92 @@ extern "C" {
* any further elliptic-curve operations from the wallet.
*/

/** This struct serves as an input parameter for passing the silent payment
* address 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
* 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 secret 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_smallest` 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. Determining the smallest outpoint from the list of transaction
* inputs is the responsibility of the caller. It is strongly recommended
* that implementations ensure they are doing this correctly by using the
* test vectors from BIP352.
*
* If necessary, the secret keys are negated to enforce the right y-parity.
* For that reason, the secret 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 outputs here are sorted by the index value
* provided in the recipient objects.
* 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
* 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_smallest: serialized (36-byte) smallest outpoint
* (lexicographically) from the transaction inputs
* taproot_seckeys: pointer to an array of pointers to taproot
* keypair inputs (can be NULL if no secret keys
* of taproot inputs are used)
* n_taproot_seckeys: the number of sender's taproot input secret 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

why can't this be secp256k1_keypair as well?

Sorry, misunderstood your original question. The rest of libsecp uses 32 byte arrays for legacy secret keys and keypairs for x-only secret keys, so I'd prefer to keep the same convention here.

* secret keys of non-taproot inputs (can be NULL
* if no secret keys of non-taproot inputs are
* used)
* n_plain_seckeys: the number of sender's non-taproot input secret
* 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,
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
276 changes: 274 additions & 2 deletions src/modules/silentpayments/main_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,282 @@
#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) {
return secp256k1_ec_pubkey_cmp((secp256k1_context *)ctx,
&(*(const secp256k1_silentpayments_recipient **)pk1)->scan_pubkey,
&(*(const secp256k1_silentpayments_recipient **)pk2)->scan_pubkey
);
}

/* 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(const secp256k1_context *ctx, 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);
secp256k1_declassify(ctx, &ss, sizeof(ss));
/* 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];
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)

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially, I believe the thinking was not having untestable branches (e.g., not able to find a preimage which causes an overflow), but I think the point you make regarding the BIP is a good one: better to stick to the specification to avoid a scenario where one implementation handles the overflow and another does not. Will update.

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

Should use the dedicated memory cleanse function and also clear out the hash object

Suggested change
memset(hash_ser, 0, sizeof(hash_ser));
secp256k1_memclear(hash_ser, sizeof(hash_ser));
secp256k1_sha256_clear(&hash);

}

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);
if (!secp256k1_pubkey_load(ctx, &P_output_ge, recipient_spend_pubkey)) {
secp256k1_scalar_clear(&t_k_scalar);
return 0;
}
ret = secp256k1_eckey_pubkey_tweak_add(&P_output_ge, &t_k_scalar);
/* tweak add only fails if t_k_scalar is equal to the dlog of P_output_ge, but t_k_scalar is the output of a collision resistant hash function. */
/* TODO: consider declassify ret */
/* TODO: but we don't want to imply this can never happen */
VERIFY_CHECK(ret);
#ifndef VERIFY
(void) ret;
#endif
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 1;
}

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;

/* 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);
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 */
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

Copy link

Choose a reason for hiding this comment

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

I had an offline conversation with @josibake. The conclusion is that there is nothing to do. The sender has to try again with a different set of pubkeys

Copy link

@Eunovo Eunovo Dec 20, 2024

Choose a reason for hiding this comment

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

@josibake Here's a diff with the test case for this edge case,
https://github.com/josibake/secp256k1/compare/9d6769f4..144584b3

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for writing a test! However, I believe this case is already covered with the ORDERC malformed key test case. Instead of adding keys that sum, we just pass in a key that is already >= the curve order.

Copy link

Choose a reason for hiding this comment

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

I believe this case is already covered with the ORDERC malformed key test case

I checked this. IIUC, the function exits earlier when you use a malformed key. Using keys that sum to SECP256K1_N uses a different code path.
There's a test for keys that sum to zero / point at infinity included in the test vectors so we still don't need to add one. I verified this by checking the coverage using

cmake -B build-cov -DCMAKE_BUILD_TYPE=Coverage
cmake --build build-cov -j 6
build-cov/bin/noverify_tests
find build-cov -name '*.gcda'|xargs gcov -t > output.gcov

ret = secp256k1_scalar_set_b32_seckey(&addend, plain_seckeys[i]);
/* TODO: We can declassify return value, because scalar set only fails if the seckey is invalid */
secp256k1_declassify(ctx, &ret, sizeof(ret));
if (!ret) {
/* TODO: clear a_sum_scalar */
return 0;
}
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;
ret = secp256k1_keypair_load(ctx, &addend, &addend_point, taproot_seckeys[i]);
/* TODO: we can declassify return value */
if (!ret) {
/* TODO: clear a_sum_scalar */
return 0;
}
secp256k1_declassify(ctx, &ret, sizeof(ret));
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.

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 believe this is now addressed after adding the constant time tests (and relevant refactors).

/* TODO: can we declassify this? */
/* Yes: We assume the adversary has access to a_sum_scalar*G */
ret = secp256k1_scalar_is_zero(&a_sum_scalar);
secp256k1_declassify(ctx, &ret, sizeof(ret));
if (ret) {
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);
/* TODO: comment */
secp256k1_declassify(ctx, &A_sum_ge, sizeof(A_sum_ge));

/* 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);
/* TODO: consider VERIFY_CHECK ??? */
if (overflow) {
return 0;
}
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)) {
/* 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 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;
if (!secp256k1_pubkey_load(ctx, &pk, &recipients[i]->scan_pubkey)) {
/* TODO: clean up */
return 0;
}
secp256k1_silentpayments_create_shared_secret(ctx, shared_secret, &a_sum_scalar, &pk);
k = 0;
}
if (!secp256k1_silentpayments_create_output_pubkey(ctx, generated_outputs[recipients[i]->index], shared_secret, &recipients[i]->spend_pubkey, k)) {
/* TODO: clean up */
return 0;
}
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));
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
memset(&shared_secret, 0, sizeof(shared_secret));
secp256k1_memclear(&shared_secret, sizeof(shared_secret));

return 1;
}

#endif
Loading