Move orphan processing to ActivateBestChain #8930

TheBlueMatt · 2016-10-15T21:45:46Z

Based on #8928 because its the only way I can run tests.
This will conflict a bit with #8865, but that's easy to resolve, and I'd rather get some eyeballs on this sooner rather than later, so am leaving it freestanding. Once #8865 goes in the orphan processing will go into the CValidationInterface which manges "net processing" stuff.

This further decouples "main" and "net" processing logic by moving
orphan processing out of the chain-connecting cs_main lock and
into its own cs_main lock, beside all of the other chain callbacks.

Once further decoupling of net and main processing logic occurs,
orphan handing should move to its own lock, out of cs_main.

Note that this will introduce a race if there are any cases where
we assume the orphan map to be consistent with the current chain
tip, however I am confident there is no such case (ATMP will fail
without DoS score in all such cases).

sipa · 2016-10-31T07:29:32Z

src/main.cpp

        // Notifications/callbacks that can run without cs_main
        if(connman)
            connman->SetBestHeight(nNewHeight);

+


sipa · 2016-10-31T07:30:12Z

src/main.cpp

@@ -3068,10 +3048,38 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
        }
        // When we reach this point, we switched to a new tip (stored in pindexNewTip).

+        // Remove orphan transactions with cs_main
+        {
+            LOCK(cs_main);


I hope we can separate nodestate and other message-processing related variables from cs_main...

@sipa Absolutely, I'm planning on doing that after the dust settles here, unless @TheBlueMatt already has it queued up. I'm planning on moving lots of what's currently in CNode to CNodeState, so breaking out the locks is a prerequisite for that.

Dont have it queued up, but, indeed, thats an obvious change after we split main.cpp.

TheBlueMatt · 2016-11-03T19:22:16Z

Closing for now. Will rebase on #9075 when its merged.

This further decouples "main" and "net" processing logic by moving orphan processing out of the chain-connecting cs_main lock and into its own cs_main lock, beside all of the other chain callbacks. Once further decoupling of net and main processing logic occurs, orphan handing should move to its own lock, out of cs_main. Note that this will introduce a race if there are any cases where we assume the orphan map to be consistent with the current chain tip, however I am confident there is no such case (ATMP will fail without DoS score in all such cases).

TheBlueMatt · 2016-11-18T03:29:14Z

Rebased on #9075...this now forms the last pull before main.cpp is split clean in two :).

instagibbs · 2016-11-18T15:08:51Z

utACK 2dd652a

rebroad · 2016-11-21T09:33:50Z

What is the rationale behind this refactoring please? What future changes is it enabling? Or what is it fixing?

laanwj · 2016-11-21T12:45:40Z

@rebroad

This further decouples "main" and "net" processing logic by moving
orphan processing out of the chain-connecting cs_main lock and
into its own cs_main lock, beside all of the other chain callbacks.

Decoupling unrelated logic to be able to split up main into validation and network handling parts.

sdaftuar · 2016-11-22T14:26:12Z

@TheBlueMatt One thing we don't currently do, but probably should, is try to reaccept orphan transactions to the mempool if their missing inputs appear in a block. After this PR, would you suggest we just do that inside PeerLogicValidation::SyncTransaction(), or would that be problematic?

sipa · 2016-11-22T22:53:37Z

utACK. You could get rid of the BOOST_FOREACH along the way, if you wish.

TheBlueMatt · 2016-11-23T00:28:00Z

@sdaftuar that seems reasonable, yea. Not for this PR, though, indeed.

sdaftuar · 2016-11-23T00:41:26Z

utACK

theuni · 2016-11-23T19:35:24Z

src/main.cpp

@@ -4744,6 +4724,30 @@ PeerLogicValidation::PeerLogicValidation(CConnman* connmanIn) : connman(connmanI
    recentRejects.reset(new CRollingBloomFilter(120000, 0.000001));
 }

+void PeerLogicValidation::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int nPosInBlock) {
+    LOCK(cs_main);


Shouldn't this check against CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK?

Hmmmmm...good point...for corectness I'll add that check to this PR, but I think we probably actually want to remove it in a future one since I believe it would allow us to clean up orphan cleanup logic in the ::TX message handling logic.

Yep, makes sense. But for now, it can at least be a short-circuit for returning without locking cs_main (not sure if there's any unlocked caller who would benefit, though).

@TheBlueMatt Your comment confuses me a bit. Wouldn't this be broken without that check?

@morcos it was a reference to the fact that we might want to remove orphan transactions which conflict with txn which were added to mempool, instead of doing that work in the ::TX message handling. I believe without the check they would step all over each other, but this could be fixed in the future.

I agree, nice to move all logic to this function.
But for now, we don't actually remove orphan txs which conflict with things accepted to mempool. We could, however, move recursive processing of orphan txs that depended on mempool accepted txs here, I agree.
Also there are other places SyncTransaction is called, such as for disconnected block txs, where it would be incorrect to remove orphan txs which conflict with txs from disconnected blocks.
In any case, the code is correct now, I just want to be sure we think about all the ways SyncTransaction is called when we change it in the future.

This makes the orphan map a part of net-processing logic instead of main logic.

theuni · 2016-11-23T22:50:08Z

utACK d2b88f9

d2b88f9 Move orphan-conflict removal from main logic into a callback (Matt Corallo) 97e2802 Erase orphans per-transaction instead of per-block (Matt Corallo) ec4525c Move orphan processing to ActivateBestChain (Matt Corallo)

rebroad · 2016-12-22T15:13:31Z

src/main.cpp

+        for (auto mi = itByPrev->second.begin(); mi != itByPrev->second.end(); ++mi) {
+            const CTransaction& orphanTx = (*mi)->second.tx;
+            const uint256& orphanHash = orphanTx.GetHash();
+            vOrphanErase.push_back(orphanHash);


Why create vOrphanErase at all? Why not simply run the EraseOrphanTx() here instead?

d2b88f9 Move orphan-conflict removal from main logic into a callback (Matt Corallo) 97e2802 Erase orphans per-transaction instead of per-block (Matt Corallo) ec4525c Move orphan processing to ActivateBestChain (Matt Corallo)

TheBlueMatt force-pushed the net_processing_3 branch 2 times, most recently from 9860cda to b4c3c3b Compare October 16, 2016 14:15

maflcko added the Refactoring label Oct 16, 2016

sipa reviewed Oct 31, 2016

View reviewed changes

src/main.cpp

// Notifications/callbacks that can run without cs_main

if(connman)

connman->SetBestHeight(nNewHeight);

Copy link

Member

sipa Oct 31, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useful.

sipa reviewed Oct 31, 2016

View reviewed changes

TheBlueMatt mentioned this pull request Nov 3, 2016

Decouple peer-processing-logic from block-connection-logic (#3) #9075

Merged

TheBlueMatt closed this Nov 3, 2016

TheBlueMatt added 2 commits November 17, 2016 17:10

Erase orphans per-transaction instead of per-block

97e2802

TheBlueMatt reopened this Nov 18, 2016

TheBlueMatt force-pushed the net_processing_3 branch from b4c3c3b to 2dd652a Compare November 18, 2016 03:28

TheBlueMatt mentioned this pull request Nov 18, 2016

Final Preparation for main.cpp Split #9183

Merged

theuni reviewed Nov 23, 2016

View reviewed changes

TheBlueMatt force-pushed the net_processing_3 branch from 2dd652a to f733b49 Compare November 23, 2016 20:02

Move orphan-conflict removal from main logic into a callback

d2b88f9

This makes the orphan map a part of net-processing logic instead of main logic.

TheBlueMatt force-pushed the net_processing_3 branch from f733b49 to d2b88f9 Compare November 23, 2016 20:29

sipa merged commit d2b88f9 into bitcoin:master Nov 24, 2016

rebroad mentioned this pull request Nov 29, 2016

Numerous "Erased 1 orphan tx included or conflicted by block" messages #9237

Closed

rebroad reviewed Dec 22, 2016

View reviewed changes

This was referenced Aug 1, 2017

Backport Bitcoin PR#9075: Decouple peer-processing-logic from block-connection-logic (#3) dashpay/dash#1560

Merged

Backport Bitcoin PR#9183: Final Preparation for main.cpp Split dashpay/dash#1561

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Move orphan processing to ActivateBestChain #8930

Move orphan processing to ActivateBestChain #8930

Uh oh!

Conversation

TheBlueMatt commented Oct 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Nov 3, 2016

Uh oh!

TheBlueMatt commented Nov 18, 2016

Uh oh!

instagibbs commented Nov 18, 2016

Uh oh!

rebroad commented Nov 21, 2016

Uh oh!

laanwj commented Nov 21, 2016

Uh oh!

sdaftuar commented Nov 22, 2016

Uh oh!

sipa commented Nov 22, 2016

Uh oh!

TheBlueMatt commented Nov 23, 2016

Uh oh!

sdaftuar commented Nov 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Nov 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!