-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
wire, netsync: change isSyncCandidate behavior #2035
wire, netsync: change isSyncCandidate behavior #2035
Conversation
NodeNetworkLimitedBlockThreshold is a constant representing how many blocks from tip a node signaling NODE_NETWORK_LIMITED must serve.
cc @ellemouton |
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.
LGTM! Very slick & useful improvement 🔥
Just left a non-blocking style comment
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.
LGTM 💯
wire/protocol.go
Outdated
@@ -151,6 +151,12 @@ func (f ServiceFlag) String() string { | |||
return s | |||
} | |||
|
|||
const ( |
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.
nit: declare up with the service flags? This location seems a bit arbitrary
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.
Addressed
isSyncCandidate is now changed to return true even if the peer is a pruned node if and only if our chaintip is within 288 blocks of the peer. Rationale: Pruned nodes that signal NODE_NETWORK_LIMITED MUST serve 288 blocks from their chaintip. If our chaintip is within that range, this peer can be a sync candidate even if they aren't an archival node.
8cecc41
to
b4992fe
Compare
Pull Request Test Coverage Report for Build 6779126378
💛 - Coveralls |
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.
LGTM 🌽
isSyncCandidate is now changed to return true even if the peer is a
pruned node if and only if our chaintip is within 288 blocks of the
peer.
Rationale being that pruned nodes that signal NODE_NETWORK_LIMITED
MUST serve 288 blocks from their chaintip. If our chaintip is within that
range, this peer can be a sync candidate even if they aren't an archival node.