Skip to content

fix: Prevent dialing connected peers#662

Merged
mergify[bot] merged 9 commits intosigp:release-v1.0.0from
AgeManning:prevent-dialing-connected-peers
Oct 8, 2025
Merged

fix: Prevent dialing connected peers#662
mergify[bot] merged 9 commits intosigp:release-v1.0.0from
AgeManning:prevent-dialing-connected-peers

Conversation

@AgeManning
Copy link
Member

Issue Addressed

The logs were indicating that we were dialing already connected peers.

This PR filters out already connected peers from the list of discovered peers. It improves the logging so that we can more easily identify our peer count, ie if we are just continually discovering already connected peers.

@dknopik
Copy link
Member

dknopik commented Oct 8, 2025

IMO a better fix would be modifying ConnectionManager::should_dial_peer. Your current approach prevents updating the peer store with a potentially updated ENR.

@dknopik dknopik added waiting-on-author v1.0.0 First Mainnet-release labels Oct 8, 2025
@mergify mergify bot added ready-for-review This PR is ready to be reviewed and removed waiting-on-author labels Oct 8, 2025
Comment on lines 68 to 74
pub fn track_subnet_peers(
subnet_id: SubnetId,
needed_subnets: &mut HashSet<SubnetId>,
peer_store: &MemoryStore<Enr>,
connection_manager: &ConnectionManager,
blocked_peers: &HashSet<PeerId>,
) -> ConnectActions {
needed_subnets.insert(subnet_id);

Self::determine_actions_for_subnets(
Copy link
Member

Choose a reason for hiding this comment

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

I think this is unrelated?

Comment on lines 74 to 78
self.needed_subnets.insert(subnet_id);

PeerDiscovery::track_subnet_peers(
subnet_id,
&mut self.needed_subnets,
self.peer_store.store(),
Copy link
Member

Choose a reason for hiding this comment

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

same

@AgeManning
Copy link
Member Author

Yeah it's unrelated.

It's this commit:

Saw this change earlier, figured it's cleaner this way

I put this in intentionally because I just saw it and figured it didn't deserve its own PR.

Can remove into its own PR if you prefer

@dknopik
Copy link
Member

dknopik commented Oct 8, 2025

Let's do it separately, I believe the way it is currently was intentional during @diegomrsantos' refactoring of the peer manager, so it might be worthwhile to discuss this

Copy link
Member

@dknopik dknopik left a comment

Choose a reason for hiding this comment

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

LGTM, good find!

@dknopik dknopik added ready-for-merge and removed ready-for-review This PR is ready to be reviewed labels Oct 8, 2025
@mergify mergify bot added the queued label Oct 8, 2025
mergify bot added a commit that referenced this pull request Oct 8, 2025
@mergify mergify bot merged commit 551cea1 into sigp:release-v1.0.0 Oct 8, 2025
17 checks passed
@mergify mergify bot removed the queued label Oct 8, 2025
petarjuki7 pushed a commit to petarjuki7/anchor that referenced this pull request Oct 16, 2025
The logs were indicating that we were dialing already connected peers.

This PR filters out already connected peers from the list of discovered peers. It improves the logging so that we can more easily identify our peer count, ie if we are just continually discovering already connected peers.


  


Co-Authored-By: Age Manning <Age@AgeManning.com>

Co-Authored-By: Daniel Knopik <daniel@dknopik.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge v1.0.0 First Mainnet-release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants