test: add coverage for immediate orphanage eviction case #31628
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31628. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
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
|
@rkrux If you can suggest alternative code that deterministically picks the right orphan to auto-evict, I would appreciate it. |
|
Oh yeah I recall that a randomly chosen orphan is evicted, I see the issue with adding this portion in the |
rkrux
left a comment
There was a problem hiding this comment.
tACK 6836d428199c0c19f7034bf6ea0855b8ec0a69d0
build, functional tests pass. In agreement with the approach.
6836d42 to
86b7d11
Compare
|
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. |
396c418 to
691752c
Compare
|
pushed what I think the fix is: make sure we sync the node with a ping/pong before bumping the mocktime. |
691752c to
cbf131c
Compare
Thoughts @glozow ? |
glozow
left a comment
There was a problem hiding this comment.
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.
| peer = self.nodes[0].add_p2p_connection(P2PInterface()) | ||
|
|
||
| peer.send_and_ping(msg_tx(tx_child["tx"])) | ||
| assert_equal(len(self.nodes[0].getorphantxs()), 0) |
There was a problem hiding this comment.
Would probably move this to the very end, given what we know about getorphantxs taking a while to update
|
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". |
Not entirely persuaded this needs coverage, but was behavior I hadn't considered before.