Decouple peer-processing-logic from block-connection-logic #8865

TheBlueMatt · 2016-10-02T20:58:32Z

This is part of a series of about 20 commits which split main.cpp into two - peer processing logic and blockchain/mempool/UTXO logic. Most of them arent much more intrusive than these, at least until there are large blocks of code moving.

This set focuses primarily on using CValidationinterface to handle peer-processing-logic updates when connecting/disconnecting blocks.

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.

theuni · 2016-10-03T17:47:58Z

src/main.cpp

+void InitPeerLogic(CConnman& connman) {
+    if (mapPeerLogics.count(&connman))
+        return;
+    mapPeerLogics.emplace(std::make_pair(&connman, std::unique_ptr<PeerLogicValidation>(new PeerLogicValidation(&connman))));


nit: don't make_pair with emplace, just use the components directly.

theuni · 2016-10-03T17:49:36Z

src/main.cpp

+        }
+    }
+};
+std::map<CConnman*, std::unique_ptr<PeerLogicValidation> > mapPeerLogics;


Using a ptr for the index feels kinda icky. Since a reference is passed and stored anyway, let's add an Id to CConnman instead. It'll be helpful in other places too.

Hmm, I feel like that is a layer violation? If CConnman is a low-level network socket/connection manager, it should have little knowledge of the fact that there is something hooked up above it to listen to validation callbacks and do things with them?

Sorry, I meant: add a GetID() to CConnman, which would return a const value set during construction. That would allow maps to be created which refer to specific connman instances without tying them to a pointer.

I think we're overengineering? Are we ever really gonna have more than one CConnman? Maybe something without even a map is simpler?

I'm hoping to be able to hook up 2 CConnmans to eachother in the future for testing against eachother, yes. I've gone to significant efforts to remove the "stuff a static global somewhere" practice from much of the net code, please don't start reintroducing.

A pointer is fine here if you'd prefer to keep it simple, we can adapt later if needed.

theuni · 2016-10-04T00:26:45Z

src/main.cpp

+            }
+        }
+        if (it != mapBlockSource.end())
+            mapBlockSource.erase(it);


Hmm, were these not erased before in the invalid case?

theuni · 2016-10-04T00:29:29Z

Concept ACK and quick code-review ACK other than the nits. I'd like to spend some time testing, though.

TheBlueMatt · 2016-10-04T02:41:22Z

No? I think after this patch set the only place mapBlockSource is erased from is this line.

On October 3, 2016 8:26:53 PM EDT, Cory Fields notifications@github.com wrote:

theuni commented on this pull request.
   const uint256 hash(block.GetHash());
   std::map<uint256, NodeId>::iterator it =
mapBlockSource.find(hash);
+
   int nDoS = 0;
   if (state.IsInvalid(nDoS)) {
       if (it != mapBlockSource.end() && State(it->second)) {
           assert (state.GetRejectCode() < REJECT_INTERNAL); //
Blocks are never rejected with internal reject codes
           CBlockReject reject = {(unsigned
char)state.GetRejectCode(), state.GetRejectReason().substr(0,
MAX_REJECT_MESSAGE_LENGTH), hash};
           State(it->second)->rejects.push_back(reject);
           if (nDoS > 0)
               Misbehaving(it->second, nDoS);
       }
   }
   if (it != mapBlockSource.end())
       mapBlockSource.erase(it);
Hmm, were these not erased before in the invalid case?

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#8865 (review)

theuni · 2016-10-04T05:33:19Z

I was just pointing out that this looks to be a memleak bugfix :)

TheBlueMatt · 2016-10-04T13:18:23Z

I dont believe it is actually any (much?) different than it used to be. Maybe it will be deleted more often on reorgs now, I didnt investigate fully. In any case, its pretty hard to DoS since you only add to the map after AcceptBlock succeeds (ie the block is syntactically valid/connects/has valid PoW)

TheBlueMatt · 2016-10-04T13:46:39Z

Switched g_connman to be a shared_ptr instead of a unique_ptr, which I think should neatly address all the outstanding nits.

In anticipation of making all the callbacks out of block processing flow through it. Note that vHashes will always have something in it since pindexFork != pindexNewTip.

