[net] Remove ForNode/ForEachNode #11604
Conversation
|
Ping @theuni. |
09e026c to
4212431
Compare
| @@ -153,6 +153,8 @@ struct CBlockReject { | |||
| * and we're no longer holding the node's locks. | |||
| */ | |||
| struct CNodeState { | |||
| //! The CConnman reference to the connection | |||
| CNode* connection; | |||
|
Strong concept ACK Getting this merged will allow removing this ugly |
| // We assert here that locks are popped in the order they were locked. | ||
| // This is a super-overly-restrictive requirement, but we need it to | ||
| // make our deadlock detection work properly. | ||
| assert((*lockstack).back().first == cs); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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).
| @@ -238,7 +240,7 @@ struct CNodeState { | |||
| //! Time of last new block announcement | |||
| int64_t m_last_block_announcement; | |||
|
|
|||
| CNodeState(CAddress addrIn, std::string addrNameIn) : address(addrIn), name(addrNameIn) { | |||
| CNodeState(CNode* connectionIn) : connection(connectionIn), address(connection->addr), name(connection->GetAddrName()) { | |||
There was a problem hiding this comment.
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 :(
There was a problem hiding this comment.
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.
|
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. |
|
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. |
| for (const uint256& hash : reverse_iterate(vHashes)) { | ||
| pnode->PushBlockHash(hash); | ||
| nodestate.connection->PushBlockHash(hash); |
There was a problem hiding this comment.
Let's just move vBlockHashesToAnnounce to CNodeState also. Then PushBlockHash can die and we don't have to mess with CNode here at all.
|
Rebase needed :-) |
|
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. |
30cd007 to
ff92f41
Compare
|
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. |
This prevents any CNode refcount entries outside of net from blocking CNode deletion.
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
It is a "protocol-level-logic" field, and thus belongs in net_processing and not in net.
This is very clearly a "protocol-level" action, so belongs in net_processing, not net.
Again, queueing pings is very much "protocol-level" logic, and belongs in net_processing, not net.
ff92f41 to
fd7e8ec
Compare
They were only used in places where they really should not have been used.
Replaces #10697.