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
net: only enforce expected services for half of outgoing connections #10441
Conversation
utACK 0f1e7d3 |
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.
utACK 0f1e7d3623f37d8ba12ea63543794c90682567bd
0f1e7d3
to
7e1718d
Compare
Updated to fix an issue pointed out by @gmaxwell |
utACK 7e1718d |
1 similar comment
utACK 7e1718d |
(re)utACK 7e1718d389d6cb1223124cefa594f136f56ec4d1 |
Perhaps consider bracing the conditionals? (at least the new ones?) |
7e1718d
to
4cc2e4d
Compare
@gmaxwell done. diff from before is cosmetic only: diff --git a/src/net.cpp b/src/net.cpp
index 1e50cfb..22d26a9 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1796,11 +1796,13 @@ void CConnman::ThreadOpenConnections()
// only consider nodes missing relevant services after 40 failed attempts and only if less than half the outbound are up.
ServiceFlags nRequiredServices = nRelevantServices;
- if (nTries >= 40 && nOutbound < (nMaxOutbound >> 1))
+ if (nTries >= 40 && nOutbound < (nMaxOutbound >> 1)) {
nRequiredServices = REQUIRED_SERVICES;
+ }
- if ((addr.nServices & nRequiredServices) != nRequiredServices)
+ if ((addr.nServices & nRequiredServices) != nRequiredServices) {
continue;
+ }
// do not allow non-default ports, unless after 50 invalid addresses selected already
if (addr.GetPort() != Params().GetDefaultPort() && nTries < 50) |
// regardless of the services assumed to be available, only require the minimum if half or more outbound have relevant services | ||
if (nOutboundRelevant >= (nMaxOutbound >> 1)) { | ||
addrConnect.nServices = REQUIRED_SERVICES; | ||
} else { |
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 think this else branch is unnecessary?
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.
It possibly downgrades the requirement from line 1799.
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.
Indeed!
|
4cc2e4d
to
b6fbfc2
Compare
Yikes, thanks. Pushed the fixup. |
also once half of all outgoing nodes have our preferred flags, require only minimal flags from the rest.
…g connections b6fbfc2 net: only enforce the services required to connect (Cory Fields) Tree-SHA512: 88943bff63213a734f3c96c45760cadaeb9ba18287c8a20c279851ebaf058a334c969028fb2180f155508e3eea4b838147382e4f2b655e7a9aa098eadc81d53e
utACK b6fbfc2 |
also once half of all outgoing nodes have our preferred flags, require only minimal flags from the rest. Github-Pull: bitcoin#10441 Rebased-From: b6fbfc2
…outgoing connections b6fbfc2 net: only enforce the services required to connect (Cory Fields) Tree-SHA512: 88943bff63213a734f3c96c45760cadaeb9ba18287c8a20c279851ebaf058a334c969028fb2180f155508e3eea4b838147382e4f2b655e7a9aa098eadc81d53e
…outgoing connections b6fbfc2 net: only enforce the services required to connect (Cory Fields) Tree-SHA512: 88943bff63213a734f3c96c45760cadaeb9ba18287c8a20c279851ebaf058a334c969028fb2180f155508e3eea4b838147382e4f2b655e7a9aa098eadc81d53e
…outgoing connections b6fbfc2 net: only enforce the services required to connect (Cory Fields) Tree-SHA512: 88943bff63213a734f3c96c45760cadaeb9ba18287c8a20c279851ebaf058a334c969028fb2180f155508e3eea4b838147382e4f2b655e7a9aa098eadc81d53e
…outgoing connections b6fbfc2 net: only enforce the services required to connect (Cory Fields) Tree-SHA512: 88943bff63213a734f3c96c45760cadaeb9ba18287c8a20c279851ebaf058a334c969028fb2180f155508e3eea4b838147382e4f2b655e7a9aa098eadc81d53e
This ensures that future flags can be turned off after being enabled without causing all outbound nodes to refuse to connect to it in the future for lack of expected services.
Should probably be backported to 0.14 as well.