Backport Bitcoin PR#9609: net: fix remaining net assertions #1575
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This avoids having some vars set if the version negotiation fails. Also copy it all into CNode at the same site. nVersion and fSuccessfullyConnected are set last, as they are the gates for the other vars. Make them atomic for that reason.
…handshake Since ForEach* are can be used to send messages to all nodes, the caller may end up sending a message before the version handshake is complete. To limit this, filter out these nodes. While we're at it, may as well filter out disconnected nodes as well. Delete unused methods rather than updating them.
This is a change in behavior, though it's much more sane now than before.
Also cleaned up the comments and moved from the header to the .cpp so that logging headers aren't needed from net.h
UdjinM6
requested changes
Aug 16, 2017
There was a problem hiding this comment.
Conceptually ok, still need to fix the failed build before merging though.
Smth like this should help:
diff --git a/src/net.cpp b/src/net.cpp
index 2cb0c75c0..cc3274716 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -66,6 +66,9 @@
const static std::string NET_MESSAGE_COMMAND_OTHER = "*other*";
+constexpr const CConnman::CFullyConnectedOnly CConnman::FullyConnectedOnly;
+constexpr const CConnman::CAllNodes CConnman::AllNodes;
+
//
// Global state variables
//
|
That's strange. Seems like incomplete implementation of C++11 to me. In C++11 Also, it looks strange that build succeeded for other targets using the same compiler. |
added 2 commits
August 16, 2017 15:32
…man. A change making ForEachNode() and ForNode() methods ignore nodes that have not completed initial handshake have been backported from Bitcoin. Unfortunately, some Dash-specific code needs to iterate over all nodes. This change introduces additional condition argument to these methods. This argument is a functional object that should return true for nodes that should be taken into account, not ignored. Two functional objects are provided in CConnman namespace: * FullyConnectedOnly returns true for nodes that have handshake completed, * AllNodes returns true for all nodes. Overloads for ForEachNode() and ForNode() methods without condition argument are left for compatibility with non-Dash-specific code. They use FullyConnectedOnly functional object for condition. Signed-off-by: Oleg Girko <ol@infoserver.lv>
Use AllNodes functional object as newly introduced condition argument for ForEachNode() and ForNode() methods of CConnman to iterate over all nodes where needed in Dash-specific code. Signed-off-by: Oleg Girko <ol@infoserver.lv>
|
I've added static member definitions anyway, for compatibility with older compilers. |
willwray
reviewed
Aug 17, 2017
src/net.cpp
Outdated
| @@ -2802,7 +2807,7 @@ bool CConnman::ForNode(const CService& addr, std::function<bool(CNode* pnode)> f | |||
| break; | |||
| } | |||
| } | |||
| return found != nullptr && func(found); | |||
| return found != nullptr && NodeFullyConnected(found) && func(found); | |||
There was a problem hiding this comment.
Inconsistent style of non-nullptr check.
Might as well update it while touching the code to the idiomatic short form found && ...
(+ another one below & these lines are edited to cond(found) in a later commit btw).
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
May 10, 2024
, bitcoin#22829, bitcoin#24079, bitcoin#24108, bitcoin#24157, bitcoin#25109 (network backports: part 5) 5dde8e7 merge bitcoin#25109: Strengthen AssertLockNotHeld assertions (Kittywhiskers Van Gogh) a1f005e merge bitcoin#24157: Replace RecursiveMutex cs_totalBytesSent with Mutex and rename it (Kittywhiskers Van Gogh) de4b4bf merge bitcoin#24108: Replace RecursiveMutex cs_addrLocal with Mutex, and rename it (Kittywhiskers Van Gogh) 2f7a138 merge bitcoin#24079: replace RecursiveMutex cs_SubVer with Mutex (and rename) (Kittywhiskers Van Gogh) 23b152c merge bitcoin#22829: various RecursiveMutex replacements in CConnman (Kittywhiskers Van Gogh) 362e310 merge bitcoin#21943: Dedup and RAII-fy the creation of a copy of CConnman::vNodes (Kittywhiskers Van Gogh) bf98ad6 merge bitcoin#22782: Remove unused MaybeSetAddrName (Kittywhiskers Van Gogh) 2b65526 merge bitcoin#21167: make CNode::m_inbound_onion public, initialize explicitly (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6001 * Dependency for #6018 * Partially reverts ff69e0d from #5336 due to `Span<CNode*>`'s incompatibility with `CConnman::NodesSnapshot::Snap()` (returning `const std::vector<CNode*>&`) ``` masternode/sync.cpp:147:18: error: no matching member function for call to 'RequestGovernanceObjectVotes' m_govman.RequestGovernanceObjectVotes(snap.Nodes(), connman); ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./governance/governance.h:360:9: note: candidate function not viable: no known conversion from 'const std::vector<CNode *>' to 'CNode &' for 1st argument int RequestGovernanceObjectVotes(CNode& peer, CConnman& connman) const; ^ ./governance/governance.h:361:9: note: candidate function not viable: no known conversion from 'const std::vector<CNode *>' to 'Span<CNode *>' for 1st argument int RequestGovernanceObjectVotes(Span<CNode*> vNodesCopy, CConnman& connman) const; ^ 1 error generated. ``` * Dash already implements its own `CNode*` iteration logic in [dash#1382](#1382) and implemented additional capabilities in [dash#1575](#1575), which meant backporting [bitcoin#21943](bitcoin#21943) involved migrating Dash-specific code to upstream logic that needed to be modified to implement expected functionality. * Unlike Bitcoin, Dash maintains a map of every raw `SOCKET` corresponding to a pointer of their `CNode` instance and uses it to translate socket sets to their corresponding `CNode*` sets. This is done to accommodate for edge-triggered modes which have an event-socket relationship, as opposed to level-triggered modes, which have a socket-event relationship. This means that `CConnman::SocketHandlerConnected()` doesn't require access to a vector of all `CNode` pointers and therefore, the argument `nodes` has been omitted. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 5dde8e7 Tree-SHA512: 5685d8ebb4fa1f10d018e60d9b0efc3100ea13ac437e7892a09ad3f86d6ac6756e4b5a08ebe70de2eabb27740678e10b975d319f2d553ae5b27dafa71dba0a9f
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is backport of Bitcoin PR bitcoin#9609.
It fixes assertion failures caused by sending messages to other nodes before initial handshake completed.
I had to add two more commits made by myself to this PR because it makes
CConnman::ForEachNode()andCConnman::ForNode()methods ignore nodes we have not completed initial handshake with, but we still need to iterate over all nodes in Dash-specific code.While reviewing this PR, please take a look at all occurences of
ForNodeandForEachNodein Dash-specific code because I'm not sure whether there are places where I've missed addingCConnman::AllNodesargument to make them not ignore nodes with incomplete initial handshake (or where I should have not added this argument).The original PR description follows.
First change is from @TheBlueMatt.
bitcoin/bitcoin@4629656 added assertions to ensure that we never send out any messages before version negotiation has completed, but a few issues remain.
In cases where the version message failed to deserialize fully, we can end up with only some of the vars set, leading to sending messages that shouldn't be sent.
Additionally, the ForEach* functions allow the caller to send messages to all nodes, including those that aren't yet fully connected. To prevent that, filter out nodes that definitely shouldn't be sending/processing messages.
As a follow-up, I'd like to remove the assert, as I think it's done its job, but I'll save that discussion for another PR because I think @TheBlueMatt mentioned that he'd prefer to keep it.