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

[edilmedeiros]

#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.

[jonatack]

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

[edilmedeiros]

Rebased after #1961.

[joaozinhom]

ACK 47ef418

[theStack]

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?

[edilmedeiros]

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.

[IsaqueFranklin]

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.