Add coverage for an invalid Taproot keypair passed alongside a valid one, following the plain secret key tests in #1931.
Both orderings are tested. The invalid-first case catches a missing early return in the keypair loop: the valid keypair leaves a nonzero sum, so the subsequent zero-sum check would not catch the failure.
cc @brunoerg
Concept ACK
I'm adding more mutation operators to my tool and nested early-return deletion is one of them, the following mutant isn't killed on master but killed with the test case added here (nice!):
diff --git a/src/modules/silentpayments/main_impl.h b/src/modules/silentpayments/main_impl.h
index e42dc1a..8d7be21 100644
--- a/src/modules/silentpayments/main_impl.h
+++ b/src/modules/silentpayments/main_impl.h
@@ -249,7 +249,6 @@ int secp256k1_silentpayments_sender_create_outputs(
if (!ret) {
secp256k1_scalar_clear(&addend);
secp256k1_scalar_clear(&seckey_sum_scalar);
- return 0;
}
if (secp256k1_fe_is_odd(&addend_point.y)) {
secp256k1_scalar_negate(&addend, &addend);
294 | @@ -295,6 +295,20 @@ static void test_send_api(void) { 295 | p2[0] = secp256k1_group_order_bytes; 296 | CHECK(secp256k1_silentpayments_sender_create_outputs(CTX, op, rp, 2, SMALLEST_OUTPOINT, NULL, 0, p2, 2) == 0); 297 | } 298 | + /* Check that an invalid keypair is caught even when it is passed alongside a valid one.
The "first nor last keypair skipped" rationale is already covered by the existing checks. What this block actually adds is the invalid-first ordering: if the early return 0; were dropped, a valid keypair followed by an invalid one still fails via the zero-sum check, but an invalid keypair followed by a valid one leaves a nonzero sum and wrongly succeeds.
Could you reword the comment to say that, e.g. "pass the invalid keypair first, followed by a valid one, so that silently skipping it is not masked by the subsequent zero-sum check"?
Reworded the comment in a044ade to explain the invalid-first case, and updated the PR description to match. Also applied the const placement suggestion in ca6b4a2.
294 | @@ -295,6 +295,20 @@ static void test_send_api(void) { 295 | p2[0] = secp256k1_group_order_bytes; 296 | CHECK(secp256k1_silentpayments_sender_create_outputs(CTX, op, rp, 2, SMALLEST_OUTPOINT, NULL, 0, p2, 2) == 0); 297 | } 298 | + /* Check that an invalid keypair is caught even when it is passed alongside a valid one. 299 | + * The invalid keypair is tested in both positions so that neither the first nor the 300 | + * last keypair is skipped by the check. */ 301 | + { 302 | + secp256k1_keypair valid_keypair; 303 | + const secp256k1_keypair *t2[2];
nit:
secp256k1_keypair const *t2[2];
No blocking issues were identified.
<details> <summary>Pull request overview</summary>
Adds silent-payments sender test coverage for rejecting invalid Taproot keypairs mixed with valid keypairs.
Changes:
</details>
<details> <summary>File summaries</summary>
| File | Description |
|---|---|
src/modules/silentpayments/tests_impl.h |
Adds mixed-validity Taproot keypair rejection tests. |
</details>
<details> <summary>Review details</summary>
</details>
💡 <a href="/bitcoin-core/secp256k1/new/master?filename=.github/skills/code-review/SKILL.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add a code-review agent skill</a> or configure MCP servers for context-aware, tailored reviews. <a href="https://docs.github.com/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review?tool=webui#mcp-servers-and-agent-skills" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn more in the docs.</a>
Concept ACK @Yudis-bit: Could you squash the commits, please?
Squashed into e5b4d58. No changes to the final diff.
Rechecked e5b4d58: all 225 local tests pass. Removing the early return in the keypair loop passes the parent tests but fails the new invalid-first assertion. Ready for another look.