Add "silentpayments" module implementing BIP352 (take 4, limited to full-node scanning)

[theStack]

Description

This PR implements BIP352 with scanning limited to full-nodes. Light-client scanning is planned to be added in a separate PR in the future. The following 7 API functions are currently introduced:

Sender side [BIP description]:

• secp256k1_silentpayments_sender_create_outputs: given a list of $n$ secret keys $a_1 ... a_n$, a serialized outpoint, and a list of recipients (each consisting of silent payments scan pubkey and spend pubkey), create the corresponding transaction outputs (x-only public keys) for the sending transaction

Receiver side, label creation [BIP description]:

• secp256k1_silentpayments_recipient_label_create: given a scan secret key and label integer, calculate the corresponding label tweak and label object
• secp256k1_silentpayments_recipient_label_serialize: given a label object, create the corresponding 33-byte serialization
• secp256k1_silentpayments_recipient_label_parse: given a 33-byte label representation, create the corresponding label object
• secp256k1_silentpayments_recipient_create_labeled_spend_pubkey: given a spend public key and a label object, create the corresponding labeled spend public key

Receiver side, scanning [BIP description]:

• secp256k1_silentpayments_recipient_prevouts_summary_create: given a list of $n$ public keys $A_1 ... A_n$ and a serialized outpoint, create a prevouts_summary object needed for scanning
• secp256k1_silentpayments_recipient_scan_outputs: given a prevouts_summary object, a recipients scan secret key and spend public key, and the relevant transaction outputs (x-only public keys), scan for outputs belonging to the recipients and and return the tweak(s) needed for spending the output(s). Optionally, a label_lookup callback function can be provided to also scan for labels.

For a higher-level overview on what these functions exactly do, it's suggested to look at a corresponding Python implementation that was created based on the secp256k1lab project (it passes the test vectors, so this "executable pseudo-code" should be correct).

Changes to the previous take

Based on the latest state of the previous PR #1698 (take 3), the following changes have been made:

