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.

Sign up for GitHub

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

More robust peer banning #3086

Merged
merged 4 commits into from
Oct 4, 2019
Merged

More robust peer banning #3086

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9b8600c
More robust peer banning
quentinlesceller Oct 4, 2019
40a6751
Typo
quentinlesceller Oct 4, 2019
28d3d96
Fix whitespace
quentinlesceller Oct 4, 2019
4dff58b
Hashmap comments
quentinlesceller Oct 4, 2019
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
16 changes: 14 additions & 2 deletions api/src/handlers/peers_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,20 @@ impl Handler for PeerHandler {
};

match command {
"ban" => w_fut!(&self.peers).ban_peer(addr, ReasonForBan::ManualBan),
"unban" => w_fut!(&self.peers).unban_peer(addr),
"ban" => match w_fut!(&self.peers).ban_peer(addr, ReasonForBan::ManualBan) {
Ok(_) => response(StatusCode::OK, "{}"),
Err(e) => response(
StatusCode::INTERNAL_SERVER_ERROR,
format!("ban failed: {:?}", e),
),
},
"unban" => match w_fut!(&self.peers).unban_peer(addr) {
Ok(_) => response(StatusCode::OK, "{}"),
Err(e) => response(
StatusCode::INTERNAL_SERVER_ERROR,
format!("unban failed: {:?}", e),
),
},
_ => return response(StatusCode::BAD_REQUEST, "invalid command"),
};

Expand Down
104 changes: 61 additions & 43 deletions p2p/src/peers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,50 +253,43 @@ impl Peers {
}
false
}