TheBlueMatt · 2016-10-04T16:50:17Z

Backed out the shared_ptr change, I misunderstood @theuni's comments, just fixed the make_pair nit and left everything else as-is: it should be easier to clean up things like class design once everything is split out.

This adds a new CValidationInterface subclass, defined in main.h, to receive notifications of UpdatedBlockTip and use that to push blocks to peers, instead of doing it directly from ActivateBestChain.

TheBlueMatt · 2016-10-04T17:56:42Z

Restructured the code after discussion on IRC with @theuni...now the CValidationInterface subclass is defined in main.h (requires an extra include, but we can drop that when we move all the stuff to another file) and init.cpp itself does the object initialization and keeps the CValidationInterface around.

laanwj · 2016-10-18T20:48:08Z

utACK a9aec5c

a9aec5c Use BlockChecked signal to send reject messages from mapBlockSource (Matt Corallo) 7565e03 Remove SyncWithWallets wrapper function (Matt Corallo) 12ee1fe Always call UpdatedBlockTip, even if blocks were only disconnected (Matt Corallo) f5efa28 Remove CConnman parameter from ProcessNewBlock/ActivateBestChain (Matt Corallo) fef1010 Use CValidationInterface from chain logic to notify peer logic (Matt Corallo) aefcb7b Move net-processing logic definitions together in main.h (Matt Corallo) 0278fb5 Remove duplicate nBlocksEstimate cmp (we already checked IsIBD()) (Matt Corallo) 87e7d72 Make validationinterface.UpdatedBlockTip more verbose (Matt Corallo)

theuni · 2016-10-18T20:58:19Z

src/main.cpp

-                    connman->ForEachNode([nNewHeight, nBlockEstimate, &vHashes](CNode* pnode) {
-                        if (nNewHeight > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : nBlockEstimate)) {
+                    connman->ForEachNode([nNewHeight, &vHashes](CNode* pnode) {
+                        if (nNewHeight > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : 0)) {


If pnode->nStartingHeight is -1, we just haven't completed the version handshake, no? Shouldn't we always refuse to relay in that case?

Or am I not seeing some other way this can happen?

Indeed, probably, though that would be a behavior change whereas this PR shouldnt have any :p.

theuni · 2016-10-18T21:31:06Z

src/validationinterface.h

@@ -50,6 +48,8 @@ class CValidationInterface {
 struct CMainSignals {
    /** Notifies listeners of updated block chain tip */
    boost::signals2::signal<void (const CBlockIndex *, const CBlockIndex *, bool fInitialDownload)> UpdatedBlockTip;
+    /** A posInBlock value for SyncTransaction which indicates the transaction was conflicted, disconnected, or not in a block */
+    static const int SYNC_TRANSACTION_NOT_IN_BLOCK = -1;


This is fine for now, but I think we ultimately want these split up.

Yea, I think medium-term we need to think hard about what goes in what signaling interface (we have two already that are highly duplicative, I have some thoughts but we should discuss)

theuni · 2016-10-18T21:37:09Z

post-merge utACK.

rebroad · 2016-10-19T03:23:17Z

src/main.cpp

+        }
+        // Relay inventory, but don't relay old inventory during initial block download.
+        connman->ForEachNode([nNewHeight, &vHashes](CNode* pnode) {
+            if (nNewHeight > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : 0)) {


The logic of this line is confusing now... If pnode->nStartingHeight = -1 then don't do (if nNewHeight > -1) but instead do (if nNewHeight > 0)?! It hardly makes any difference, it will return true, therefore simply do if (nNewHeight > pnode->nStartingHeight - 2000) without any need for the ? operator.

Anyway, why make it always true and not make use of the nBlocksEstimate as was previously done?

Fixed in #8958

See the earlier commit: "Remove duplicate nBlocksEstimate cmp (we already checked IsIBD())".

sipa · 2016-10-30T20:43:43Z

Posthummus utACK.

InitialBlockDownload upstream changes Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7208 - bitcoin/bitcoin#8007 - bitcoin/bitcoin#9053 - Excluding second commit (requires bitcoin/bitcoin#8865) - bitcoin/bitcoin#10388

…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

maflcko added the Refactoring label Oct 2, 2016

laanwj added the P2P label Oct 3, 2016

theuni reviewed Oct 3, 2016

View reviewed changes

theuni reviewed Oct 4, 2016

View reviewed changes

src/main.cpp

}

}

if (it != mapBlockSource.end())

mapBlockSource.erase(it);

Copy link

Member

theuni Oct 4, 2016

Choose a reason for hiding this comment

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

Hmm, were these not erased before in the invalid case?

TheBlueMatt added 2 commits October 4, 2016 12:35

Make validationinterface.UpdatedBlockTip more verbose

87e7d72

In anticipation of making all the callbacks out of block processing flow through it. Note that vHashes will always have something in it since pindexFork != pindexNewTip.

Remove duplicate nBlocksEstimate cmp (we already checked IsIBD())

0278fb5

TheBlueMatt force-pushed the net_processing_1 branch from 7366465 to 8cbd3c2 Compare October 4, 2016 16:46

TheBlueMatt added 6 commits October 4, 2016 13:36

Move net-processing logic definitions together in main.h

aefcb7b

Use CValidationInterface from chain logic to notify peer logic

fef1010

This adds a new CValidationInterface subclass, defined in main.h, to receive notifications of UpdatedBlockTip and use that to push blocks to peers, instead of doing it directly from ActivateBestChain.

Remove CConnman parameter from ProcessNewBlock/ActivateBestChain

f5efa28

Always call UpdatedBlockTip, even if blocks were only disconnected

12ee1fe

Remove SyncWithWallets wrapper function

7565e03

Use BlockChecked signal to send reject messages from mapBlockSource

a9aec5c

TheBlueMatt force-pushed the net_processing_1 branch from 8cbd3c2 to a9aec5c Compare October 4, 2016 17:55

TheBlueMatt mentioned this pull request Oct 15, 2016

Move orphan processing to ActivateBestChain #8930

Merged

laanwj merged commit a9aec5c into bitcoin:master Oct 18, 2016

theuni reviewed Oct 18, 2016

View reviewed changes

TheBlueMatt mentioned this pull request Oct 18, 2016

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

Merged

theuni reviewed Oct 18, 2016

View reviewed changes

gmaxwell mentioned this pull request Oct 19, 2016

Improve logic for advertising blocks #8958

Closed

rebroad reviewed Oct 19, 2016

View reviewed changes

sdaftuar mentioned this pull request Oct 31, 2016

Fix handling of invalid compact blocks #9026

Merged

This was referenced Jul 28, 2017

Backport Bitcoin PR#8865: Decouple peer-processing-logic from block-connection-logic dashpay/dash#1556

Merged

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

Merged

str4d mentioned this pull request May 15, 2018

InitialBlockDownload upstream changes zcash/zcash#3263

Merged

furszy mentioned this pull request Jul 3, 2020

Up to CTransactionRef point back ports to do list. PIVX-Project/PIVX#1726

Closed

21 tasks

random-zebra mentioned this pull request Aug 2, 2020

[Validation] Reduce cs_main locks during ConnectTip/SyncWithWallets PIVX-Project/PIVX#1721

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 #8865

Decouple peer-processing-logic from block-connection-logic #8865

Uh oh!

Conversation

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

Uh oh!

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 Oct 4, 2016

Uh oh!

TheBlueMatt commented Oct 4, 2016

Uh oh!

theuni commented Oct 4, 2016

Uh oh!

TheBlueMatt commented Oct 4, 2016

Uh oh!

TheBlueMatt commented Oct 4, 2016

Uh oh!

TheBlueMatt commented Oct 4, 2016

Uh oh!

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

Uh oh!

Uh oh!

laanwj commented Oct 18, 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!

theuni commented Oct 18, 2016

Uh oh!

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

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!

sipa commented Oct 30, 2016

Uh oh!

Uh oh!

TheBlueMatt commented Oct 2, 2016 •

edited

Loading

TheBlueMatt commented Oct 4, 2016 •

edited

Loading

rebroad Oct 19, 2016 •

edited

Loading