#36405 test: look for the wtxid in p2p_blocksonly
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36405 test: look for the wtxid in p2p_blocksonly | Ready | 2 | Strong | 0 + 5 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Fixes an assertion in `test/functional/p2p_blocksonly.py` to check for a transaction's wtxid rather than its txid when verifying that RPC-submitted transactions are not announced to block-relay-only peers. It also adds an outbound full-relay peer connection to positively assert receipt of the wtxid announcement.
Problem: The test peer negotiates `wtxidrelay` and the test transaction contains a witness, meaning announcements use the wtxid instead of the txid. Because the test previously asserted the absence of the txid, the check passed vacuously even if the node improperly announced the transaction.
Evidence of importance:
- PR description: "The peer is a P2PTxInvStore, which negotiates wtxidrelay, so an announcement would carry the wtxid. The MiniWallet transaction has a witness, so its txid and wtxid differ, and the check passes whether or not the node announces it."
- Commit message: "With the connection opened as outbound-full-relay rather than block-relay-only, the old assertion still passes and the new one fails."
Risk of a mistake: low (tests). Modifies only functional test assertions and adds a test peer connection.
Changed since the previous assessment: Added an outbound-full-relay peer to the functional test to positively assert that the wtxid is announced, ensuring the absence check on the block-relay connection cannot pass vacuously.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- nothing: PR has multiple ACKs and all comments are resolved
Agreement
Strong
substantive support, no open objection (AndreaDiazCorreia)
Thread notes: Added an ACK from AndreaDiazCorreia with substantive testing and code review rationale. All claims remain settled and nothing is pending.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| maflcko (member) | suggestion, correctness | Add another peer to check that the transaction is received, to prevent future silent test breakage. | accepted · fix pushed · author adjusted the peer check and maflcko ACKed | answered | yes | 2026-10-12: 'maybe there should be another peer to check it is received for it? Otherwise the test will likely silently break again in the future?' Status from: 2026-10-14: maflcko: 'lgtm ACK 22535b460568be47e57aadfbe50d489becb77577' |
| maflcko (member) | suggestion, approach | Use sync_with_ping instead of waiting for a broadcast when mocktime is used without the unbroadcast set. | accepted · fix pushed · author dropped the wait and used sync_with_ping in 22535b4, and maflcko ACKed | answered | yes | 2026-10-13: 'There is no need to wait for a broadcast, when not using the unbroadcast set for this test. Also, with mocktime, this should probably be a sync_with_ping instead?' Status from: 2026-10-14: maflcko: 'lgtm ACK 22535b460568be47e57aadfbe50d489becb77577' |
Support:
- mercie-ux (contributor) (Concept ACK, 2026-10-07): (no reason given) [not substantive]
- maflcko (member) (ACK, 2026-10-14): lgtm [not substantive]
- AndreaDiazCorreia (none) (ACK, 2026-10-26) [both: p2p_blocksonly.py]: Ran p2p_blocksonly.py with v1 and v2 transport. With conn changed to outbound-full-relay, the new assertion fails while the old txid check still passes. Using txid in both assertions also fails at the positive control.
Participants:
- mercie-ux (contributor, support, 1 statements 2026-10-07 to 2026-10-07): Concept ACK without rationale
- maflcko (member, support, 3 statements 2026-10-12 to 2026-10-14): ACKed after author addressed suggestions on self-checking assert and sync_with_ping
- AndreaDiazCorreia (none, support, 1 statements 2026-10-26 to 2026-10-26): ACK with testing of failure and control cases across v1 and v2 transport
Files
7 lines under test/bench/ci.
- test/functional/p2p_blocksonly.py +5/-2
Card
The short text the ranking pass compares across PRs.
Fixes an assertion in functional test p2p_blocksonly.py by checking for the transaction wtxid instead of txid. Because the test peer negotiates wtxidrelay and the transaction has a witness, any announcement uses the wtxid, causing the previous txid absence check to pass vacuously even if the transaction was announced. The PR also adds an outbound-full-relay peer to positively assert that the wtxid was broadcast.
Data
thread read through 2026-10-26 (7 statements), head 22535b4605
labels: Tests
Categories
P2P
P3 · test coverage (ranking pass: Test-only repair of a vacuous assertion; ordered inside the test cluster.)
Fixes a vacuous test assertion where checking for txid instead of wtxid allowed the test to pass even if the node improperly announced the transaction to block-relay-only peers. The underlying node relay behavior is not reported broken, but fixing the negative test assertion ensures future regressions in transaction privacy and relay filtering are caught.
Membership: Touches `test/functional/p2p_blocksonly.py` to fix the verification of transaction announcement suppression to block-relay-only peers.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 0