Add "silentpayments" module implementing BIP352 (take 5, using "LabelSet" scanning approach)

[theStack]

Summary

This PR implements BIP352 with scanning limited to full-nodes. Compared to #1765, an alternative scanning method called the "LabelSet approach" (initially proposed by @w0xlt) is used, with the primary goal of fixing the worst-case scanning attack, discovered and discussed at length in previous takes 3 and 4. In constrast to the previously used "BIP approach", the scanning cost depends primarily on the number of labels (something the user can control and the module can set a limit on), rather than the number of transaction outputs (limited only by Bitcoin's consensus rules, i.e. the block weight limit). See https://gist.github.com/theStack/25c77747838610931e8bbeb9d76faf78 for a high-level comparison between the two approaches with benchmarks, and a Python implementation based on secp256k1lab.

Pros and cons

Other than mitigating adversarial scanning scenarios, the LabelSet approach offers additional advantages:

• simpler API: passing an explicit list of labels to scan for is much simpler than having to maintain a label cache data structure and provide a callback function; passing the transaction outputs as raw 32-byte x-only pubkeys is also simpler for the user, as no prior secp256k1_xonly_pubkey_parse calls are needed anymore (see Add "silentpayments" module implementing BIP352 (take 4, limited to full-node scanning) #1765 (comment) for more details about API and implementation between the two approaches)
• faster also for the common case (i.e. no match), if the number of labels used is reasonably small, as recent benchmarks indicate (in theory, the LabelSet approach should be faster whenever the number of labels L is smaller than twice the number of the scanned tx's taproot outputs N, i.e. if L < 2 * N).
• simpler implementation: scanning resembles the flow on the sender side (i.e. start with unlabeled public key, end with the taproot output), and there is no need to handle the missing y-parity information in x-only public keys (by trying both)

The main drawback of the LabelSet approach is that every additional label increases the scanning cost by at least one EC point addition. As a result, it is definitely not suitable for use cases that require a large number of labels.

Limiting the number of labels

To be on the safe side and still avoid the worst-case scanning attack, it was suggested to limit the number of supported labels in the module. Currently, this limit is not enforced in code; it only appears as a WARNING in the API header documentation, with the recommendation to not use more than 20 (that's the number where the worst-case takes about a second on my machine, see benchmarks below). It is unclear whether this is sufficient. We may want to enforce a hard limit instead, or introduce a configurable limit (for example at compile time or via an API call). This would better communicate that using values outside the recommended range is done at the user's own risk.

For a minimalist solution, one could in theory support only a single label for now. That's of course very restrictive, but would still comply with the minimum BIP requirements (see https://github.com/bitcoin/bips/blob/fc00f51c229088c447b3694cca9bf14ace0e1a96/bip-0352.mediawiki?plain=1#L131). The advantage would be that it would result in the simplest possible API and implementation, and expectations would be very clearly expressed to the user, if the parameter might be even called e.g. change_label.

Benchmarks

Benchmarks have been updated to show average scenarios over a N/L matrix (N...number of tx outputs, L... number of labels) as well as the worst case scenario:

Results on my machine

$ ./build/bin/bench silentpayments
Benchmark                               ,    Min(us)    ,    Avg(us)    ,    Max(us)

silentpayments_scan_nomatch_N=10_L=0    ,    41.1       ,    41.1       ,    41.2
silentpayments_scan_nomatch_N=10_L=1    ,    42.1       ,    42.1       ,    42.2
silentpayments_scan_nomatch_N=10_L=2    ,    43.1       ,    43.2       ,    43.2
silentpayments_scan_nomatch_N=10_L=5    ,    46.1       ,    46.2       ,    46.2
silentpayments_scan_nomatch_N=10_L=10   ,    51.6       ,    51.6       ,    51.7
silentpayments_scan_nomatch_N=10_L=20   ,    62.6       ,    62.7       ,    62.7
silentpayments_scan_nomatch_N=10_L=50   ,    96.4       ,    96.5       ,    96.6
silentpayments_scan_nomatch_N=10_L=100  ,   153.0       ,   153.0       ,   153.0

silentpayments_scan_nomatch_N=100_L=0   ,    43.5       ,    43.5       ,    43.5
silentpayments_scan_nomatch_N=100_L=1   ,    44.4       ,    44.5       ,    44.5
silentpayments_scan_nomatch_N=100_L=2   ,    45.5       ,    45.5       ,    45.5
silentpayments_scan_nomatch_N=100_L=5   ,    48.5       ,    48.5       ,    48.6
silentpayments_scan_nomatch_N=100_L=10  ,    53.9       ,    54.0       ,    54.0
silentpayments_scan_nomatch_N=100_L=20  ,    65.0       ,    65.0       ,    65.1
silentpayments_scan_nomatch_N=100_L=50  ,    99.0       ,    99.0       ,    99.1
silentpayments_scan_nomatch_N=100_L=100 ,   156.0       ,   156.0       ,   156.0

silentpayments_scan_nomatch_N=2325_L=0  ,   220.0       ,   221.0       ,   222.0
silentpayments_scan_nomatch_N=2325_L=1  ,   221.0       ,   222.0       ,   223.0
silentpayments_scan_nomatch_N=2325_L=2  ,   222.0       ,   223.0       ,   223.0
silentpayments_scan_nomatch_N=2325_L=5  ,   226.0       ,   227.0       ,   228.0
silentpayments_scan_nomatch_N=2325_L=10 ,   232.0       ,   233.0       ,   234.0
silentpayments_scan_nomatch_N=2325_L=20 ,   243.0       ,   244.0       ,   245.0
silentpayments_scan_nomatch_N=2325_L=50 ,   279.0       ,   280.0       ,   280.0
silentpayments_scan_nomatch_N=2325_L=100,   339.0       ,   339.0       ,   340.0

silentpayments_scan_worstcase_L=1       , 416567.0       , 416567.0       , 416567.0
silentpayments_scan_worstcase_L=2       , 444503.0       , 444503.0       , 444503.0
silentpayments_scan_worstcase_L=5       , 531165.0       , 531165.0       , 531165.0
silentpayments_scan_worstcase_L=10      , 674923.0       , 674923.0       , 674923.0
silentpayments_scan_worstcase_L=20      , 961857.0       , 961857.0       , 961857.0
silentpayments_scan_worstcase_L=50      , 1820950.0       , 1820950.0       , 1820950.0
silentpayments_scan_worstcase_L=100     , 3249931.0       , 3249931.0       , 3249931.0

History and details about the "BIP approach"

Prior takes of the "silentpayments" module (PRs #1471, #1519, #1698, #1765) implemented scanning for labels as suggested in BIP352 (the "BIP approach"): for each taproot output, subtract the unlabeled output candidate and check if the result is in a pre-calculated label cache. This method has the useful property of being performance-independent of the number of labels to scan for, as the lookups in the label cache are constant and fast, if implemented in a proper efficient data structure like a hash map (even hundreds of thousands of labels can be checked without a noticeable loss in performance, see the showcase in https://groups.google.com/g/bitcoindev/c/bP6ktUyCOJI/m/HGCF9YxNAQAJ).

One drawback of the BIP approach is that it scales poorly for pathological transactions containing a large number of outputs, all corresponding to Silent Payments recipients sharing the same scan key. In the worst-case scenario of a single, very large transaction filling an entire block (note that such a transaction would be non-standard, but still consensus-valid), scanning could take several minutes for the targeted recipient (see #1698 (review)). Proposed mitigations in takes 3 and 4 that were based on the BIP approach turned out to be insufficient to fix the problem, and it is currently believed that this is an inherent problem that could only be solved by a protocol change (by e.g. limiting the Silent Payments eligible transactions to a smaller number of outputs).

Therefore, a different way of scanning has been proposed that works in the opposite direction: iterate over a list of explicitly passed labels, add the unlabeled output candidate and check if the result is in the list of taproot outputs (via binary search to be efficient), i.e. the "LabelSet approach".

Recommended review music

https://www.youtube.com/watch?v=Hm-q80gA7NI

[fjahr]

Concept ACK per my rationale in the take 4 PR

To be on the safe side and still avoid the worst-case scanning attack, it was suggested to limit the number of supported labels in the module. Currently, this limit is not enforced in code; it only appears as a WARNING in the API header documentation, with the recommendation to not use more than 20 (that's the number where the worst-case takes about a second on my machine, see benchmarks below). It is unclear whether this is sufficient. We may want to enforce a hard limit instead, or introduce a configurable limit (for example at compile time or via an API call).

I think it's good to add this documentation and warn of the performance penalties with a specific number as a reference is good but I still think we should probably add a hard limit too which can be higher. I was thinking of the order of 1000 without having run any actual benchmarks. From a high level perspective I think such a number implies that the user probably doesn't generate new labels manually and uses the static sp addresses in different places (the typical usage pattern we mostly expect I think). Instead such a user may have some kind of automation in place that churns out new sp addresses with new labels and that the number of labels may potentially grow unbounded. The goal would be that such a user runs into this limit rather than that their servers are brought to their knees unexpectedly, which should be the better outcome in my mind. But curious what others think.

[theStack]

@fjahr: Sounds reasonable. I've added a hard limit on the number of labels as suggested, defined by a public API constant SECP256K1_SILENTPAYMENTS_MAX_LABELS, currently set to 500 [1] (very arbitrary, no idea if 15 seconds is still okay or not, but more than that seems definitely too much in my opinion). The n_label_entries parameter for the scan function is ARG_CHECKed against that, i.e. any values exceeding that are treated as illegal.

Some optional ideas on top that came to my mind:

• we could wrap the limit #define in a conditional #ifndef block in order to allow the user to override the limit at compile-time; if that's the case, one could emit a warning in the #else block via #warning to notify the user that this is outside of the recommended range and could lead to very high worst-case scanning times (EDIT: I guess this doesn't work in general, considering that it would need a recompilation of the silentpayments module for the user-defined limit to take effect).
• we could limit the number of labels already at label creation time, in the sense that only values in the range of $0..MAX-1$ are allowed to be passed as $m$ parameter to _label_create. This would allow to limit labels as early as possible (it's kind of bad if one can create labeled addresses, but then not be able to scan for them), but on the other hand that assumes that labels are only ever created in a contiguous range starting from zero. The currently proposed sp() output descriptor format allows to specify arbitrary ranges, see BIP Draft: Silent Payment Output Script Descriptors bitcoin/bips#2047 (comment). Considering that, limiting the $m$ values to one range seems a bit too restrictive.

[1] worst-case benchmarks on my machine:

$ ./build/bin/bench silentpayments_scan_worstcase
Benchmark                               ,    Min(us)    ,    Avg(us)    ,    Max(us)

silentpayments_scan_worstcase_L=1       , 415553.0       , 415553.0       , 415553.0
silentpayments_scan_worstcase_L=2       , 444280.0       , 444280.0       , 444280.0
silentpayments_scan_worstcase_L=5       , 529870.0       , 529870.0       , 529870.0
silentpayments_scan_worstcase_L=10      , 672437.0       , 672437.0       , 672437.0
silentpayments_scan_worstcase_L=20      , 956493.0       , 956493.0       , 956493.0
silentpayments_scan_worstcase_L=50      , 1808294.0       , 1808294.0       , 1808294.0
silentpayments_scan_worstcase_L=100     , 3224289.0       , 3224289.0       , 3224289.0
silentpayments_scan_worstcase_L=200     , 6058590.0       , 6058590.0       , 6058590.0
silentpayments_scan_worstcase_L=500     , 14562453.0       , 14562453.0       , 14562453.0

[w0xlt]

Approach ACK. Will review in detail soon.

For the reasons outlined in the "Pros and cons" section of the PR description, particularly the simpler API, improved common-case performance for small label sets and mitigation of the worst-case scanning attack—the LabelSet approach seems like the right default choice.

Given that most users are expected to have a relatively small number of labels (likely well within the SECP256K1_SILENTPAYMENTS_MAX_LABELS bound), this should cover the typical use case nicely.

If there's sufficient demand, a hybrid approach could always be explored in a future iteration to accommodate more specialized scenarios.

[Sjors]

It might be good to have a separate Github issue to discuss this.

I think it's fine to initially go for the LabelSet approach, since many current implementations are mobile, which means they benefit from the safety this provides and won't run into the label limit anytime soon.

To reduce review burden on this PR, we can add the hybrid approach in a followup, where we pick the BIP mechanism based on e.g. the 8 * n_tx_outputs < n_labels + 2 heuristic (w0xlt@6c76c7c#diff-4d053be8d1f6d948b412f26ae89711a9dcd2a2683da581cb52b9f6757480361b).

I do think that hybrid approach is useful. Someone who goes through the trouble of automating label generation might happily add some CPU cores if lets them avoid the complexity of splitting their users across multiple watch keys. Especially if the cost of an attack far outweighs the cost to deal with it.

We could document the hardware required to guarantee that a worst case block takes less than 10 minutes to process. For the hybrid approach that's just a single number IIUC. For the LabelSet approach it's a function of L. With that in place a warning should be enough IMO, for L > ? based on lower end hardware (mobile) and e.g. a 10 second worst case.

[Eunovo]

I had an offline conversation with @RubenSomsen about this. The conversations around the scanning approaches have been focused on the time it takes to complete a tx scan, but it's also important that we consider the time it takes to scan an entire Block.

BIP-style scanning scales with the number of outputs, independently of the number of transactions.

LabelSet scanning scales with the number of labels * the number of transactions. LabelSet will be Fast when you scan Blocks with few transactions and few labels, but it will get slower compared to BIP-style scanning as the number of transactions increases, even without an increase in the number of labels.

I think we should do more Block scanning benchmarks. I am interested in doing this myself next week. I can use LabelSet scanning in bitcoin/bitcoin#32966 and benchmark Block scanning times.

[theStack]

@Sjors: @Eunovo: I've opened issue #1799 for general discussions about the different scanning approaches and replied to your comments there, to keep the discussion this PR focused on the concrete implementation of the LabelSet approach.

[Sjors]

@theStack would it also make sense to split everything pre-label out of this PR so it can be merged earlier? Or is it too entangled?

[theStack]

@theStack would it also make sense to split everything pre-label out of this PR so it can be merged earlier? Or is it too entangled?

The two scanning approaches have very different APIs. We could in theory add a dedicated API function for no-labels scanning that is shared in both PRs (and create a separate PR that only contains that), but considering that there is a strong recommendation in the BIP to always scan at least for the change label (m=0), I don't think it would be very useful in practice.
So the only splitting possibility I see would be a PR that contains only sending capabilities (previously suggested in take 3, see #1698 (comment)).