[p2p] Wait to Reserve Until Handshake Complete #1170
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| debug!(?peer, "verified handshake"); | ||
|
|
||
| // Check if the peer is listenable | ||
| if !tracker |
There was a problem hiding this comment.
There is extra overhead from messaging the tracker twice per handshake (rather than the once before), however, this is required with the current code to prevent this trivial attack.
There was a problem hiding this comment.
Pull Request Overview
This PR defers reserving a peer connection until after the handshake completes, adding a new listenable predicate and integrating it throughout the tracker and listener logic.
- Introduce
listenablemethods in both lookup and discovery records, directories, and mailbox APIs - Add
Message::Listenablevariant & mailbox helpers to query listenability - Update listener actor to verify handshake, check
listenable, complete handshake, then reserve connection
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| p2p/src/authenticated/lookup/actors/tracker/record.rs | Add listenable predicate to peer records |
| p2p/src/authenticated/lookup/actors/tracker/ingress.rs | Add Listenable message & mailbox helper |
| p2p/src/authenticated/lookup/actors/tracker/directory.rs | Implement listenable in directory API |
| p2p/src/authenticated/lookup/actors/tracker/actor.rs | Handle Message::Listenable and add tests |
| p2p/src/authenticated/lookup/actors/listener.rs | Delay reservation until after handshake complete |
| p2p/src/authenticated/discovery/actors/tracker/{record,ingress,directory,actor}.rs | Mirror lookup changes for discovery tracker |
| p2p/src/authenticated/discovery/actors/listener.rs | Delay reservation until after handshake complete |
Comments suppressed due to low confidence (3)
p2p/src/authenticated/lookup/actors/tracker/directory.rs:204
- Add a doc comment explaining when a peer is considered listenable (e.g., allowed and not connected) to match the style of
dialable.
pub fn listenable(&self, peer: &C) -> bool {
p2p/src/authenticated/discovery/actors/tracker/directory.rs:314
- Consider adding a doc comment here as well to describe the conditions under which a peer is listenable.
pub fn listenable(&self, peer: &C) -> bool {
p2p/src/authenticated/lookup/actors/tracker/actor.rs:323
- Relying on a fixed sleep can make tests brittle. Consider using a notification or awaiting an explicit confirmation from the tracker instead of
sleepto synchronize test setup.
context.sleep(Duration::from_millis(10)).await;
c41fe5e
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #1170 +/- ##
==========================================
+ Coverage 91.09% 91.17% +0.07%
==========================================
Files 216 216
Lines 57779 57889 +110
==========================================
+ Hits 52634 52780 +146
+ Misses 5145 5109 -36
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Related: #1132
Until key confirmation, we don't know if the dialer was in fact the peer. To prevent an adversary from trivially blocking a peer (rate limiting inbound connections by spamming handshake requests), we should wait to reserve a connection to a peer until the handshake is complete.
In an ideal world, we would also check if the dialing peer is already rate limited but
governordoesn't expose this capability (and we instead just rely on the handshake rate limiter as an overlay limit which would probably be hit first in most cases anyways).