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

Subnet search #79

Open
wants to merge 9 commits into
base: unstable
Choose a base branch
from
Open

Subnet search #79

wants to merge 9 commits into from
+283 −46

Conversation

diegomrsantos
Copy link

@diegomrsantos diegomrsantos commented Dec 23, 2024 •
edited
Loading

Issue Addressed

Related to #35

Proposed Changes

  • Dependencies: Updated dependencies in Cargo.toml including ethereum_serde_utils, ethereum_ssz, and ssz_types.
  • Discovery Module: Added support for subnet queries, including new enums and functions for handling subnets.
  • ENR Modifications: Enhanced build_enr to include a "subnets" field and added related bitfield functions.
  • Network Initialization: Included a call to start_subnet_query during network behavior setup.
  • Protocol Identity: Introduced ProtocolId struct, implemented ProtocolIdentity for it, updated Discovery to use Discv5, replaced ssv_node_predicate with tcp_predicate, and refined debug statements. The discv5's ProtocolID field is an identifier that nodes use to diverge away from other discv5 nodes (such as Ethereum). In the ssv network, it's set to 'ssvdv5' so that we find only SSV nodes in discovery right now. The ssv field in the ENR isn't necessary anymore.

Additional Info

It advertises and searches for subnet number 9. This is an arbitrary choice for test purposes and does not have any specific significance.

@diegomrsantos diegomrsantos force-pushed the subnet-search branch 2 times, most recently from fbf8961 to 13d8afb Compare January 7, 2025 12:16
@diegomrsantos diegomrsantos marked this pull request as ready for review January 7, 2025 14:08
@diegomrsantos diegomrsantos added the ready-for-review This PR is ready to be reviewed label Jan 8, 2025
dknopik
dknopik reviewed Jan 8, 2025
View reviewed changes
anchor/network/src/discovery.rs Outdated Show resolved Hide resolved
anchor/network/src/discovery.rs Outdated Show resolved Hide resolved
AgeManning
AgeManning reviewed Jan 13, 2025
View reviewed changes
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

Nice.

I notice the subnet queries are still a TODO, so just noting that.

I don't actually know about the SSV subnet specification. We might be able to simplify some things by having all the subnets long-lived (i.e we dont have to try and rotate or find peers for various subnets given a fixed operator set).

In Lighthouse we've recently changed to deterministic subnets, where the first few bits of a node-id is used to determine who should listen on which subnet. This way we can search for a node-id with a given prefix (so the XOR metric in kad) is efficient at finding them, rather than a random walk. We probably should suggest doing the same for SSV.

The other thing we need to know is how the backbone structure is formed. i.e which nodes are supposed to be permantently subscribed to which subnet.

All of these things are just spec related, maybe we already know. This PR looks good to me as base structure to move on from. Nice :)

anchor/network/src/discovery.rs Show resolved Hide resolved
anchor/network/src/discovery.rs Show resolved Hide resolved
@diegomrsantos
Copy link
Author

I notice the subnet queries are still a TODO, so just noting that.

What part exactly? In this PR we can search for peers in a subnet, some comments may be misleading. I left them as there are more things to do, this is the bare minimum.

AgeManning
AgeManning reviewed Jan 14, 2025
View reviewed changes
anchor/network/src/discovery.rs
subnets_searched_for = ?subnets_searched_for,
"Grouped subnet discovery query yielded no results.",
);
// TODO queries.iter().for_each(|query| {
Copy link
Member

Choose a reason for hiding this comment

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

I was referring to these todos

Copy link
Author

Choose a reason for hiding this comment

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

It's because it's based on LH code and there're more things that I don't understand yet and I'm not sure it's necessary now. What do you think?

anchor/network/src/discovery.rs
Err(e) => {
warn!(error = %e, "Subnet query failed");
}
}
// TODO handle subnet queries
Copy link
Member

Choose a reason for hiding this comment

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

And this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dknopik dknopik dknopik left review comments

@AgeManning AgeManning AgeManning left review comments

@jxs jxs Awaiting requested review from jxs

@jking-aus jking-aus Awaiting requested review from jking-aus

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
ready-for-review This PR is ready to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@diegomrsantos @AgeManning @dknopik