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.
Move the sha256_tag_test_internal function out of the musig module
into tests.c. This makes it available to other modules wishing to verify tagged
hashes without needing to duplicate the function.
Change the function signature to expect a const unsigned char and update
the tagged hash tests to use static const unsigned char character
arrays (where necessary).
Add a comment for each tag. This is done as a convenience for checking
the strings against the protocol specifications, where the tags are
normally specified as strings.
Update tests in the ellswift and schnorrsig modules to use the
sha256_tag_test_internal helper function.
Add a routine for the entire sending flow which takes a set of private keys,
the smallest outpoint, and list of recipients and returns a list of
x-only public keys by performing the following steps:
1. Sum up the private keys
2. Calculate the input_hash
3. For each recipient group:
3a. Calculate a shared secret
3b. Create the requested number of outputs
This function assumes a single sender context in that it requires the
sender to have access to all of the private keys. In the future, this
API may be expanded to allow for a multiple senders or for a single
sender who does not have access to all private keys at any given time,
but for now these modes are considered out of scope / unsafe.
Internal to the library, add:
1. A function for creating shared secrets (i.e., a*B or b*A)
2. A function for generating the "SharedSecret" tagged hash
3. A function for creating a single output public key
Add function for creating a label tweak. This requires a tagged hash
function for labels. This function is used by the receiver for creating
labels to be used for a) creating labeled addresses and b) to populate
a labels cache when scanning.
Add function for creating a labeled spend pubkey. This involves taking
a label tweak, turning it into a public key and adding it to the spend
public key. This function is used by the receiver to create a labeled
silent payment address.
Add tests for the label API.
Add routine for scanning a transaction and returning the necessary
spending data for any found outputs. This function works with labels via
a lookup callback and requires access to the transaction outputs.
Requiring access to the transaction outputs is not suitable for light
clients, but light client support is enabled by exposing the
`_create_shared_secret` and `_create_output_pubkey` functions in the
API. This means the light client will need to manage their own scanning
state, so wherever possible it is preferrable to use the
`_recipient_scan_ouputs` function.
Add an opaque data type for passing around the summed input public key (A_sum)
and the input hash tweak (input_hash). This data is passed to the scanner
before the ECDH step as two separate elements so that the scanner can
multiply b_scan * input_hash before doing ECDH.
Add functions for deserializing / serializing a public_data object to
and from a public key. When serializing a public_data object, the
input_hash is multplied into A_sum. This is so the object can be stored
as public key for wallet rescanning later, or to vend to light clients.
For the light client, a `_parse` function is added which parses the
compressed public key serialization into a `public_data` object.
Finally, add test coverage for the receiving API.
Demonstrate sending, scanning, and light client scanning.
Add a benchmark for a full transaction scan and for scanning a single
output. Only benchmarks for scanning are added as this is the most
performance critical portion of the protocol.
Co-authored-by: Sebastian Falbesoner <91535+thestack@users.noreply.github.com>
Add the BIP-352 test vectors. The vectors are generated with a Python script
that converts the .json file from the BIP to C code:
$ ./tools/tests_silentpayments_generate.py test_vectors.json > ./src/modules/silentpayments/vectors.h
Co-authored-by: Jonas Nick <2582071+jonasnick@users.noreply.github.com>
Co-authored-by: Sebastian Falbesoner <91535+thestack@users.noreply.github.com>
Test midstate tags used in silent payments.
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_summary392+ * - `_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.
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.
(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.)