-
Notifications
You must be signed in to change notification settings - Fork 170
[p2p] Limit Block Duration #2570
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
Conversation
Deploying monorepo with
|
| Latest commit: |
f7f6369
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://65f89e6f.monorepo-eu0.pages.dev |
| Branch Preview URL: | https://bounded-block.monorepo-eu0.pages.dev |
| pub fn listen(&mut self, peer: &C) -> Option<Reservation<C>> { | ||
| /// Returns `Ok(Reservation)` on success, or an error indicating why the reservation failed. | ||
| pub fn listen(&mut self, peer: &C) -> Result<Reservation<C>, super::ingress::ListenError> { | ||
| use super::ingress::ListenError; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this
2ec666d to
27860f7
Compare
| context, | ||
| |peer| tracker.acceptable(peer), | ||
| |peer| { | ||
| let mut tracker = tracker.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is jank but technically this the way you'd need to do this given the interface of listen
| /// - We are not already connected or reserved | ||
| /// - The ingress address is allowed (DNS enabled, Socket IP is global or private IPs allowed) | ||
| /// | ||
| /// Note: Blocked peers are never dialable regardless of expiry since we don't retain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should change this?
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
commonware-mcp | f7f6369 | Jan 01 2026, 11:28 PM |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #2570 +/- ##
==========================================
- Coverage 92.63% 92.62% -0.01%
==========================================
Files 355 355
Lines 102890 103042 +152
==========================================
+ Hits 95314 95446 +132
- Misses 7576 7596 +20
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| |peer| tracker.acceptable(peer), | ||
| |peer| { | ||
| let mut tracker = tracker.clone(); | ||
| async move { tracker.acceptable(peer).await == tracker::Acceptable::Yes } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just make this async?
| .await | ||
| { | ||
| Ok(x) => x, | ||
| Err(StreamError::PeerRejected(bytes)) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make error generic over key?
| Err(StreamError::PeerRejected(bytes)) => { | ||
| // Decode the peer public key and query for rejection reason | ||
| if let Ok(peer) = C::PublicKey::read(&mut bytes.as_slice()) { | ||
| match tracker.acceptable(peer).await { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super racy and inefficient.
| /// Attempt to block a peer, updating the metrics accordingly. | ||
| pub fn block(&mut self, peer: &C) { | ||
| if self.peers.get_mut(peer).is_some_and(|r| r.block()) { | ||
| let blocked_until = self.context.current() + self.block_duration; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
saturating add
| self.peers.get(peer).is_some_and(|r| r.acceptable()) | ||
| /// Returns the acceptance status for a peer. | ||
| pub fn acceptable(&self, peer: &C) -> super::ingress::Acceptable { | ||
| use super::ingress::Acceptable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove inline import
| pub fn block(&mut self) -> bool { | ||
| if matches!(self.address, Address::Blocked | Address::Myself) { | ||
| pub fn block(&mut self, blocked_until: SystemTime) -> bool { | ||
| if matches!(self.address, Address::Blocked(_) | Address::Myself) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use later of block times?
| /// IPs of eligible peers we should accept connections from. | ||
| pub registered_ips: HashSet<IpAddr>, | ||
| /// IPs of blocked peers with their expiry times. | ||
| pub blocked_ips: std::collections::HashMap<IpAddr, SystemTime>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just import?
| // Send the updated listenable IPs to the listener. | ||
| let _ = self.listener.send(self.directory.listenable()).await; | ||
| // Send the updated filter to the listener. | ||
| let filter = ListenerFilter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
send just blocked IPs here (don't need to resend listenable?)
|
|
||
| // Check whether the IP is registered | ||
| if !self.attempt_unregistered_handshakes && !self.registered_ips.contains(&ip) { | ||
| self.handshakes_blocked.inc(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add metrics for different block reasons.
# Conflicts: # p2p/src/authenticated/lookup/actors/tracker/actor.rs # p2p/src/authenticated/lookup/actors/tracker/directory.rs # p2p/src/authenticated/lookup/actors/tracker/mod.rs # p2p/src/authenticated/lookup/actors/tracker/record.rs # p2p/src/authenticated/lookup/network.rs
Closes: #2556
Closes: #2557
TODO
Summary
This PR adds time-based block expiry for p2p peers. When a peer is blocked, the block automatically expires after a configurable duration (default: 1 hour for production, 1 minute for tests). This allows nodes that were blocked due to temporary issues (misconfiguration, one-off bugs) to reconnect without requiring restarts of all other nodes.
Changes
Discovery Module
block_durationconfiguration fieldAddress::Blockednow storesSystemTime(block expiry time) instead of being a unit variantnow: SystemTimeparameter to check block expiry:blocked(now),eligible(now),want(now, ...),dialable(now, ...),acceptable(now),reserve(now),update(now, ...)Acceptableenum (Yes,Blocked,Unknown) for detailed rejection loggingLookup Module
block_durationconfiguration fieldAddress::Blockednow storesSystemTime(block expiry time)ListenerFilterstruct carries blocked IPs with expiry times to listenerBehavior
block_duration