Skip to content
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

Conversation

kcalvinalvin
Copy link
Collaborator

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.

NodeNetworkLimitedBlockThreshold is a constant representing how many
blocks from tip a node signaling NODE_NETWORK_LIMITED must serve.
@Roasbeef
Copy link
Member

Roasbeef commented Oct 4, 2023

cc @ellemouton

Copy link
Contributor

@ellemouton ellemouton left a 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

netsync/manager.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@halseth halseth left a 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 (
Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

netsync/manager.go Outdated Show resolved Hide resolved
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.
@kcalvinalvin kcalvinalvin force-pushed the 2023-09-25-change-is-sync-candidate-behavior branch from 8cecc41 to b4992fe Compare November 7, 2023 02:13
@coveralls
Copy link

Pull Request Test Coverage Report for Build 6779126378

  • 3 of 40 (7.5%) changed or added relevant lines in 2 files are covered.
  • 7 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.02%) to 55.689%

Changes Missing Coverage Covered Lines Changed/Added Lines %
netsync/manager.go 0 37 0.0%
Files with Coverage Reduction New Missed Lines %
mempool/mempool.go 1 66.75%
peer/peer.go 6 74.16%
Totals Coverage Status
Change from base Build 6776433285: -0.02%
Covered Lines: 27150
Relevant Lines: 48753

💛 - Coveralls

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🌽

@Roasbeef Roasbeef merged commit 55ac06b into btcsuite:master Dec 9, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants