Suggested by theStack: add a new check on silent payments vector tests: if N outputs are found, ensure they all have distinct x-only pubkeys.
This catches the mutant from #1943 (j_start + i / 2 -> (j_start + i) / 2 in secp256k1_silentpayments_check_label_batch), which maps several matches to the same tx output and previously went undetected.
906 | @@ -907,6 +907,12 @@ void run_silentpayments_test_vector_receive(const struct bip352_test_vector *tes 907 | } 908 | } 909 | CHECK(n_found == subtest->num_found_output_pubkeys); 910 | + /* all found outputs must have distinct x-only pubkeys */ 911 | + for (i = 0; i < n_found; i++) { 912 | + for (j = i + 1; j < n_found; j++) { 913 | + CHECK(secp256k1_xonly_pubkey_cmp(CTX, &found_outputs[i]->output, &found_outputs[j]->output) != 0); 914 | + } 915 | + }
nit: could put this into an else-branch for if (subtest->full_check) { ... } above, as for full check test vectors the outputs are already verified in detail
Makes sense, done on 1c306ddf8a249fc52b9a1ca3eabf9ba88c539e19
ACK 00a051ac3d626a8e5ba32070bd2c084fe2e0fb19
Nice! We could be even more thorough by checking the set of found outputs must be a subset of the scanned outputs, but for a very simple structural sanity check this seems more than enough, considering it killed a mutant :zombie:. For more detailed checks new test vectors could be introduced that contain the expected outputs.
906 | @@ -907,6 +907,12 @@ void run_silentpayments_test_vector_receive(const struct bip352_test_vector *tes 907 | } 908 | } 909 | CHECK(n_found == subtest->num_found_output_pubkeys); 910 | + /* all found outputs must have distinct x-only pubkeys */ 911 | + for (i = 0; i < n_found; i++) { 912 | + for (j = i + 1; j < n_found; j++) {
note for other reviewers: this has quadratic time complexity, but given that n_found is bounded by 2323 ( SECP256K1_SILENTPAYMENTS_RECIPIENT_GROUP_LIMIT) and the loop body is cheap, this seems fine; the run-time of the tests didn't increase noticeably on my machine
Thanks @theStack for reviewing. Force-pushed 1c306ddf8a249fc52b9a1ca3eabf9ba88c539e19 addressing #1950 (review)
re-ACK 1c306ddf8a249fc52b9a1ca3eabf9ba88c539e19
Suggested by theStack in #1943.
904 | @@ -905,6 +905,13 @@ void run_silentpayments_test_vector_receive(const struct bip352_test_vector *tes 905 | CHECK(found_label_tweak != NULL); 906 | } 907 | } 908 | + } else { 909 | + /* all found outputs must have distinct x-only pubkeys */
Now that this comment appears behind the else, it looked to me as if the distinctness property is somehow implied only in the else branch. Something like this would make it clearer:
/* Not performing a full check, so resort to a structural sanity check: */
/* All outputs must have distinct x-only pubkeys. */
I agree, done on 1c6f7930848009281edc468b547dd2056c175901
utACK 1c6f7930848009281edc468b547dd2056c175901
re-ACK 1c6f7930848009281edc468b547dd2056c175901