Decouple peer-processing-logic from block-connection-logic (#2) #8969

TheBlueMatt · 2016-10-18T20:59:59Z

This is the second part of a series of about 20 commits which split main.cpp into two - peer processing logic and blockchain/mempool/UTXO logic, after #8865.

This set focuses on random bits of interconnection left over after #8865 (the largest diff is actually #8968, which this is based on just to avoid needless confliction).

I haven't significantly tested this as my normal test machine is largely unavailable atm, but most of the changes here are pretty straight-forward.

CodeShark · 2016-10-20T20:19:07Z

Concept ACK

TheBlueMatt · 2016-10-27T15:33:56Z

Removed the " Remove pfrom parameter from ProcessNewBlock" commit, as it was subtely broken, and fixing it requires a bit more work which would complicate this PR.

theuni · 2016-10-27T17:13:39Z

src/main.cpp

@@ -5847,8 +5850,8 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,

            PartiallyDownloadedBlock& partialBlock = *it->second.second->partialBlock;
            ReadStatus status = partialBlock.FillBlock(block, resp.txn);
+            MarkBlockAsReceived(resp.blockhash); // it is now an empty pointer


We want it marked even in the READ_STATUS_FAILED case?

theuni · 2016-10-27T17:58:49Z

src/main.cpp

@@ -4281,18 +4284,12 @@ void UnloadBlockIndex()
    mempool.clear();
    mapOrphanTransactions.clear();
    mapOrphanTransactionsByPrev.clear();
-    nSyncStarted = 0;


Am I correct in assuming that everything removed here will eventually be making its way into PeerLogicValidation?

There should be no need (see elaborated commit message).

theuni · 2016-10-27T18:01:47Z

src/main.cpp

@@ -1581,6 +1591,8 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
        BOOST_FOREACH(const uint256& hashTx, vHashTxToUncache)
            pcoinsTip->Uncache(hashTx);
    }
+    // After we've (potentially) uncached entries, ensure our coins cache is still within its size limits
+    FlushStateToDisk(state, FLUSH_STATE_PERIODIC);


I don't know the mempool behavior well enough to review this... is it ok to be potentially flushing while processing orphans recursively?

FlushStateToDisk shouldn't effect anything that mempool (or anything, actually) cares about except for memory usage, I believe.

FlushStateToDisk should be fine to be called as long as no child CCoinsViewCache exists (which isn't the case here).

kazcw

utACK

kazcw · 2016-10-28T21:23:22Z

src/main.cpp

@@ -1581,6 +1591,8 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
        BOOST_FOREACH(const uint256& hashTx, vHashTxToUncache)
            pcoinsTip->Uncache(hashTx);
    }
+    // After we've (potentially) uncached entries, ensure our coins cache is still within its size limits
+    FlushStateToDisk(state, FLUSH_STATE_PERIODIC);


Minor edge case: a non-fatal error in FlushState (currently only low disk space) would override a MODE_INVALID, preventing misbehavior accounting and reject messages for the current tx. Since MODE_ERROR from FlushState is ignored in this case anyway, I think it'd be more robust to give FlushState a dummy CValidationState.

Heh, true, though in all of those cases we AbortNode, sooooo

sipa · 2016-10-30T22:20:16Z

utACK

UnloadBlockIndex is only used during init if we end up reindexing to clear our block state so that we can start over. However, at that time no connections have been brought up as CConnman hasn't been started yet, so all of the network processing state logic is empty when its called. Additionally, the initialization of the recentRejects set is moved to InitPeerLogic.

This will result in many more calls to CheckBlockIndex when connecting a list of headers (eg in ::HEADERS messages processing) but its only enabled in debug mode, and that should mostly just be during IBD, so it should be OK.

TheBlueMatt · 2016-10-31T14:10:34Z

