[net_processing] Improve/upgrade orphan transactions handling #2301
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
random-zebra
left a comment
random-zebra
left a comment
There was a problem hiding this comment.
Really nice PR (big thumb up for 94af4c9 and 0db5343).
Code review ACK, with only a minor concern over the maximum orphan size.
Would be good to cherry-pick another two simple commits from upstream (bitcoin#14626 and bitcoin/bitcoin@4c0731f from 19596), which are strictly related to these changes, but we can do it in a follow-up PR too.
Author
|
Updated per zebra's feedback, adding bitcoin#14626 and bitcoin/bitcoin@4c0731f . Plus the max orphan tx size change to support large shield transactions. |
This prevents higher order orphans and other junk from holding positions in the orphan map. Parents delayed twenty minutes are more are unlikely to ever arrive. The freed space will improve the orphan matching success rate for other transactions.
…ted. An orphan whos parents were rejected is never going to connect, so there is little utility in keeping it. Orphans also helpfully tell us what we're missing, so go ahead and treat it as INVed.
Although this increases node memory usage in the worst case by perhaps 30MB, the current behavior causes severe issues with dependent tx relay.
This is another violation of the one definition rule, as the type for mapOrphanTransactionsByPrev did not match the one in net_processing.cpp anymore. As it now depends on a custom Iterator, it seems too much hassle to correctly expose it to the tests. Instead, this commit just removes the one test it was referenced in.
furszy
force-pushed
the
2021_upgrade_orphan_tx_handling
branch
2 times, most recently
from
add8c14 to
97c6a03
Compare
Author
|
Rebased on master due conflicts. Ready to go. |
… connected. Eliminating a leak that causes the orphan map to always grow to its maximum size. Extra information: I have skipped several PRs from upstream, this was first inside ConnectBlock, then was moved to ActivateBestChain and finally ended up moving it to net_processing under the SyncTransaction slot that we don't have anymore because we are much more updated.
This should (marginally) speed up validationinterface queue draining by avoiding a cs_main lock in one client.
The previous code was biased towards evicting transactions whose txid has a larger gap (lexicographically) with the previous txid in the orphan pool.
In the logic for requesting missing parents of orphan transactions, parent transactions with multiple outputs being spent by the given orphan were being processed multiple times. Fix this by deduplicating the set of missing parent txids first.
Accepting them up to the max supported size.
furszy
force-pushed
the
2021_upgrade_orphan_tx_handling
branch
from
97c6a03 to
57221af
Compare
Fuzzbawls
approved these changes
Apr 16, 2021
| if (ptx->ContainsZerocoins()) { | ||
| // Don't even try to check zerocoins at all. | ||
| Misbehaving(pfrom->GetId(), 100); | ||
| LogPrint(BCLog::NET, " misbehaving peer, received a zc transaction, peer: %s\n", pfrom->GetAddrName()); |
Collaborator
There was a problem hiding this comment.
nit: un-necessary whitespace in log output
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Upgraded the orphan transactions management process, getting us much closer to upstream. Skipped several intermediate PRs because our code is way too far ahead of them in some areas like the validation interface and block processing after #2209. Plus added a net_processing zc cleanup.
Important changes:
cs_mainlock for accessingmapOrphanTransactionsandmapOrphanTransactionsByPrev.