Also handle/resolve orphan TXs when parents appear in a block #3139
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
codablock
changed the title
|
utACK to merge after rebase |
UdjinM6
reviewed
Oct 4, 2019
This makes orphan processing work like handling getdata messages: After every actual transaction validation attempt, interrupt processing to deal with messages arriving from other peers.
codablock
force-pushed
the
pr_orphanhandling
branch
from
1feb863 to
9a06702
Compare
Author
|
rebased on develop |
src/net_processing.cpp
Outdated
Member
There was a problem hiding this comment.
can you squash this commit in af7124e9e7cfe6c825ccb121b7292852b6400880?
codablock
force-pushed
the
pr_orphanhandling
branch
from
9a06702 to
a12b03e
Compare
PastaPastaPasta
approved these changes
Oct 7, 2019
Member
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
I hate changing tests because I'm sure it will cause conflicts down the road backporting, always prefer Dash specific tests. Otherwise, utACK
Author
Author
|
@codablock Ah, ok 👍 |
PastaPastaPasta
added a commit
that referenced
this pull request
Aug 4, 2025
, bitcoin#25683, bitcoin#23679, bitcoin#26216, bitcoin#26295, bitcoin#26513, bitcoin#26515, bitcoin#26727, bitcoin#25374, bitcoin#25272, bitcoin#26408, bitcoin#25112, bitcoin#26702, bitcoin#25932, bitcoin#26822 (auxiliary backports: part 27) f45de22 merge bitcoin#26822: don't allow past absolute timestamp in `setban` (Kittywhiskers Van Gogh) 920e85d test: adjust mocktime bump to match upstream in `p2p_disconnect_ban` (Kittywhiskers Van Gogh) b1a95d8 merge bitcoin#25932: Simplify backtrack logic (Kittywhiskers Van Gogh) eff27a4 merge bitcoin#26702: drop unused `FindWalletTx` parameter and rename (Kittywhiskers Van Gogh) 62998f3 merge bitcoin#25112: Move error message formatting of NonFatalCheckError to cpp (Kittywhiskers Van Gogh) 27db044 merge bitcoin#26408: Remove spam from debug log (Kittywhiskers Van Gogh) 12acde7 merge bitcoin#25272: guard and alert about a wallet invalid state during chain sync (Kittywhiskers Van Gogh) 385306d merge bitcoin#25374: remove unused `create_confirmed_utxos` helper (Kittywhiskers Van Gogh) a8d29f9 merge bitcoin#26727: remove optional from fStateStats fields (Kittywhiskers Van Gogh) a0701e9 merge bitcoin#26515: skip getpeerinfo for a peer without CNodeStateStats (Kittywhiskers Van Gogh) c9eedc8 merge bitcoin#26513: Make static nLastFlush and nLastWrite Chainstate members (Kittywhiskers Van Gogh) 859d64e txorphanage: adapt block-based orphan reprocessing to refactor (Kittywhiskers Van Gogh) 4172102 merge bitcoin#26295: Replace global g_cs_orphans lock with local (Kittywhiskers Van Gogh) 3665919 fix: protect `m_orphan_tx_size` with `g_cs_orphans` (Kittywhiskers Van Gogh) f12e6cc fix: scan outputs in `GetCandidatesForBlock`, add regression test (Kittywhiskers Van Gogh) 42aa8a7 merge bitcoin#26216: Limit outpoints.size in txorphan target to avoid OOM (Kittywhiskers Van Gogh) b376c09 merge bitcoin#23679: Sanitize port in `addpeeraddress()` (Kittywhiskers Van Gogh) cd6c2b5 merge bitcoin#25683: log `nEvicted` message in LimitOrphans then return void (Kittywhiskers Van Gogh) 6cd09ef merge bitcoin#25641: Fix `-Wparentheses` gcc warning (Kittywhiskers Van Gogh) bc69f80 merge bitcoin#25624: Fix assert bug in txorphan target (Kittywhiskers Van Gogh) fc68188 merge bitcoin#25447: add low-level target for txorphanage (Kittywhiskers Van Gogh) Pull request description: ## Additional information * Since [dash#3121](#3121), Dash has limited the orphan transaction map by the accumulated size of all transactions in the map instead of transaction count (like upstream). [bitcoin#25447](bitcoin#25447) introduces a new fuzzing harness that operates on upstream assumptions. To bridge the gap, `DEFAULT_MAX_ORPHAN_TRANSACTIONS` has been defined in the harness to mean the highest transaction count _assuming_ each transaction is of maximum size, which means the effective worst-case count is 100 transactions. This is already documented in `net_processing.h` ([source](https://github.com/dashpay/dash/blob/bd589dd6f434c0c6cc8f0c3e2cf299785bad6d07/src/net_processing.h#L31-L32)) but has also been explicitly documented for the harness ([source](https://github.com/dashpay/dash/blob/fc68188e3227dc0912ef3c508f6d45ff1d7614ee/src/test/fuzz/txorphan.cpp#L27-L28)). * Dash introduced an additional trigger for orphan reprocessing in [dash#3139](#3139) to allow for faster processing when parents appear in a block. Currently, the implementation scans through the _inputs_ of each transaction to check against the outpoint map ([source](https://github.com/dashpay/dash/blob/86071f9df4d052de6f1df64a5d269a67a6756d29/src/txorphanage.cpp#L186-L192)). This does not seem to be intended behavior as it's using the input index to search through outpoints when it should be iterating through outpoints like `AddChildrenToWorkSet()` ([source](https://github.com/dashpay/dash/blob/86071f9df4d052de6f1df64a5d269a67a6756d29/src/txorphanage.cpp#L148C19-L159)). Though despite this, the code still worked _if_ either of the following conditions were met. * There are an even number of inputs and outputs (in which case a match is found as if iterating through the set of outputs); or * There are more inputs than outputs (in which case a match is found even if all `find()`s `i > vout.size()` are wasted operations) The test case introduced in [dash#3139](#3139) met this criteria and passed though the code would _not_ work as expected if * There are more outputs than inputs (in which case the loop will bail prematurely as `vin.size() > vout.size()` and we're scanning `vins` for outpoint positions) This has been rectified by modifying `GetCandidatesForBlock()` to use `AddChildrenToWorkSet()` and adding a unit test to document this behavior. * [bitcoin#26295](bitcoin#26295) does a substantial refactor of the transaction "orphanage" and part of that refactor is solidifying `ProcessOrphanTx()` as dealing with `Peer`s (as it's located in `net_processing` and not available through the interface). This conflicts with the capability introduced in [dash#3139](#3139) as there is no peer to attribute it to, which is relevant as the newer code introduces a map between peer ID and the orphan work map ([source](https://github.com/bitcoin/bitcoin/blob/7082ce3e88d77456d60a5a66bd38807fbd66f311/src/txorphanage.h#L75-L76)). This has been worked around by using the ID `-1` to mean block connection triggered orphan work ([source](https://github.com/dashpay/dash/blob/859d64e98fc1b5bf613f526ef76e5fc66851fe13/src/txorphanage.cpp#L204-L209), [source](https://github.com/dashpay/dash/blob/859d64e98fc1b5bf613f526ef76e5fc66851fe13/src/net_processing.cpp#L2015-L2024)). This is because no valid peer can be assigned a negative ID as it starts from 0 ([source](https://github.com/dashpay/dash/blob/bd589dd6f434c0c6cc8f0c3e2cf299785bad6d07/src/net.h#L1735)) and increments ([source](https://github.com/dashpay/dash/blob/bd589dd6f434c0c6cc8f0c3e2cf299785bad6d07/src/net.cpp#L3897-L3900)). An example of this kind of usage is InstantSend code ([source](https://github.com/dashpay/dash/blob/859d64e98fc1b5bf613f526ef76e5fc66851fe13/src/instantsend/signing.cpp#L252)). * This change, alongside relevant changes to `ProcessOrphanTx()` arguments and other Dash-specific code has been segmented out as a distinct commit to allow for ease of review. This also means that the commit that backports [bitcoin#26295](bitcoin#26295) cannot be compiled on its own due to build errors from not-yet adapted code. * Additionally, `ProcessOrphanTx()` assumes `Peer`-triggered orphan resolution and doesn't account for the possibility that the processing of a transaction may further populate the orphan set. This is a key assumption in block-based orphan processing and to retain this intended functionality, `TxOrphanage::HaveMoreWork()` has been defined to influence the return value of `ProcessOrphanTx()` to affect the resolution loop ([source](https://github.com/dashpay/dash/blob/859d64e98fc1b5bf613f526ef76e5fc66851fe13/src/net_processing.cpp#L2015-L2024)) to exhaust all work before continuing ahead. ## Breaking changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK f45de22 UdjinM6: utACK f45de22 Tree-SHA512: 8f6a46984091d6f43d4fb97d13edc9dfa7affa1eb79ab37e14f49078b5d738308c6044f9b2b3f5e0d1fe2efde27b7bae14727e14e1ccd5d21eb695162d9c4615
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
This should fix many of the mempool inconsistencies seen on mainnet when high load appears with many chained transactions.
This also backports bitcoin#15644 as we need the orphan handling to be callable from outside of ProcessMessage.
The PR is currently built on top of #3127 as it adds more testing to orphan TX handling.