Squashed (without rebase - there are currently no merge conflicts so no need).
One tiny change from 479eeccda37f0d53904b32f55decdb65742d3136 to f5b960b which just to reduce total diff size (see https://0bin.net/paste/2JB1+Ayq2N8VKw7P#TmdS1p5JcsuvvWKClIkcSOoSNQDMGJBB9HdzG+Yg7iz).

theuni · 2016-10-31T23:05:24Z

utACK f5b960b, with the same caveat as before: I'm not confident enough with FlushStateToDisk to ACK, but no reason to believe the changes are harmful.

laanwj · 2016-11-03T15:31:20Z

utACK f5b960b

…ic (#2) f5b960b Move nTimeBestReceived updating into net processing code (Matt Corallo) d8670fb Move all calls to CheckBlockIndex out of net-processing logic (Matt Corallo) d6ea737 Remove network state wipe from UnloadBlockIndex. (Matt Corallo) fc0c24f Move MarkBlockAsReceived out of ProcessNewMessage (Matt Corallo) 65f35eb Move FlushStateToDisk call out of ProcessMessages::TX into ATMP (Matt Corallo)

…n logic 94cc49f Move nTimeBestReceived updating into net processing code (Matt Corallo) d2029cd Move all calls to CheckBlockIndex out of net-processing logic (random-zebra) 44b2f68 Remove network state wipe from UnloadBlockIndex. (Matt Corallo) 0b9b4e2 Move MarkBlockAsReceived out of ProcessNewBlock (random-zebra) b093584 Move FlushStateToDisk call out of ProcessMessages::TX into ATMP (Matt Corallo) 5c4e7c6 Use BlockChecked signal to send reject messages from mapBlockSource (Matt Corallo) 14af52a Always call UpdatedBlockTip, even if blocks were only disconnected (Matt Corallo) 3982e0e Remove CConnman parameter from ProcessNewBlock/ActivateBestChain (Matt Corallo) 48ee837 [Refactoring] Set connman best height in UpdatedBlockTip (random-zebra) 3137b55 Use CValidationInterface from chain logic to notify peer logic (random-zebra) 69ef798 Move net-processing logic definitions together in main.h (Matt Corallo) cc51897 Remove duplicate nBlocksEstimate cmp (we already checked IsIBD()) (Matt Corallo) 625acdd [Cleanup] Remove unused signals/functions in validation interface (random-zebra) ab94b70 Make validationinterface.UpdatedBlockTip more verbose (Matt Corallo) Pull request description: Backports bitcoin#8865 and bitcoin#8969, in preparation for the long-awaited net_processing/validation split. ACKs for top commit: Fuzzbawls: ACK 94cc49f furszy: Looking good, ACK 94cc49f and merging.. Tree-SHA512: 5e17f9f018d1d1898bce77baefa0b614aaa59b883fbca3f851603208ed93d3a66141cd54f6faaa0bfbef24a6dbefcb16a55dc260f27d2c5ecd4c3f8297fd4e76

fanquake added Refactoring P2P labels Oct 19, 2016

TheBlueMatt force-pushed the net_processing_2 branch 2 times, most recently from 35a1ba4 to 4667608 Compare October 27, 2016 15:33

theuni reviewed Oct 27, 2016

View reviewed changes

TheBlueMatt mentioned this pull request Oct 28, 2016

Introduce CBlockchain and move CheckBlockHeader #8087

Closed

kazcw approved these changes Oct 28, 2016

View reviewed changes

TheBlueMatt added 5 commits October 31, 2016 10:08

Move FlushStateToDisk call out of ProcessMessages::TX into ATMP

65f35eb

Move MarkBlockAsReceived out of ProcessNewMessage

fc0c24f

Move nTimeBestReceived updating into net processing code

f5b960b

TheBlueMatt force-pushed the net_processing_2 branch from 479eecc to f5b960b Compare October 31, 2016 14:10

TheBlueMatt mentioned this pull request Nov 3, 2016

Fix handling of invalid compact blocks #9026

Merged

laanwj merged commit f5b960b into bitcoin:master Nov 3, 2016

OlegGirko mentioned this pull request Jul 30, 2017

Backport Bitcoin PR#8969: Decouple peer-processing-logic from block-connection-logic (#2) dashpay/dash#1558

Merged

random-zebra mentioned this pull request Sep 24, 2020

[Refactor] Decouple peer-processing from block-connection logic PIVX-Project/PIVX#1866

Merged

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

Decouple peer-processing-logic from block-connection-logic (#2) #8969

Decouple peer-processing-logic from block-connection-logic (#2) #8969

Uh oh!

Conversation

TheBlueMatt commented Oct 18, 2016

Uh oh!

CodeShark commented Oct 20, 2016

Uh oh!

TheBlueMatt commented Oct 27, 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!

kazcw left a comment

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!

sipa commented Oct 30, 2016

Uh oh!

TheBlueMatt commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Oct 31, 2016

Uh oh!

laanwj commented Nov 3, 2016

Uh oh!

Uh oh!

TheBlueMatt commented Oct 31, 2016 •

edited

Loading