test: add coverage for immediate orphanage eviction case #31628

instagibbs commented at 3:36 PM on January 9, 2025: member

Not entirely persuaded this needs coverage, but was behavior I hadn't considered before.

DrahtBot commented at 3:36 PM on January 9, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31628.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux
Concept ACK	glozow

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31829 (p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs by glozow)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Jan 9, 2025

DrahtBot added the label CI failed on Jan 11, 2025

DrahtBot removed the label CI failed on Jan 13, 2025

rkrux commented at 12:03 PM on January 15, 2025: contributor

Seems like a very specific condition is being tested. Would it be possible to add this inside the test_max_orphan_amount test instead of creating a new one?

Here right after the orphanage length is asserted and right before the orphanage is cleared. https://github.com/bitcoin/bitcoin/pull/31628/files#diff-f7a7f89dc6ff73d829dbf856a922767096b11fe34ffcae1a482ca1c544611981R649

instagibbs commented at 1:59 PM on January 15, 2025: member

@rkrux If you can suggest alternative code that deterministically picks the right orphan to auto-evict, I would appreciate it.

rkrux commented at 2:07 PM on January 15, 2025: contributor

Oh yeah I recall that a randomly chosen orphan is evicted, I see the issue with adding this portion in the test_max_orphan_amount test now.

in test/functional/p2p_orphan_handling.py:598 in 6836d42819 outdated

 586 | @@ -587,6 +587,30 @@ def test_orphan_txid_inv(self):
 587 |          assert_equal(node.getmempoolentry(tx_child["txid"])["wtxid"], tx_child["wtxid"])
 588 |          assert_equal(len(node.getorphantxs()), 0)
 589 |  
 590 | +    @cleanup
 591 | +    def test_immediately_erased_orphan(self):
 592 | +        self.log.info("Check that we fetch parents of an orphan, even if the orphan is not persisted in orphanage")
 593 | +
 594 | +        # Causes immediate eviction from the orphanage which normally happens randomly with

rkrux commented at 2:38 PM on January 15, 2025:

normally happens randomly

Ahh sorry, I missed the randomly part.

rkrux approved

rkrux commented at 2:41 PM on January 15, 2025: contributor

tACK 6836d428199c0c19f7034bf6ea0855b8ec0a69d0

build, functional tests pass. In agreement with the approach.

DrahtBot added the label Needs rebase on Jan 16, 2025

instagibbs force-pushed on Jan 16, 2025

instagibbs commented at 8:55 PM on January 16, 2025: member

https://github.com/bitcoin/bitcoin/actions/runs/12815476373/job/35734215113

having a real test failure that I can't seem to replicate, investigating

edit: mocktime is being logged as happening before the orphan tx being received, need to sync.

