test: make p2p_orphan_handling's never-requested checks able to fail #36406

fametrano commented at 8:32 PM on October 1, 2026: contributor

assert_never_requested compares its argument with the hashes in the node's getdata messages, which are ints. test_orphan_inherit_rejection passes it hex strings, so its four checks pass whatever the node requests.

This passes ints, which needs two more changes. The rejected parent is now relayed from peer2: relaying a transaction makes the node request it from that peer (the parent has no witness, so the request is for its txid), and from peer1 that request would fail the peer1 check. Mocktime is bumped before the peer1 and peer2 checks, because the node requests an orphan's missing parent only after a delay; without the bump those checks would pass before any request could be sent.

It also fixes a comment: the parent is rejected for its weight (tx-size), not its fee.

To see each check fail, change the scenario so that the node does request the hash. The commit message lists the changes I used.

Found while porting these tests to https://github.com/btclib-org/bitcoin-node-tests.

Made with my usual tools: a computer, the Internet and an LLM. The mistakes, as usual, are all mine.

DrahtBot added the label Tests on Oct 1, 2026

DrahtBot commented at 8:32 PM on October 1, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36406
Ralph review: https://git.fish.foo/bitcoin/bitcoin/pulls/36406#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	instagibbs

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

in test/functional/p2p_orphan_handling.py:436 in e275297a4a

 434 |          self.relay_transaction(peer1, child["tx"])
 435 |          assert_equal(0, len(node.getrawmempool()))
 436 |          assert not tx_in_orphanage(node, child["tx"])
 437 | -        peer1.assert_never_requested(parent_overly_large_nonsegwit["txid"])
 438 | +        self.nodes[0].bumpmocktime(TXREQUEST_TIME_SKIP)
 439 | +        peer1.assert_never_requested(int(parent_overly_large_nonsegwit["txid"], 16))

instagibbs commented at 1:36 PM on October 2, 2026:

nit: think this works here and elsewhere and is less verbose

        peer1.assert_never_requested(parent_overly_large_nonsegwit["txid"].txid_int)

in test/functional/p2p_orphan_handling.py:427 in e275297a4a outdated

 422 | @@ -423,29 +423,31 @@ def test_orphan_inherit_rejection(self):
 423 |          assert_not_equal(child["txid"], child["tx"].wtxid_hex)
 424 |          assert_not_equal(grandchild["txid"], grandchild["tx"].wtxid_hex)
 425 |  
 426 | -        # Relay the parent. It should be rejected because it pays 0 fees.
 427 | -        self.relay_transaction(peer1, parent_overly_large_nonsegwit["tx"])
 428 | +        # Relay the parent. It should be rejected because it exceeds the maximum standard weight.
 429 | +        self.relay_transaction(peer2, parent_overly_large_nonsegwit["tx"])

instagibbs commented at 1:36 PM on October 2, 2026:

since we're changing this to peer2, and it's non-obvious, very brief comment why?

          # Relay it from peer2: below, we check that the node does not ask peer1 for the parent after
          # peer1 relays the child. Relaying the parent from peer1 would make that check fail, since the
          # node requests the parent from the relaying peer by wtxid, which equals its txid.

instagibbs commented at 1:41 PM on October 2, 2026: member

concept ACK, good catches

Could we add this to assert_never_requested to remove the footgun and find misuse elsewhere going forward:

assert isinstance(txhash, int), f"txhash must be an int, got {type(txhash).__name__}"

test: make orphan inherit-rejection getdata checks able to fail

PeerTxRelayer.assert_never_requested compares the hash of every request
received, an int, with its argument, and test_orphan_inherit_rejection
passed it hex strings. An int never equals a str, so these checks passed
whatever the node requested. Pass ints instead.

Two more changes let the checks see the requests they are about:

- Relay the parent from peer2 rather than peer1. Relaying it made the
  node request it from peer1, so the check that peer1 never requested
  the parent fails once it compares ints.
- Bump mocktime by TXREQUEST_TIME_SKIP before the peer1 and peer2
  checks. The node requests an orphan's missing parent after a delay,
  so without the bump the request has not been sent when they run.

Also correct the comment on why the parent is rejected: it exceeds the
maximum standard weight, and the node rejects it with tx-size.

To check each assertion can fail, the scenario was mutated in a scratch
copy so that the node does request the hash, and each call site was
evaluated with both the old and the new argument. The old one passed
every time, and the new one failed:
- parent not relayed: peer1 is asked for the parent, peer2 for the
  child's txid;
- child not relayed: peer2 is asked for the child's txid;
- child relayed from peer2: peer2 is asked for the child's wtxid;
- neither child nor grandchild relayed: peer3 is asked for the child's
  txid.
Without the mocktime bumps, the first two mutations pass with ints too.

a3c4ad12e9

test: require an int in PeerTxRelayer.assert_never_requested

The requests it checks carry int hashes, so any other type never
matches and the check passes whatever the node requested.

5f22014aca

fametrano force-pushed on Oct 2, 2026

fametrano commented at 3:07 PM on October 2, 2026: contributor

Thanks, all three taken, and rebased on master. The int check is in its own commit; it trips nowhere else, because assert_never_requested is only defined and used in this test. x["txid"] is a hex string, so I used x["tx"].txid_int instead of x["txid"].txid_int.

instagibbs commented at 3:10 PM on October 2, 2026: member

ACK https://github.com/bitcoin/bitcoin/pull/36406/commits/5f22014aca2d071edfea95e8d3d4ba0bebbc57ca