Add BIP352 module (take 3)

[josibake]

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.

[josibake]

Updated 6264c3d -> 9e85256 (2025_00 -> 2025_01, compare)

• Added documentation for expectations around label_lookup pointer lifetimes (h/t @antonilol)
• Update docs to accurately reflect that label_context is optional (h/t @antonilol)
• Added a test case for passing a lookup callback with a null context (which required some small updates to the test label lookup function)

[real-or-random]

Sorry, stopping CI here. We're about to make a release and need to the CI. :)

We'll restart the jobs here afterwards.

[josibake]

Update 9e85256 -> a4db279 (2025_01 -> 2025_02, compare)

• Update the constant time tests to cover the _recipient_created_shared_secret and _recipient_created_output_pubkey functions (h/t @theStack )
• Remove no longer needed TODO comments and clarify why a constant time test without a label lookup function is sufficient for _recipient_scan_outputs

[josibake]

Rebased on top of 0.7.0 release 🎉 a4db279 -> e35bede (2025_02 -> 2025_02_rebase, compare)

[josibake]

I did a deep dive on using (*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 josibake@5a10880 and josibake/bitcoin@5835d98

[josibake]

Updated e35bede -> 1a84908 (2025_02_rebase -> 2025_03, compare)

• Added a test case for the _recipient_create_output_pubkey corner case (h/t @theStack)
• Removed the VERIFY_CHECK in favour of returning an error

[josibake]

Update 1a84908 -> 2948a9b (2025_03 -> 2025_04, compare)

• Fixed valgrind error in test
• Update the example to use EXIT_SUCCESS/EXIT_FAILURE (h/t @theStack)
• Clear shared secret variable consistently (and update comment) (h/t @theStack)
• Add comment explaining why we declassify the pubkey sum (h/t @theStack)

Thanks for the thorough review, @theStack !

[josibake]

Update 2948a9b -> 64ecd6c (2025_04 -> 2025_05, compare)

• Remove no longer needed TODO comment regarding _cmov
• Remove todo comment regarding input_hash, now that this is properly specified in the BIP

cc @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 😅 )

[josibake]

Updated 64ecd6c -> 3c4af8f (2025_05 -> 2025_06, compare)

• Updates the benchmarks per @theStack 's suggestion to have separate benchmarks for _full_scan and full_scan_with_labels
• Leave a TODO comment for a follow-up to make the labels benchmark more representative of real world usage
• Cleans up the benchmark arguments and formatting

[theStack]

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.

diff

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 8ee13ce..f612a84 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -583,13 +583,13 @@ jobs:
       fail-fast: false
       matrix:
         env_vars:
