Skip to content
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

chore: be less strict when slashing trusted peers #7052

Merged
merged 2 commits into from
Mar 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 80 additions & 4 deletions crates/net/network/src/peers/manager.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use crate::{
error::{BackoffKind, SessionError},
peers::{
reputation::{is_banned_reputation, DEFAULT_REPUTATION},
reputation::{
is_banned_reputation, DEFAULT_REPUTATION, MAX_TRUSTED_PEER_REPUTATION_CHANGE,
},
ReputationChangeWeights, DEFAULT_MAX_COUNT_CONCURRENT_DIALS,
DEFAULT_MAX_COUNT_PEERS_INBOUND, DEFAULT_MAX_COUNT_PEERS_OUTBOUND,
},
Expand Down Expand Up @@ -349,15 +351,37 @@ impl PeersManager {
self.peers.get(peer_id).map(|peer| peer.reputation)
}

/// Apply the corresponding reputation change to the given peer
/// Apply the corresponding reputation change to the given peer.
///
/// If the peer is a trusted peer, it will be exempt from reputation slashing for certain
/// reputation changes that can be attributed to network conditions. If the peer is a
/// trusted peer, it will also be less strict with the reputation slashing.
pub(crate) fn apply_reputation_change(&mut self, peer_id: &PeerId, rep: ReputationChangeKind) {
let outcome = if let Some(peer) = self.peers.get_mut(peer_id) {
// First check if we should reset the reputation
if rep.is_reset() {
peer.reset_reputation()
} else {
let reputation_change = self.reputation_weights.change(rep);
peer.apply_reputation(reputation_change.as_i32())
let mut reputation_change = self.reputation_weights.change(rep).as_i32();
if peer.is_trusted() {
// exempt trusted peers from reputation slashing for
if matches!(
rep,
ReputationChangeKind::Dropped |
ReputationChangeKind::BadAnnouncement |
ReputationChangeKind::Timeout |
ReputationChangeKind::AlreadySeenTransaction
) {
return
}

// also be less strict with the reputation slashing for trusted peers
if reputation_change < MAX_TRUSTED_PEER_REPUTATION_CHANGE {
// this caps the reputation change to the maximum allowed for trusted peers
reputation_change = MAX_TRUSTED_PEER_REPUTATION_CHANGE;
}
}
peer.apply_reputation(reputation_change)
}
} else {
return
Expand Down Expand Up @@ -1962,6 +1986,58 @@ mod tests {
}
}

#[tokio::test]
async fn test_reputation_change_trusted_peer() {
let peer = PeerId::random();
let socket_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 1, 2)), 8008);
let mut peers = PeersManager::default();
peers.add_trusted_peer(peer, socket_addr);

match event!(peers) {
PeerAction::PeerAdded(peer_id) => {
assert_eq!(peer_id, peer);
}
_ => unreachable!(),
}
match event!(peers) {
PeerAction::Connect { peer_id, remote_addr } => {
assert_eq!(peer_id, peer);
assert_eq!(remote_addr, socket_addr);
}
_ => unreachable!(),
}

assert_eq!(peers.peers.get_mut(&peer).unwrap().state, PeerConnectionState::PendingOut);
peers.on_active_outgoing_established(peer);
assert_eq!(peers.peers.get_mut(&peer).unwrap().state, PeerConnectionState::Out);

peers.apply_reputation_change(&peer, ReputationChangeKind::BadMessage);

{
let p = peers.peers.get(&peer).unwrap();
assert_eq!(p.state, PeerConnectionState::Out);
// not banned yet
assert!(!p.is_banned());
}

// ensure peer is banned eventually
loop {
peers.apply_reputation_change(&peer, ReputationChangeKind::BadMessage);

let p = peers.peers.get(&peer).unwrap();
if p.is_banned() {
break;
}
}

match event!(peers) {
PeerAction::Disconnect { peer_id, .. } => {
assert_eq!(peer_id, peer);
}
_ => unreachable!(),
}
}

#[tokio::test]
async fn test_reputation_management() {
let peer = PeerId::random();
Expand Down
7 changes: 7 additions & 0 deletions crates/net/network/src/peers/reputation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@ const BAD_PROTOCOL_REPUTATION_CHANGE: i32 = i32::MIN;
// todo: current value is a hint, needs to be set properly
const BAD_ANNOUNCEMENT_REPUTATION_CHANGE: i32 = REPUTATION_UNIT;

/// The maximum reputation change that can be applied to a trusted peer.
/// This is used to prevent a single bad message from a trusted peer to cause a significant change.
/// This gives a trusted peer more leeway when interacting with the node, which is useful for in
/// custom setups. By not setting this to `0` we still allow trusted peer penalization but less than
/// untrusted peers.
pub(crate) const MAX_TRUSTED_PEER_REPUTATION_CHANGE: Reputation = 2 * REPUTATION_UNIT;
Comment on lines +40 to +45
Copy link
Member

Choose a reason for hiding this comment

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

isn't this rather a minimum since REPUTATION_UNIT is negative?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's negative, but it should act as the maximum reputation change we want to apply:

max(4 * Unit, MAX_TRUSTED_PEER_REPUTATION_CHANGE) == MAX_TRUSTED_PEER_REPUTATION_CHANGE


/// Returns `true` if the given reputation is below the [`BANNED_REPUTATION`] threshold
#[inline]
pub(crate) fn is_banned_reputation(reputation: i32) -> bool {
Expand Down
Loading