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

More robust peer banning #3086

Merged
merged 4 commits into from
Oct 4, 2019
Merged

Conversation

quentinlesceller
Copy link
Member

Handle the errors and propagate them instead of just logging.

servers/src/grin/seed.rs Outdated Show resolved Hide resolved
Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

👍 yes!

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

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.

p2p/src/peers.rs Outdated Show resolved Hide resolved
p2p/src/peers.rs Outdated
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.

p2p/src/peers.rs Outdated Show resolved Hide resolved
@quentinlesceller
Copy link
Member Author

Will merge once tests pass.

@quentinlesceller quentinlesceller merged commit eefd87a into mimblewimble:master Oct 4, 2019
@quentinlesceller quentinlesceller deleted the ban branch October 4, 2019 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants