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

1. 
   TheBlueMatt commented at 8:58 pm on October 2, 2016: member

   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.

2. MarcoFalke added the label Refactoring on Oct 2, 2016
3. laanwj added the label P2P on Oct 3, 2016
4. in src/main.cpp: in e8c5e11b61 outdated

   4698+std::map<CConnman*, std::unique_ptr<PeerLogicValidation> > mapPeerLogics;
   4699+
   4700+void InitPeerLogic(CConnman& connman) {
   4701+    if (mapPeerLogics.count(&connman))
   4702+        return;
   4703+    mapPeerLogics.emplace(std::make_pair(&connman, std::unique_ptr<PeerLogicValidation>(new PeerLogicValidation(&connman))));
   

   ---

   ---

   

   theuni commented at 5:47 pm on October 3, 2016:

   nit: don’t make_pair with emplace, just use the components directly.
5. in src/main.cpp: in e8c5e11b61 outdated

   4693+                }
   4694+            });
   4695+        }
   4696+    }
   4697+};
   4698+std::map<CConnman*, std::unique_ptr<PeerLogicValidation> > mapPeerLogics;
   

   ---

   ---

   

   theuni commented at 5:49 pm on October 3, 2016:

   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.

   ---

   

   TheBlueMatt commented at 9:14 pm on October 3, 2016:

   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?

   ---

   

   theuni commented at 10:17 pm on October 3, 2016:

   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.

   ---

   

   TheBlueMatt commented at 10:20 pm on October 3, 2016:

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

   ---

   

   theuni commented at 11:37 pm on October 3, 2016:

   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.

6. in src/main.cpp: in 780528adc6 outdated

   4697+                if (nDoS > 0)
   4698+                    Misbehaving(it->second, nDoS);
   4699+            }
   4700+        }
   4701+        if (it != mapBlockSource.end())
   4702+            mapBlockSource.erase(it);
   

   ---

   ---

   

   theuni commented at 0:26 am on October 4, 2016:

   Hmm, were these not erased before in the invalid case?
7. 
   theuni commented at 0:29 am on October 4, 2016: member

   Concept ACK and quick code-review ACK other than the nits. I’d like to spend some time testing, though.
8. 
   TheBlueMatt commented at 2:41 am on October 4, 2016: member

   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#pullrequestreview-2631559

9. 
   theuni commented at 5:33 am on October 4, 2016: member

   I was just pointing out that this looks to be a memleak bugfix :)
10. 
    TheBlueMatt commented at 1:18 pm on October 4, 2016: member

    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)
11. 
    TheBlueMatt commented at 1:46 pm on October 4, 2016: member

    Switched g_connman to be a shared_ptr instead of a unique_ptr, which I think should neatly address all the outstanding nits.
12. Make validationinterface.UpdatedBlockTip more verbose

    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.

    87e7d72807
13. Remove duplicate nBlocksEstimate cmp (we already checked IsIBD()) 0278fb5f48
14. TheBlueMatt force-pushed on Oct 4, 2016
15. 
    TheBlueMatt commented at 4:50 pm on October 4, 2016: member

    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.
16. Move net-processing logic definitions together in main.h aefcb7b70c
17. Use CValidationInterface from chain logic to notify peer logic

    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.

    fef1010199
18. Remove CConnman parameter from ProcessNewBlock/ActivateBestChain f5efa28393
19. Always call UpdatedBlockTip, even if blocks were only disconnected 12ee1fe018
20. Remove SyncWithWallets wrapper function 7565e03b96
21. Use BlockChecked signal to send reject messages from mapBlockSource a9aec5c24d
22. TheBlueMatt force-pushed on Oct 4, 2016
23. 
    TheBlueMatt commented at 5:56 pm on October 4, 2016: member

    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.
24. 
    laanwj commented at 8:48 pm on October 18, 2016: member

    utACK a9aec5c
25. laanwj merged this on Oct 18, 2016
26. laanwj closed this on Oct 18, 2016

    ---

27. laanwj referenced this in commit 05998da5a7 on Oct 18, 2016
28. in src/main.cpp: in 0278fb5f48 outdated

    3092-                    nBlockEstimate = Checkpoints::GetTotalBlocksEstimate(chainparams.Checkpoints());
    3093                 if(connman) {
    3094-                    connman->ForEachNode([nNewHeight, nBlockEstimate, &vHashes](CNode* pnode) {
    3095-                        if (nNewHeight > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : nBlockEstimate)) {
    3096+                    connman->ForEachNode([nNewHeight, &vHashes](CNode* pnode) {
    3097+                        if (nNewHeight > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : 0)) {
    

    ---

    ---

    

    theuni commented at 8:58 pm on October 18, 2016:

    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?

    ---

    

    TheBlueMatt commented at 9:15 pm on October 18, 2016:

    Indeed, probably, though that would be a behavior change whereas this PR shouldnt have any :p.
29. in src/validationinterface.h: in 7565e03b96 outdated

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

    ---

    ---

    

    theuni commented at 9:31 pm on October 18, 2016:

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

    ---

    

    TheBlueMatt commented at 9:35 pm on October 18, 2016:

    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)
30. 
    theuni commented at 9:37 pm on October 18, 2016: member

    post-merge utACK.
31. in src/main.cpp: in a9aec5c24d

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

    ---

    ---

    

    rebroad commented at 3:23 am on October 19, 2016:

    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?

    ---

    

    rebroad commented at 3:28 am on October 19, 2016:

    Fixed in #8958

    ---

    

    sipa commented at 8:36 pm on October 30, 2016:

    See the earlier commit: “Remove duplicate nBlocksEstimate cmp (we already checked IsIBD())”.
32. 
    sipa commented at 8:43 pm on October 30, 2016: member

    Posthummus utACK.
33. zkbot referenced this in commit ebd25d1744 on Jul 16, 2018
34. zkbot referenced this in commit 6d605ffbbe on Jul 17, 2018
35. zkbot referenced this in commit 3835cbb57f on Jul 17, 2018
36. furszy referenced this in commit 585c9fed6d on Sep 26, 2020
37. DrahtBot locked this on Sep 8, 2021

Contributors
TheBlueMatt theuni laanwj rebroad sipa

Labels
Refactoring P2P