Skip to content

P2P: fix peer-set desync that can abort the node - #667

Open
thomasbuilds wants to merge 1 commit into
Cuprate:mainfrom
thomasbuilds:fix-peer-set-id-reuse
Open

P2P: fix peer-set desync that can abort the node#667
thomasbuilds wants to merge 1 commit into
Cuprate:mainfrom
thomasbuilds:fix-peer-set-id-reuse

Conversation

@thomasbuilds

@thomasbuilds thomasbuilds commented Aug 6, 2026

Copy link
Copy Markdown

On clearnet a peer can have its InternalPeerID reused by a connection in the opposite direction: drop the outbound connection we hold, then reconnect inbound from the address we dialled. ClosedConnectionFuture carries only that id, so when it fires the entry in peers may belong to the new connection rather than the one that closed.

remove_dead_peers removed it regardless, dropping the StoredClient and disconnecting a live peer. outbound_peers was only pruned when that entry was outbound, so a slot taken over by an inbound connection left a stale id for random_peer_for_stem to unwrap. As the workspace sets panic = "abort", that ends the process rather than the request.

Drop outbound_peers rather than fix its bookkeeping: it had a single consumer, and keeping it in sync with peers is what failed. Outbound peers are filtered from peers on demand, so an inbound peer cannot be selected for Dandelion++ stemming, and an entry is only removed once its own handle is closed.

@github-actions github-actions Bot added the A-p2p Area: Related to P2P. label Aug 7, 2026
@thomasbuilds
thomasbuilds force-pushed the fix-peer-set-id-reuse branch from 7a06b31 to 319f862 Compare August 9, 2026 04:42
Comment thread p2p/p2p/src/peer_set.rs Outdated
On clearnet a peer can have its `InternalPeerID` reused by a connection
in the opposite direction: drop the outbound connection we hold, then
reconnect inbound from the address we dialled. `ClosedConnectionFuture`
carries only that id, so when it fires the entry in `peers` may belong
to the new connection rather than the one that closed.

`remove_dead_peers` removed it regardless, dropping the `StoredClient`
and disconnecting a live peer. `outbound_peers` was only pruned when
that entry was outbound, so a slot taken over by an inbound connection
left a stale id for `random_peer_for_stem` to `unwrap`. As the workspace
sets `panic = "abort"`, that ends the process rather than the request.

Drop `outbound_peers` rather than fix its bookkeeping: it had a single
consumer, and keeping it in sync with `peers` is what failed. Outbound
peers are filtered from `peers` on demand, so an inbound peer cannot be
selected for Dandelion++ stemming, and an entry is only removed once its
own handle is closed.
@thomasbuilds
thomasbuilds force-pushed the fix-peer-set-id-reuse branch from 319f862 to 627468b Compare August 12, 2026 01:51
@thomasbuilds
thomasbuilds requested a review from Boog900 August 12, 2026 01:53
@Boog900 Boog900 added this to the cuprated v0.1.0 preview 2 milestone Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-p2p Area: Related to P2P.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants