tests: silentpayments: cover labeled output beyond the first label batch #1943

secp256k1_silentpayments_check_label_batch maps a candidate index within a batch back to a tx output index via j_start + i / 2. In the first batch j_start == 0, so an incorrect mapping is indistinguishable from the correct one. The only existing test with labeled outputs past the first batch is the K_max test vector, which only checks the number of found outputs. For example, this mutant survives the current tests:

diff --git a/src/modules/silentpayments/main_impl.h b/src/modules/silentpayments/main_impl.h
index 0ea3ee1..533be40 100644
--- a/src/modules/silentpayments/main_impl.h
+++ b/src/modules/silentpayments/main_impl.h
@@ -589,7 +589,7 @@ static int secp256k1_silentpayments_check_label_batch(
         *label_tweak = label_lookup(label33, label_context);
         if (*label_tweak != NULL) {
             *label_ge = label_candidates_ge[i];
-            return (int)(j_start + i / 2);
+            return (int)((j_start + i) / 2);
         }
     }
     return -1;

This PR adds test_recipient_scan_label_batch_index. It puts LABEL_BATCH_SIZE non-matching outputs before a labeled output, so the match lands in the second batch.

Nice finding, Concept ACK

None of the existing tests put a labeled output past the first batch

There is one BIP-352 test vector where 2323 tx outputs are labeled matches and findings in higher batches (j_start > 0) occur, so an incorrect tx output index calculation with the mutant patch in the PR description is at least already exercised. But it unfortunately remains undetected, as the checking is not very thorough for this one, only the number of found outputs is verified and nothing else. Maybe we could do something like "if there are N matches, ensure that those are all with different output x-only pubkeys". This would have caught this issue I think, as with the mutant patch several matches map to the same tx outputs. Ideas welcome (could be in a different PR though).

I opened PR #1950 and confirmed that the suggested new check on test vectors of silent payments does capture the mutant described here.

Is this PR still necessary? This one checks for exacts outputs and is more explicit about testing the even and the odd candidate slots because it scans with p and -p.

Is this PR still necessary? This one checks for exacts outputs and is more explicit about testing the even and the odd candidate slots because it scans with p and -p.

I think it makes sense to keep this PR as well, explicitly verifying outputs and exercising both y-parities are useful additions. I didn't try, but I suppose it would be possible to come up with a mutant patch that's caught by this PR, but not by #1950.