-
Notifications
You must be signed in to change notification settings - Fork 809
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
[Merged by Bors] - Subnet discovery fixes #2095
Conversation
edefdb4
to
a1f23be
Compare
This reverts commit 05c74ac.
85b3365
to
bacf505
Compare
@@ -357,7 +360,8 @@ impl<TSpec: EthSpec> PeerDB<TSpec> { | |||
let log = &self.log; | |||
self.peers.iter_mut() | |||
.filter(move |(_, info)| { | |||
info.is_connected() && info.on_subnet(subnet_id) | |||
// TODO(pawan): should log a message or have a metric for when metadata and gossipsub diverge |
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.
Can we action or remove this TODO
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 we should log something
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.
Sorry forgot about this. Will add a log.
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.
@AgeManning This is actually a bit tricky. gossipsub and metadata attnets can give conflicting results because it takes PING_INTERVAL
(30secs) in the worst case for a peer to communicate an updated metadata.
For e.g. peer sends a ping -> peer subscribes to some subnet -> updates its metadata attnets -> wait 30 secs -> send ping with updated metadata
So we have this 30 sec period when metadata subscriptions don't agree with gossipsub subscriptions.
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 we should just drop the logging here as it won't give us useful info most of the times.
Co-authored-by: Age Manning <[email protected]>
c876b82
to
0aea8da
Compare
0aea8da
to
0745024
Compare
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
bors r+ |
## Issue Addressed N/A ## Proposed Changes Fixes multiple issues related to discovering of subnet peers. 1. Subnet discovery retries after yielding no results 2. Metadata updates if peer send older metadata 3. peerdb stores the peer subscriptions from gossipsub
Pull request successfully merged into unstable. Build succeeded: |
Issue Addressed
N/A
Proposed Changes
Fixes multiple issues related to discovering of subnet peers.