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

Basic peer manager based on libp2p peer_store and connection_limits #126

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

Conversation

dknopik
Copy link
Member

@dknopik dknopik commented Feb 5, 2025

Draft of basic peer manager using connection_limits and a preliminary version of peer_store.

This is just a draft, as the exact design of peer_store is still TBD and a few things are missing.

@dknopik
Copy link
Member Author

dknopik commented Feb 10, 2025

I propose we review and merge this, even though upstream has not merged the peer store yet. We need something to test. Very open for other suggestions though

@dknopik dknopik marked this pull request as ready for review February 10, 2025 14:52
@dknopik dknopik requested review from diegomrsantos and jxs February 10, 2025 14:53
Comment on lines +46 to +65
beacon_node_fallback = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
bls = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
eth2 = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
eth2_config = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
eth2_network_config = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
health_metrics = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
lighthouse_network = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
metrics = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
safe_arith = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
sensitive_url = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
slashing_protection = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
slot_clock = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
task_executor = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2", default-features = false, features = [
"tracing",
] }
types = { git = "https://github.com/sigp/lighthouse", rev = "1a77f7a0" }
unused_port = { git = "https://github.com/sigp/lighthouse", rev = "1a77f7a0" }
validator_metrics = { git = "https://github.com/sigp/lighthouse", rev = "1a77f7a0" }
validator_services = { git = "https://github.com/sigp/lighthouse", rev = "1a77f7a0" }
validator_store = { git = "https://github.com/sigp/lighthouse", rev = "1a77f7a0" }
types = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
unused_port = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
validator_metrics = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
validator_services = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }
validator_store = { git = "https://github.com/sigp/lighthouse", rev = "b71b5f2" }

Choose a reason for hiding this comment

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

Is it possible to define rev only once?

# we might need to explicitly list even more crates here if something fails
libp2p = { git = "https://github.com/dknopik/rust-libp2p.git", rev = "170816f" }
libp2p-identity = { git = "https://github.com/dknopik/rust-libp2p.git", rev = "170816f" }
libp2p-mplex = { git = "https://github.com/dknopik/rust-libp2p.git", rev = "170816f" }

Choose a reason for hiding this comment

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

why is mplex necessary?

@@ -11,7 +11,7 @@ ethereum_ssz = "0.8.1"
ethereum_ssz_derive = "0.8.1"
futures = { workspace = true }
hex = "0.4.3"
libp2p = { version = "0.54", default-features = false, features = [
libp2p = { git = "https://github.com/dknopik/rust-libp2p.git", rev = "170816f", default-features = false, features = [

Choose a reason for hiding this comment

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

Can we use only the peer store as a standalone dep from this repo?

@@ -192,24 +197,39 @@ impl Network {
self.swarm
.behaviour_mut()
.discovery
.start_subnet_query(vec![subnet]);
.set_subscribed(subnet, true);

Choose a reason for hiding this comment

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

Could we do it in a different PR?

@@ -267,6 +266,22 @@ impl Discovery {
);
}

pub fn set_subscribed(&mut self, subnet: SubnetId, subscribed: bool) {

Choose a reason for hiding this comment

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

Could we do it in a different PR?

.filter_map(|(enr, _)| manager.discovered_peer(enr))
.collect::<Vec<_>>();
for dial in to_dial {
let _ = self.swarm.dial(dial);

Choose a reason for hiding this comment

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

Comment on lines +30 to +35
pub peer_store: peer_store::Behaviour<MemoryStore<Enr>>,
pub connection_limits: connection_limits::Behaviour,
pub connected: HashSet<PeerId>,
pub needed_subnets: HashSet<SubnetId>,
pub target_peers: usize,
pub max_with_priority_peers: usize,

Choose a reason for hiding this comment

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

Should those be pub?

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.

2 participants