This PR implements BIP352 - Silent payments. It is recommended to read through the BIP before reviewing this PR.
This is a continuation of the work in #1519 and only opened as a new PR due to the comment history on #1519 becoming quite long and difficult to sift through. It is recommended reviewers go through #1519 for background context, if interested.
337+ * pubkey as an argument and returns a pointer to
338+ * the label tweak if the label exists, otherwise
339+ * returns a NULL pointer (NULL if labels are not
340+ * used)
341+ * label_context: pointer to a label context object (NULL if
342+ * labels are not used)
0 * label_context: pointer to a label context object (NULL if
1 * labels are not used or context is not needed)
(was changed after #1519 (review))
(lookup, NULL) argument combination.
335+ * label_lookup: pointer to a callback function for looking up
336+ * a label value. This function takes a label
337+ * pubkey as an argument and returns a pointer to
338+ * the label tweak if the label exists, otherwise
339+ * returns a NULL pointer (NULL if labels are not
340+ * used)
label_lookup should be valid. I think it is obvious it should be valid until the next call of label_lookup, but it is currently not clear (from the docs) whether it should remain valid until secp256k1_silentpayments_recipient_scan_outputs returns.
The current implementation of secp256k1_silentpayments_recipient_scan_outputs does not need this (from looking at the code) and in a safe Rust abstraction for this function (code, does contain some outdated comments) suggested for https://github.com/rust-bitcoin/rust-secp256k1/pull/721 (WIP bindings to this pull request’s code) I relied on this not being a requirement (though that can be changed).
Hi @antonilol , thanks for the review and bindings code! Agreed that this should be documented. How about something like:
0diff --git a/include/secp256k1_silentpayments.h b/include/secp256k1_silentpayments.h
1index 2e71405..11fa596 100644
2--- a/include/secp256k1_silentpayments.h
3+++ b/include/secp256k1_silentpayments.h
4@@ -311,6 +311,11 @@ typedef struct secp256k1_silentpayments_found_output {
5 * the recipient uses labels. This allows for checking if a label exists in
6 * the recipients label cache and retrieving the label tweak during scanning.
7 *
8+ * If used, the `label_lookup` function must return a pointer to a 32-byte label
9+ * tweak if the label is found, or NULL otherwise. The returned pointer must remain
10+ * valid until the next call to `label_lookup` or until the function returns,
11+ * whichever comes first. It is not retained beyond that.
12+ *
13 * For the labels cache, `secp256k1_silentpayments_recipient_create_label`
14 * can be used.
15 *
I think the above is the most precise requirement, but perhaps a simpler alternative would be to require that the returned pointer (and labels cache, if used) be valid until _scan_outputs returns? You mention you rely on this not being the case today, but does this make a material difference if we do require that the returned pointer and labels cache be valid for the lifetime of the _scan_outputs function?
Looks good!
I think the above is the most precise requirement, but perhaps a simpler alternative would be to require that the returned pointer (and labels cache, if used) be valid until
_scan_outputsreturns? You mention you rely on this not being the case today, but does this make a material difference if we do require that the returned pointer and labels cache be valid for the lifetime of the_scan_outputsfunction?
This depends on the function signature on the Rust side, I have considered 2 cases. The difference is in what the user has to return from label_lookup.
First, there is the one in the proof of concept binding I mentioned here, the user has to return any byte array of length 32 or nothing (translates to NULL pointer).
The C side accepts a pointer so the Rust side stores this byte array on the stack and gives the C side a pointer to that. This place on the stack is reused, so requiring that the pointer lives for longer than until the next call of label_lookup means that previous return values need to be kept and some dynamic allocation would be needed.
I see this as the easiest for user to use. As far as I can tell no borrow checker issues can arise from this design.
Second, the user has to return a reference to a byte array of length 32 or nothing. The reference is required to live for as long as the function runs. This is because in Rust it is not (yet) possible to declare the reference is only required to live until the next call of label_lookup.
This is simple to implement because it is basically a pointer cast and is compatible with _scan_outputs needing pointers to be valid for the lifetime of the function.
This one is harder to use, especially for label_lookups that are not simply looking up in a data structure. Returning a reference to any byte array that is not stored outside the function’s stack frame is not allowed and storing it on the fly like the first case here probably requires the user to use unsafe code (needs a data structure that allows adding elements while there exist references to its existing elements because the reference is required to live for as long as the function runs) and has the same need for dynamic allocation like the first case can have.
The validity of the context pointer should be completely up to the users of the C library (I consider the Rust binding to also be a user of the C library).
I will also link this discussion in the Rust bindings pull request.
_scan_outputs would require keeping around pointers to every label found, even after they had been used by _scan_outputs. This seems undesirable, e.g., imagine a transaction with ~15k labeled outputs all paying to a single recipient. Your explanation seems to confirm my understanding.
Updated 6264c3d -> 9e85256 (2025_00 -> 2025_01, compare)
label_lookup pointer lifetimes (h/t @antonilol)label_context is optional (h/t @antonilol)338+ ret = secp256k1_silentpayments_recipient_scan_outputs(ctx, found_outputs, &n_found_outputs, tx_outputs, 1, key, &public_data, &recipient.labeled_spend_pubkey, NULL, NULL);
339+ CHECK(ret == 1);
340+
341+ /* TODO: this fails */
342+ /* CHECK(secp256k1_silentpayments_recipient_create_shared_secret(ctx, shared_secret, key, &public_data)); */
343+ /* TODO: test secp256k1_silentpayments_recipient_create_output_pubkey */
in commit de508a78ac66b93b0ff83b419cc6e149950ecc25: took a look into these TODOs, I think the following patch would tackle them (note that the serialize/parse-roundtrip looks superfluous at first sight, but it is needed to set the “combined” flag in the public data, which is ARG_CHECKEDed for 1 in _recipient_create_shared_secret):
0diff --git a/src/ctime_tests.c b/src/ctime_tests.c
1index 17964b9..83596a3 100644
2--- a/src/ctime_tests.c
3+++ b/src/ctime_tests.c
4@@ -116,6 +116,8 @@ static void run_tests(secp256k1_context *ctx, unsigned char *key) {
5 const secp256k1_xonly_pubkey *xonly_pubkeys[1];
6 secp256k1_pubkey plain_pubkey;
7 const secp256k1_pubkey *plain_pubkeys[1];
8+ unsigned char public_data_ser[33] = { 0 };
9+ unsigned char shared_secret[33] = { 0 };
10 #endif
11
12 for (i = 0; i < 32; i++) {
13@@ -338,9 +340,10 @@ static void run_tests(secp256k1_context *ctx, unsigned char *key) {
14 ret = secp256k1_silentpayments_recipient_scan_outputs(ctx, found_outputs, &n_found_outputs, tx_outputs, 1, key, &public_data, &recipient.labeled_spend_pubkey, NULL, NULL);
15 CHECK(ret == 1);
16
17- /* TODO: this fails */
18- /* CHECK(secp256k1_silentpayments_recipient_create_shared_secret(ctx, shared_secret, key, &public_data)); */
19- /* TODO: test secp256k1_silentpayments_recipient_create_output_pubkey */
20+ CHECK(secp256k1_silentpayments_recipient_public_data_serialize(ctx, public_data_ser, &public_data));
21+ CHECK(secp256k1_silentpayments_recipient_public_data_parse(ctx, &public_data, public_data_ser));
22+ CHECK(secp256k1_silentpayments_recipient_create_shared_secret(ctx, shared_secret, key, &public_data));
23+ CHECK(secp256k1_silentpayments_recipient_create_output_pubkey(ctx, &xonly_pubkey, shared_secret, &recipient.labeled_spend_pubkey, 0));
24
25 #endif
26 }
27diff --git a/src/modules/silentpayments/main_impl.h b/src/modules/silentpayments/main_impl.h
28index 3fc42f5..7d3ad41 100644
29--- a/src/modules/silentpayments/main_impl.h
30+++ b/src/modules/silentpayments/main_impl.h
31@@ -708,6 +708,7 @@ int secp256k1_silentpayments_recipient_create_shared_secret(const secp256k1_cont
32 ret &= secp256k1_scalar_set_b32_seckey(&rsk, recipient_scan_key32);
33 ret &= secp256k1_silentpayments_recipient_public_data_load_pubkey(ctx, &A_tweaked_ge, public_data);
34 /* If there are any issues with the recipient scan key or public data, return early */
35+ secp256k1_declassify(ctx, &ret, sizeof(ret));
36 if (!ret) {
37 return 0;
38 }
Sorry, stopping CI here. We’re about to make a release and need to the CI. :)
We’ll restart the jobs here afterwards.
Update 9e85256 -> a4db279 (2025_01 -> 2025_02, compare)
_recipient_created_shared_secret and _recipient_created_output_pubkey functions (h/t @theStack )_recipient_scan_outputs
(*arg)[size] in this PR and opened #1710 for discussion, since this is a broader topic than just this PR. The relevant changes for here and the downstream Bitcoin Core PRs are https://github.com/josibake/secp256k1/commit/5a1088066b2ce5e2e77b4e4bc190575d1171c374 and https://github.com/josibake/bitcoin/commit/5835d987477fc8eae391e4d1cc5033d921925ea1
145+ }
146+ ret = secp256k1_eckey_pubkey_tweak_add(&P_output_ge, &t_k_scalar);
147+ /* 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. */
148+ /* TODO: consider declassify ret */
149+ /* TODO: but we don't want to imply this can never happen */
150+ VERIFY_CHECK(ret);
_pubkey_tweak_add indeed fails? Initially I assumed that doing that would introduce a branch that can’t be tested in practice, but I think it is: by picking arbitrary shared_secret33 and k values, and calculating recipient_labeled_spend_pubkey as -G^(_create_t_k(shared_secret33, k)) the tweaking should fail with these three values. Will try to come up with some test code later to verify.
Ok, managed to come up with a test that makes the VERIFY_CHECK on the _pubkey_tweak_add return value fail:
0diff --git a/src/modules/silentpayments/tests_impl.h b/src/modules/silentpayments/tests_impl.h
1index a1acab7..2a10791 100644
2--- a/src/modules/silentpayments/tests_impl.h
3+++ b/src/modules/silentpayments/tests_impl.h
4@@ -367,6 +367,23 @@ static void test_recipient_api(void) {
5 CHECK_ILLEGAL(CTX, secp256k1_silentpayments_recipient_create_output_pubkey(CTX, &t, NULL, &p, 0));
6 CHECK_ILLEGAL(CTX, secp256k1_silentpayments_recipient_create_output_pubkey(CTX, &t, o, NULL, 0));
7
8+ /* check the _recipient_create_output_pubkey cornercase where internal tweaking would fail;
9+ this is the case if the recipient spend public key is P = -(create_t_k(shared_secret, k))*G */
10+ {
11+ const unsigned char *shared_secret = o;
12+ const uint32_t k = 0;
13+ secp256k1_scalar t_k;
14+ unsigned char t_k_ser[32];
15+ secp256k1_pubkey fake_spend_pubkey;
16+ secp256k1_xonly_pubkey output_xonly;
17+
18+ secp256k1_silentpayments_create_t_k(&t_k, shared_secret, k);
19+ secp256k1_scalar_get_b32(t_k_ser, &t_k);
20+ CHECK(secp256k1_ec_pubkey_create(CTX, &fake_spend_pubkey, t_k_ser));
21+ CHECK(secp256k1_ec_pubkey_negate(CTX, &fake_spend_pubkey));
22+ CHECK(secp256k1_silentpayments_create_output_pubkey(CTX, &output_xonly, shared_secret, &fake_spend_pubkey, k) == 0);
23+ }
24+
25 n_f = 0;
26 labels_cache.entries_used = 0;
27 CHECK(secp256k1_silentpayments_recipient_scan_outputs(CTX, fp, &n_f, tp, 1, ALICE_SECKEY, &pd, &p, &label_lookup, &labels_cache));
I assume that in practice a user can’t be tricked into such a scenario though, as the recipient spend public key is known/generated before the shared secret is.
247+ }
248+ /* Compute input_hash = hash(outpoint_L || (a_sum * G)) */
249+ secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &A_sum_gej, &a_sum_scalar);
250+ secp256k1_ge_set_gej(&A_sum_ge, &A_sum_gej);
251+ /* TODO: comment */
252+ secp256k1_declassify(ctx, &A_sum_ge, sizeof(A_sum_ge));
Suggestion for the comment to tackle another TODO 🔫 (IIUC, the non-constant timing of group element serialization seems to ultimately be caused by variable-time normalization of field elements):
0 /* Need to declassify the pubkey sum because serializing a group element (done in the
1 `_calculate_input_hash` call following) is not a constant-time operation */
2 secp256k1_declassify(ctx, &A_sum_ge, sizeof(A_sum_ge));
142+ secp256k1_scalar_clear(&t_k_scalar);
143+ return 0;
144+ }
145+ /* `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.
146+ * This will never happen under normal usage, but we handle this error to anyways to protect against this function being called with a malicious
147+ * B_spend argument, i.e., B_spend = -G^(_create_t_k(shared_secret33, k))
only noticing now that in one of my earlier comments I was using multiplicative notation (cryptocamp leaving its marks already I guess 😁)
0 * B_spend argument, i.e., B_spend = -(_create_t_k(shared_secret33, k))*G
-” symbol in -G^(_create_t_k(shared_secret33, k)) is supposed to mean. :stuck_out_tongue:
283+ secp256k1_silentpayments_create_shared_secret(ctx, shared_secret, &a_sum_scalar, &pk);
284+ k = 0;
285+ }
286+ if (!secp256k1_silentpayments_create_output_pubkey(ctx, generated_outputs[recipients[i]->index], shared_secret, &recipients[i]->labeled_spend_pubkey, k)) {
287+ secp256k1_scalar_clear(&a_sum_scalar);
288+ return 0;
146+
147+ /* Before we can call actual API functions, we need to create a "context" */
148+ secp256k1_context* ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE);
149+ if (!fill_random(randomize, sizeof(randomize))) {
150+ printf("Failed to generate randomness\n");
151+ return 1;
Update 1a84908 -> 2948a9b (2025_03 -> 2025_04, compare)
EXIT_SUCCESS/EXIT_FAILURE (h/t @theStack)Thanks for the thorough review, @theStack !
Update 2948a9b -> 64ecd6c (2025_04 -> 2025_05, compare)
_cmovinput_hash, now that this is properly specified in the BIPcc @jonasnick and @real-or-random regarding the use of a VERIFY_CHECK in favour of returning an error, when returning an error results in an untestable branch. I’m happy with the approach here were we use a VERIFY_CHECK for input_hash and t_k to check for an overflow of the curve order. However, given this is something we’ve discussed a few times in the post, would be great to hear your thoughts on this and I’m happy to defer to whatever you both think is best.
This should address all of the outstanding TODOs (at least the ones we left comments for 😅 )
106+ k
107+ ));
108+ }
109+}
110+
111+static void bench_silentpayments_full_tx_scan(void* arg, int iters) {
It might make sense to split this benchmark into “labeled” and “unlabeled” variants in order to also get a feeling how overhead the labels scanning causes, with something like e.g.:
0diff --git a/src/modules/silentpayments/bench_impl.h b/src/modules/silentpayments/bench_impl.h
1index 1dd1e5a..366d172 100644
2--- a/src/modules/silentpayments/bench_impl.h
3+++ b/src/modules/silentpayments/bench_impl.h
4@@ -108,7 +108,7 @@ static void bench_silentpayments_output_scan(void* arg, int iters) {
5 }
6 }
7
8-static void bench_silentpayments_full_tx_scan(void* arg, int iters) {
9+static void bench_silentpayments_full_tx_scan(void* arg, int iters, int use_labels) {
10 int i;
11 size_t n_found = 0;
12 secp256k1_silentpayments_found_output *found_output_ptrs[2];
13@@ -116,6 +116,8 @@ static void bench_silentpayments_full_tx_scan(void* arg, int iters) {
14 const secp256k1_xonly_pubkey *tx_input_ptrs[2];
15 bench_silentpayments_data *data = (bench_silentpayments_data*)arg;
16 secp256k1_silentpayments_recipient_public_data public_data;
17+ const secp256k1_silentpayments_label_lookup label_lookup_fn = use_labels ? label_lookup : NULL;
18+ const void *label_context = use_labels ? label_cache : NULL;
19
20 for (i = 0; i < 2; i++) {
21 found_output_ptrs[i] = &data->found_outputs[i];
22@@ -135,11 +137,19 @@ static void bench_silentpayments_full_tx_scan(void* arg, int iters) {
23 data->scan_key,
24 &public_data,
25 &data->spend_pubkey,
26- label_lookup, label_cache)
27+ label_lookup_fn, label_context)
28 );
29 }
30 }
31
32+static void bench_silentpayments_full_tx_scan_unlabeled(void *arg, int iters) {
33+ bench_silentpayments_full_tx_scan(arg, iters, 0);
34+}
35+
36+static void bench_silentpayments_full_tx_scan_labeled(void *arg, int iters) {
37+ bench_silentpayments_full_tx_scan(arg, iters, 1);
38+}
39+
40 static void run_silentpayments_bench(int iters, int argc, char** argv) {
41 bench_silentpayments_data data;
42 int d = argc == 1;
43@@ -147,7 +157,8 @@ static void run_silentpayments_bench(int iters, int argc, char** argv) {
44 /* create a context with no capabilities */
45 data.ctx = secp256k1_context_create(SECP256K1_FLAGS_TYPE_CONTEXT);
46
47- if (d || have_flag(argc, argv, "silentpayments")) run_benchmark("silentpayments_full_tx_scan", bench_silentpayments_full_tx_scan, bench_silentpayments_scan_setup, NULL, &data, 10, iters);
48+ if (d || have_flag(argc, argv, "silentpayments")) run_benchmark("silentpayments_full_tx_scan_labeled", bench_silentpayments_full_tx_scan_labeled, bench_silentpayments_scan_setup, NULL, &data, 10, iters);
49+ if (d || have_flag(argc, argv, "silentpayments")) run_benchmark("silentpayments_full_tx_scan_unlabeled", bench_silentpayments_full_tx_scan_unlabeled, bench_silentpayments_scan_setup, NULL, &data, 10, iters);
50 if (d || have_flag(argc, argv, "silentpayments")) run_benchmark("silentpayments_output_scan", bench_silentpayments_output_scan, bench_silentpayments_scan_setup, NULL, &data, 10, iters);
51
52 secp256k1_context_destroy(data.ctx);
0Benchmark , Min(us) , Avg(us) , Max(us)
1
2silentpayments_full_tx_scan_labeled, 94.6 , 96.6 , 101.0
3silentpayments_full_tx_scan_unlabeled, 82.7 , 84.2 , 85.9
4silentpayments_output_scan , 85.3 , 87.5 , 90.7
Agreed. I added the dummy labels originally to make sure I was reporting worst-case1 numbers for the full scan benchmark, and the idea was to add more benchmarks in a follow-up that more accurately benchmark labels. However, I think what you have here is nicer and we can still leave a TODO comment to add a more representative benchmark for scanning with labels.
worst case in the sense that all of the scan-with-labels code paths are hit during the benchmark. Its not an actual worst case in that you could have a very large, poorly implemented labels cache which would necessarily slow down scanning ↩︎
std::unordered_map?). But no hard feelings on the details of where/how, I agree that seeing such a followup would be very interesting indeed.
Updated 64ecd6c -> 3c4af8f (2025_05 -> 2025_06, compare)
_full_scan and full_scan_with_labels30+ int diff = p1[i] - p2[i];
31+ if (diff != 0) {
32+ return diff;
33+ }
34+ }
35+ return EXIT_SUCCESS;
in 070ea002339bf10603da5dd772795c7ccae3af51, function my_memcmp_var
0 return 0;
as this doesn’t indicate a process execution status
In the CI commit, could add the silent payments module also to the native macOS arm64 job (as done for musig recently in #1699), e.g.
0diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
1index 8ee13ce..f612a84 100644
2--- a/.github/workflows/ci.yml
3+++ b/.github/workflows/ci.yml
4@@ -583,13 +583,13 @@ jobs:
5 fail-fast: false
6 matrix:
7 env_vars:
8- - { WIDEMUL: 'int64', RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes' }
9+ - { WIDEMUL: 'int64', RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes', SILENTPAYMENTS: 'yes' }
10 - { WIDEMUL: 'int128_struct', ECMULTGENPRECISION: 2, ECMULTWINDOW: 4 }
11- - { WIDEMUL: 'int128', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes' }
12+ - { WIDEMUL: 'int128', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes', SILENTPAYMENTS: 'yes' }
13 - { WIDEMUL: 'int128', RECOVERY: 'yes' }
14- - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes' }
15- - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes', CC: 'gcc' }
16- - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes', CPPFLAGS: '-DVERIFY' }
17+ - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes', SILENTPAYMENTS: 'yes' }
18+ - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes', SILENTPAYMENTS: 'yes', CC: 'gcc' }
19+ - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes', SILENTPAYMENTS: 'yes', CPPFLAGS: '-DVERIFY' }
20 - BUILD: 'distcheck'
21
22 steps:
Updated 3c4af8f -> eb32d06 (2025_06 -> 2025_07, compare)
While working through the test coverage, I realised the two public data internal functions public_data_load_pubkey and public_data_load_input_hash are unnecessary, as they were just thin wrappers around secp256k1 function calls that never return an error.
I’ve removed those functions and instead ensured we always check a public data object for correctness before accessing the internals.
48+
49+/** Create Silent Payment outputs for recipient(s).
50+ *
51+ * Given a list of n secret keys a_1...a_n (one for each silent payment
52+ * eligible input to spend), a serialized outpoint, and a list of recipients,
53+ * create the taproot outputs.
59+ * payment. Determining the smallest outpoint from the list of transaction
60+ * inputs is the responsibility of the caller. It is strongly recommended
61+ * that implementations ensure they are doing this correctly by using the
62+ * test vectors from BIP352.
63+ *
64+ * If necessary, the secret keys are negated to enforce the right y-parity.
57+ * inputs). This value MUST be the smallest outpoint out of all of the
58+ * transaction inputs, otherwise the recipient will be unable to find the
59+ * payment. Determining the smallest outpoint from the list of transaction
60+ * inputs is the responsibility of the caller. It is strongly recommended
61+ * that implementations ensure they are doing this correctly by using the
62+ * test vectors from BIP352.
6@@ -7,10 +7,299 @@
7 #define SECP256K1_MODULE_SILENTPAYMENTS_MAIN_H
8
9 #include "../../../include/secp256k1.h"
10+#include "../../../include/secp256k1_extrakeys.h"
11 #include "../../../include/secp256k1_silentpayments.h"
12
13-/* TODO: implement functions for sender side. */
14+/** Sort an array of silent payment recipients. This is used to group recipients by scan pubkey to
15+ * ensure the correct values of k are used when creating multiple outputs for a recipient. */
In eeed5671de059d5812571dc0bfcaee12f10f71e2 silentpayments: sending: from offline conversation my understanding is as follows:
We don’t add a tie breaker here (e.g. ->index) because:
If so, it would be good to say this in a comment so nobody is a) confused b) tempted later to introduce a footgun.
(the temptation might come from a user of this library who finds that their tests are breaking half of the time because they had an ordering related bug - or just natural programmer desire for deterministic sort )
Following up on this, I misspoke regarding 2). Its not about being constant time (heapsort is $O(n\log{}n)$ time, $O(1)$ space), but rather heapsort is unstable so there isn’t a straightforward way to do a stable, e.g., multi-key, sort.
Even if there were a straightforward way, I’d still push back since the protocol is designed to not rely on ordering. Rather, the BIP only specifies grouping by scan key and sorting is just our way of implementing “grouping.” I’ll flesh out the comment to make this more clear.
34+ #if defined(_MSC_VER) && (_MSC_VER < 1933)
35+ #pragma warning(pop)
36+ #endif
37+}
38+
39+/** Set hash state to the BIP340 tagged hash midstate for "BIP0352/Inputs". */
BIP0352/...?
hashlib it’s unfortunately not possible to access the internal state :/).
see https://gist.github.com/real-or-random/a4aaae5d04eee9f63e7a2e43d25bc2b1#file-sha256_midstate-py
But I think the “proper"TM way is that josie adds a test that checks if the finalized hash is the same as when recomputed from scratch, see https://github.com/bitcoin-core/secp256k1/blob/e523e4f90e1b1c0fba49cd8a08016e1a8dff9232/src/modules/musig/tests_impl.h#L551-L586. Then the reviewer only needs to review this check.
294+ if (!secp256k1_silentpayments_create_output_pubkey(ctx, generated_outputs[recipients[i]->index], shared_secret, &recipients[i]->labeled_spend_pubkey, k)) {
295+ secp256k1_scalar_clear(&a_sum_scalar);
296+ secp256k1_memclear(&shared_secret, sizeof(shared_secret));
297+ return 0;
298+ }
299+ k++;
k later will cause the recipient to not find outputs > k
25@@ -25,6 +26,93 @@ extern "C" {
26 * any further elliptic-curve operations from the wallet.
27 */
28
29+/** This struct serves as an input parameter for passing the silent payment
30+ * address data to `silentpayments_sender_create_outputs`.
31+ *
32+ * The index field is for when more than one address is being sent to in
33+ * a transaction. Index is set to the position of this recipient in the
266+ *
267+ * More specifically, this ensures `k` is incremented from 0 to the number of requested outputs for each recipient group,
268+ * where a recipient group is all addresses with the same scan public key.
269+ */
270+ secp256k1_silentpayments_recipient_sort(ctx, recipients, n_recipients);
271+ last_recipient = *recipients[0];
In eeed5671de059d5812571dc0bfcaee12f10f71e2 silentpayments: sending: so the last shall be first, and the first last :-)
I think it would be more clear to store recipients[0]->scan_pubkey instead and call it current_scan_pubkey.
Reviewed eeed5671de059d5812571dc0bfcaee12f10f71e2 silentpayments: sending again and it looks good to me (not a C expert). Comments mainly about documentation and one variable name suggestion.
It could make sense to split 993d34b7ae3d4a8b6d3a8957b309b38e7e50e796 tests: add BIP-352 test vectors and move the send-side tests directly after this commit. Ditto for the send example in ce7cb982d4aceefd8bb8591fd069c3a0b81d1ef2 silentpayments: add examples/silentpayments.c.
It could make sense to split https://github.com/bitcoin-core/secp256k1/commit/993d34b7ae3d4a8b6d3a8957b309b38e7e50e796 tests: add BIP-352 test vectors and move the send-side tests directly after this commit. Ditto for the send example in https://github.com/bitcoin-core/secp256k1/commit/ce7cb982d4aceefd8bb8591fd069c3a0b81d1ef2 silentpayments: add examples/silentpayments.c.
By splitting the test vectors commit into sending and receiving commits, it would be possible to break out just sending into its own PR (recall that sending can be implemented without implementing receiving). If this makes it easier to review and merge sending, I’m open to it. This would allow downstream work on the Bitcoin Core sending PR to proceed, while continued review on receiving happens here.
On the flipside, I do feel like the module in its current state is close to finished so I’m not sure how much benefit splitting at this stage would bring. I dropped a note in the #secp256k1 IRC; will wait to hear from other reviewers/maintainers before making a decision here.
247+ }
248+ /* To keep things simple, we cast the tx_output_ptr array to remove the
249+ * const qualifer, so that we can create the outputs. We want the const
250+ * qualifer because this same array will be passed to the scan function
251+ * later in the example.
252+ */
tx_output_ptrs is now happening at the scan function below (from non-const to const rather than the other way round)
454+ for (i = 0; i < n_found_outputs; i++) {
455+ printf(" ");
456+ secp256k1_xonly_pubkey_serialize(ctx,
457+ serialized_xonly,
458+ &found_outputs[i].output
459+ );
in e0c225c047645ae92db877f661ede385f4f8603d:
0 ret = secp256k1_xonly_pubkey_serialize(ctx,
1 serialized_xonly,
2 &found_outputs[i].output
3 );
4 assert(ret);
just to evaluate all API call return values for consistency (here and in the light client scanning loop below)
87+ * multiple outputs for the same recipient.
88+ * n_recipients: the number of recipients. This is equal to the
89+ * total number of outputs to be generated as each
90+ * recipient may passed multiple times to generate
91+ * multiple outputs for the same recipient
92+ * outpoint_smallest: serialized (36-byte) smallest outpoint
in e0c7a09415de00e94e8a8c8fc3c5154d00cc0739:
0 * outpoint_smallest36: serialized (36-byte) smallest outpoint
(also in the description above)
410+ * incremented for each additional output created
411+ * or after each output found when scanning)
412+ */
413+SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipient_create_output_pubkey(
414+ const secp256k1_context *ctx,
415+ secp256k1_xonly_pubkey *P_output_xonly,
P_ prefix as we don’t use this naming scheme anywhere else in this or other modules (I think I introduced that in the first take of the BIP352 module for some reason)
Left some comments with minor suggestions regarding API header and the example, I’m planning to finish the full review this week.
No strong opinion on whether a split-up into sending/receiving PRs makes sense, from a reviewing point of view it likely doesn’t make a difference for me at this point. I could imagine it’s potentially brittle to only release the sending part if there is no strong test coverage provided that the generated outputs can also be found by receivers, so I’d slightly lean to keep as-is, I guess.
Updated b26c87d -> 393a2a2 (2025_08 -> 2025_09, compare)
example fix-ups (h/t @theStack)s/outpoint_smallest/outpoint_smallest36/ (h/t @theStack )s/P_output_xonly/output_xonly/ (h/t @theStack )
@theStack thanks for the continued review!I could imagine it’s potentially brittle to only release the sending part if there is no strong test coverage provided that the generated outputs can also be found by receivers
For this, I think it would be sufficient to show that the same inputs/outputs from the test vectors used in the sending PR are the same ones used in the receiving PR. But overall, I’m also leaning towards keeping everything together unless its expected that the sending commit is largely good to go but more review is required for the benchmarks, example, and receiving commits.
Updated 393a2a2 -> 96b2362 (2025_09 -> 2025_10, compare)
cc @real-or-random @jonasnick on the header documentation. A majority of the error cases are can only happen if the user provides malformed inputs, or the output of a hash function results in something greater than the curve order. I opted to document these, but wanted to confirm to make sure I am doing something consistent with the rest of the library.
55@@ -53,7 +56,7 @@ typedef struct secp256k1_silentpayments_recipient {
56 * public keys are excluded from silent payments eligible inputs; see BIP352
57 * for more information.
58 *
59- * `outpoint_smallest` refers to the smallest outpoint lexicographically
60+ * `outpoint_smallest36` refers to the smallest outpoint lexicographically
68+ * (taproot) outputs or not.
69+ *
70+ * Returns: 1 if creation of outputs was successful.
71+ * 0 if the input private keys sum to zero,
72+ * or input_hash or hash(shared_secret || k) are invalid scalars
73+ * (stastically improbable).
0 * (statistically improbable).
(here and in many other instances)
60+ * payment. Determining the smallest outpoint from the list of transaction
61+ * inputs is the responsibility of the caller. It is strongly recommended
62+ * that implementations ensure they are doing this correctly by using the
63+ * test vectors from BIP352.
64+ *
65+ * If necessary, the taproot secret keys are negated to enforce even y-parity.
libsecp256k1 already has different data types for plain keys and taproot keys, which I think is explanation enough for a user as to why we have two different arguments for accepting secret keys in this function.
I’ve convinced myself that the implementation in this PR matches the BIP352 specification and the usual quality requirements, especially regarding treatment of secret data (constant-time ops, clearing out the memory) are met from what I can tell.
The remaining comments are just nits for typos/minimizing-diff and a general question about the detail level of header API documentation. Other than that, I think I’d be ready to give a “take it with a grain of salt, i’m not a cryptographer”-ACK at this point :detective:
232+ memset(&r[1].labeled_spend_pubkey.data, 0, sizeof(secp256k1_pubkey));
233+ CHECK_ILLEGAL(CTX, secp256k1_silentpayments_sender_create_outputs(CTX, op, rp, 2, SMALLEST_OUTPOINT, NULL, 0, p, 1));
234+ {
235+ secp256k1_pubkey tmp = r[1].labeled_spend_pubkey;
236+ memset(&r[1].labeled_spend_pubkey, 0, sizeof(r[1].labeled_spend_pubkey));
237+ CHECK_ILLEGAL(CTX, secp256k1_silentpayments_sender_create_outputs(CTX, op, rp, 2, SMALLEST_OUTPOINT, NULL, 0, p, 1));
memset is a no-op and the test a repetition of the one four lines above. Was the idea here to run the previous test without damaging the spend pubkey of r[1] first? (would make sense considering that it is saved and restored via tmp)
14+ * Seckey: secret key for Alice
15+ * Outputs: generated outputs from Alice's secret key and Bob/Carol's
16+ * scan public keys
17+ * Smallest Outpoint: smallest outpoint lexicographically from the transaction
18+ * orderc: a scalar which overflows the secp256k1 group order
19+ * Malformed Seckey: a seckey that is all zeros
270+/** Callback function for label lookups
271+ *
272+ * This function is implemented by the recipient to check if a value exists in
273+ * the recipients label cache during scanning.
274+ *
275+ * For creating the labels cache,
nit: slightly more specific (as the current wording implies that the actual cache data structure can be created with this module)
0 * For creating the labels cache data (label public key and label tweak),
or just
0 * For creating the labels cache data,
_create_label function.
130@@ -131,28 +131,28 @@ static void secp256k1_silentpayments_create_t_k(secp256k1_scalar *t_k_scalar, co
131 secp256k1_sha256_clear(&hash);
132 }
133
134-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_labeled_spend_pubkey, uint32_t k) {
135- secp256k1_ge P_output_ge;
136+static int secp256k1_silentpayments_create_output_pubkey(const secp256k1_context *ctx, secp256k1_xonly_pubkey *output_xonly, const unsigned char *shared_secret33, const secp256k1_pubkey *recipient_labeled_spend_pubkey, uint32_t k) {
500+ */
501+ secp256k1_ge_from_bytes(&ge, &public_data->data[5]);
502+ secp256k1_scalar_set_b32(&input_hash_scalar, &public_data->data[5 + 64], NULL);
503+ secp256k1_eckey_pubkey_tweak_mul(&ge, &input_hash_scalar);
504+ secp256k1_eckey_pubkey_serialize(&ge, output33, &pubkeylen, 1);
505+ VERIFY_CHECK(pubkeylen == 33);
VERIFY_CHECK the return values of these two function calls
661+ VERIFY_CHECK(ret);
662+ secp256k1_pubkey_save(&found_outputs[n_found]->label, &label_ge);
663+ } else {
664+ found_outputs[n_found]->found_with_label = 0;
665+ /* Set the label public key with an invalid public key value */
666+ secp256k1_memclear(&found_outputs[n_found]->label, sizeof(secp256k1_pubkey));
memset is sufficient here, as this is no stack data (and not secret data either)
706+ }
707+ secp256k1_silentpayments_create_shared_secret(ctx, shared_secret33, &rsk, &A_tweaked_ge);
708+
709+ /* Explicitly clear secrets */
710+ secp256k1_scalar_clear(&rsk);
711+ return ret;
equivalent but more explicit (ret can only be 1 at this point)
0 return 1;
273@@ -273,6 +274,10 @@ src/wycheproof/ecdh_secp256k1_test.h:
274 mkdir -p $(@D)
275 python3 $(top_srcdir)/tools/tests_wycheproof_generate_ecdh.py $(top_srcdir)/src/wycheproof/ecdh_secp256k1_test.json > $@
276
277+src/modules/silentpayments/vectors.h:
278+ mkdir -p $(@D)
279+ python3 $(top_srcdir)/tools/tests_silentpayments_generate.py $(top_srcdir)/src/modules/silentpayments/bip352_send_and_receive_test_vectors.json > $@
(if at all, I assume test vectors are usually just updated by manual script runs rather than involving the build system)
The current idea is that you can manually delete the files, and the build system will then regenerate them (and you don’t need to worry about how this is done). And yes, that’s restricted to autotools, which is fine because it’s a developer-only thing. Decoupling this from the build system entirely would also be an option.
Anyway, none of this is related to this PR.
408+ enable_module_schnorrsig=yes
409+
410+ if test x"$enable_module_extrakeys" = x"no"; then
411+ AC_MSG_ERROR([Module dependency error: You have disabled the extrakeys module explicitly, but it is required by the silentpayments module.])
412+ fi
413+ enable_module_extrakeys=yes
https://github.com/bitcoin-core/secp256k1/pull/1698/commits/ca2538a8782628437488ca32c49f52957aff5f69 I’d drop this check for consistency. (But nothing bad happens if we keep it.)
204+ }
205+ ARG_CHECK(outpoint_smallest36 != NULL);
206+ /* ensure the index field is set correctly */
207+ for (i = 0; i < n_recipients; i++) {
208+ ARG_CHECK(recipients[i]->index == i);
209+ }
I vaguely remember discussing this with you and @jonasnick previously where we were debating back and forth on whether the caller should set this or we should set it internally. The preference was to have the caller set the value as this seemed the least confusing of our options, but I can’t recall the reasons why 😅 .
If we keep the index field in the struct, then I do think we should have the caller set it, as it seems even more confusing to have the caller create the recipient structs without fully initialising them. That leaves removing the field entirely from the struct, but I don’t think there is a (sane) way to keep track of the original ordering internally so that we can ensure we map the generated outputs back to the original ordering.
So I’d prefer to leave it as-is, but perhaps we can better explain this in the API documentation?
index field is always zero (maybe renaming it to something like _internal, to express that the user doesn’t have to care what it is used for), and then set it internally, but not sure if that is better.
index field, but this seems hard in C (or at least, it comes with a bunch of other disadvantages). So yes, let’s keep this unless someone has a great idea to get rid of this field without introducing any other overhead. In any case, having the field is safe because we check that the value is set correctly. It’s just a minor annoyance for the caller.
API in which the user passes a data structure which doesn’t even have an index field, but this seems hard in C (or at least, it comes with a bunch of other disadvantages).
Just wanted to confirm this is where I landed as well, i.e., its annoying to have the field but trying to work around the field is even more annoying.
36+ * The spend public key field is named `labeled_spend_pubkey` to indicate this
37+ * spend public key may be tweaked with an optional label. This is not relevant
38+ * for the sender and is purely a documentation convention to differentiate
39+ * between other uses of `spend_pubkey` in this API, where it is meant to refer
40+ * to the unlabeled spend public key.
41+ */
https://github.com/bitcoin-core/secp256k1/commit/1a5f53f2cf7d3228b7ed147282ea5bd796957cbf some writing improvements. if you take them, please reformat
0/** The data from a single recipient address
1 *
2 * This struct serves as an input argument to `silentpayments_sender_create_outputs`.
3 *
4 * `index` must be set to the position (starting with 0) of this recipient in the
5 * `recipients` array passed to `silentpayments_sender_create_outputs`. It is
6 * used to map the returned generated outputs back to the original recipient.
7 *
8 * Note:
9 * The spend public key field is named `labeled_spend_pubkey` to indicate that this
10 * spend public key may have been optionally tweaked with a label by the sender.
11 * This is purely a documentation convention to differentiate
12 * between other uses of `spend_pubkey` in this API, where it is meant to refer
13 * to the unlabeled spend public key.
14 * Whether `labeled_spend_pubkey` has actually been tagged is irrelevant
15 * for the sender and
16 */
spend_pubkey and unlabeled_spend_pubkey or (raw_ or whatever) instead?
spend_pubkey and unlabeled_spend_pubkey, which I think is much better than what we had before.
8+
9+#include "../../../include/secp256k1_silentpayments.h"
10+
11+/** Constants
12+ *
13+ * orderc: a scalar which overflows the secp256k1 group order
CHECK(secp256k1_ec_seckey_verify(CTX, orderc) == 0); . This saves the reviewer from looking up the group order and doing the comparison manually.
30+static unsigned char BOB_ADDRESS[2][33] = {
31+ {
32+ 0x02,0x15,0x40,0xae,0xa8,0x97,0x54,0x7a,
33+ 0xd4,0x39,0xb4,0xe0,0xf6,0x09,0xe5,0xf0,
34+ 0xfa,0x63,0xde,0x89,0xab,0x11,0xed,0xe3,
35+ 0x1e,0x8c,0xde,0x4b,0xe2,0x19,0x42,0x5f,0x23
109+ recipient_ptrs, 3,
110+ SMALLEST_OUTPOINT,
111+ NULL, 0,
112+ seckey_ptrs, 1
113+ );
114+ CHECK(ret);
https://github.com/bitcoin-core/secp256k1/commit/1a5f53f2cf7d3228b7ed147282ea5bd796957cbf
0 CHECK(ret == 1);
this is what the docs promise.
21@@ -22,6 +22,7 @@ Features:
22 * Optional module for Schnorr signatures according to [BIP-340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki).
23 * Optional module for ElligatorSwift key exchange according to [BIP-324](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki).
24 * Optional module for MuSig2 Schnorr multi-signatures according to [BIP-327](https://github.com/bitcoin/bips/blob/master/bip-0327.mediawiki).
25+* Optional module for Silent Payments send and receive according to [BIP-352](https://github.com/bitcoin/bips/blob/master/bip-0352.mediawiki).
73+ * provided in the recipient objects.
74+ * In: recipients: pointer to an array of pointers to silent payment
75+ * recipients, where each recipient is a scan public
76+ * key, a spend public key, and an index indicating
77+ * its position in the original ordering. The
78+ * recipient array will be sorted in place, but
What about:
0diff --git a/include/secp256k1_silentpayments.h b/include/secp256k1_silentpayments.h
1index f547207..108c61c 100644
2--- a/include/secp256k1_silentpayments.h
3+++ b/include/secp256k1_silentpayments.h
4@@ -80,14 +80,15 @@ typedef struct secp256k1_silentpayments_recipient {
5 * recipients, where each recipient is a scan public
6 * key, a spend public key, and an index indicating
7 * its position in the original ordering. The
8- * recipient array will be sorted in place, but
9- * generated outputs are saved in the
10- * `generated_outputs` array to match the ordering
11- * from the index field. This ensures the caller is
12- * able to match the generated outputs to the
13- * correct silent payment addresses. The same
14- * recipient can be passed multiple times to create
15- * multiple outputs for the same recipient.
16+ * recipient array will be grouped by scan public key
17+ * in place (as specified in BIP0352), but generated
18+ * outputs are saved in the `generated_outputs` array
19+ * to match the original ordering (using the index
20+ * field). This ensures the caller is able to match
21+ * the generated outputs to the correct silent
22+ * payment addresses. The same recipient can be
23+ * passed multiple times to create multiple outputs
24+ * for the same recipient.
25 * n_recipients: the number of recipients. This is equal to the
26 * total number of outputs to be generated as each
27 * recipient may passed multiple times to generate
13-/* TODO: implement functions for sender side. */
14+/** Sort an array of silent payment recipients. This is used to group recipients by scan pubkey to
15+ * ensure the correct values of k are used when creating multiple outputs for a recipient.
16+ *
17+ * Note: only grouping, not deterministic ordering, of the scan public keys is required by the protocol.
18+ * As such, users of the library cannot and should not rely on deterministic sorting of the recipients.
secp256k1_silentpayments_sender_create_outputs sorting the recipients? If yes, this contradicts the API docs. (And the implementation file is the wrong place to make such a statement.)
Looking at this with fresh eyes, I think the need for this comment arose from the fact the API docs were perhaps giving too much information about the sorting implementation and creating confusion.
I think it would be better to remove any mentions of sorting from the API, e.g., #1698 (review), and then remove the “Note: " clarifying comment.
Updated 2b57d2a -> a428424 (2025_12 -> 2025_13, compare)
spend_pubkey to indicate a spend public key that might also have a tweak and unlabeled_spend_pubkey we mean to refer the unlabeled spend public key (vs labeled_spend_pubkey and spend_pubkey) (h/t @real-or-random )Thanks for the review @real-or-random , much prefer your wording suggestions. I pushed a version of the docs that I think helps minimise confusion about sorting, but curious to hear your thoughts as I imagine these docs will still need a bit of refining.
593+
594+ /* Calculate P_output = B_spend + t_k * G
595+ * This can fail if t_k is the negation of B_spend, but this is statistically
596+ * improbable as t_k is the output of a hash function. */
597+ ret = secp256k1_eckey_pubkey_tweak_add(&P_output_ge, &t_k_scalar);
598+ VERIFY_CHECK(ret);
We did discuss this and decided it was improbable due to t_k_scalar being the output of a hash function, where the allowable hash preimage values can only come from the senders spendable UTXOs.
That being said, you’re right that we can create a test case for this by first picking a t_k_scalar value and then choosing a B_spend that is the negation of t_k_scalar. In practice, I don’t think this could ever happen since the sender is not in control of B_spend, but you could have a malicious receiver that puts out silent payment addresses with maliciously crafted B_spends. Based on that, I think it’s worth adding a test case and handling the error in this function.
Yes, we use VERIFY_CHECK like an assertion, i.e., there should be no API calls that make the asserted condition false. Now, we make an exception for inputs that are “cryptographically hard to find” (i.e., a computationally limited adversary will find them only with negligible probability under hardness assumptions that we accept). This is justified because noone will actually be able to hit the assertion in the real world (unless our assumptions turn out to be wrong).
But I think we should not extend our set of assumptions to things like “the API function is called on inputs created from spendable UTXOs in some existing blockchain” or similar. Or in other words: if you can come up with a test case against the public API that triggers that VERIFY_CHECK, then it should not be a VERIFY_CHECK.
Or in other words: if you can come up with a test case against the public API that triggers that VERIFY_CHECK, then it should not be a VERIFY_CHECK.
Thanks for the explanation, I’ve always been a bit hazy on when/how to use VERIFY_CHECK appropriately; this cleared it up for me! Will add a test for this and remove the VERIFY_CHECK in my next push.
286+ CHECK(secp256k1_ec_pubkey_negate(CTX, &neg_spend_pubkey));
287+ CHECK(secp256k1_silentpayments_recipient_create_labeled_spend_pubkey(CTX, &ls, &s, &neg_spend_pubkey) == 0);
288+ memset(&l, 0, sizeof(l));
289+ CHECK_ILLEGAL(CTX, secp256k1_silentpayments_recipient_create_labeled_spend_pubkey(CTX, &ls, &s, &l));
290+ }
291+}
Updated 99aef92 -> 2164d5c (2025_14 -> 2025_15, compare)
Primarily updates the sending commit, with a few changes to the receiving commit:
Thanks for the thorough review, @real-or-random ! I reviewed each fixup commit and ended up taking all of them, as I agree with the rational and much prefer the wording(s) you suggested. Also, thanks for the fixup! branch; the branch made it super simple to integrate your changes.
18+
19+/* Use my_memcmp_var instead of memcmp.
20+ *
21+ * Normally, memcmp should be fine, but we use my_memcmp_var
22+ * here to avoid a false positive from valgrind on macOS.
23+ * TODO: remove this in the event the bug is fixed with valgrind in the future.
184+ * can use this serialization with
185+ * `secp256k1_silentpayments_recipient_public_data_parse`.
186+ */
187+typedef struct secp256k1_silentpayments_recipient_public_data {
188+ unsigned char data[101];
189+} secp256k1_silentpayments_recipient_public_data;
I suggest calling this a _transaction_summary. This is more descriptive.
Perhaps then public can be dropped because it’s a bit more implied from the name. And this is also implied because the function that creates it doesn’t get secret data. I think I slightly tend to drop public then. (My thinking is that, if it was secret data, then secret should totally be an explicit part of the name to warn the user, but it’s okay to drop public.) It should still be mentioned in the docs that this is just public data.
secp256k1_silentpayments_recipient_transaction_summary (my favorite)secp256k1_silentpayments_recipient_public_transaction_summarysecp256k1_silentpayments_recipient_public_tx_summarysecp256k1_silentpayments_recipient_public_tx_summary
Agree with dropping “public.” My only nit with calling this a transaction summary is that it contains data not found in a transaction, i.e., the prevout scriptPubKeys. The prevouts are referenced in a transaction (by outpoint). I think this is important because calling it transaction summary to me implies that I can create this with only the transaction itself (which is not true).
What about _recipient_inputs_summary? , or _prevouts_summary?
recipient_prevouts_summary is the right name. Technically speaking, what references a prevout is a transaction input, so _recipient_inputs_summary is similarly imprecise (though already much better than transaction_summary).
prevout_summary.
prevouts_summary is probably more correct 😅 Not sure why I didn’t make it plural. Can update in a later push to prevouts_summary if we feel that would be better.
prevouts_summary everywhere.
392+ * - `_recipient_public_data_serialize` only accepts a public_data object with combined = false
393+ * and then performs an EC mult before serializing the resulting public key as a compressed
394+ * public key
395+ * - `_recpient_public_data_parse` assumes the input represents a previously serialized
396+ * public_data object and always deserializes into a public_data object with combined = true
397+ * (and the input_hash portion zeroed out).
I find this unexpected. What I expect from any data structure that comes with serialize and parse methods is that serializing and re-parsing it doesn’t make a functional difference.
The need for boolean indicates that there should perhaps be two different data structures. Or, simpler: declare the serialized thing to be something else. This means that one way to get this cleaner is to split the scan function into two, one that takes recipient_public_data and one that takes serialized public data. But wait, we already have it like that: secp256k1_silentpayments_recipient_create_shared_secret is purely for light clients.
That suggests dropping _recpient_public_data_parse (and perhaps renaming _recipient_public_data_serialize to _recipient_public_data_export).
There’s a slight loss of functionality: The docs of _recipient_public_data_create say: “If calling this function for simply aggregating the public transaction data for later use, the caller can save the result with silentpayments_recipient_public_data_serialize”. Is this really a use case we’d like to offer? It’s not great in terms of performance. It will double the amount of necessary EC mults. Note that the caller could simply store the opaque data structure in memory for later use. Only if the caller wants to write it to permanent storage, then serialization will be necessary.
My thinking is that if this is not an important use case, then we should just drop it. But if we want this use case, then we suggest just that there should be a serialization function that serializes the “uncombined” variant for saving EC mults at the cost of increased storage requirements.
Alternative way: I can see why you want just one data structure. This is simply about lazy evaluation of the EC mult, and whether it has already happened or not is merely an implementation detail and should not make an observable difference to the API caller (except in terms of performance). But then I suggest implementing it like a real lazy evaluation:
_recipient_public_data_create always creates a public_data object with combined = false_recipient_public_data_serialize accepts any public_data object. If combined = false, it performs the EC mult before serializing the resulting public key as a compressed public key. If combined = true, it simply serializes_recpient_public_data_parse assumes the input represents a previously serialized public_data object and always deserializes into a public_data object with combined = true (and the input_hash portion zeroed out)._recipient_create_shared_secret can also accept both combined = true and combined = false. But if we think that calling _create_shared_secret latter necessarily implies the caller is doing something wrong, then my first suggestion above is probably better.
Agree that having _serialize and _parse methods that do not roundtrip is a bit code smelly.
If calling this function for simply aggregating the public transaction data for later use … Only if the caller wants to write it to permanent storage, then serialization will be necessary.
The comment in the code is a bit confusing. The only reason a caller would want to “aggregate for later use” is when building a silent payments index, for personal use (e.g., fast wallet rescans), or for serving the data to external light clients.
I much prefer your alternative suggestion, as this seems simpler than having multiple data structures and easier to document. I also think full nodes keeping their own silent payments index for fast wallet rescans and for supporting light clients will be a very common (and important!) use case to support.
_serialize can incur an EC mult in some cases. I don’t think there are any other performance implications that need documenting, since calling _recipient_create_shared_secret will always involve one EC mult, and sometimes a scalar mult, depending on the value of combined. But a scalar mult seems negligible enough that it shouldn’t warrant documenting.
I attached some high-level comments on the receiver API. I think these should be addressed before I dig deeper into the receiving code.
Some style things:
Full sentences in the comments should end with a period for readability (with the exception that short imperatives are fine). In some sentences my brain really expects more input after the end of the comment, e.g., “Additionally, we zero out the 32 bytes where the input_hash would be” (What would the hash be? :D)
And I’d prefer to avoid “formula-style” variable names like A_sum, B_m, P_output, and t_k in code. (The latter is borderline, I guess; at least it’s not capital.) Sometimes these appear only in the comments, but that can be confusing then because there are two names for the same thing, e.g., recipient_spend_pubkey vs B_spend. I think most (all?) of these can be resolved by replacing “A” with “input” or “sender,” as appropriate, and “B” with “receiver.”
On the other hand, having a “normal” variable name in the code but also the formula-style name in the comments or API docs may be a good idea if you assume that the reader/user is familiar with the BIP. But then I’d make it explicit in the comments, e.g., have a translation table somewhere (could even be a general comment at the top of the file) or say something like “recipient_spending_key (“B_spend” in the BIP)” whenever a variable occurs first in a comment.
To be honest, I’m not sure if it’s better to keep this connection to the BIP or avoid it entirely. An ideal API would be understandable without the BIP, but since the API covers only the cryptographic core of SP, any user will necessarily need to know some details from the BIP that go beyond what the API will do.
I pushed some (totally non-essential) fixup suggestions for the Python code to https://github.com/real-or-random/secp256k1/tree/bip352-silentpayments-module-2025-vector-fixups .
Related to this: I see why we need the ripemd160 and bech32m Python modules to parse addresses from the test vectors JSON file. This feels a bit wrong. This SP module covers some cryptographic core of SP, which does not deal with address encoding issues. So these files belong to full wallet implementations such as Bitcoin Core (where you got them from, I assume).
The only way around this that I see is that the intermediate results computed from the addresses (i.e., the extracted input pubkeys) would additionally be included in the JSON file. Do you think that would be a reasonable solution and acceptable from your point of view as a BIP author? Are there any drawbacks to this? I don’t see any, but I might be missing something.
edit: Oh, or we could compute the input pubkeys from the seckeys in the JSON. Preferably in the C tests because this will require no Python EC code.
(It’s not the end of the world to have this in our repo. I think this is my tendency as a maintainer to avoid code. The more code we have, the more we’ll need to maintain.)
Some style things:
Thanks for the feedback! Agree with all of your points and will implement. Regarding keeping notation in line with the BIP or removing entirely, I’d prefer to avoid the BIP as much as possible. As you said, the API should be understandable without the BIP.
How the BIP and module are written today is more of a historic “oopsie.” Ideally, the protocol should have been specified in a more module way, i.e., a cryptographic primitive (lets call it “non-interactive-payment”, or “multiparty ECDH”) and then a Bitcoin specific BIP that specifies how to use this NIP/MPECDH crypto primitive in a bitcoin specific context.
TLDR; I think we should keep this module more aligned with how things “should” have been done.
intermediate results computed from the addresses (i.e., the extracted input pubkeys) would additionally be included in the JSON file. Do you think that would be a reasonable solution and acceptable from your point of view as a BIP author? Are there any drawbacks to this? I don’t see any, but I might be missing something.
Regarding the tests, I’d love to rip out all the Bitcoin specific python code. I dont see any drawbacks toward including intermediary results in the test files, and this would also be useful for index builders wishing to reuse the same set of test vectors. I’ve had a TODO for awhile now to update the BIP test vectors; I’ll prioritise that work and update the PR here.
As always, thanks for the thorough review! I had one question that I’d like your input on, otherwise I’ll work on implementing the rest of your feedback over the next couple days.
Rebased 2164d5c -> bd745e1 (2025_15 -> 2025_15-rebased, compare)
Rebased on master to include https://github.com/bitcoin-core/secp256k1/pull/1725
64+ int ret;
65+
66+ secp256k1_silentpayments_sha256_init_inputs(&hash);
67+ secp256k1_sha256_write(&hash, outpoint_smallest36, 36);
68+ ret = secp256k1_eckey_pubkey_serialize(pubkey_sum, pubkey_sum_ser, &len, 1);
69+#ifdef VERFIY
Just a minor typo, yet it causes a sanity check for pubkey serialization to be skipped without warning.
0#ifdef VERIFY
433+
434+ /* Compute input public keys sum: A_sum = A_1 + A_2 + ... + A_n */
435+ secp256k1_gej_set_infinity(&A_sum_gej);
436+ for (i = 0; i < n_plain_pubkeys; i++) {
437+ ret &= secp256k1_pubkey_load(ctx, &addend, plain_pubkeys[i]);
438+ secp256k1_gej_add_ge_var(&A_sum_gej, &A_sum_gej, &addend, NULL);
addend other than expected (maybe invalid) is supplied to secp256k1_gej_add_ge_var?
_gej_add_ge_var with a bad addend and fails early on the first bad key, instead of wasting the callers time with additional work. Also updated the tests to explicitly test both bad plain pubkeys and bad taproot x-only pubkeys.
From what I can tell, nothing.
Indeed. secp256k1_gej_add_ge_var will compute garbage (garbage in, garbage out), but there won’t be any crashes or UB. But when looking deeper, there’s an interesting question; see #1736.
328+ secp256k1_silentpayments_sha256_init_label(&hash);
329+ secp256k1_sha256_write(&hash, recipient_scan_key32, 32);
330+ secp256k1_write_be32(m_serialized, m);
331+ secp256k1_sha256_write(&hash, m_serialized, sizeof(m_serialized));
332+ secp256k1_sha256_finalize(&hash, label_tweak32);
333+
nit (in int secp256k1_silentpayments_recipient_create_label):
0secp256k1_memclear(m_serialized, sizeof(m_serialized));
1secp256k1_sha256_clear(&hash);
289+ if (!secp256k1_silentpayments_create_output_pubkey(ctx, generated_outputs[recipients[i]->index], shared_secret, &recipients[i]->spend_pubkey, k)) {
290+ secp256k1_scalar_clear(&a_sum_scalar);
291+ secp256k1_memclear(&shared_secret, sizeof(shared_secret));
292+ return 0;
293+ }
294+ k++;
nit: ensure there’s room for the increment
0 VERIFY_CHECK(k < SIZE_MAX);
1 k++;
2
Updated bd745e1 -> d7281b9 (2025_15-rebased -> 2025_16, compare)
s/public_data/prevout_summary/ (h/t @real-or-random)prevout_summary object as input to be a proper lazy evaluation (h/t @real-or-random)s/B_m/labeled_spend_pubkey/ etc (h/t @real-or-random)VERIFY typo (h/t @w0xlt)In addition to what is explicitly mentioned above, I also slightly reworded a few comments and removed comments that I felt were simply restating what the code does without adding any extra information.
Thanks for the continued review @real-or-random . I think your suggestions for variable names and comments have improved the readability quite a bit. Also much happier with the prevout_summary naming and treating the prevout_summary object as a lazy evaluation.
Also, thanks @w0xlt for the thorough review! Much happier with the error handling and glad you caught the VERIFY typo.
The only way around this that I see is that the intermediate results computed from the addresses
Working on an update to the BIP test vectors to include intermediate outputs. Once I have that pull request open, I’ll update the test code here to remove the bech32m and ripemd160 python code.
146+/** Create Silent Payment labeled spend public key.
147+ *
148+ * Given a recipient's spend public key and a label, calculate the
149+ * corresponding labeled spend public key:
150+ *
151+ * labeled_spend_pubkey = recipient_spend_pubkey + label * G
in 163dc82627e864b202b1e919ee39d69df89f12f8:
0 * labeled_spend_pubkey = recipient_spend_pubkey + label
(as label is already a pubkey, not a scalar)
341+ VERIFY_CHECK(ctx != NULL);
342+ ARG_CHECK(labeled_spend_pubkey != NULL);
343+ ARG_CHECK(recipient_spend_pubkey != NULL);
344+ ARG_CHECK(label != NULL);
345+
346+ /* Calculate labeled_spend_pubkey = recipient_spend_pubkey + label * G
0 /* Calculate labeled_spend_pubkey = recipient_spend_pubkey + label
95+ VERIFY_CHECK(ret && len == 33);
96+#else
97+ (void)ret;
98+#endif
99+
100+ /* Leaking these values would break indistiguishability of the transaction, so clear them. */
0 /* Leaking these values would break indistinguishability of the transaction, so clear them. */
(here and in 5 other instances, in both the sending and receiving commits)
264+ * Returns: 1 if the data was able to be parsed.
265+ * 0 if the sequence is invalid (e.g., does not represent a valid
266+ * public key).
267+ *
268+ * Args: ctx: pointer to a context object.
269+ * Out: public_data: pointer to a silentpayments_recipient_prevout_summary object. If 1 is
0 * Out: prevout_summary: pointer to a silentpayments_recipient_prevout_summary object. If 1 is
388+ * 0 if the recipient scan key is invalid.
389+ * Args: ctx: pointer to a context object
390+ * Out: shared_secret33: pointer to the resulting 33-byte shared secret
391+ * In: recipient_scan_key32: pointer to the recipient's 32 byte scan key
392+ * public_data: pointer to the public_data object, loaded using
393+ * `_recipient_public_data_parse`
0 * prevout_summary: pointer to the public_data object, loaded using
1 * `_recipient_prevout_summary_parse`
387+ *
388+ * - `_recipient_prevout_summary_create` always creates a prevout_summary object with combined = false
389+ * - `_recipient_prevout_summary_serialize` only accepts a prevout_summary object with combined = false
390+ * and then performs an EC mult before serializing the resulting public key as a compressed
391+ * public key
392+ * - `_recpient_prevout_summary_parse` assumes the input represents a previously serialized
0 * - `_recipient_prevout_summary_parse` assumes the input represents a previously serialized
_prevout_summary is an improvement. :+1:
Updated d7281b9 -> f3c0ad6 (2025_16 -> 2025_17, compare)
s/prevout_summary/prevouts_summary/ - should have done this in the last push, apologies for the churn on thisThe main change in this push is being able to remove the ripemd160.py and bech32m.py files from the reference code! Thanks @macgyver13 for making these changes , both here and in the BIP repo!
Updated f3c0ad6 -> aa85bfb (2025_17 -> 2025_18, compare)
tests_silentpayments_generate.py to not skip test case (h/t @real-or-random )tests_silentpayments_generate.py to be more idiomatic python/more readable (h/t @real-or-random )tests_impl.h to handle extra test case (h/t @real-or-random)Thanks for the fixup! branch for the test vectors, @real-or-random ! I added you as a coauthor.
384+ * the resulting point serialized as a compressed public key, i.e., input_hash * prevout_pubkey_sum.
385+ *
386+ * For the each function:
387+ *
388+ * - `_recipient_prevouts_summary_create` always creates a prevouts_summary object with combined = false
389+ * - `_recipient_prevouts_summary_serialize` only accepts a prevouts_summary object with combined = false
_create and _parse are still correct, I think)
ctime_tests, where we were doing a serialization roundtrip to get the prevouts summary object into a combined = true state.
329+ secp256k1_memclear(m_serialized, sizeof(m_serialized));
330+ secp256k1_sha256_clear(&hash);
331+ return secp256k1_ec_pubkey_create(ctx, label, label_tweak32);
332+}
333+
334+int secp256k1_silentpayments_recipient_create_labeled_spend_pubkey(const secp256k1_context *ctx, secp256k1_pubkey *labeled_spend_pubkey, const secp256k1_pubkey *recipient_spend_pubkey, const secp256k1_pubkey *label) {
0int secp256k1_silentpayments_recipient_create_labeled_spend_pubkey(const secp256k1_context *ctx, secp256k1_pubkey *labeled_spend_pubkey, const secp256k1_pubkey *unlabeled_spend_pubkey, const secp256k1_pubkey *label) {
to match the parameter name in the API header
705+
706+ secp256k1_scalar_clear(&rsk);
707+ return 1;
708+}
709+
710+int secp256k1_silentpayments_recipient_create_output_pubkey(const secp256k1_context *ctx, secp256k1_xonly_pubkey *output_xonly, const unsigned char *shared_secret33, const secp256k1_pubkey *recipient_spend_pubkey, const uint32_t k)
0int secp256k1_silentpayments_recipient_create_output_pubkey(const secp256k1_context *ctx, secp256k1_xonly_pubkey *output_xonly, const unsigned char *shared_secret33, const secp256k1_pubkey *spend_pubkey, const uint32_t k)
to match the parameter name in the API header
146+/** Create Silent Payment labeled spend public key.
147+ *
148+ * Given a recipient's spend public key and a label, calculate the
149+ * corresponding labeled spend public key:
150+ *
151+ * labeled_spend_pubkey = recipient_spend_pubkey + label
0 * labeled_spend_pubkey = unlabeled_spend_pubkey + label
sorry to dig into the same line once again, missed this in the last review round :sweat_smile:
might be worth it to grep for recipient_spend occurences in general, if the plan is to get rid of that terminology completely (and only use {labeled_,unlabeled_,}spend_pubkey)
{labeled_,unlabeled_,}spend_pubkey and also update the scan_key32 argument to remove the recipient_ prefix. Much cleaner, imo.
$ git grep -i public.*data. I think it’s not entirely wrong in all cases, as the prevouts summary is indeed public data :-) but instances that say “public data object” should probably be changed to “prevouts_summary object” for consistency.
67+ *
68+ * Returns: 1 if creation of outputs was successful.
69+ * 0 if the input private keys sum to zero,
70+ * or input_hash or hash(shared_secret || k) are invalid scalars
71+ * (statistically improbable).
72+ * Args: ctx: pointer to a context object
0 * Args: ctx: pointer to a context object (not secp256k1_context_static)
as the sending function needs the generator point multiplication context (for calculating the pubkey sum from the seckey sum), and this is how we usually denote that in the API header (see also #1737)
Updated aa85bfb -> 09d1aee (2025_18 -> 2025_19, compare)
s/public data/prevouts data/ (h/t @theStack){labeled_, unlabeled_,}spend_pubkey consistently (h/t @theStack)recipient_ prefix from scan_key32 arg - this better matches the convention we are using for scan_pubkey, spend_pubkeyprevouts_summary is a proper lazy evaulationctx cannot be static (h/t @theStack)223+ if (!ret) {
224+ secp256k1_scalar_clear(&addend);
225+ secp256k1_scalar_clear(&seckey_sum_scalar);
226+ return 0;
227+ }
228+ secp256k1_declassify(ctx, &ret, sizeof(ret));
seems that this declassify call is a duplicate from the one a few lines above and can be removed
(ctime tests still pass for me locally)
128+ * label_tweak = hash(scan_key || m)
129+ * label = label_tweak * G
130+ *
131+ * Returns: 1 if label tweak and label creation was successful.
132+ * 0 if label_tweak32 is an invalid scalar (statistically improbable).
133+ * Args: ctx: pointer to a context object
in the _create_label API header:
0 * Args: ctx: pointer to a context object (not secp256k1_context_static)
compared to the sending function it’s a bit less obvious that a static context can’t be used, as we don’t check that ecmult_gen_ctx is built manually, but call another API function ec_pubkey_create which requires this
674+
675+ /* Leaking these values would break indistinguishability of the transaction, so clear them. */
676+ secp256k1_scalar_clear(&rsk_scalar);
677+ secp256k1_scalar_clear(&t_k_scalar);
678+ secp256k1_memclear(shared_secret, sizeof(shared_secret));
679+ return ret;
in _recipient_scan_outputs:
0 return 1;
as the “single point of return” approach isn’t followed anymore and any failures return early
569+ if (!combined) {
570+ secp256k1_scalar input_hash_scalar;
571+ secp256k1_scalar_set_b32(&input_hash_scalar, &prevouts_summary->data[5 + 64], NULL);
572+ secp256k1_scalar_mul(&rsk_scalar, &rsk_scalar, &input_hash_scalar);
573+ }
574+ ret &= secp256k1_pubkey_load(ctx, &spend_pubkey_ge, spend_pubkey);
0 ret = secp256k1_pubkey_load(ctx, &spend_pubkey_ge, spend_pubkey);
(as the previous failure would return early)
588+ /* Calculate output = spend_pubkey + t_k * G.
589+ * This can fail if t_k is the negation of spend_pubkey, but this happens only
590+ * with negligible probability as t_k is the output of a hash function. */
591+ if (!secp256k1_eckey_pubkey_tweak_add(&output_ge, &t_k_scalar)) {
592+ secp256k1_scalar_clear(&rsk_scalar);
593+ return 0;
shared_secret and t_k_scalar here
t_k_scalar. Not sure if it appears in the BIP but it’s “BIP-style”
348+ * return early
349+ */
350+ ret = secp256k1_pubkey_load(ctx, &labeled_spend_pubkey_ge, unlabeled_spend_pubkey);
351+ ret &= secp256k1_pubkey_load(ctx, &label_addend, label);
352+ if (!ret) {
353+ return ret;
nit, ret is always zero here:
0 return 0;
183+ * `secp256k1_silentpayments_recipient_prevouts_summary_serialize`. The serialization is
184+ * intended for sending the prevout summary data to light clients. Light clients
185+ * can use this serialization with
186+ * `secp256k1_silentpayments_recipient_prevouts_summary_parse`.
187+ */
188+typedef struct secp256k1_silentpayments_recipient_prevouts_summary {
naming nit: I wonder if we could drop the recipient part of the structure (especially considering that the prevout information actually represents key/input material from the sender side, so it could be even potentially confusing?)
0typedef struct secp256k1_silentpayments_prevouts_summary {
recipient is consistent. On the other hand, it suffices that only function names have the sender and recipient infixes. If you believe it could be confusing in this case, then it may be better to drop it.
sender and recipient infixes should be sufficient and would prefer to keep the names simple, wherever possible. Its also a good point that including recipient in the struct name is at best redundant and at worst confusing.
584+ while (1) {
585+ secp256k1_ge output_ge = spend_pubkey_ge;
586+ secp256k1_silentpayments_create_t_k(&t_k_scalar, shared_secret, k);
587+
588+ /* Calculate output = spend_pubkey + t_k * G.
589+ * This can fail if t_k is the negation of spend_pubkey, but this happens only
0 * This can fail if t_k * G is the negation of spend_pubkey, but this happens only
(or alternatively, s/spend_pubkey/spend_seckey/, though that would more abstractly refer to a scalar that is not in the scope of this function)
171+ const unsigned char * const *plain_seckeys,
172+ size_t n_plain_seckeys
173+) {
174+ size_t i, k;
175+ secp256k1_scalar seckey_sum_scalar, addend, input_hash_scalar;
176+ secp256k1_ge input_pubkey_sum_ge;
prevouts_summary struct description above uses prevout_pubkey_sum
57+ hash->s[7] = 0xcd06903eul;
58+
59+ hash->bytes = 64;
60+}
61+
62+static void secp256k1_silentpayments_calculate_input_hash(unsigned char *input_hash, const unsigned char *outpoint_smallest36, secp256k1_ge *pubkey_sum) {
VERIFY_CHECK and comments), so the two call-sites in sending/receiving don’t have to do it
_calculate_input_hash_scalar easier to understand without the the duplicated overflow checking and comment explaining why.
79+/** Labels
80+ *
81+ * The structs and call back function are implemented here as a demonstration
82+ * of how the label lookup callback is meant to query a label cache and return
83+ * the label tweak when a match is found. This is for demonstration purposes
84+ * only and not optimized. In a production usecase, it is expected that the
0 * only and not optimized. In a production use case, it is expected that the
Or just “in production”
114+ }
115+ return NULL;
116+}
117+
118+int main(void) {
119+ enum { N_INPUTS = 2, N_OUTPUTS = 3 };
#define.
208+ (*(sp_addresses[i]))[1],
209+ 33
210+ );
211+ if (!ret) {
212+ printf("\n");
213+ printf("Something went wrong, this is not a valid silent payments address.");
\n before the message? In the other error outputs, the \n comes after the message.
142+ {
143+ secp256k1_keypair sender_keypairs[N_INPUTS];
144+ const secp256k1_keypair *sender_keypair_ptrs[N_INPUTS];
145+ secp256k1_silentpayments_recipient recipients[N_OUTPUTS];
146+ const secp256k1_silentpayments_recipient *recipient_ptrs[N_OUTPUTS];
147+ unsigned char (*sp_addresses[N_OUTPUTS])[2][33];
136+ * leakage. See `secp256k1_context_randomize` in secp256k1.h for more
137+ * information about it. This should never fail. */
138+ ret = secp256k1_context_randomize(ctx, randomize);
139+ assert(ret);
140+
141+ /*** Sending ***/
0 /*** Sending (Alice) ***/
274+ *
275+ * Since Bob has access to the full transaction, scanning is simple:
276+ *
277+ * 1. Collect the relevant prevouts data from the transaction
278+ * and call `_silentpayments_recipient_prevouts_summary_create`
279+ * 2. Call `_silentpayments_recipient_scan_outputs`
secp256k1 prefix in user-facing docs.
297+ * These steps are only necessary if the recipient is using the
298+ * optional labels feature. Recipients not using labels can
299+ * ignore these steps and simply pass `NULL` for the
300+ * label_lookup and label_context arguments:
301+ *
302+ * _silentpayments_recipient_scan_outputs(..., NULL, NULL);
0 * secp256k1_silentpayments_recipient_scan_outputs(..., NULL, NULL);