bip352: clarify/fix specification to disallow sender to create outputs the receiver can't find #2153

#2106 introduced a limit on how many keys the receiver is allowed to scan. But the sender specification still allows for creating outputs that exceed that limit, risking loosing funds. I tried to organize each commit so to make review as easy as possible:

1. 4b9b490: Adds an additional check in the sender specification to disallow creating more keys/outputs than the receiver will surely scan (as per the specification).
2. 2028356: Adds an additional functional test requirement for wallet implementors.
3. 0de6dfb: Changes the reference implementation so that groups of silent payment addresses are calculated in a more sensible way (see details below). This commit WILL FAIL with the test vectors.
4. c14fd21: Implement the proposed check in the reference implementation.
5. 58c4ac6: Suggests minor redaction clarifications and fix typos.

Rationale

The specification of the sender and receiver seems to be out of sync, i.e. the sender can create a spec-compliant transaction such that a spec-compliant receiver will not find all funds received, implying the loss of funds. This is an edge case more theoretical than practical. I consulted @RubenSomsen about this possibility and we agreed opening a PR straight away would be safe for current users of silent payments.

#2106 introduced the K_max parameter which spirit is to limit how much effort the receiver is allowed to perform when scanning a candidate silent payment transaction.

But things are not as clear in the sender side: the spec asks the sender to:

Group receiver silent payment addresses by B_scan (e.g. each group consists of one B_scan and one or more B_m). If any of the groups exceed the limit of K_max (=2323) silent payment addresses, fail.

Let's analyze the case in which one is using a single silent payment address, but want to create 2324 (K_max + 1) outputs for the receiver. This is exactly scenario was covered by a new test case added together with #2106.

Well, there's one silent payment address, so that would make for one group with a single B_m within. Then, the required check (group size should not exceed K_max) should not fail and we are going to proceed with output generation with no more additional checks required by the specification. We would end up with a transaction with 2324 outputs, but the receiver will only scan for 2323 (K_max) and miss one output.

Note that the receiver could continue scanning anyhow and find all his funds, but version 1.1.x of the spec won't allow this.

About the changes in the reference implementation

Turns out that the test case expected result is that the described transaction creation should fail. And it does so in the reference implementation.

#2106 introduced a field count on the recipients specification for tests and added a test case in which the sender is given a single silent payment address and wants to create 2324 outputs for it, just as described above.

Before calling create_outputs(), which is the reference implementation of this specification, the test harness will clone the silent payment address count times and pass those copies to create_outputs(). Thus, in the sender process, it will create a group with 2324 elements, all of which carry the same silent payment address (i.e. the same B_m), and will fail as per the spec.

I don't believe this to be what any developer would implement from reading the specification:

Group receiver silent payment addresses by B_scan (e.g. each group consists of one B_scan and one or more B_m).

Indeed, I found this during a deep dive on the silent payments specification at Vinteum together with @lorenzolfm, @oleonardolima, @lucasdcf, @IsaqueFranklin, @joaozinhom, and @themhv and we all agreed we would not implement the specification like in the reference implementation.

We started looking at what wallets that support silent payments currently do and some didn't even implemented v1.1.0 yet, which introduced the described interpretation.

Pinging BIP authors @RubenSomsen, @theStack and @josibake for feedback, please.

Rebased after #1961.

ACK 47ef418a112c70b829da03cfe4b875a39098e6ba

Thanks for taking a deeper look at this!

