-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backport Bitcoin PR#8113: Rework addnode behaviour #1525
Conversation
src/net.cpp
Outdated
CNode* pnode = FindNode((CService)addrConnect); | ||
if (pnode) | ||
{ | ||
pnode->AddRef(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pnode->AddRef();
should be replaced with the same logic as in 396-401 i.e.
// we have existing connection to this node but it was not a connection to masternode,
// change flag and add reference so that we can correctly clear it later
if(fConnectToMasternode && !pnode->fMasternode) {
pnode->AddRef();
pnode->fMasternode = true;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about backporting bitcoin#9626 that in both cases replaces
pnode->AddRef();
return pnode;
with
LogPrintf("Failed to open new connection, already connected\n");
return NULL;
So, looks to me that after this backport, upgrading node's status to masternode can be done with one line, like
pnode->fMasternode = pnode->fMasternode || fConnectToMasternode;
BTW, what was the reason to do pnode->AddRef();
in this place only when upgrading node's status to masternode, not always (like in bitcoin).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AddRef
for ("normal") existing connection in bitcoin was a bug #924 bitcoin#8372. Using AddRef
in situations where we select node with existing connection as a masternode to connect to, allowed us to track some related bugs iirc. These AddRef
s and corresponding Release
in ThreadSocketHandler
for fMasternode
flag are probably not needed anymore, so I guess they can be safely removed (in both places simultaniously ofc).
Also, I think triggering fMasternode
flag can be simplified even further:
pnode->fMasternode |= fConnectToMasternode;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll use AddRef()
for now for uniformity, and then will remove it later when backporting bitcoin#9626.
src/net.cpp
Outdated
{ | ||
pnode->AddRef(); | ||
{ | ||
LOCK(cs_vNodes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cs_vNodes
should be locked before FindNode
on line 428
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue was fixed in bitcoin#9626 as well.
OK, I've pushed branch with updated changes. Upgrading node status to masternode added, but I'd rather leave locking issues to be fixed when I backport bitcoin#9626. |
One check failed because it just got stuck. Can somebody please restart that failed test? |
Travis is probably having some issues... "An error occurred. The job could not be restarted."... Even though https://www.traviscistatus.com/ says "All Systems Operational" 🤔 I'll try restarting it a bit later. |
Hmmm... I was able to restart some random build in my own repo, but can't restart jobs in dashpay anymore... @schinzelh any idea? @OlegGirko meanwhile you can probably do |
@UdjinM6 same here, can't restart the builds, must be a Travis issue. |
@UdjinM6 This may be the root cause Seems like they have updated the image recently and now "old" builds can't be restarted anymore... |
Pls rebase (no conflicts, just to make sure that travis workaround works as expected + to trigger new build and make sure that all tests pass) |
* Use CNode::addeName to track whether a connection to a name is already open * A new connection to a previously-connected by-name addednode is only opened when the previous one closes (even if the name starts resolving to something else) * At most one connection is opened per addednode (even if the name resolves to multiple) * Unify the code between ThreadOpenAddedNodeConnections and getaddednodeinfo * Information about open connections is always returned, and the dns argument becomes a dummy * An IP address and inbound/outbound is only reported for the (at most 1) open connection
I've pushed rebased branch, let's see test results now. |
All Travis tests were successful. |
utACK |
utACK (assuming future fix of LOCK issue) |
This is backport of Bitcoin PR bitcoin#8113.
This change is needed because it changes code that will be changed by further PRs.
The original PR description follows.
This is an alternative to bitcoin#8097 that:
ThreadOpenAddedNodeConnections
andgetaddednodeinfo
, without any DNS resolving from RPC.getaddednodeinfo
(turning it into a dummy)GetAddedNodeInfo
in net.cpp.addnode
logic that tries all resolved addresses for a name is pushed down intoConnectSocketByName
(picking at random).addnode
logic to prevent multiple connections to the same IP is pushed down intoConnectNode
(by storing colliding names into the CNode's addrName field).Untested.