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

TheBlueMatt commented at 8:59 pm on October 18, 2016: member

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.

fanquake added the label Refactoring on Oct 19, 2016

fanquake added the label P2P on Oct 19, 2016

CodeShark commented at 8:19 pm on October 20, 2016: contributor

Concept ACK

TheBlueMatt force-pushed on Oct 21, 2016

TheBlueMatt force-pushed on Oct 27, 2016

TheBlueMatt commented at 3:33 pm on October 27, 2016: member

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.

in src/main.cpp: in 66a0a96f40 outdated

5849@@ -5847,8 +5850,8 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5850 
5851             PartiallyDownloadedBlock& partialBlock = *it->second.second->partialBlock;
5852             ReadStatus status = partialBlock.FillBlock(block, resp.txn);
5853+            MarkBlockAsReceived(resp.blockhash); // it is now an empty pointer

theuni commented at 5:13 pm on October 27, 2016:

We want it marked even in the READ_STATUS_FAILED case?

TheBlueMatt commented at 7:11 pm on October 27, 2016:

Fixed.

in src/main.cpp: in 43cb80ba7d outdated

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

theuni commented at 5:58 pm on October 27, 2016:

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

TheBlueMatt commented at 6:38 pm on October 27, 2016:

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

in src/main.cpp: in ca4b15bc23 outdated

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

theuni commented at 6:01 pm on October 27, 2016:

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

TheBlueMatt commented at 6:40 pm on October 27, 2016:

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

kazcw commented at 9:23 pm on October 28, 2016:

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.

TheBlueMatt commented at 4:45 pm on October 29, 2016:

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

sipa commented at 9:58 pm on October 30, 2016:

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

kazcw approved

kazcw commented at 10:01 pm on October 28, 2016: contributor

utACK

sipa commented at 10:20 pm on October 30, 2016: member

utACK

Move FlushStateToDisk call out of ProcessMessages::TX into ATMP 65f35eb91b

Move MarkBlockAsReceived out of ProcessNewMessage fc0c24f67b

Remove network state wipe from UnloadBlockIndex.

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.

d6ea737be1

Move all calls to CheckBlockIndex out of net-processing logic

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.

d8670fb103

Move nTimeBestReceived updating into net processing code f5b960be4e

TheBlueMatt commented at 2:10 pm on October 31, 2016: member

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

TheBlueMatt force-pushed on Oct 31, 2016

theuni commented at 11:05 pm on October 31, 2016: member

utACK f5b960be4e9a9ab669e1436194fa904ccba58900, 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 commented at 3:31 pm on November 3, 2016: member

utACK f5b960b

laanwj merged this on Nov 3, 2016

laanwj closed this on Nov 3, 2016

laanwj referenced this in commit 3665483be7 on Nov 3, 2016

furszy referenced this in commit 585c9fed6d on Sep 26, 2020

MarcoFalke locked this on Sep 8, 2021

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