Add main-specific node state & move ban score

sipa commented at 0:35 am on November 18, 2013: member

Currently, the protocol processing happens mostly in main, while most of its data structures are defined in net. This leads to weird situations where they need to access eachother’s locks, and it is unclear what functionality belongs where.

This pull request introduces a main-specific CNodeState, which is managed entirely by main, so doesn’t require CNode’s locks. The intention is to move all processing-related fields in CNode to CNodeState, so ultimately main doesn’t need access to CNode anymore, decoupling the two. This is a long way out, and we’ll probably want to separate protocol processing to a different module than block validation, but it’s what we have now.

For now, only node banning is moved to CNodeState. The reason for this is asynchronous processing. When blocks or transactions are processed by background threads, or in any way not directly the result of a single message being processed, we may want to attribute errors still to the original sender’s DoS score. Going back and forth between main and net for this becomes increasingly ugly, as this can be done perfectly well inside main.

The direct motivation for this is an attempt at implementing BIP37-filtered-block-based fetching, and headers-first.

laanwj commented at 2:24 pm on November 19, 2013: member

Code changes look good

sipa commented at 11:34 pm on November 20, 2013: member

Rebased to trigger pulltester evaluation.

sipa commented at 0:53 am on November 21, 2013: member

I missed the should-not-disconnect-localhost behaviour, thanks pulltester!

in src/main.cpp: in 6a14a6e8b5 outdated

188+
189+void FinalizeNode(const CNode* pnode) {
190+    LOCK(cs_main);
191+    mapNodeState.erase(pnode);
192+}
193+}

Diapolo commented at 4:00 pm on November 21, 2013:

Seems like a dup }?

sipa commented at 4:56 pm on November 21, 2013:

No, it’s the end of the anonymous namespace.

sipa commented at 12:32 pm on November 22, 2013: member

@gavinandresen @gmaxwell Some ACKs?

mikehearn commented at 10:51 am on November 27, 2013: contributor

A lock assertion in State() would be nice, it’s hard to see from the diff that it truly is always called with cs_main held.

But unfortunately it seems that boost locks don’t support that :( Perhaps the LOCK macros could be extended to support it.

in src/rpcnet.cpp: in fed1b52ada outdated

4@@ -5,6 +5,7 @@
5 
6 
7 #include "bitcoinrpc.h"

gavinandresen commented at 5:27 am on November 29, 2013:

Merge conflict fix: replace bitcoinrpc.h include with: #include “rpcserver.h”

in src/main.cpp: in fed1b52ada outdated

153+namespace {
154+// Maintain validation-specific state about nodes, protected by cs_main, instead
155+// by CNode's own locks. This simplifies asynchronous operation, where
156+// processing of incoming data is done after the ProcessMessage call returns,
157+// and we're no longer holding the node's locks.
158+// The CNode* here is only used as an identifier, and should not be dereferenced.

gavinandresen commented at 5:59 am on November 29, 2013:

If it is just used as an identifier, use void* instead of CNode* …

gavinandresen commented at 7:23 am on November 29, 2013: contributor

I’m getting core dumps on shutdown with this, because the cs_main CCriticalSection is being globally-destructed on my machine before instance_of_cnetcleanup.

Order of events at shutdown: cs_main deleted instance_of_cnetcleanup deleted: foreach CNode* pnode: delete pnode CNode::~CNode : GetNodeSignals().FinalizeNode(this); Signal calls FinalizeNode slot, which tries to LOCK(cs_main) : seg fault.

sipa commented at 4:16 pm on December 7, 2013: member

It seems there was no UnregisterNodeSignals() called anywhere, which should prevent signals being delivered at global-destruct time. I added one in Shutdown().

Add main-specific node state b2864d2fb3

sipa commented at 1:56 pm on December 8, 2013: member

Rebased and modified NodeId to be a simple sequentially-assigned integer, instead of an encapsulated const CNode*. Reason: simpler, and doesn’t risk confusing an old disconnected node with a new one, if they get allocated in the same memory area.

BitcoinPullTester commented at 2:15 pm on December 8, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b2864d2fb35fa85e32c76e111f8900598e0bb69d for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

gavinandresen commented at 3:05 am on December 9, 2013: contributor

ACK.

I found another crash-at-shutdown bug, but it has nothing to do with this code.

gavinandresen referenced this in commit 7202d9d9bf on Dec 9, 2013

gavinandresen merged this on Dec 9, 2013

gavinandresen closed this on Dec 9, 2013

in src/main.cpp: in b2864d2fb3

3220@@ -3153,7 +3221,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv)
3221     else if (pfrom->nVersion == 0)
3222     {
3223         // Must have a version message before anything else
3224-        pfrom->Misbehaving(1);
3225+        Misbehaving(pfrom->GetId(), 1);

rebroad commented at 4:38 am on May 9, 2014:

Why pfrom->GetId() and not just pfrom->id ?

sipa commented at 9:57 am on May 9, 2014:

In sane software design, you don’t expose internal state, and use accessor methods like these to query it from the outside. I know the code currently violates it almost everywhere, but that doesn’t mean we need to continue that practice.

in src/net.h: in b2864d2fb3

338@@ -326,6 +339,9 @@ class CNode
339 
340 public:
341 
342+    NodeId GetId() const {

rebroad commented at 4:39 am on May 9, 2014:

@sipa Why is this function needed please?

Bushstar referenced this in commit cb33702b74 on Apr 8, 2020

DrahtBot locked this on Sep 8, 2021

Add main-specific node state & move ban score #3276