/// Ban a peer, disconnecting it if we're currently connected
pub fn ban_peer(&self, peer_addr: PeerAddr, ban_reason: ReasonForBan) {
if let Err(e) = self.update_state(peer_addr, State::Banned) {
error!("Couldn't ban {}: {:?}", peer_addr, e);
return;
}

if let Some(peer) = self.get_connected_peer(peer_addr) {
debug!("Banning peer {}", peer_addr);
// setting peer status will get it removed at the next clean_peer
match peer.send_ban_reason(ban_reason) {
Err(e) => error!("failed to send a ban reason to{}: {:?}", peer_addr, e),
Ok(_) => debug!("ban reason {:?} was sent to {}", ban_reason, peer_addr),
};
peer.set_banned();
peer.stop();

let mut peers = match self.peers.try_write_for(LOCK_TIMEOUT) {
Some(peers) => peers,
None => {
error!("ban_peer: failed to get peers lock");
return;
}
};
peers.remove(&peer.info.addr);
pub fn ban_peer(&self, peer_addr: PeerAddr, ban_reason: ReasonForBan) -> Result<(), Error> {
self.update_state(peer_addr, State::Banned)?;

match self.get_connected_peer(peer_addr) {
Some(peer) => {
debug!("Banning peer {}", peer_addr);
// setting peer status will get it removed at the next clean_peer
peer.send_ban_reason(ban_reason)?;
peer.set_banned();
peer.stop();
let mut peers = match self.peers.try_write_for(LOCK_TIMEOUT) {
quentinlesceller marked this conversation as resolved.
Show resolved Hide resolved
Some(peers) => peers,
None => {
error!("ban_peer: failed to get peers lock");
return Err(Error::PeerException);
}
};
peers.remove(&peer.info.addr);
Ok(())
}
None => return Err(Error::PeerNotFound),
}
}

/// Unban a peer, checks if it exists and banned then unban
pub fn unban_peer(&self, peer_addr: PeerAddr) {
pub fn unban_peer(&self, peer_addr: PeerAddr) -> Result<(), Error> {
debug!("unban_peer: peer {}", peer_addr);
match self.get_peer(peer_addr) {
Ok(_) => {
if self.is_banned(peer_addr) {
if let Err(e) = self.update_state(peer_addr, State::Healthy) {
error!("Couldn't unban {}: {:?}", peer_addr, e);
}
} else {
error!("Couldn't unban {}: peer is not banned", peer_addr);
}
}
Err(e) => error!("Couldn't unban {}: {:?}", peer_addr, e),
};
// check if peer exist
self.get_peer(peer_addr)?;
if self.is_banned(peer_addr) {
self.update_state(peer_addr, State::Healthy)?;
Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to return on the previous line, I remember @antiochp like to be more explicit though

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

[totally off-topic]

My concern is mainly around multiple calls in a block of code -

// inconsistent across multiple lines (easy to miss that `three()` returns a Result)
{
    one()?;
    two()?;
    three()
}

// more consistency, less line noise if we introduce additional lines or reorder lines etc.
{
    one()?;
    two()?;
    three()?;
    Ok(())
}

Also we definitely have cases where you need the ? to translate errors appropriately.
Not sure this is an issue here though.

I'm definitely coming around to single line blocks of code returning on that single line as you suggest @hashmap. As long as the errors convert correctly.

// Also an option if necessary
{
    Ok(one()?)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but requires more lines, may require to introduce a temp var and last but not least - we unwrap Result on line 3 and then wrap it on line 4, instead directly returning it as is. Same with the last approach. I hope the compiler produces the same code though.

} else {
error!("Couldn't unban {}: peer is not banned", peer_addr);
quentinlesceller marked this conversation as resolved.
Show resolved Hide resolved
return Err(Error::PeerNotBanned);
}
}

fn broadcast<F>(&self, obj_name: &str, inner: F) -> u32
Expand Down Expand Up @@ -605,7 +598,12 @@ impl ChainAdapter for Peers {
"Received a bad block {} from {}, the peer will be banned",
hash, peer_info.addr,
);
self.ban_peer(peer_info.addr, ReasonForBan::BadBlock);
self.ban_peer(peer_info.addr, ReasonForBan::BadBlock)
.map_err(|e| {
let err: chain::Error =
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and below - perhaps just return error chain::ErrorKind::Other(format!("ban peer error :{:?}", e)).into() instaead of defining a temp var and returnning it?

Copy link
Member Author

@quentinlesceller quentinlesceller Oct 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I have no choice otherwise Rust complains (cannot infer type thing).

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately cannot be done. Rust need type annotations for that one.

chain::ErrorKind::Other(format!("ban peer error :{:?}", e)).into();
err
})?;
Ok(false)
} else {
Ok(true)
Expand All @@ -625,7 +623,12 @@ impl ChainAdapter for Peers {
"Received a bad compact block {} from {}, the peer will be banned",
hash, peer_info.addr
);
self.ban_peer(peer_info.addr, ReasonForBan::BadCompactBlock);
self.ban_peer(peer_info.addr, ReasonForBan::BadCompactBlock)
.map_err(|e| {
let err: chain::Error =
chain::ErrorKind::Other(format!("ban peer error :{:?}", e)).into();
err
})?;
Ok(false)
} else {
Ok(true)
Expand All @@ -640,7 +643,12 @@ impl ChainAdapter for Peers {
if !self.adapter.header_received(bh, peer_info)? {
// if the peer sent us a block header that's intrinsically bad
// they are either mistaken or malevolent, both of which require a ban
self.ban_peer(peer_info.addr, ReasonForBan::BadBlockHeader);
self.ban_peer(peer_info.addr, ReasonForBan::BadBlockHeader)
.map_err(|e| {
let err: chain::Error =
chain::ErrorKind::Other(format!("ban peer error :{:?}", e)).into();
err
})?;
Ok(false)
} else {
Ok(true)
Expand All @@ -655,7 +663,12 @@ impl ChainAdapter for Peers {
if !self.adapter.headers_received(headers, peer_info)? {
// if the peer sent us a block header that's intrinsically bad
// they are either mistaken or malevolent, both of which require a ban
self.ban_peer(peer_info.addr, ReasonForBan::BadBlockHeader);
self.ban_peer(peer_info.addr, ReasonForBan::BadBlockHeader)
.map_err(|e| {
let err: chain::Error =
chain::ErrorKind::Other(format!("ban peer error :{:?}", e)).into();
err
})?;
Ok(false)
} else {
Ok(true)
Expand Down Expand Up @@ -701,7 +714,12 @@ impl ChainAdapter for Peers {
"Received a bad txhashset data from {}, the peer will be banned",
peer_info.addr
);
self.ban_peer(peer_info.addr, ReasonForBan::BadTxHashSet);
self.ban_peer(peer_info.addr, ReasonForBan::BadTxHashSet)
.map_err(|e| {
let err: chain::Error =
chain::ErrorKind::Other(format!("ban peer error :{:?}", e)).into();
err
})?;
Ok(true)
} else {
Ok(false)
Expand Down
2 changes: 2 additions & 0 deletions p2p/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ pub enum Error {
peer: Hash,
},
Send(String),
PeerNotFound,
PeerNotBanned,
PeerException,
Internal,
}
Expand Down
4 changes: 3 additions & 1 deletion servers/src/grin/seed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,9 @@ fn monitor_peers(
let interval = Utc::now().timestamp() - x.last_banned;
// Unban peer
if interval >= config.ban_window() {
peers.unban_peer(x.addr);
if let Err(e) = peers.unban_peer(x.addr) {
error!("failed to unban peer {}: {:?}", x.addr, e);
quentinlesceller marked this conversation as resolved.
Show resolved Hide resolved
}
debug!(
"monitor_peers: unbanned {} after {} seconds",
x.addr, interval
Expand Down
8 changes: 6 additions & 2 deletions servers/src/grin/sync/header_sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,12 @@ impl HeaderSync {
if now > *stalling_ts + Duration::seconds(120)
&& header_head.total_difficulty < peer.info.total_difficulty()
{
self.peers
.ban_peer(peer.info.addr, ReasonForBan::FraudHeight);
if let Err(e) = self
.peers
.ban_peer(peer.info.addr, ReasonForBan::FraudHeight)
{
error!("failed to ban peer {}: {:?}", peer.info.addr, e);
}
info!(
"sync: ban a fraud peer: {}, claimed height: {}, total difficulty: {}",
peer.info.addr,
Expand Down