• rebased on master (adapting to the new unit test framework added in Introduce (mini) unit test framework #1734; thanks to furszy for his mini-guide)
• removed functionality that is only relevant for light clients (_prevout_summary_{parse,serialize}, __recipient_create_output_pubkeys), adapted tests and benchmark accordingly
• adapted examples to have both Bob and Carol do full-node scanning, with a TODO to switch the latter to light-client scanning once available in the future
• addressed various review comments, if still relevant (Add BIP352 module (take 3) #1698 (comment), Add BIP352 module (take 3) #1698 (comment), Add BIP352 module (take 3) #1698 (comment), Add BIP352 module (take 3) #1698 (comment))

The scope reduction isn't immediately visible in commit count (only one commit was only introducing light-client relevant functionality and could be completely removed), but the review burden compared #1698 is still significantly lower in terms of LOC, especially in the receiving commit.

Open questions / TODOs

• Recent proposals of reducing the worst-case scanning time (see posts by w0xlt and jonasnick, Add BIP352 module (take 3) #1698 (comment) ff.) are not taken into account yet. ➡️ solved by marking already-found outputs, see Add "silentpayments" module implementing BIP352 (take 4, limited to full-node scanning) #1765 (comment) ✔️
• Not providing prevouts_summary (de)serialization functionality yet in the API poses the risk that users try to do it anyway by treating the opaque object as "serialized". How to cope with that? Is adding a "don't do this" comment in API header sufficient? ➡️ solved by mentioning a "don't do this" comment in the API header (same phrasing as in other modules), see Add "silentpayments" module implementing BIP352 (take 4, limited to full-node scanning) #1765 (comment) ✔️

[w0xlt]

Added the optimized version on top of this PR:
w0xlt@8d16914

For more context:
#1698 (comment)

[theStack]

Small supplementary update: I've created a corresponding Python implementation of the provided API functions based on secp256k1lab (https://github.com/theStack/secp256k1lab/blob/add_bip352_module_review_helper/src/secp256k1lab/bip352.py) (also linked in the PR description). The hope is that this makes reviewing this PR a bit easier by having a less noisy, "executable pseudo-code"-like description on what happens under the hood. The code passes the BIP352 test vectors and hence should be correct.

Added the optimized version on top of this PR: w0xlt@8d16914

For more context: #1698 (comment)

Thanks for rebasing on top of this PR, much appreciated! I will take a closer look within the next days.

[w0xlt]

Nit: Not related to optimization, but the diff below removes some redundant public-key serialization code:

diff --git a/src/modules/silentpayments/main_impl.h b/src/modules/silentpayments/main_impl.h
index 106da20..922433d 100644
--- a/src/modules/silentpayments/main_impl.h
+++ b/src/modules/silentpayments/main_impl.h
@@ -21,6 +21,19 @@
 /** magic bytes for ensuring prevouts_summary objects were initialized correctly. */
 static const unsigned char secp256k1_silentpayments_prevouts_summary_magic[4] = { 0xa7, 0x1c, 0xd3, 0x5e };
 
+/* Serialize a ge to compressed 33 bytes. Keeps eckey_pubkey_serialize usage uniform
+ * (expects non-const ge*), and centralizes the VERIFY_CHECK. */
+static SECP256K1_INLINE void secp256k1_sp_ge_serialize33(const secp256k1_ge* in, unsigned char out33[33]) {
+    size_t len = 33;
+    secp256k1_ge tmp = *in;
+    int ok = secp256k1_eckey_pubkey_serialize(&tmp, out33, &len, 1);
+#ifdef VERIFY
+    VERIFY_CHECK(ok && len == 33);
+#else
+    (void)ok;
+#endif
+}
+
 /** Sort an array of silent payment recipients. This is used to group recipients by scan pubkey to
  *  ensure the correct values of k are used when creating multiple outputs for a recipient.
  *
@@ -68,13 +81,11 @@ static int secp256k1_silentpayments_calculate_input_hash_scalar(secp256k1_scalar
     secp256k1_sha256 hash;
     unsigned char pubkey_sum_ser[33];
     unsigned char input_hash[32];
-    size_t len;
     int ret, overflow;
 
     secp256k1_silentpayments_sha256_init_inputs(&hash);
     secp256k1_sha256_write(&hash, outpoint_smallest36, 36);
-    ret = secp256k1_eckey_pubkey_serialize(pubkey_sum, pubkey_sum_ser, &len, 1);
-    VERIFY_CHECK(ret && len == sizeof(pubkey_sum_ser));
+    secp256k1_sp_ge_serialize33(pubkey_sum, pubkey_sum_ser);
     secp256k1_sha256_write(&hash, pubkey_sum_ser, sizeof(pubkey_sum_ser));
     secp256k1_sha256_finalize(&hash, input_hash);
     /* Convert input_hash to a scalar.
@@ -85,15 +96,13 @@ static int secp256k1_silentpayments_calculate_input_hash_scalar(secp256k1_scalar
      * an error to ensure strict compliance with BIP0352.
      */
     secp256k1_scalar_set_b32(input_hash_scalar, input_hash, &overflow);
-    ret &= !secp256k1_scalar_is_zero(input_hash_scalar);
+    ret = !secp256k1_scalar_is_zero(input_hash_scalar);
     return ret & !overflow;
 }
 
 static void secp256k1_silentpayments_create_shared_secret(const secp256k1_context *ctx, unsigned char *shared_secret33, const secp256k1_ge *public_component, const secp256k1_scalar *secret_component) {
     secp256k1_gej ss_j;
     secp256k1_ge ss;
-    size_t len;
-    int ret;
 
     secp256k1_ecmult_const(&ss_j, public_component, secret_component);
     secp256k1_ge_set_gej(&ss, &ss_j);
@@ -103,12 +112,7 @@ static void secp256k1_silentpayments_create_shared_secret(const secp256k1_contex
      * impossible at this point considering we have already validated the public key and
      * the secret key.
      */
-    ret = secp256k1_eckey_pubkey_serialize(&ss, shared_secret33, &len, 1);
-#ifdef VERIFY
-    VERIFY_CHECK(ret && len == 33);
-#else
-    (void)ret;
-#endif
+    secp256k1_sp_ge_serialize33(&ss, shared_secret33);
 
     /* Leaking these values would break indistinguishability of the transaction, so clear them. */
     secp256k1_ge_clear(&ss);
@@ -585,7 +589,6 @@ int secp256k1_silentpayments_recipient_scan_outputs(
                 secp256k1_ge output_negated_ge, tx_output_ge;
                 secp256k1_gej tx_output_gej, label_gej;
                 unsigned char label33[33];
-                size_t len;
 
                 secp256k1_xonly_pubkey_load(ctx, &tx_output_ge, tx_outputs[j]);
                 secp256k1_gej_set_ge(&tx_output_gej, &tx_output_ge);
@@ -595,7 +598,6 @@ int secp256k1_silentpayments_recipient_scan_outputs(
                 secp256k1_ge_neg(&output_negated_ge, &output_ge);
                 secp256k1_gej_add_ge_var(&label_gej, &tx_output_gej, &output_negated_ge, NULL);
                 secp256k1_ge_set_gej_var(&label_ge, &label_gej);
-                ret = secp256k1_eckey_pubkey_serialize(&label_ge, label33, &len, 1);
                 /* Serialize must succeed because the point was just loaded.
                  *
                  * Note: serialize will also fail if label_ge is the point at infinity, but we know
@@ -603,7 +605,7 @@ int secp256k1_silentpayments_recipient_scan_outputs(
                  * Thus, we know that label_ge = tx_output_gej + output_negated_ge cannot be the
                  * point at infinity.
                  */
-                VERIFY_CHECK(ret && len == 33);
+                secp256k1_sp_ge_serialize33(&label_ge, label33);
                 label_tweak = label_lookup(label33, label_context);
                 if (label_tweak != NULL) {
                     found = 1;
@@ -617,7 +619,6 @@ int secp256k1_silentpayments_recipient_scan_outputs(
                 secp256k1_gej_neg(&label_gej, &tx_output_gej);
                 secp256k1_gej_add_ge_var(&label_gej, &label_gej, &output_negated_ge, NULL);
                 secp256k1_ge_set_gej_var(&label_ge, &label_gej);
-                ret = secp256k1_eckey_pubkey_serialize(&label_ge, label33, &len, 1);
                 /* Serialize must succeed because the point was just loaded.
                  *
                  * Note: serialize will also fail if label_ge is the point at infinity, but we know
@@ -625,7 +626,7 @@ int secp256k1_silentpayments_recipient_scan_outputs(
                  * Thus, we know that label_ge = tx_output_gej + output_negated_ge cannot be the
                  * point at infinity.
                  */
-                VERIFY_CHECK(ret && len == 33);
+                secp256k1_sp_ge_serialize33(&label_ge, label33);
                 label_tweak = label_lookup(label33, label_context);
                 if (label_tweak != NULL) {
                     found = 1;

[w0xlt]

nit: The following diff removes the implicit cast and clarifies that k is 4 bytes

diff --git a/src/modules/silentpayments/main_impl.h b/src/modules/silentpayments/main_impl.h
index 922433d..d94aed6 100644
--- a/src/modules/silentpayments/main_impl.h
+++ b/src/modules/silentpayments/main_impl.h
@@ -512,7 +512,8 @@ int secp256k1_silentpayments_recipient_scan_outputs(
     secp256k1_xonly_pubkey output_xonly;
     unsigned char shared_secret[33];
     const unsigned char *label_tweak = NULL;
-    size_t j, k, found_idx;
+    size_t j, found_idx;
+    uint32_t k;
     int found, combined, valid_scan_key, ret;
 
     /* Sanity check inputs */

[jonasnick]

Thanks @theStack for the new PR. I can confirm that this PR is a rebased version of #1698, with the light client functionality removed and comments addressed, except for:

• #1698 (comment)
• #1698 (comment)
• #1698 (review) (only the last one, "elemement")

[jonasnick]

Not providing prevouts_summary (de)serialization functionality yet in the API poses the risk that users try to do it anyway by treating the opaque object as "serialized". How to cope with that? Is adding a "don't do this" comment in API header sufficient?

Is there a reason for serializing prevouts_summary without light client functionality? If not, I think the don't do this comment is sufficient. Right now, in contrast to the docs of all other opaque objects, this is missing, however:

The exact representation of data inside the opaque data structures is implementation defined and not guaranteed to be portable between different platforms or versions.

[theStack]

@w0xlt, @jonasnick: Thanks for the reviews! I've addressed the suggested changes:

• in _recpient_scan_outputs: changed the type of k to uint32_t (comment above)
• in _recipient_create_label: added a scan key validity check (+added a test for that) (#1698 - comment)
• unified all mentions of "Silent Payments" to title case in the header API and example (#1698 - comment)
• fixed typo s/elemement/element/ (#1698 - review)
• in _recipient_scan_outputs: fixed comment in second label candidate (review above)
• extended the API header comment for the _prevouts_summary opaque data structure, to point out that the data structure is implementation defined (like docs of all other opaque structs) (comment above)

Nit: Not related to optimization, but the diff below removes some redundant public-key serialization code:

Given that this compressed-pubkey-serialization pattern shows up repeatedly also in other modules (ellswift, musig), I think it would make the most sense to add a general helper (e.g. in eckey{,_impl}.h), which could be done in an independent PR. I've opened issue #1773 to see if there is conceptual support for doing this.

Not providing prevouts_summary (de)serialization functionality yet in the API poses the risk that users try to do it anyway by treating the opaque object as "serialized". How to cope with that? Is adding a "don't do this" comment in API header sufficient?

Is there a reason for serializing prevouts_summary without light client functionality? If not, I think the don't do this comment is sufficient.

Good point, I can't think of a good reason for full nodes wanting to serialize prevouts_summary.

[nymius]

To address the open questions, I’ve reviewed the proposed changes by @w0xlt on 8d16914.

I'm going to focus more on the key aspects I extracted from the review and the merits of each change, rather on the big O improvement claims, because I didn't get that far.

These are multiple different changes rather than a single one, so to make the review easier I suggest to brake it in multiple commits. I would state on each of them the purpose and the real case scenario where the change would be relevant.

Also, I would use clearer names for the variables or at least document their purpose.

The changes I've identified so far are the following:

• Improve label lookup using hash table: I think this is implementation dependent and should be improved by the user rather than by the library itself.
  Examples, as part of the documentation, are a usage demonstration, although can point the user the best practices, I prefer clarity rather than performance on them. For example, on the rust-secp256k1 bindings, I used a HashMap for the example because it is a familiar standard structure for Rust users. If they would like to gain more performance there, they have other tools available to replace that structure by themselves.
• On secp256k1_silentpayments_recipient_sort_cmp, I understood the change: (r1->index < r2->index) ? -1 : (r1->index > r2->index) ? 1 : 0 is to make the unstable secp256k1_hsort implementation stable. Considering it only affects secp256k1_silentpayments_sender_create_outputs, which didn't receive more changes than a variable removal, what are the performance improvements there?
• SECP256K1_SP_SCAN_BATCH: I just learned about it (ge_set_gej_all) during this review, but seems that affine to jacobian conversion is needed for the comparison against labels, and is faster to do it in batches. I think the impact of this improvement will only affect the small subset of transaction with large amount of outputs. A good benchmark for this would be coinjoin transactions, although is not supported by BIP 352, a test case is doable.
• Double head: I'm not sure of the target of this change, I guess is to skip already found or scanned inputs, but couldn't figure out what is tracking each head.
• Binary tree search for x-only lookups: I think it explain itself, faster lookups on the not labeled case. This may have its merits, but I need to remove the other changes to have a clear answer.

In general I agree with @jonasnick that we should define a clear target to benchmark and improve. As I've said before, the base case should be a wallet with a single label for change.
For other improvements, I would try to match up plausible real world scenarios before making complex changes in the code base of the PR.
Finally, by looking at bench_silentpayments_full_tx_scan, the use_labels case is very simple. If we want to test performance improvements on the label lookup, I would start there.

In conclusion, from the proposed commit and the discussion around it, the only changes I've found clear enough to consider are:

• Tracking of found outputs: simple enough, small performance improvement for the usual case, better for larger transactions.

[w0xlt]

Thanks @nymius for reviewing the changes, addressing the main points, and proposing a simplification.

I’m currently splitting the optimization commit into smaller pieces to make it easier to review.
I’ll also take a closer look at your commit and run it against the benchmark files.

The only part of the discussion that still feels a bit ambiguous is the “base” or “usual” case.
From my understanding of #1698 (review), the concern is not about typical usage, but rather about an attacker crafting malicious transactions with many outputs, causing the scanning process to take hours.

So the goal of this optimization would be to mitigate that scenario, not the collaborative one.

[w0xlt]

I ran the examples/silentpayments_mixed_1.c file with simplified the version suggested by @nymius jonasnick@311b4eb . It shows slightly worse performance (see below), but the simpler approach may still be worth it

Without the secp256k1_silentpayments_recipient_sort_cmp stabilization, I got 82s for the complex version vs. 114s for the simpler one. Whether the 30s difference justifies the additional complexity is up for discussion — I don’t have a strong opinion.

Answering the questions: secp256k1_silentpayments_recipient_sort_cmp stabilization + heads speed up the non-adversarial case. The stable sort ensures that the transaction outputs are ordered sequentially by the index $k$.

The optimized receiver implementation relies on a heuristic (the head pointers) that assumes the next output it is looking for ($k+1$) is located immediately after the previous one ($k$).

• With Stable Sort: The scanner complexity is roughly O(N) (Linear).
• Without Stable Sort: The scanner complexity degrades to O(N²) (Quadratic).

[theStack]

@w0xlt, @nymius: Thanks for investigating this deeper. I've now also had a chance to look at the suggested optimizations and came to similar conclusions as stated in #1765 (comment). I particularly agree with the stated points that the changes should not increase complexity significantly and that the most important optimization candidate to consider for mitigating the worst-case scanning attack is "skip outputs that we have already found" (as previously stated by @jonasnick, see #1698 (comment) and jonasnick@311b4eb). I don't think stabilizing the sorting helps at all, since this is something that happens at the sender side, and we can't rely on the attacker using a specific implementation (even if they did, it's trivial for them to shuffle the outputs after creation).

For the proposed target to benchmark, I'm proposing the following modified example that exhibits the worst-case scanning time based on a labels cache with one entry (for change outputs), by creating a tx with 23255 outputs [1] all targeted for Bob: 1df4287

Shower-thought from this morning: what if we treat the tx_outputs input as actual list that we modify, and remove an entry if it is found? This would have a similar effect as the "track found outputs" idea, but without the need of dynamic memory allocation. It's a ~10-lines diff and seems to work fine: theStack@9aba470
It reduces the run-time of the proposed example above from ~10 minutes to roughly ~2 seconds on my machine.

Any thoughts on this? Maybe I'm still missing something.

[1] that's an upper bound of maximum outputs per block: floor(1000000/43) = 23255

[nymius]

Shower-thought from this morning: what if we treat the tx_outputs input as actual list that we modify, and remove an entry if it is found? This would have a similar effect as the "track found outputs" idea, but without the need of dynamic memory allocation. It's a ~10-lines diff and seems to work fine: theStack@9aba470 It reduces the run-time of the proposed example above from ~10 minutes to roughly ~2 seconds on my machine.

Any thoughts on this? Maybe I'm still missing something.

1df4287 is a good target.
These are the runtime times I've obtained testing against this target:

Branch	Runtime
theStack/secp256k1@9aba470	~1.41s
theStack/secp256k1@9103229 (baseline)	~9m
jonasnick/secp256k1@311b4eb	~1.49s

I had to increase stack size to be able to fit all N_OUTPUT size allocations in the example.

Initially I preferred the is_found allocation rather than the element shifts. But your solution seems to be more performant.

[w0xlt]

@theStack Yes — if we want to keep only the adversarial-scenario optimizations, we can drop sort stabilization and the extra heads.

I like your idea of avoiding dynamic memory allocation. That’s a very interesting direction. On my machine, the scan completes in about 0.4s, which feels like a good balance between simplicity and the optimization needed for the labeled case.

Below are the changes I had to make for your example to run on my machine and to record the scan time.

diff --git a/examples/silentpayments.c b/examples/silentpayments.c
index 5e71e73..d43332f 100644
--- a/examples/silentpayments.c
+++ b/examples/silentpayments.c
@@ -10,6 +10,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <time.h>
 
 #include <secp256k1_extrakeys.h>
 #include <secp256k1_silentpayments.h>
@@ -112,15 +113,21 @@ const unsigned char* label_lookup(
     return NULL;
 }
 
+static secp256k1_xonly_pubkey tx_inputs[N_INPUTS];
+static const secp256k1_xonly_pubkey *tx_input_ptrs[N_INPUTS];
+static secp256k1_xonly_pubkey tx_outputs[N_OUTPUTS];
+static secp256k1_xonly_pubkey *tx_output_ptrs[N_OUTPUTS];
+static secp256k1_silentpayments_found_output found_outputs[N_OUTPUTS];
+static secp256k1_silentpayments_found_output *found_output_ptrs[N_OUTPUTS];
+static secp256k1_silentpayments_recipient recipients[N_OUTPUTS];
+static const secp256k1_silentpayments_recipient *recipient_ptrs[N_OUTPUTS];
+/* 2D array for holding multiple public key pairs. The second index, i.e., [2],
+ * is to represent the spend and scan public keys. */
+static unsigned char (*sp_addresses[N_OUTPUTS])[2][33];
+
 int main(void) {
     unsigned char randomize[32];
     unsigned char serialized_xonly[32];
-    secp256k1_xonly_pubkey tx_inputs[N_INPUTS];
-    const secp256k1_xonly_pubkey *tx_input_ptrs[N_INPUTS];
-    secp256k1_xonly_pubkey tx_outputs[N_OUTPUTS];
-    secp256k1_xonly_pubkey *tx_output_ptrs[N_OUTPUTS];
-    secp256k1_silentpayments_found_output found_outputs[N_OUTPUTS];
-    secp256k1_silentpayments_found_output *found_output_ptrs[N_OUTPUTS];
     secp256k1_silentpayments_prevouts_summary prevouts_summary;
     secp256k1_pubkey unlabeled_spend_pubkey;
     struct labels_cache bob_labels_cache;
@@ -209,11 +216,6 @@ int main(void) {
     {
         secp256k1_keypair sender_keypairs[N_INPUTS];
         const secp256k1_keypair *sender_keypair_ptrs[N_INPUTS];
-        secp256k1_silentpayments_recipient recipients[N_OUTPUTS];
-        const secp256k1_silentpayments_recipient *recipient_ptrs[N_OUTPUTS];
-        /* 2D array for holding multiple public key pairs. The second index, i.e., [2],
-         * is to represent the spend and scan public keys. */
-        unsigned char (*sp_addresses[N_OUTPUTS])[2][33];
         unsigned char seckey[32];
 
         printf("Sending...\n");
@@ -340,6 +342,9 @@ int main(void) {
              *        `secp256k1_silentpayments_recipient_prevouts_summary_create`
              *     2. Call `secp256k1_silentpayments_recipient_scan_outputs`
              */
+            clock_t start, end;
+            double cpu_time_used;
+
             ret = secp256k1_silentpayments_recipient_prevouts_summary_create(ctx,
                 &prevouts_summary,
                 smallest_outpoint,
@@ -356,14 +361,20 @@ int main(void) {
 
             /* Scan the transaction */
             n_found_outputs = 0;
+            
+            start = clock();
             ret = secp256k1_silentpayments_recipient_scan_outputs(ctx,
                 found_output_ptrs, &n_found_outputs,
-                (const secp256k1_xonly_pubkey * const *)tx_output_ptrs, N_OUTPUTS,
+                (const secp256k1_xonly_pubkey **)tx_output_ptrs, N_OUTPUTS,
                 bob_scan_key,
                 &prevouts_summary,
                 &unlabeled_spend_pubkey,
                 label_lookup, &bob_labels_cache /* NULL, NULL for no labels */
             );
+            end = clock();
+            cpu_time_used = ((double) (end - start)) / CLOCKS_PER_SEC;
+            printf("Bob's scan took %f seconds\n", cpu_time_used);
+            
             if (!ret) {
                 printf("This transaction is not valid for Silent Payments, skipping.\n");
                 return EXIT_SUCCESS;
@@ -435,7 +446,7 @@ int main(void) {
             n_found_outputs = 0;
             ret = secp256k1_silentpayments_recipient_scan_outputs(ctx,
                 found_output_ptrs, &n_found_outputs,
-                (const secp256k1_xonly_pubkey * const *)tx_output_ptrs, 1, /* dummy scan with one output (we only care about Bob) */
+                (const secp256k1_xonly_pubkey **)tx_output_ptrs, 1, /* dummy scan with one output (we only care about Bob) */
                 carol_scan_key,
                 &prevouts_summary,
                 &unlabeled_spend_pubkey,

[theStack]

@nymius, @w0xlt: Thanks once again for the quick feedback and for benchmarking! Shortly after my previous comment, I've been notified about yet another approach to tackle the worst-case scanning time attack (kudos to @furszy for bringing up the idea!), that I think is even more elegant: we can use the pointers in the tx_outputs list directly to track outputs by setting them to NULL if one has been found, and accordingly only treat them if they are non-NULL. With this, it's an only four lines of code change: theStack@2087f92. It kind of combines the previous two approaches of jonasnick@311b4eb and theStack@9aba470 (-> mark spent outputs, but not in a newly allocated array, but by modifying the tx_outputs input list, in order to avoid dynamic memory allocation), with very similar run-time results.

The only tiny drawback about these non-malloc approaches might be that something that is conceptually an "in" parameter is modified, which might be a bit unsound in a strict API design sense. On the other hand, it shouldn't matter for the user (I doubt that these lists passed in would ever be reused for anything else after by the callers), and we already do the same in the sending API for the recipients, so it's probably fine.

@theStack Yes — if we want to keep only the adversarial-scenario optimizations, we can drop sort stabilization and the extra heads.

The way I see it currently, code paths for non-adversarial scenarios with increasing k values would be hit so rarely in practice, that I'm sceptical that it's worth it put much effort into those optimizations. When scanning, the vast majority of transactions won't have any matches in the first place. Out of those few that do have a match, the vast majority will very likely again not contain any repeated recipient (IMHO it doesn't make that much sense to do that, unless the recipient explicitly asks "I want to receive my payment split up in multiple UTXOs, but still in a single tx"?), so in the bigger picture those optimizations wouldn't matter all that much, and I'd assume that the dominant factor should be by far all the (unavoidable) ECDH computations per transaction. But that's still more of a guess and it's still good to already have optimization ideas at hand if we need them in the future.

[w0xlt]

@theStack Thanks for continuing to refine the optimization. The deletion approach performs slightly better (0.40 s vs. 0.45 s), likely because deleting items shrinks the array and cuts the number of loop iterations by about 50% compared to nullifying them.

[theStack]

To summarize, the following table shows the proposed mitigations for the worst-case scanning attack so far, with benchmark results from my machine. The previous baseline commit with the worst-case example has been updated to include @w0xlt's changes, in order to work without stack size limit changes.
(EDIT: These benchmark results are based on a baseline that doesn't represent the worst-case and are thus not representative, as noticed by w0xlt below.)

Branch	Approach	Runtime
theStack@c16252d (Branch baseline)	modified example to exercise worst-case scanning, no fix	641.391838s
theStack@ec27977 (Branch fix1_...)	mark found outputs in calloced array	0.543969s
theStack@135ca0a (Branch fix2_...)	remove matched outputs by shifting remaining entries	0.514952s
theStack@8360150 (Branch fix3_...)	mark found outputs by NULL in tx_outputs input array	0.544740s

The run-times of the fixes vary slightly (the removal approach "fix2" being the fastest, confirming #1765 (comment) above), but are all in the same ballpark. I don't think exact performance results matter much here, as the goal of the mitigation should be to IMHO roughly cut the run-time down from "minutes" to "seconds" (and remember, this is already for the absolute worst-case, one giant non-standard transaction filling out a whole block, and it can only slow down one specific SP recipient). Thus, I decided to pick the the simplest approach that avoids dynamic memory allocation, i.e. fix number 3 using NULL as marker in tx_outputs.

With that tackled, I believe that all of the open questions and TODOs are addressed now (updated the PR description accordingly). The latest force-push also includes a rebase on master (to include the CI fix #1771).

[furszy]

went through 2c07380 only and left a few comments. Will keep moving.

[furszy]

In 2c07380:

Aren't we missing an infinity check here?
We ensure it during save through secp256k1_ge_to_bytes but the load internal secp256k1_ge_from_bytes doesn't seem to be checking it.

[theStack]

secp256k1_ge_from_bytes interprets a 64-byte array as affine coordinates (32 bytes for x,y each), so the notion of infinity doesn't exist here, the called function secp256k1_ge_from_storage always sets r->infinity = 0;.

As I currently understand it, we generally assume that user-facing opaque data types are only created and modified by public API functions, and the only protection against manual tampering are the magic bytes. Once that ARG_CHECK is passed in a _load function, we just assume that the object must have been created with the corresponding _save counterpart.

[real-or-random]

As I currently understand it, we generally assume that user-facing opaque data types are only created and modified by public API functions, and the only protection against manual tampering are the magic bytes. Once that ARG_CHECK is passed in a _load function, we just assume that the object must have been created with the corresponding _save counterpart.

Indeed, and I don't think we want to make any guarantees about what happens if the caller passes a modified object. And the magic bytes are just a best-effort protection against the user passing an opaque object of the wrong type.

[furszy]

In 2c07380:

q: why not include the label magic number in the output? It seems like a good way to ensure we're parsing a label and not just an arbitrary compressed group element.

[theStack]

Good question. The primary use-case of serializing labels right now is creating the label cache (at least for the "BIP approach" of scanning implemented in this PR), and for that I'd say it makes sense to target the most dense serialization for space-efficiency; having all key entries in the cache repeatedly prefixed with the exact same four bytes (increasing its size by >10%) seems wasteful, same for e.g. a large number of labels are saved to disk. (Sure, the users could manually strip those four bytes away and add them again before calling _label_parse, but that doesn't seem great either).

Leaving this open for others to weigh in, maybe it's worth it to do. In any case it seems that label serialization should be specified somewhere, I only notice now that it's apparently not specified in the BIP.

[real-or-random]

In any case it seems that label serialization should be specified somewhere,

It's not entirely clear in this case, but I agree that this is rather a protocol-level concern: interoperability could indeed matter if you want to use the same label DB with different scanning implementations. And in this case, size indeed matters, so I think we should not add the magic bytes.

[furszy]

In 2c07380:

Should be able to remove this line, the _ec_pubkey_create_helper() call you do below performs the same sk validity check.

[theStack]

Note that the _ec_pubkey_create_helper() call below performs a generator point multiplication on a different scalar, namely hash(b_scan || m), so its success doesn't tell us anything on whether the scan key is valid or not.

[fjahr]

Comments about the two approaches in general and ideas on how to concretely move on with this PR would be much appreciated. I see merit in both approaches, they are just addressing different use-cases, depending on how many labels are planned to be used. The drawback in the "BIP approach" is that it suffers (I think inherently) from the worst-case scanning attack, independently on how many labels are used. If that is a no-go, I think the "LabelSet approach" is a realistic option, with the drawback that it doesn't scale well for large-amount-of-labels use-cases. Maybe that's fine for the first release of the module, if that's well-documented.

(Ignorance warning: I have mostly been reading along here but I have not read all the context from the previous PRs and I have not reviewed all the code)

I guess you are hinting at the same thought I had in your last sentence: I think one approach that would be most defensive (as in should be completely safe for users) would be to only go with LabelSet and also set a max value for the number of labels that can be used. That max number would be somewhat arbitrary but we could set a target time on a target hardware and arrive at some benchmark that we feel comfortable with allowing. This of course wouldn't prevent users from using more labels but in order to do so they would need to call scanning multiple times which should make sure they know what they are getting into in terms of performance. They would likely read the docs and then complain here that this restriction should be removed because they have a use-case. Then we could consider further actions based on this feedback.

This still seems doable from a implementation POV and lifting the limit in a future version shouldn't cause any compatibility problems. Doing anything resembling that to mitigate the worst case for the BIP approach seems much worse UX wise and much harder to explain.

Also, I guess if we stay with the API from LabelSet internally the BIP approach could still be used if the number of labels is high and the number of outputs is manageable. But there might be some thresholds that users run into that are much harder to explain in the docs. And I honestly have no clue if the label_context/label_lookup UX is something that users desire, lacking the historical context from the previous PRs.

FWIW, my concerns with the worst case attack are somewhat mild but we should remember that this might be used in some of the most adversarial contexts imaginable: The users could a taker of donations with powerful political adversaries or an exchange with fierce competition for example. That's why I would have a hard time feeling comfortable with letting users just adapt it at this point.

[Eunovo]

===== BIP approach (calculate label candidates for each output, look them up in labels cache) =====
L= 1: [N=2: 351 us] [N=5: 200 us] [N=10: 231 us] [N=20: 261 us] [N=50: 289 us] [N=100: 451 us] [N=200: 745 us]
L= 2: [N=2: 81 us] [N=5: 109 us] [N=10: 82 us] [N=20: 153 us] [N=50: 218 us] [N=100: 379 us] [N=200: 631 us]
L= 3: [N=2: 55 us] [N=5: 59 us] [N=10: 75 us] [N=20: 99 us] [N=50: 185 us] [N=100: 299 us] [N=200: 586 us]
L= 5: [N=2: 52 us] [N=5: 58 us] [N=10: 70 us] [N=20: 90 us] [N=50: 171 us] [N=100: 302 us] [N=200: 514 us]
L=10: [N=2: 48 us] [N=5: 53 us] [N=10: 62 us] [N=20: 86 us] [N=50: 153 us] [N=100: 269 us] [N=200: 489 us]
L=20: [N=2: 44 us] [N=5: 52 us] [N=10: 64 us] [N=20: 86 us] [N=50: 147 us] [N=100: 267 us] [N=200: 481 us]
L=50: [N=2: 44 us] [N=5: 47 us] [N=10: 61 us] [N=20: 91 us] [N=50: 153 us] [N=100: 257 us] [N=200: 469 us]

===== Label-set approach (calculate output candidate for each label, look it up in outputs) =====
L= 1: [N=2: 39 us] [N=5: 38 us] [N=10: 37 us] [N=20: 38 us] [N=50: 41 us] [N=100: 48 us] [N=200: 64 us]
L= 2: [N=2: 37 us] [N=5: 36 us] [N=10: 36 us] [N=20: 37 us] [N=50: 41 us] [N=100: 46 us] [N=200: 65 us]
L= 3: [N=2: 40 us] [N=5: 36 us] [N=10: 37 us] [N=20: 39 us] [N=50: 43 us] [N=100: 49 us] [N=200: 75 us]
L= 5: [N=2: 39 us] [N=5: 39 us] [N=10: 41 us] [N=20: 42 us] [N=50: 47 us] [N=100: 51 us] [N=200: 84 us]
L=10: [N=2: 47 us] [N=5: 47 us] [N=10: 45 us] [N=20: 46 us] [N=50: 49 us] [N=100: 55 us] [N=200: 71 us]
L=20: [N=2: 58 us] [N=5: 56 us] [N=10: 58 us] [N=20: 55 us] [N=50: 59 us] [N=100: 66 us] [N=200: 84 us]
L=50: [N=2: 88 us] [N=5: 85 us] [N=10: 93 us] [N=20: 88 us] [N=50: 94 us] [N=100: 100 us] [N=200: 120 us]

These benchmark results from #1765 (comment) are interesting. I expect the BIP scanning approach benchmark to report increasing scanning times as N increases, independent of L, because we expect constant lookup times for labels, but this benchmark reports the scanning time using the BIP approach for L=1; N=2 to be several times higher than L=50; N=2. The scanning times shouldn't vary too much as we vary L (unless we drop it to 0).

It is also surprising to see such a large difference between the BIP approach and LabelSet scanning times for L=1; N=2.

I ran theStack@8eced64 on my machine and obtained the following results, which are more in line with my expectations:

Benchmark                               ,    Min(us)    ,    Avg(us)    ,    Max(us)    

sp_full_scan_BIP-algo_L=0_N=2           ,    48.4       ,    48.7       ,    49.3    
sp_full_scan_BIP-algo_L=0_N=5           ,    48.5       ,    48.6       ,    48.9    
sp_full_scan_BIP-algo_L=0_N=10          ,    48.6       ,    48.7       ,    48.8    
sp_full_scan_BIP-algo_L=0_N=20          ,    48.9       ,    49.0       ,    49.2    
sp_full_scan_BIP-algo_L=0_N=50          ,    49.6       ,    49.7       ,    49.8    
sp_full_scan_BIP-algo_L=0_N=100         ,    49.9       ,    50.8       ,    51.1    
sp_full_scan_BIP-algo_L=0_N=200         ,    51.0       ,    51.4       ,    52.4    
-----
sp_full_scan_BIP-algo_L=1_N=2           ,    52.4       ,    53.0       ,    53.8    
sp_full_scan_BIP-algo_L=1_N=5           ,    58.0       ,    58.2       ,    58.5    
sp_full_scan_BIP-algo_L=1_N=10          ,    64.9       ,    66.8       ,    68.1    
sp_full_scan_BIP-algo_L=1_N=20          ,    82.9       ,    83.6       ,    84.6    
sp_full_scan_BIP-algo_L=1_N=50          ,   143.0       ,   144.0       ,   146.0    
sp_full_scan_BIP-algo_L=1_N=100         ,   234.0       ,   239.0       ,   241.0    
sp_full_scan_BIP-algo_L=1_N=200         ,   412.0       ,   414.0       ,   417.0    
-----

sp_full_scan_LabelSet-algo_L=0_N=2      ,    48.8       ,    48.9       ,    49.3    
sp_full_scan_LabelSet-algo_L=0_N=5      ,    49.0       ,    49.1       ,    49.3    
sp_full_scan_LabelSet-algo_L=0_N=10     ,    48.2       ,    49.6       ,    50.3    
sp_full_scan_LabelSet-algo_L=0_N=20     ,    49.3       ,    49.4       ,    49.6    
sp_full_scan_LabelSet-algo_L=0_N=50     ,    56.5       ,    58.1       ,    59.5    
sp_full_scan_LabelSet-algo_L=0_N=100    ,    71.8       ,    74.1       ,    75.7    
sp_full_scan_LabelSet-algo_L=0_N=200    ,   112.0       ,   112.0       ,   112.0    
-----
sp_full_scan_LabelSet-algo_L=1_N=2      ,    49.6       ,    49.7       ,    49.9    
sp_full_scan_LabelSet-algo_L=1_N=5      ,    50.0       ,    50.1       ,    50.5    
sp_full_scan_LabelSet-algo_L=1_N=10     ,    50.8       ,    50.9       ,    51.0    
sp_full_scan_LabelSet-algo_L=1_N=20     ,    52.9       ,    53.0       ,    53.1    
sp_full_scan_LabelSet-algo_L=1_N=50     ,    60.6       ,    60.7       ,    60.9    
sp_full_scan_LabelSet-algo_L=1_N=100    ,    73.0       ,    75.5       ,    76.4    
sp_full_scan_LabelSet-algo_L=1_N=200    ,   109.0       ,   112.0       ,   114.0    
-----
sp_full_scan_LabelSet-algo_L=2_N=2      ,    48.8       ,    50.1       ,    50.7    
sp_full_scan_LabelSet-algo_L=2_N=5      ,    51.0       ,    51.1       ,    51.2    
sp_full_scan_LabelSet-algo_L=2_N=10     ,    49.5       ,    51.3       ,    52.5    
sp_full_scan_LabelSet-algo_L=2_N=20     ,    53.4       ,    53.8       ,    54.0    
sp_full_scan_LabelSet-algo_L=2_N=50     ,    59.7       ,    60.9       ,    61.9    
sp_full_scan_LabelSet-algo_L=2_N=100    ,    73.6       ,    75.4       ,    77.8    
sp_full_scan_LabelSet-algo_L=2_N=200    ,   109.0       ,   109.0       ,   109.0    
-----
sp_full_scan_LabelSet-algo_L=3_N=2      ,    49.4       ,    50.0       ,    51.6    
sp_full_scan_LabelSet-algo_L=3_N=5      ,    49.7       ,    49.9       ,    50.4    
sp_full_scan_LabelSet-algo_L=3_N=10     ,    50.4       ,    50.6       ,    50.8    
sp_full_scan_LabelSet-algo_L=3_N=20     ,    52.3       ,    52.5       ,    52.8    
sp_full_scan_LabelSet-algo_L=3_N=50     ,    59.9       ,    60.1       ,    60.3    
sp_full_scan_LabelSet-algo_L=3_N=100    ,    76.3       ,    78.8       ,    79.6    
sp_full_scan_LabelSet-algo_L=3_N=200    ,   113.0       ,   115.0       ,   116.0    
-----
sp_full_scan_LabelSet-algo_L=5_N=2      ,    53.9       ,    54.0       ,    54.1    
sp_full_scan_LabelSet-algo_L=5_N=5      ,    54.3       ,    54.4       ,    54.6    
sp_full_scan_LabelSet-algo_L=5_N=10     ,    55.2       ,    55.3       ,    55.4    
sp_full_scan_LabelSet-algo_L=5_N=20     ,    57.3       ,    57.5       ,    57.8    
sp_full_scan_LabelSet-algo_L=5_N=50     ,    62.6       ,    64.6       ,    65.6    
sp_full_scan_LabelSet-algo_L=5_N=100    ,    77.5       ,    78.6       ,    79.6    
sp_full_scan_LabelSet-algo_L=5_N=200    ,   112.0       ,   113.0       ,   114.0    
-----
sp_full_scan_LabelSet-algo_L=10_N=2     ,    56.0       ,    57.2       ,    59.0    
sp_full_scan_LabelSet-algo_L=10_N=5     ,    58.1       ,    59.4       ,    60.2    
sp_full_scan_LabelSet-algo_L=10_N=10    ,    60.2       ,    60.7       ,    61.2    
sp_full_scan_LabelSet-algo_L=10_N=20    ,    62.6       ,    63.0       ,    63.9    
sp_full_scan_LabelSet-algo_L=10_N=50    ,    70.7       ,    71.0       ,    71.2    
sp_full_scan_LabelSet-algo_L=10_N=100   ,    86.3       ,    87.4       ,    88.3    
sp_full_scan_LabelSet-algo_L=10_N=200   ,   119.0       ,   123.0       ,   128.0    
-----
sp_full_scan_LabelSet-algo_L=20_N=2     ,    69.0       ,    69.6       ,    70.5    
sp_full_scan_LabelSet-algo_L=20_N=5     ,    71.2       ,    71.2       ,    71.3    
sp_full_scan_LabelSet-algo_L=20_N=10    ,    72.5       ,    72.6       ,    72.7    
sp_full_scan_LabelSet-algo_L=20_N=20    ,    74.9       ,    75.0       ,    75.2    
sp_full_scan_LabelSet-algo_L=20_N=50    ,    83.4       ,    83.5       ,    83.6    
sp_full_scan_LabelSet-algo_L=20_N=100   ,    97.8       ,    99.3       ,   101.0    
sp_full_scan_LabelSet-algo_L=20_N=200   ,   137.0       ,   138.0       ,   139.0    
-----
sp_full_scan_LabelSet-algo_L=50_N=2     ,    97.2       ,    99.5       ,   100.0    
sp_full_scan_LabelSet-algo_L=50_N=5     ,   101.0       ,   102.0       ,   103.0    
sp_full_scan_LabelSet-algo_L=50_N=10    ,   103.0       ,   104.0       ,   105.0    
sp_full_scan_LabelSet-algo_L=50_N=20    ,   107.0       ,   107.0       ,   108.0    
sp_full_scan_LabelSet-algo_L=50_N=50    ,   114.0       ,   116.0       ,   117.0    
sp_full_scan_LabelSet-algo_L=50_N=100   ,   132.0       ,   133.0       ,   134.0    
sp_full_scan_LabelSet-algo_L=50_N=200   ,   165.0       ,   167.0       ,   168.0    

IIUC, the BIP scanning approach is designed to keep scanning times low as the number of labels increases. If we expect most transactions to have N << L, then the BIP scanning approach will be faster overall, especially when there is a significant number of labels. The LabelSet scanning approach takes almost twice as much time as the BIP approach for L=50;N=2.

@w0xlt 's benchmark results with Batch Inversion look promising, but I expect it to be slower than the BIP scanning approach when we scan real blocks, even with a relatively low number of labels.

[theStack]

@fjahr: Thanks for weighing in! After much back-and-forth over the last few weeks on the two approaches and their trade-offs, I've reached very similar conclusions and am now convinced that shipping the module with the BIP approach as-is would not be a good idea, and "LabelSet with limits" approach as you sketched out is the safest option. It seems that the worst-case scanning attack for large-number-of-labels use-cases could only be fixed at the protocol level anyways (by e.g. limiting the size of SP eligible transactions to 100kvB, or more concretely by bounding the number of taproot outputs), and until that is done the BIP approach should probably not be used at all.

I've opened a PR following the proposed "LabelSet scanning with a limit on labels" path: #1792 (by now only suggesting a limit by documentation, but that can easily be changed), where the discussion can be continued.

@Eunovo:

These benchmark results from #1765 (comment) are interesting. I expect the BIP scanning approach benchmark to report increasing scanning times as N increases, independent of L, because we expect constant lookup times for labels, but this benchmark reports the scanning time using the BIP approach for L=1; N=2 to be several times higher than L=50; N=2. The scanning times shouldn't vary too much as we vary L (unless we drop it to 0).

It is also surprising to see such a large difference between the BIP approach and LabelSet scanning times for L=1; N=2.

I remember also seeing these large differences for low L/N values and believe this was a flaw in the benchmark itself (when it was hand-written in the example binary, rather than using our benchmark framework); for some reason the very first benchmark result line showed significantly longer run-times than later ones, even when the order L or N values was re-arranged. With the "actual benchmark" as you ran it, I couldn't reproduce this large gap anymore.

IIUC, the BIP scanning approach is designed to keep scanning times low as the number of labels increases. If we expect most transactions to have N << L, then the BIP scanning approach will be faster overall, especially when there is a significant number of labels. The LabelSet scanning approach takes almost twice as much time as the BIP approach for L=50;N=2.

That matches my general understanding as well. The theoretical cross-over point should be at $2 * N &lt; L$, as that's the number of EC point additions needed for each approach (2*N BIP approach, L for the LabelSet approach).

[theStack]

Tackled the latest review suggestions #1765 (comment), #1765 (comment), #1765 (comment).

Also added TODO comments for limiting the k values both on the sender and receiver side, as this would be a proposed mitigation against the worst-case scanning issue (previously suggested by @RubenSomsen). Since this is a protocol change (backwards-incompatible at least in theory) and needs some more research and a BIP update, I'll leave this PR in draft state until that preparatory work is ready.

[theStack]

The PR has been updated to implement the suggested "limited-k" protocol restriction with an example value of K_max=1000: 66e41d0 (see discussion #1799 (comment) ff.)
It also includes the common case and worst-case scanning benchmarks with different group sizes (i.e. K_max limits), so reviewers can test out on how different limits would map to worst-scanning scanning times on their machines: e3e4162

I'm keeping the PR as a draft, since the limit used (K_max=1000) is still just an example number and more discussion is needed before a concrete change in BIP-352 is applied.