address: Add failed to dial addresses bucket #478
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Collaborator
dmitry-markin
left a comment
dmitry-markin
left a comment
There was a problem hiding this comment.
One way this can interfere with normal operation is if somebody launches the node with different peer ID on the same port, realizes the mistake, and launches the original node.
This way one can get their validator completely banned by the network.
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
lexnv
added a commit
that referenced
this pull request
Nov 24, 2025
Squashed commit of the following: commit 45f842a Author: Alexandru Vasile <alexandru.vasile@parity.io> Date: Mon Nov 24 15:51:54 2025 +0000 address/tests: Add tests for the known bad addrs Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> commit 684bf13 Author: Alexandru Vasile <alexandru.vasile@parity.io> Date: Mon Nov 24 15:35:45 2025 +0000 manager: Rename addr failure to UNRECOVERABLE_FAILURES Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> commit b514e49 Author: Alexandru Vasile <alexandru.vasile@parity.io> Date: Mon Nov 24 15:35:03 2025 +0000 manager: Expire bad addresses Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> commit 1aa1fdc Author: Alexandru Vasile <alexandru.vasile@parity.io> Date: Fri Nov 21 08:54:36 2025 +0000 address: Make fn private Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> commit ba0ce24 Author: Alexandru Vasile <alexandru.vasile@parity.io> Date: Fri Nov 21 08:50:00 2025 +0000 address: Mark addresses as unreachable on terminal error Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> commit 2751bc7 Author: Alexandru Vasile <alexandru.vasile@parity.io> Date: Fri Nov 21 08:41:40 2025 +0000 address: Introduce unrecoverable addresses Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
| const MAX_ADDRESSES: usize = 64; | ||
|
|
||
| /// Duration for which a bad address is kept in the known bad list. | ||
| const BAD_ADDRESS_EXPIRATION: Duration = Duration::from_secs(15 * 60); // 15 Minutes |
Collaborator
There was a problem hiding this comment.
Suggested change
| const BAD_ADDRESS_EXPIRATION: Duration = Duration::from_secs(15 * 60); // 15 Minutes | |
| const BAD_ADDRESS_EXPIRATION: Duration = Duration::from_secs(5 * 60); // 5 Minutes |
What about 5 mins?
Comment on lines
-230
to
-236
| // The eviction algorithm favours addresses with higher scores. | ||
| // | ||
| // This algorithm has the following implications: | ||
| // - it keeps the best addresses in the store. | ||
| // - if the store is at capacity, the worst address will be evicted. | ||
| // - an address that is not dialed yet (with score zero) will be preferred over an address | ||
| // that already failed (with negative score). |
Collaborator
There was a problem hiding this comment.
Have you deleted the comments on purpose?
| /// This will remove the address from the store and add it to the known bad addresses | ||
| /// with a timestamp. | ||
| fn unrecoverable_address(&mut self, address: Multiaddr) { | ||
| self.addresses.remove(&address); |
Collaborator
There was a problem hiding this comment.
This will lead to permanently banning the address even after the ban expires, better to exclude the address when getting it in addresses().
| pub fn addresses(&self, limit: usize) -> Vec<Multiaddr> { | ||
| let mut records = self.addresses.values().cloned().collect::<Vec<_>>(); | ||
| records.sort_by(|lhs, rhs| rhs.score.cmp(&lhs.score)); | ||
| records.into_iter().take(limit).map(|record| record.address).collect() |
Collaborator
There was a problem hiding this comment.
We should check the banned addresses here and unban the ones with the ban expired & exclude the ones with the active ban.
Comment on lines
+222
to
+223
| DialError::NegotiationError(NegotiationError::PeerIdMismatch(_, _)) => | ||
| scores::UNRECOVERABLE_FAILURES, |
Collaborator
There was a problem hiding this comment.
In this case we should ban the address, but not set its score to i32::MIN to allow connecting once the ban expires. I don't think we can do this using the magic number, better to return a dedicated type, or at least (i32, bool).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR ensures that litep2p keeps track of addresses that failed to dial in the past, up to 64 addresses.
This ensures resources are preserved and failed addresses are no longer dialed.
Litep2p will no longer dial addresses that are marked as unrecoverable:
DialError::AddressErrorNegotiationError::PeerIdMismatch, which means the address corresponds to a different peer than the expected one