test: tx orphan handling #28199
test: tx orphan handling #28199
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
bbd5459 to
29ec234
Compare
|
Concept ACK Good to have these tests prior to refactoring |
|
Concept ACK, looks like some great additional coverage. |
|
Concept ACK |
|
Concept ACK |
29ec234 to
abe8536
Compare
| # Node receives transaction. It attempts to obfuscate the exact timing at which this | ||
| # transaction entered its mempool. Send unsolicited because otherwise we need to wait for | ||
| # request delays. | ||
| peer_normal.send_and_ping(msg_tx(tx_parent_arrives["tx"])) |
There was a problem hiding this comment.
| peer_normal.send_and_ping(msg_tx(tx_parent_arrives["tx"])) | |
| peer_normal.send_and_ping(msg_tx(tx_parent_arrives["tx"])) | |
| assert tx_parent_arrives["txid"] in node.getrawmempool() |
my brain for some reason was thinking it was in the orphan pool as well
There was a problem hiding this comment.
Should we add an rpc to allow querying the orphanage?
There was a problem hiding this comment.
Should we add an rpc to allow querying the orphanage?
Could do that, as there's a bit of black box-ness here. Though I also think that the ideal situation would be to write unit tests if we're interested in the exact contents of the orphanage.
|
Concept ACK |
|
ACK abe8536 #28199 (comment) would be nice to be slightly cleaned up for future readers |
|
thanks for the concept acks @dergoegge @jamesob @brunoerg @Empact @jonatack, would appreciate a review of test too 🙏 |
There was a problem hiding this comment.
I think we should probably prefer to test behaviours that are intentional/desirable, rather than behaviours that just happen to be how it got implemented.
Yeah the purpose of writing these tests was largely documentation and not changing behavior unintentionally.
I think most of the things here are intentional, though can think of at least one thing that seems to be "that's just how it got implemented." I was trying to figure out whether we would want to use a notfound for a parent tx to mean we should give up on processing the orphan that depends on it. Currently we don't do anything since we don't track the fact that the request is related to an orphan. But since we just request based on !AlreadyHaveTx we might reject due to a long-confirmed parent. I suppose it would be different if validation told net_processing which inputs were missing and request those. Though that doesn't seem worth implementing if we can just do ancestor package relay.
abe8536 to
7a82b98
Compare
e6a9d75 to
d011a23
Compare
Have each TestNode keep track of the last timestamp it called setmocktime with, and add a bumpmocktime() function to bump by a number of seconds. Makes it easy to fast forward n seconds without keeping track of what the last timestamp was.
d011a23 to
9eac5a0
Compare
| self.nodes[0].bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) | ||
| peer.sync_with_ping() | ||
| assert_equal(len(peer.last_message["getdata"].inv), 2) | ||
| peer.wait_for_parent_requests([int(txid_conf_old, 16), int(missing_tx["txid"], 16)]) |
There was a problem hiding this comment.
Requesting the already confirmed transaction is surprising to me. Is this expected to change in future work (if so a comment mentioning that would be useful) or is it intended behavior?
There was a problem hiding this comment.
Requesting the already confirmed transaction is surprising to me.
Yep, was strange to me too, but AlreadyHaveTx is peerman's best metric for determining what the missing parents are (validation doesn't say which one(s) are missing). Also afaiu it's considered uncommon to (1) have an orphan (2) have a transaction that spends a very old coin, so I'm guessing this is quite rare.
Is this expected to change in future work
I'm not aware of any plans to change the legacy way of requesting orphan parents. But with package relay we'd ask for the list of unconfirmed ancestors, so we wouldn't have this kind of false positive.
| assert grandchild["txid"] != grandchild["tx"].getwtxid() | ||
|
|
||
| # Relay the parent. It should be rejected because it pays 0 fees. | ||
| self.relay_transaction(peer1, parent_low_fee_nonsegwit["tx"]) |
There was a problem hiding this comment.
Could check that parent_low_fee_nonsegwit is not in the mempool as done in the other test cases.
| self.num_nodes = 1 | ||
| self.extra_args = [[]] | ||
|
|
||
| def create_parent_and_child(self): |
There was a problem hiding this comment.
My bad!
I can also open a follow-up myself if someone feels like merging this as is.
If it helps, they were written to be used in #28031. I pulled these tests out to merge before the refactors and I guess forgot to remove the helpers that aren't used (yet).
| self._getdata_received = [] | ||
|
|
||
| @property | ||
| def tx_received(self): |
| peer.wait_for_parent_requests([int(missing_grandparent["txid"], 16)]) | ||
|
|
||
| # The node should put the orphan into the orphanage and request missing_parent, skipping | ||
| # missing_parent_orphan because it already has it in the orphanage. |
There was a problem hiding this comment.
Could add another assertion here that missing_parent_orphan is indeed only requested once.
|
|
||
| confirmed_utxos = [self.wallet_nonsegwit.get_utxo() for _ in range(4)] | ||
| assert all([utxo["confirmations"] > 0 for utxo in confirmed_utxos]) | ||
| self.log.info("Test handling of multiple orphans with missing parents that are already being requested") |
There was a problem hiding this comment.
nit: I would argue that the (or the first) log should always be the first line of the test. Otherwise it may be confusing if an error happens in the code above it may look like the previous test failed.
| self.nodes[0].bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) | ||
| # There are 3 missing parents. missing_parent_A and missing_parent_AB should be requested. | ||
| # But inflight_parent_AB should not, because there is already an in-flight request for it. | ||
| peer_orphans.wait_for_parent_requests([int(missing_parent_A["txid"], 16), int(missing_parent_AB["txid"], 16)]) |
There was a problem hiding this comment.
nit: I was a bit irritated that that inflight_parent_AB not being requested isn't checked here but I see it follows below. For expressiveness you could consider duplicating the check here or adding a small comment.
|
|
||
| self.log.info("Test that an orphan with rejected parents, along with any descendants, cannot be retried with an alternate witness") | ||
| parent_low_fee_nonsegwit = self.wallet_nonsegwit.create_self_transfer(fee_rate=0) | ||
| assert_equal(parent_low_fee_nonsegwit["txid"], parent_low_fee_nonsegwit["tx"].getwtxid()) |
There was a problem hiding this comment.
nit: This sanity check is repeated multiple times. I think it would be enough to do it once when setting up the MiniWallet since that seems to be the only way there could be an issue, if somehow MiniWallet is broken.
|
reACK 9eac5a0 |
9eac5a0 [functional test] transaction orphan handling (glozow) 61e77bb [test framework] make it easier to fast-forward setmocktime (glozow) Pull request description: I was doing some mutation testing (through reckless refactoring) locally and found some specific behaviors in orphan handling that weren't picked up by tests. Adding some of these test cases now can maybe help with reviewing refactors like bitcoin#28031. - Parent requests aren't sent immediately. A delay is added and the requests are filtered by AlreadyHaveTx before they are sent, which means you can't use fake orphans to probe precise arrival timing of a tx. - Parent requests include all that are not AlreadyHaveTx. This means old confirmed parents may be requested. - The node does not give up on orphans if the peer responds to a parent request with notfound. This means that if a parent is an old confirmed transaction (in which notfound is expected), the orphan should still be resolved. - Rejected parents can cause an orphan to be dropped, but it depends on the reason and only based on txid. - Rejected parents can cause an orphan to be rejected too, by both wtxid and txid. - Requests for orphan parents should be de-duplicated with "regular" txrequest. If a missing parent has the same hash as an in-flight request, it shouldn't be requested. - Multiple orphans with overlapping parents should not cause duplicated parent requests. ACKs for top commit: instagibbs: reACK bitcoin@9eac5a0 dergoegge: reACK 9eac5a0 achow101: ACK 9eac5a0 fjahr: Code review ACK 9eac5a0 Tree-SHA512: 85488dc6a3f62cf0c38e7dfe7839c01215b44b172d1755b18164d41d01038f3a749451241e4eba8b857fd344a445740b21d6382c45977234b21460e3f53b1b2a
Summary: ``` I was doing some mutation testing (through reckless refactoring) locally and found some specific behaviors in orphan handling that weren't picked up by tests. Adding some of these test cases now can maybe help with reviewing refactors like #28031. - Parent requests aren't sent immediately. A delay is added and the requests are filtered by AlreadyHaveTx before they are sent, which means you can't use fake orphans to probe precise arrival timing of a tx. - Parent requests include all that are not AlreadyHaveTx. This means old confirmed parents may be requested. - The node does not give up on orphans if the peer responds to a parent request with notfound. This means that if a parent is an old confirmed transaction (in which notfound is expected), the orphan should still be resolved. - Rejected parents can cause an orphan to be dropped, but it depends on the reason and only based on txid. - Rejected parents can cause an orphan to be rejected too, by both wtxid and txid. - Requests for orphan parents should be de-duplicated with "regular" txrequest. If a missing parent has the same hash as an in-flight request, it shouldn't be requested. - Multiple orphans with overlapping parents should not cause duplicated parent requests. ``` Backport of [[bitcoin/bitcoin#28199 | core#28199]]. The test_orphan_inherit_rejection case makes no sense without segwit so it has been removed, and well as some changes related to the absence of segwit and/or mempool ancestors accounting. Depends on D16505. Test Plan: ./test/functional/test_runner.py p2p_orphan_handling Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D16503
Summary: ``` I was doing some mutation testing (through reckless refactoring) locally and found some specific behaviors in orphan handling that weren't picked up by tests. Adding some of these test cases now can maybe help with reviewing refactors like #28031. - Parent requests aren't sent immediately. A delay is added and the requests are filtered by AlreadyHaveTx before they are sent, which means you can't use fake orphans to probe precise arrival timing of a tx. - Parent requests include all that are not AlreadyHaveTx. This means old confirmed parents may be requested. - The node does not give up on orphans if the peer responds to a parent request with notfound. This means that if a parent is an old confirmed transaction (in which notfound is expected), the orphan should still be resolved. - Rejected parents can cause an orphan to be dropped, but it depends on the reason and only based on txid. - Rejected parents can cause an orphan to be rejected too, by both wtxid and txid. - Requests for orphan parents should be de-duplicated with "regular" txrequest. If a missing parent has the same hash as an in-flight request, it shouldn't be requested. - Multiple orphans with overlapping parents should not cause duplicated parent requests. ``` Backport of [[bitcoin/bitcoin#28199 | core#28199]]. The test_orphan_inherit_rejection case makes no sense without segwit so it has been removed, and well as some changes related to the absence of segwit and/or mempool ancestors accounting. Depends on D16505. Test Plan: ./test/functional/test_runner.py p2p_orphan_handling Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D16503
I was doing some mutation testing (through reckless refactoring) locally and found some specific behaviors in orphan handling that weren't picked up by tests. Adding some of these test cases now can maybe help with reviewing refactors like #28031.