[net] Remove ForNode/ForEachNode #11604

TheBlueMatt commented at 12:31 AM on November 4, 2017: member

They were only used in places where they really should not have been used.

Replaces #10697.

TheBlueMatt commented at 12:32 AM on November 4, 2017: member

Ping @theuni.

fanquake added the label P2P on Nov 4, 2017

fanquake requested review from theuni on Nov 4, 2017

in src/net_processing.cpp:157 in 6af84675e3 outdated

 152 | @@ -153,6 +153,8 @@ struct CBlockReject {
 153 |   * and we're no longer holding the node's locks.
 154 |   */
 155 |  struct CNodeState {
 156 | +    //! The CConnman reference to the connection
 157 | +    CNode* connection;

promag commented at 1:47 AM on November 4, 2017:

const?

TheBlueMatt force-pushed on Nov 4, 2017

promag commented at 10:45 AM on November 5, 2017: member

Concept ACK.

Are all changes required to remove ForNode and ForEachNode?

practicalswift commented at 12:33 PM on November 6, 2017: contributor

Strong concept ACK

Getting this merged will allow removing this ugly -Wthread-safety work-around: https://github.com/bitcoin/bitcoin/pull/11226/commits/5a890d376f7a296a6a4a7ce0d076012ef920b239

in src/sync.cpp:131 in 4212431503 outdated

 127 | +static void pop_lock(void* cs)
 128 |  {
 129 | +    // We assert here that locks are popped in the order they were locked.
 130 | +    // This is a super-overly-restrictive requirement, but we need it to
 131 | +    // make our deadlock detection work properly.
 132 | +    assert((*lockstack).back().first == cs);

theuni commented at 7:57 PM on November 7, 2017:

If I'm understanding this correctly, I think it may cause issues with recursive locks that are currently harmless (though potentially troublesome)? e.g.:

recursive_mutex a;
recursive_mutex b;
lock(a)
lock(a)
lock(b)
lock(b)
unlock(a)
unlock(b)
unlock(b)
unlock(a)

TheBlueMatt commented at 8:01 PM on November 7, 2017:

Yes, those will now be DEBUG_LOCKORDER failures, but it appears to currently not be violated, and would detect some potentially significant issues, consider:

lock(a) lock(b) unlock(a) (this now pops b off the stack, so lockorder thinks we have a locked) lock(a) (this is a deadlock cause we've now violated lockorder, but DEBUG_LOCKORDER thinks that we're just re-locking a).

in src/net_processing.cpp:247 in 688eb681c5 outdated

 239 | @@ -238,7 +240,7 @@ struct CNodeState {
 240 |      //! Time of last new block announcement
 241 |      int64_t m_last_block_announcement;
 242 |  
 243 | -    CNodeState(CAddress addrIn, std::string addrNameIn) : address(addrIn), name(addrNameIn) {
 244 | +    CNodeState(CNode* connectionIn) : connection(connectionIn), address(connection->addr), name(connection->GetAddrName()) {

theuni commented at 8:02 PM on November 7, 2017:

I agree with passing an opaque handle in directly here, but making it easier for net_processing to use CNode's internals is the opposite of the direction we should be headed in :(

TheBlueMatt commented at 8:18 PM on November 7, 2017:

Sure, but incremental steps - its clearly the right direction, even if CNode will have more fields made private over the coming months...its not like we have to merge things which reference more CNode fields aside from clear const descriptors that just describe the connection itself.

theuni commented at 8:15 PM on November 7, 2017: member

This really clashes with the libevent work, which totally reworks the disconnection logic.

Can we revisit this down the road a bit? It's a bit frustrating to have some of these things already worked out, but stuck waiting on per-requisite PR review.

That said, concept ACK on moving RelayTransaction/nStartingHeight out of net.

TheBlueMatt commented at 8:22 PM on November 7, 2017: member

How does this conflict with libevent? The disconnect logic is still based on telling CConnman to do X with a peer, no? And FinalizeNode being moved around shouldn't matter too much, just as long as its not called at the same time as a call to one of the other NetEventsInterface calls.

This is a rather simple PR, I'm happy to rebase it on some different disconnect-order tweaks that are pulled out of libevent (I assume you're gonna reorg them prior to pulling in the whole libevent reword as a separate thing) but it'd be a shame for a fix like this which mostly just pulls net and net_processing apart further to wait on a major rework of net.

in src/net_processing.cpp:920 in e76117bfc4 outdated

 913 | +        for (auto& node : mapNodeState) {
 914 | +            CNodeState& nodestate = node.second;
 915 | +            if (nNewHeight > (nodestate.nStartingHeight != -1 ? nodestate.nStartingHeight - 2000 : 0)) {
 916 |                  for (const uint256& hash : reverse_iterate(vHashes)) {
 917 | -                    pnode->PushBlockHash(hash);
 918 | +                    nodestate.connection->PushBlockHash(hash);

theuni commented at 10:36 PM on November 14, 2017:

Let's just move vBlockHashesToAnnounce to CNodeState also. Then PushBlockHash can die and we don't have to mess with CNode here at all.

practicalswift commented at 10:37 PM on November 14, 2017: contributor

Rebase needed :-)

theuni commented at 11:14 PM on November 14, 2017: member

Discussed this with @TheBlueMatt today and we agreed that it makes sense to split this into 2 PRs: 1 to move a bunch of stuff to CNodeState, then a follow-up to argue about how to best remove ForEachNode.

TheBlueMatt force-pushed on Nov 15, 2017

TheBlueMatt commented at 11:42 PM on November 15, 2017: member

Heh, to follow up on the 2PRs comment, @theuni sent me down a rabbit hole and suddenly I found myself in a farmiliar place - trying to pull CNodeState out of cs_main...I'm probably going to revive that work soon based on an approach @theuni suggested, but this PR will have to stand alone for some time yet. Please review with that in mind.

[net] FinalizeNode on the message handler thread

This prevents any CNode refcount entries outside of net from
blocking CNode deletion.

502c942dc6

[net_processing] Keep a reference to the CNode in CNodeState

The CNode/CNodeState terminology is rather outdated now - CNode
should be considered representative of the connection for the
purposes of telling CConnman where to send data, so giving
CNodeState a reference to it makes perfect sense. This also allows
us to migrate off of CConnman's ForEachNode/ForNode

6c8fd96085

Move nStartingHeight out of CNode into CNodeState

It is a "protocol-level-logic" field, and thus belongs in
net_processing and not in net.

c01abb14da

Remove all references to ForEachNode/ForNode in net_processing e72cc5f74e

Use net_processing's RelayTransaction to relay txn, not CConnman

This is very clearly a "protocol-level" action, so belongs in
net_processing, not net.

ce353ce2e8

Replace final ForEachNode with a QueuePingForAllPeers

Again, queueing pings is very much "protocol-level" logic, and
belongs in net_processing, not net.

a744e30c74

[net] Remove CConnman's (now unused) ForEachNode and ForNode 2dda567283

Improve LeaveCritical strictness fd7e8ecb42

TheBlueMatt force-pushed on Nov 16, 2017

MarcoFalke added the label Needs rebase on Jun 6, 2018

TheBlueMatt closed this on Jun 14, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

MarcoFalke locked this on Dec 16, 2021