Note that the sole reason for introducing a count field on the test vector specification was to have a more efficient encoding for them, as we wanted to avoid huge files in the multiple MB range (both the .json file in this repository and the generated test vectors in libsecp256k1, see discussion #2106 (comment) ff.). However, I doubt that any wallet implementation would want to organize a recipient list in this way internally, especially since other information about the same silent payments recipient addresses might differ (different output amount would be the most obvious example). I'd rather expect that the number of total recipients would always exactly match the number of created outputs, and thus a group can have multiple repeated $B_m$ entries. If I'm not missing anything, we could simply state that point more clearly in that sentence:

Group receiver silent payment addresses by B_scan (e.g. each group consists of one B_scan and one or more B_m).

and then there should be no need to do any other changes?

If I'm not missing anything, we could simply state that point more clearly in that sentence:

Group receiver silent payment addresses by B_scan (e.g. each group consists of one B_scan and one or more B_m).

and then there should be no need to do any other changes?

Maybe, since this grouping operation is the source of the confusion. Feels like the specification change introduced by #2106 was redacted after the reference implementation (I was not there), not the other way around, and it leaked this grouping solution into the spec.

I couldn't came out with an alternative wording that expresses the spirit of the grouping (limit the amount of outputs the sender can generate to a single receiver) without potentially introducing new edge cases. The cleanest I could get was adding the check on top of k since the specification is quite "implementation oriented", i.e. it's almost a pseudocode algorithm that induces a specific implementation.

I'd rather expect that the number of total recipients would always exactly match the number of created outputs, and thus a group can have multiple repeated B_m entries.

Agree, but I still think the specification should specifically disallow the pathological cases we can anticipate (e.g. splitting a payment into many outputs for "no apparent reason"). I can imagine someone forcing a constant amount for each utxo trying to mimic a coinjoin tx, and thus splitting a single payment in more than one output even if not needed. During the deep dive, we were indeed trying to attack the protocol, not only understand it.

I'd rather expect that the number of total recipients would always exactly match the number of created outputs, and thus a group can have multiple repeated B m entries.

Yes, I agree with that, however for clarity sake I think it would be better to just check the maximum number of outputs to be smaller than k_max, or at least indicate that in the BIP. I don't know if it's clear enough the way it is written now for wallet implementations to understand that the maximum number of outputs for a single B_scan needs to be 2323.

I think the main inconsistency this PR is trying to solve is that, on the spec, where the BIP is defining how to create the outputs, we have something like this:

Group receiver silent payment addresses by B_scan (e.g. each group consists of one B_scan and one or more B_m) If any of the groups exceed the limit of Kmax (=2323) silent payment addresses, fail.[18]

And then the spec proceeds to a for for each group, and later, for each B_m in the group we have this:

Optionally, repeat with k++ to create additional outputs for the current B_m

We are checking for the number of recipients, but the most important check would be for the number of outputs for a single B_scan, and if I were a wallet implementation, this line here lets me just increase the number of outputs to my heart's content.

I think this leaves room for naive implementations (or dumb AI agents) to create more outputs for a single B_scan than the receiver would actually scan, exceeding the k_max limit. I agree that any decent developer would not implement things like that, but I think it would be useful to clarify this in the spec.

Also, I have been looking into some wallets that implement silent payments, specifically Cake Wallet, Blue Wallet and Wasabi. Blue Wallet and Wasabi did not event implement the check for the k_max in the recipient group, much less for the number of outputs for a single B_scan. Cake Wallet has a weird implementation and also does not check for the k_max on the group yet. So I understand that most wallets do not follow the spec and nor need to, and I would not expect them to implement the spec to the letter just because the BIP says so, but it would be very nice to clarify in the BIP the true nature of k_max, and that the really important check would be the one in the number of outputs for a B_scan. I think that is the nature of what this PR is trying to accomplish.

Hey, what’s the status here?

I'm not convinced that the described scenario of potential BIP misinterpretation warrants a change in the reference implementation and/or test vectors. If anything, I'd prefer to clarify the grouping mechanism in text with a simple change. As stated earlier, the currently used (B_m, count) representation in the test vectors was introduced merely for a space-efficient encoding of .json file, and I wouldn't expect to find that being used in any real-world implementation (outside of tests). Looking at a few existing SP wallets/libraries and how they structure groups, it seems all of them represent the B_m values in the groups dictionary as lists rather than sets:

• SparrowWallet (Drongo): Map<ECKey, List<SilentPayment>> scanKeyGroups [1]
• SPDK: silent_payment_groups: HashMap<PublicKey, (SharedSecret, Vec<SilentPaymentAddress>) [2]
• BlueWallet: type SilentPaymentGroup = { Bscan: Uint8Array; BmValues: Array<[Uint8Array, number | undefined, number]>; }; [3] (note that the number denotes an index, not a counter)

This indicates that, at least in those implementations, repeated addresses within a group are not treated any differently, i.e. duplicates are fine. Under that interpretation, the current "if any of the grouping sizes exceed K_max, fail" check should already be sufficient to achieve the following "number of outputs for a B_scan" check:

it would be very nice to clarify in the BIP the true nature of k_max, and that the really important check would be the one in the number of outputs for a B_scan

I might still miss something though, happy to hear other opinions.

[1] https://github.com/sparrowwallet/drongo/blob/077d2142cc3aad84f6f58868cf8f17fc61027fdc/src/main/java/com/sparrowwallet/drongo/silentpayments/SilentPaymentUtils.java#L279 [2] https://github.com/cygnet3/spdk/blob/8394a0cfa08d974f7aed1a1112a3d4caee6e63c0/silentpayments/src/sending.rs#L49 [3] https://github.com/BlueWallet/SilentPayments/blob/37addd49b919281e0dc0eb40d0af46ecf9ef26fd/src/index.ts#L30

I'm not sure what's the best way forward, probably to close the PR. It's not clear to me how the ownership model for the bips work. I'm bringing light to a potential problem and suggesting alterations to avoid it in good faith. If we failed to convince the authors of the problem, I don't know what more can be done.

The first commit brings the suggested fix in the text alone, it can be cherry-picked. I couldn't figure out a better way to explain the group concept, it truly feels awkward. Anyways, it's goal is to limit the number of outputs one can generate for a single receiver, but as we argue, it does not achieve that.

A specification is supposed to protect future implementations. The commits changing the reference code show a potential implementation that abuses a spec-compliant receiver capability (and propose how to fix it). And it does so by processing the input test vectors differently (more literally) from the expected (something that people do all the time…).

I think this point is particularly relevant as there's no bip committee that will evaluate/attest candidate implementations for compliance. We have to achieve this by trying our best so that every developer interprets the spec in the same way.

Finally, checking what current implementations do merely checks if there's a problem today (which we briefly checked before opening this here, otherwise we would have proposed fixes with discretion in each project prior to opening this). The specification is the source of correctness for the implementations, not the other way around.

PS: just to not be misinterpreted by excess of emphasis, I truly want silent payments to be prevalent in the ecosystem, it's a step in the right direction.

Hey @edilmedeiros , apologies for the slow response on this, been busy! A big thanks to you and the Vintuem team for digging into the spec, especially with a goal of hardening. Much appreciated.

I do think the claims in the PR are a bit overblown, but will agree there is some ambiguity in the text. My intended reading of the spec is that the sender is grouping "output requests," i.e. "I wan't this many taproot outputs in the final transaction." It was not meant to read as unique silent payment addresses. We mention elsewhere in the BIP that the same silent payment address can be used repeatedly to get multiple unique outputs. With this understanding, the same $B_{m}$ would appear in the group multiple times and the existing $k_{max}$ group size check is respected.

However, just because this is clear to me doesn't mean it's clear in the text! I’d prefer if we kept this PR as a minimal text-only clarification. What if we change:

Group receiver silent payment addresses by B_scan (e.g. each group consists of one B_scan and one or more B_m) If any of the groups exceed the limit of K_max (=2323) silent payment addresses, fail.

to:

Group the B_m values for the outputs to be created by B_scan (e.g. each group consists of one B_scan and one or more B_m entries). If multiple outputs are to be created for the same silent payment address, include its B_m in the group once for each output. If any group contains more than K_max (=2323) B_m entries, fail.

And changing:

Optionally, repeat with k++ to create additional outputs for the current B_m If no additional outputs are required, continue to the next B_m with k++

to:

Continue to the next B_m with k++

With that wording, I think the intended algorithm is more clear. What do you think @edilmedeiros , @IsaqueFranklin and @theStack ?

@edilmedeiros: Sorry if my previous comments came across as overly dismissive or discouraging, that was not my intention at all! Discussing this is valuable even if we don't fully agree yet on what the concrete solution should look like. I think the main point of disagreement really stems from how "group" is defined. There is a definition the BIP authors had in mind (-> each group item represents one output to be created), and an alternative one that the authors of this PR interpreted (-> each group item represents a unique address). There are two ways to fix this:

1. clarify the grouping mechanism to match the originally intended definition; the existing K_max check works as intended, no other changes are needed
2. follow the alternative grouping definition, adapt the reference implementation accordingly, with a K_max check at a different place

The PR in its current state follows approach 2), and I'd claim that this would create potentially more confusion in the ecosystem as 1), even more so as apparently no existing implementations seem to have followed that alternative definition so far (judging merely by looking at the used data structures). Other than enabling a more direct exercising of the test vectors introduced in #2106 (regarding the introduced "count" field), I don't see why such a grouping representation would be practical for wallets. So I agree that there is a problem in the sense that the current spec is ambiguous, but I'm not convinced that the current state of the PR is the right solution. @josibake: I fully agree with the analysis and your suggestion sounds good to me.