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.

Sign up for GitHub

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

Jump to bottom

Network altair gossip and discovery #2302

Merged
merged 68 commits into from
Jul 14, 2021
Merged

Network altair gossip and discovery #2302

merged 68 commits into from
Jul 14, 2021

Conversation

pawanjay176
Copy link
Member

@pawanjay176 pawanjay176 commented Apr 6, 2021 •
edited
Loading

Proposed Changes

Implements gossip and discovery changes for altair hard fork. Builds on network-altair-rpc branch.

@pawanjay176 pawanjay176 changed the title Network altair gossip [WIP] Network altair gossip Apr 6, 2021
@paulhauner paulhauner added the work-in-progress PR is a work-in-progress label Apr 13, 2021
@pawanjay176 pawanjay176 force-pushed the network-altair branch 3 times, most recently from fd7fadf to 6b06b22 Compare April 21, 2021 11:01
@pawanjay176 pawanjay176 changed the title [WIP] Network altair gossip [WIP] Network altair gossip and discovery Apr 26, 2021
@pawanjay176 pawanjay176 force-pushed the network-altair branch 2 times, most recently from f54c9ec to 4573f9f Compare April 30, 2021 09:42
@pawanjay176
Copy link
Member Author

@AgeManning would appreciate an initial pass over this guy as well.
It seems to be running fine on prater for now. Also tested with all local testnets where all nodes run rpc v2.

This does not contain the changes for a sync committee subnet service which makes subscriptions for sync committee subnets yet.

@pawanjay176 pawanjay176 added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels May 4, 2021
@pawanjay176 pawanjay176 force-pushed the network-altair-gossip branch 2 times, most recently from cfb9fc8 to db847c3 Compare May 5, 2021 08:42
@pawanjay176 pawanjay176 added work-in-progress PR is a work-in-progress and removed ready-for-review The code is ready for review labels May 5, 2021
@pawanjay176 pawanjay176 force-pushed the network-altair-gossip branch 2 times, most recently from 1d78f67 to 5b6fe3d Compare May 12, 2021 08:06
@pawanjay176 pawanjay176 force-pushed the network-altair branch 2 times, most recently from 6cef740 to d251539 Compare May 17, 2021 13:44
@pawanjay176 pawanjay176 force-pushed the network-altair-gossip branch 2 times, most recently from ae7994b to 2abce36 Compare May 18, 2021 17:06
@pawanjay176 pawanjay176 added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels May 18, 2021
pawanjay176
pawanjay176 commented May 18, 2021
View reviewed changes
beacon_node/eth2_libp2p/src/discovery/mod.rs
@@ -727,7 +768,11 @@ impl<TSpec: EthSpec> Discovery<TSpec> {
};
// predicate for finding nodes with a matching fork and valid tcp port
let eth2_fork_predicate = move |enr: &Enr| {
enr.eth2() == Ok(enr_fork_id.clone()) && (enr.tcp().is_some() || enr.tcp6().is_some())
// `next_fork_epoch` and `next_fork_version` can be different so that
Copy link
Member Author

Choose a reason for hiding this comment

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

@AgeManning this might cause compatibility issues with lighthouse nodes that are not altair enabled.
A non altair enabled node will reject an altair node during discovery because it requires the entire enr fork id to be the same. However, nodes with different next_fork_epoch and next_fork_version in their enr fork id should still be accepted according to the spec.

Copy link
Member

Choose a reason for hiding this comment

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

Can we just match on either current fork digest or next fork version?
I think its fine to discover peers who say they are on the next fork digest. After we dial we'll kick them if they are not compatible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't get you.
I'm saying if node A (old networking) runs a discovery query and finds node B (altair networking), then it will reject node B in the discovery step because node A is running predicate enr_fork_digest_a == enr_fork_digest_b which is false. So old nodes will not be able to discover altair nodes (other way works fine). Not sure if I'm missing something here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tested this in a local testnet and it is an issue. The non-altair node rejects the altair nodes. Made a PR to fix the issue here #2433

@pawanjay176
Copy link
Member Author

@AgeManning this is passing all tests and ready for review.
I have tested it out on prater for backwards compatibility, ran a local testnet to test the enr fork id and gossipsub topics switching logic.

Also, there's a potential incompatibility with the enr predicate that I have highlighted above.

pawanjay176 and others added 11 commits June 18, 2021 17:45
@pawanjay176
Merge branch 'network-altair' into network-altair-gossip
c773da7
@pawanjay176
Change libp2p dependency for message_id_fn
0c10a8f
@pawanjay176
appease clippy
3a8e9e6
@pawanjay176
Add sync committee related metrics
1d79fcc
@michaelsproul @pawanjay176
Expand and refactor fork functions
51c9a26
@michaelsproul @pawanjay176
Simplify compute_subnets_for_sync_committee
5905944
@pawanjay176
Fix subnet service test
73bf044
@pawanjay176
Add v2 protocols for OutboundRequests missed in the merge
8b45823
@michaelsproul
Minify slashing protection interchange data (sigp#2380)
6583ce3 
## Issue Addressed

Closes sigp#2354

## Proposed Changes

Add a `minify` method to `slashing_protection::Interchange` that keeps only the maximum-epoch attestation and maximum-slot block for each validator. Specifically, `minify` constructs "synthetic" attestations (with no `signing_root`) containing the maximum source epoch _and_ the maximum target epoch from the input. This is equivalent to the `minify_synth` algorithm that I've formally verified in this repository:

https://github.com/michaelsproul/slashing-proofs

## Additional Info

Includes the JSON loading optimisation from sigp#2347
@pawanjay176
Handle sync committee gossip errors
3e7eb81
@pawanjay176
Remove delay for updating fork
6bbb343
platschi and others added 15 commits June 22, 2021 00:17
@platschi
Fix typo in advanced networking docs (sigp#2412)
2357a41 
## Proposed Changes

Fixing a typo in the advanced networking docs which mentions ``--target-peer`` instead of the correct ``--target-peers`` flag
@realbigsean
fix the expected error in the sync contribution verification test at …
186125e 
…the sync period boundary
@michaelsproul
Merge remote-tracking branch 'origin/unstable' into altair
0a4c4f5
@realbigsean @michaelsproul
Updates related to merge with unstable
c878ff5
@realbigsean @michaelsproul
Couple more unstable merge fixes
95e3069
@pawanjay176
Split rpc encode/decode into smaller functions
a71eff3
@michaelsproul
Import some relevant funcs from the VC
ffd2b75 
Based on "Expand and refactor fork functions"
@michaelsproul
Fix issues with skip slots at fork boundaries
7481e9c
@michaelsproul
Fix clippy again
0ca1662
@michaelsproul
Merge remote-tracking branch 'origin/altair' into sync-committee-pools
8386024
@realbigsean
alpha.8 updates
ad32be2
@pawanjay176
Pop sync committee tasks from beacon processor
29e8230
@pawanjay176
Merge branch 'sync-committee-pools' into network-altair-gossip
5b492a7
@pawanjay176
Fix some gossip scoring severity issue
9d960ba
@pawanjay176
Fix bug in peer_manager; Improve logging around subnet discoveries
4bd66ac
@pawanjay176 pawanjay176 merged commit c041fd0 into sigp:network-altair Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelsproul michaelsproul michaelsproul left review comments

@AgeManning AgeManning AgeManning left review comments

Assignees
No one assigned
Labels
waiting-on-author The reviewer has suggested changes and awaits thier implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@pawanjay176 @michaelsproul @AgeManning @paulhauner @realbigsean @platschi