-          - { WIDEMUL: 'int64',  RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes' }
+          - { WIDEMUL: 'int64',  RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes', SILENTPAYMENTS: 'yes' }
           - { WIDEMUL: 'int128_struct', ECMULTGENPRECISION: 2, ECMULTWINDOW: 4 }
-          - { WIDEMUL: 'int128',                  ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes' }
+          - { WIDEMUL: 'int128',                  ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes', SILENTPAYMENTS: 'yes' }
           - { WIDEMUL: 'int128', RECOVERY: 'yes' }
-          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes' }
-          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes', CC: 'gcc' }
-          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes', CPPFLAGS: '-DVERIFY' }
+          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes', SILENTPAYMENTS: 'yes' }
+          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes', SILENTPAYMENTS: 'yes', CC: 'gcc' }
+          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes', SILENTPAYMENTS: 'yes', CPPFLAGS: '-DVERIFY' }
           - BUILD: 'distcheck'
 
     steps:

(untested)

[josibake]

Thanks all for the ongoing review! I'm working on implementing the feedback so far and expect to have an updated branch by Friday, at the latest.

[josibake]

Updated ac4a726 -> a0d2a33 (2025_27 -> 2025_28, compare)

• Makefile and typo fixes (h/t @stratospher)
• examples/silentpayments.c rewrite
  • Various typo and wording fixes (h/t @jonasnick)
  • Demonstrates usage of the new _recipient_create_output_pubkeys function
  • Includes an address creation example, demonstrating usage of the label functions (h/t @jonasnick)
  • Various other simplifications
• Move magic bytes out of header into implementation file (h/t @nymius)
• Remove _recipient_{create_shared_secret, create_output_pubkey} functions in favour of a _recipient_create_output_pubkeys function (h/t @nymius)
  • This removes all references to k and provides a simpler API for light clients, without exposing internal details of the protocol
  • This allows for clients to check for a k=0 output, and then proceed to downloading the full transaction if a match is found
• Various doc fixes in the header file (h/t @jonasnick , @nymius )
• Update _prevouts_summary_{parse, serialize} functions to work with both 33 and 65 byte serializations
  • For indexes that are not storage constrained, being able to save the prevouts_summary object as a 65 byte uncompressed key saves the index from needing to calculate the y value when scanning, e.g., in an outsourced scanning use case
• Update benchmarks, tests to use new _create_output_pubkeys function
• Update test coverage
• Fix the buffer overflow cases in the _scan_outputs function (h/t @jonasnick , @theStack )

This is quite an extensive push based on a lot of excellent review. Its likely I've missed some feedback and will be polishing over the next few days, but wanted to get the bulk of the changes up. Thanks again for all the continued review!

[nymius]

All my previous concerns have been addressed. From a first read I extracted the some nits. Let me know if the following patch is easier to apply:

diff --git a/include/secp256k1_silentpayments.h b/include/secp256k1_silentpayments.h
index b8357b8..da40acf 100644
--- a/include/secp256k1_silentpayments.h
+++ b/include/secp256k1_silentpayments.h
@@ -187,7 +187,7 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipien
  *  `secp256k1_silentpayments_recipient_prevouts_summary_create`. Can be serialized with
  *  `secp256k1_silentpayments_recipient_prevouts_summary_serialize`. The serialization is
  *  intended for storing the object or sending the prevout summary data to light clients.
- *  The serialization is is parsed with
+ *  The serialization is parsed with
  *  `secp256k1_silentpayments_recipient_prevouts_summary_parse`.
  */
 typedef struct secp256k1_silentpayments_prevouts_summary {
@@ -250,7 +250,7 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipien
  *
  *  Serializing a prevouts_summary object created with `_recipent_prevouts_summary_create` will result in
  *  an EC multiplication. This allows for a more compact serialization, but also means a serialized
- *  prevouts_summary will not parse back to a the same prevouts_summary object (due to the EC multiplication).
+ *  prevouts_summary will not parse back to the same prevouts_summary object (due to the EC multiplication).
  *
  *  Returns: 1 always.
  *
@@ -386,7 +386,7 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipien
 
 /** Create Silent Payment output public keys.
  *
- *  Given a scan key, a prevouts_summary, and array of recipient spend public keys,
+ *  Given a scan key, a prevouts_summary, and an array of recipient spend public keys,
  *  create the silent payments output public keys.
  *
  *  This function is used by the recipient when scanning for outputs without
diff --git a/src/modules/silentpayments/main_impl.h b/src/modules/silentpayments/main_impl.h
index c0f4822..52cfa1f 100644
--- a/src/modules/silentpayments/main_impl.h
+++ b/src/modules/silentpayments/main_impl.h
@@ -97,7 +97,7 @@ static void secp256k1_silentpayments_create_shared_secret(const secp256k1_contex
 
     secp256k1_ecmult_const(&ss_j, public_component, secret_component);
     secp256k1_ge_set_gej(&ss, &ss_j);
-    /* We declassify the shared secret group elemement because serializing a group element is a non-constant time operation. */
+    /* We declassify the shared secret group element because serializing a group element is a non-constant time operation. */
     secp256k1_declassify(ctx, &ss, sizeof(ss));
     /* This can only fail if the shared secret is the point at infinity, which should be
      * impossible at this point considering we have already validated the public key and

[nymius]

Any reason for this to not be const secp256k1_pubkey * const *spend_pubkeys? like _pubkeys in _silentpayments_recipient_prevouts_summary_create or _seckeys in _silentpayments_sender_create_outputs

[josibake]

Yes, outputs_xonly is the out-param where we will write the generated xonly public keys. This is the same as found_outputs in the scanning function and generated_outputs in the _create_outputs function.

[nymius]

Are we referring to the same line? I was talking about line 421: const secp256k1_pubkey **spend_pubkeys.

[nymius]

Sorry, I missed a word in the original comment. I edited it. tldr:
Why not const secp256k1_pubkey **spend_pubkeys -> const secp256k1_pubkey * const *spend_pubkeys?

[josibake]

Ah, gotcha! Sorry, I misunderstood your original comment. This should be const secp256k1_pubkey * const *spend_pubkeys, as you suggest.

[josibake]

Thanks for the review @nymius , let me know if you uncover anything else while working with the new API! Also, thanks for the patch; very easy to apply and much appreciated!

[theStack]

Glad to see the updated API, I agree that the light client scanning interface makes much more sense this way, and that supporting prevouts summary (de)serialization in both compressed and uncompressed pubkey format is useful. Left some mostly nitty comments below, some of them are likely not related to the latest changes, but I just missed them in earlier review rounds. Haven't looked at tests and examples of the updated API functions yet.

[theStack]

_ec_pubkey_cmp itself doesn't do any sorting, should that be

Suggested change

	* Note: secp256k1_ec_pubkey_cmp uses heap sort, which is unstable. Developers cannot and should not
	* Note: secp256k1_silentpayments_recipient_sort uses heap sort, which is unstable. Developers cannot and should not

instead? (Or maybe just "note that we use heap sort, ...")

[theStack]

case-sensitivity-yocto-nit: noticed that sometimes in the API header the "Silent Payment" is used, sometimes "silent payment" is, each also in singular and plural, might be worth it to use only one variant for consistency.

[theStack]

in _recipient_create_label: I think it would make sense to also error if the passed scan key is invalid, to prevent the user of creating unspendable labels

[theStack]

nit: not saying it's wrong, but it seems unnecessary and at least very unusual to use const for parameters that are passed by value (feel free to ignore if that was discussed before, I suspect there was a reason for introducing it as I can't remember seeing it from earlier review rounds)

[theStack]

nit: seems that having only one of the size and flags parameters would be sufficient. I suspect it's done like that to be more in-line with existing API functions (thinking of _ec_pubkey_serialize)?

[theStack]

Suggested change

	* returned, it is set to a parsed version of input33.
	* In: input33: pointer to a serialized silentpayments_prevouts_summary.
	* returned, it is set to a parsed version of input.
	* In: input: pointer to a serialized silentpayments_prevouts_summary.

also, the description of the inputlen parameter is missing

[theStack]

needs update, now that serialization in both compressed and uncompressed public key format is possible

[theStack]

Suggested change

	for (i = 0; i <= n_tx_outputs; i++) {
	for (i = 0; i < n_tx_outputs; i++) {

off-by-one, IIUC

[theStack]

in _recipient_create_output_pubkeys: should also clear out the shared_secret here

[theStack]

nit, here and in some other similar comments before _scalar_set_b32 calls: could probably just drop the "to ensure the value is less than the curve order" part, as that sounds like that's the only reason we do the conversion (maybe it was in the past, when that function returned the result as byte-array, but I don't remember).

[furszy]

Left a little help for the new test framework changes (will need to rebase this PR as it conflicts with master). The changes are small, I think you’ll like what we came up with.

[furszy]

In 6fe3a7a:

This now should be:

static const struct tf_test_entry tests_silentpayment[] = {
    CASE1(test_recipient_sort),
    CASE1(test_send_api),
};

And also need to include the framework:

#include "../../unit_test.h"

[furszy]

In 6fe3a7a:

On the modules registry, this should be

#ifdef ENABLE_MODULE_SILENTPAYMENTS
    MAKE_TEST_MODULE(silentpayment),
#endif

[jonasnick]

I noticed that scanning performance is quite poor in the worst case. Consider a maliciously crafted 1mB transaction where all outputs are SP outputs to the recipient, who uses labels. There are about 1mB/32B tx outputs. Thus the outer loop runs n_tx_outputs = 31250 times and the inner loop 1 + 2 + ... + (n_tx_outputs - 1) + n_tx_outputs = n_tx_outputs * (n_tx_outputs + 1)/2 times. Each inner loop calls ge_set_gej at most twice, which takes 2us on my machine. So it would take me 16 minutes to scan such a transaction. I believe if the BIP mandated that the transaction outputs need to be ordered by k (i.e., smaller k needs to have a lower output index), scanning could be implemented in linear time. But requiring such an order may put too many constraints on transaction creation.

EDIT: I previously assumed 4mB worth of outputs which is of course not possible. This would have led to 4.3 hours of scanning.

[jonasnick]

ret is unused.

[jonasnick]

ret is unchecked

[jonasnick]

Is there a reason we assert that a negligible event didn't happen but in the case of recipient_create_label we handle the negligible event?

[jonasnick]

This is useful and we had a similar line for label1 (label1 = tx_output - generated_output). Did you remove that intentionally?

[jonasnick]

We can remove i and use n_found in it's place. Or even going one step further, we can remove n_found and replace it with k unless you think it hurts readability. I think it helps with understanding what is going on.

[w0xlt]

The following patch seems to optimize recipient scanning from O(n²) to O(n log n):

w0xlt@8d68d64

Performance Impact (macOS M3, 128GB running the modified silentpayments.c example file):

N_OUTPUTS	Before Patch	After Patch	Speedup
2,000	~8.7 seconds	~55 ms	~158x
5,000	~106 seconds	~148 ms	~716x
10,000	~733 seconds	~337 ms	~2175x

The above N_OUTPUTS uses N different labels.

[antonilol]

The following patch seems to optimize recipient scanning from O(n²) to O(n log n):

What exactly is optimized here? It looks like the example uses hash based lookup now

[w0xlt]

What exactly is optimized here?

• Implements a binary search index for unlabeled outputs, reducing lookup time from O(n) to O(log n) per candidate.

• Pre-decode & Batch Operations:

  • Pre-decodes all transaction outputs to Jacobian coordinates once (eliminating redundant decoding)
  • Batch-normalizes up to 64 label candidates together
  • Tracks already-matched outputs to skip them in subsequent iterations

• Moving Search Heads: Implements persistent cursors that remember scanning position across iterations, so scans don't restart at j = 0.

• Complexity

  • Unlabeled: O(n²) → O(n log n) to build the index + O(m log n) for m matches.
  • Labeled: Worst-case remains O(n × k), but with much smaller constants due to batching, predecoding and rolling cursors.

The commit is: w0xlt@8d68d64

P.S.: I'm not suggesting that these approaches be adopted in this PR. They could be explored to address any scanning time concerns, if they turn out to be valid.

[jonasnick]

Thank you @woxlt for your suggestions. I played around with w0xlt@1ea5fa2 and particularly liked the O(n log(n)) fast path idea for wallets that do not use labels and the application of ge_set_gej_all.

As for your benchmarks, I believe the main reason for the speedup here is that your code tracks already found outputs and skips them in subsequent iterations. The example file you used for benchmarks scans through a transaction where all outputs go the same receiver, who uses labels, and the outputs are ordered by k.

With a simple patch to the current scanning code (jonasnick@311b4eb) that just keeps track of the found outputs, I get about the same speedup as your commit. Of course, this is just one particular benchmark.

Thus, I think in order to evaluate potential improvements, we need to select the benchmarks we care about.

1. Wallets that don't use labels will be much faster with your code (if they own multiple outputs in the same transaction, which should practically never happen?).
2. If we consider worst case transactions for wallets that do use labels, I don't think the "moving search heads" in your code help. So I'm not sure if they're worth the complexity.
3. Do you have a benchmark that shows that "Pre-decodes all transaction outputs to Jacobian coordinates" is a significant optimization? I only checked the "example" benchmark and it didn't seem to make a difference.
4. We should also have a realistic benchmark for the average transaction we'd encounter on the chain.

I have a branch (jonasnick@47a4681) with a couple of simplifications to your code that, for example, avoid redundant serializations, simplify tracking of found outputs (only one way to track instead of two that are needed to be kept in-sync), and uses the same code for scanning of "candidate family #1" and "#2" to avoid code duplication.

Lastly, your code uses malloc which has some practical issues we'd like to sort out but they certainly aren't insurmountable.

[nymius]

1. Wallets that don't use labels will be much faster with your code (if they own multiple outputs in the same transaction, which should practically never happen?).

To my best understanding, this is a very unlikely case. A wallet with a single label, the change label, as the base case, would be more realistic.

4. We should also have a realistic benchmark for the average transaction we'd encounter on the chain.

I think wallet with single change label is the best candidate for this.

[w0xlt]

Hi @jonasnick Thank you very much for the feedback and for simplifying the code — I also believe that's essential.

The simplified branch (https://github.com/jonasnick/secp256k1/tree/bip352-optimized-simplified) runs fine for all files except silentpayments_mixed_2_example.
On my machine, it takes about 65 s, while the non-simplified one finishes in around 0.28 s.

silentpayments_mixed_2_example shuffles 10,000 labeled Carol outputs with 1,005 random outputs.

The slowdown seems to come from removing the stateful label‑scan that walks the transaction outputs exactly once with two moving cursors (“heads”) and reuses decoded points. As a result it restarts from output 0 for every new k, re‑decodes points over and over, and ends up doing ~O(n²) label checks instead of ~O(n).

The latest commit is w0xlt@8d68d64 (code is more organized), which might be a better base.