2025-01-16T19:57:07.4089701Z [0;34m node0 2025-01-16T19:17:06.119184Z [httpworker.5] [rpc/request.cpp:241] [parse] [rpc] ThreadRPCServer method=setmocktime user=__cookie__ [0m
2025-01-16T19:57:07.4090684Z [0;34m node0 2025-01-16T19:17:06.140209Z (mocktime: 2025-01-16T19:18:00Z) [msghand] [txmempool.cpp:700] [check] [mempool] Checking mempool with 0 transactions and 0 inputs [0m
2025-01-16T19:57:07.4092466Z [0;34m node0 2025-01-16T19:17:06.140514Z (mocktime: 2025-01-16T19:18:00Z) [msghand] [net_processing.cpp:2974] [ProcessInvalidTx] [mempoolrej] e74691cbd1b181269301baed4266a2174cd4198fcf95591048c4ad17640f9f5b (wtxid=b1326a3ec4d996c0684c69414f627666690b7350fae829586f3b3f302a662a6f) from peer=0 was not accepted: bad-txns-inputs-missingorspent [0m
2025-01-16T19:57:07.4094799Z [0;34m node0 2025-01-16T19:17:06.140650Z (mocktime: 2025-01-16T19:18:00Z) [msghand] [txorphanage.cpp:47] [AddTx] [txpackages] stored orphan tx e74691cbd1b181269301baed4266a2174cd4198fcf95591048c4ad17640f9f5b (wtxid=b1326a3ec4d996c0684c69414f627666690b7350fae829586f3b3f302a662a6f), weight: 415 (mapsz 1 outsz 1) [0m
2025-01-16T19:57:07.4096652Z [0;34m node0 2025-01-16T19:17:06.140755Z (mocktime: 2025-01-16T19:18:00Z) [msghand] [node/txdownloadman_impl.cpp:414] [operator()] [txpackages] added peer=0 as a candidate for resolving orphan b1326a3ec4d996c0684c69414f627666690b7350fae829586f3b3f302a662a6f [0m
2025-01-16T19:57:07.4098462Z [0;34m node0 2025-01-16T19:17:06.140853Z (mocktime: 2025-01-16T19:18:00Z) [msghand] [txorphanage.cpp:93] [EraseTx] [txpackages]    removed orphan tx e74691cbd1b181269301baed4266a2174cd4198fcf95591048c4ad17640f9f5b (wtxid=b1326a3ec4d996c0684c69414f627666690b7350fae829586f3b3f302a662a6f) after 0s [0m
2025-01-16T19:57:07.4099909Z [0;34m node0 2025-01-16T19:17:06.140924Z (mocktime: 2025-01-16T19:18:00Z) [msghand] [txorphanage.cpp:152] [LimitOrphans] [txpackages] orphanage overflow, removed 1 tx [0m
2025-01-16T19:57:07.4100971Z [0;34m node0 2025-01-16T19:17:49.758568Z (mocktime: 2025-01-16T19:18:00Z) [scheduler] [net.cpp:2404] [StartExtraBlockRelayPeers] [net] enabling extra block-relay-only peers [0m
2025-01-16T19:57:07.4101985Z [0;34m node0 2025-01-16T19:32:04.757492Z (mocktime: 2025-01-16T19:18:00Z) [scheduler] [net.cpp:2367] [DumpAddresses] [net] Flushed 0 addresses to peers.dat  1ms [0m
2025-01-16T19:57:07.4102951Z [0;34m node0 2025-01-16T19:47:04.759071Z (mocktime: 2025-01-16T19:18:00Z) [scheduler] [net.cpp:2367] [DumpAddresses] [net] Flushed 0 addresses to peers.dat  1ms [0m

DrahtBot removed the label Needs rebase on Jan 17, 2025

instagibbs force-pushed on Jan 17, 2025

instagibbs commented at 2:51 PM on January 17, 2025: member

pushed what I think the fix is: make sure we sync the node with a ping/pong before bumping the mocktime.

test: add coverage for immediate orphanage eviction case cbf131c49d

instagibbs force-pushed on Jan 17, 2025

rkrux approved

rkrux commented at 2:04 PM on January 28, 2025: contributor

reACK cbf131c49df3fe029f00f7758f15f57363ca11f6

Main diff is the usage of send_and_ping over send_message.

fanquake commented at 4:38 PM on February 21, 2025: member

Not entirely persuaded this needs coverage, but was behavior I hadn't considered before.

Thoughts @glozow ?

in test/functional/p2p_orphan_handling.py:608 in cbf131c49d

 603 | +        tx_child = self.wallet.create_self_transfer(utxo_to_spend=tx_parent["new_utxo"])
 604 | +
 605 | +        peer = self.nodes[0].add_p2p_connection(P2PInterface())
 606 | +
 607 | +        peer.send_and_ping(msg_tx(tx_child["tx"]))
 608 | +        assert_equal(len(self.nodes[0].getorphantxs()), 0)

glozow commented at 7:24 PM on February 25, 2025:

Would probably move this to the very end, given what we know about getorphantxs taking a while to update

glozow commented at 7:26 PM on February 25, 2025: member

I don't think it hurts to have coverage for this. Gripes would be (1) this isn't necessarily the behavior we want, just the behavior we have (2) it uses -maxorphantxs=0 and imo we probably don't care to keep this config option / a limit on count long term? So ACK assuming we are relaxed about removing it later.

instagibbs commented at 7:04 PM on February 27, 2025: member

I'm not convinced this is the right approach in the end. As @glozow mentions this isn't testing desired behavior, and the behavior will likely dramatically change soon, requiring ripping out much of the test.

I think it's better to use fuzzers to get coverage of this and check the invariants like "don't crash".

instagibbs closed this on Feb 27, 2025

bitcoin locked this on Jul 30, 2026