Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
10 changes: 8 additions & 2 deletions dash-spv/src/client/chainlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,16 @@ impl<
let chain_state = self.state.read().await;
{
let mut storage = self.storage.lock().await;
self.chainlock_manager
if let Err(e) = self
.chainlock_manager
.process_chain_lock(chainlock.clone(), &chain_state, &mut *storage)
.await
.map_err(SpvError::Validation)?;
{
// Penalize the peer that relayed the invalid ChainLock
let reason = format!("Invalid ChainLock: {}", e);
let _ = self.network.penalize_last_message_peer_invalid_chainlock(&reason).await;
return Err(SpvError::Validation(e));
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
drop(chain_state);

Expand Down
22 changes: 22 additions & 0 deletions dash-spv/src/network/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,28 @@ pub trait NetworkManager: Send + Sync {

Ok(())
}

/// Penalize the last peer that sent us a message by adjusting reputation.
/// Default implementation is a no-op for managers without reputation.
async fn penalize_last_message_peer(
&self,
_score_change: i32,
_reason: &str,
) -> NetworkResult<()> {
Ok(())
}

/// Convenience: penalize last peer for an invalid ChainLock.
async fn penalize_last_message_peer_invalid_chainlock(
&self,
reason: &str,
) -> NetworkResult<()> {
self.penalize_last_message_peer(
crate::network::reputation::misbehavior_scores::INVALID_CHAINLOCK,
reason,
)
.await
}
}

/// TCP-based network manager implementation.
Expand Down
30 changes: 30 additions & 0 deletions dash-spv/src/network/multi_peer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1050,7 +1050,37 @@ impl NetworkManager for MultiPeerNetworkManager {

Ok(())
}
} // end match
} // end send_message

async fn penalize_last_message_peer(
&self,
score_change: i32,
reason: &str,
) -> NetworkResult<()> {
// Get the last peer that sent us a message
if let Some(addr) = self.get_last_message_peer().await {
self.reputation_manager.update_reputation(addr, score_change, reason).await;
}
Ok(())
}

async fn penalize_last_message_peer_invalid_chainlock(
&self,
reason: &str,
) -> NetworkResult<()> {
if let Some(addr) = self.get_last_message_peer().await {
// Apply misbehavior score and a short temporary ban
self.reputation_manager
.update_reputation(addr, misbehavior_scores::INVALID_CHAINLOCK, reason)
.await;

// Short ban: 10 minutes for relaying invalid ChainLock
self.reputation_manager
.temporary_ban_peer(addr, Duration::from_secs(10 * 60), reason)
.await;
}
Ok(())
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

async fn receive_message(&mut self) -> NetworkResult<Option<NetworkMessage>> {
Expand Down
18 changes: 18 additions & 0 deletions dash-spv/src/network/reputation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,24 @@ impl PeerReputationManager {
}
}

/// Temporarily ban a peer for a specified duration, regardless of score.
/// This can be used for critical protocol violations (e.g., invalid ChainLocks).
pub async fn temporary_ban_peer(&self, peer: SocketAddr, duration: Duration, reason: &str) {
let mut reputations = self.reputations.write().await;
let reputation = reputations.entry(peer).or_default();

reputation.banned_until = Some(Instant::now() + duration);
reputation.ban_count += 1;

log::warn!(
"Peer {} temporarily banned for {:?} (ban #{}, reason: {})",
peer,
duration,
reputation.ban_count,
reason
);
}
Comment on lines +319 to +335

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Critical: Double ban_count increment for invalid ChainLocks.

The temporary_ban_peer method increments ban_count at line 323. However, in dash-spv/src/network/multi_peer.rs at lines 1074-1081, the invalid ChainLock handling calls both update_reputation (which may increment ban_count at line 246 if score exceeds threshold) AND temporary_ban_peer (which always increments ban_count).

This results in ban_count being incremented twice for a single violation, corrupting the ban statistics.

Solution 1 (recommended): Add a parameter to control whether to increment ban_count:

-    pub async fn temporary_ban_peer(&self, peer: SocketAddr, duration: Duration, reason: &str) {
+    pub async fn temporary_ban_peer(&self, peer: SocketAddr, duration: Duration, reason: &str, increment_ban_count: bool) {
         let mut reputations = self.reputations.write().await;
         let reputation = reputations.entry(peer).or_default();
 
         reputation.banned_until = Some(Instant::now() + duration);
-        reputation.ban_count += 1;
+        if increment_ban_count {
+            reputation.ban_count += 1;
+        }
 
         log::warn!(
             "Peer {} temporarily banned for {:?} (ban #{}, reason: {})",

Then in multi_peer.rs line 1080:

-            self.reputation_manager
-                .temporary_ban_peer(addr, Duration::from_secs(10 * 60), reason)
-                .await;
+            self.reputation_manager
+                .temporary_ban_peer(addr, Duration::from_secs(10 * 60), reason, false)
+                .await;

Solution 2: Only call temporary_ban_peer without calling update_reputation first, and have temporary_ban_peer handle both the ban and score update.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// Temporarily ban a peer for a specified duration, regardless of score.
/// This can be used for critical protocol violations (e.g., invalid ChainLocks).
pub async fn temporary_ban_peer(&self, peer: SocketAddr, duration: Duration, reason: &str) {
let mut reputations = self.reputations.write().await;
let reputation = reputations.entry(peer).or_default();
reputation.banned_until = Some(Instant::now() + duration);
reputation.ban_count += 1;
log::warn!(
"Peer {} temporarily banned for {:?} (ban #{}, reason: {})",
peer,
duration,
reputation.ban_count,
reason
);
}
/// Temporarily ban a peer for a specified duration, regardless of score.
/// This can be used for critical protocol violations (e.g., invalid ChainLocks).
pub async fn temporary_ban_peer(&self, peer: SocketAddr, duration: Duration, reason: &str, increment_ban_count: bool) {
let mut reputations = self.reputations.write().await;
let reputation = reputations.entry(peer).or_default();
reputation.banned_until = Some(Instant::now() + duration);
if increment_ban_count {
reputation.ban_count += 1;
}
log::warn!(
"Peer {} temporarily banned for {:?} (ban #{}, reason: {})",
peer,
duration,
reputation.ban_count,
reason
);
}
🤖 Prompt for AI Agents
In dash-spv/src/network/reputation.rs around lines 316 to 332,
temporary_ban_peer always increments reputation.ban_count causing duplicate
increments when callers also call update_reputation (e.g., multi_peer.rs lines
1074-1081); modify temporary_ban_peer to accept a boolean flag like
increment_ban_count: bool (default true) and only increment ban_count when that
flag is true, then update the multi_peer.rs call handling invalid ChainLocks to
call temporary_ban_peer(..., false) (or remove the extra update_reputation call)
so a single violation increments ban_count exactly once.


/// Record a connection attempt
pub async fn record_connection_attempt(&self, peer: SocketAddr) {
let mut reputations = self.reputations.write().await;
Expand Down
Loading