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.