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

net: Allow connections from misbehavior banned peers #14929

Merged
merged 1 commit into from Jan 29, 2019

Conversation

@gmaxwell
Copy link
Member

gmaxwell commented Dec 11, 2018

This allows incoming connections from peers which are only banned
due to an automatic misbehavior ban if doing so won't fill inbound.

These peers are preferred for eviction when inbound fills, but may
still be kept if they fall into the protected classes. This
eviction preference lasts the entire life of the connection even
if the ban expires.

If they misbehave again they'll still get disconnected.

The main purpose of banning on misbehavior is to prevent our
connections from being wasted on unhelpful peers such as ones
running incompatible consensus rules. For inbound peers this
can be better accomplished with eviction preferences.

A secondary purpose was to reduce resource waste from repeated
abuse but virtually any attacker can get a nearly unlimited
supply of addresses, so disconnection is about the best we can
do.

This can reduce the potential from negative impact due to incorrect misbehaviour bans.

@gmaxwell

This comment has been minimized.

Copy link
Member Author

gmaxwell commented Dec 11, 2018

@promag
Copy link
Member

promag left a comment

The obvious code review..

Show resolved Hide resolved src/net.cpp Outdated
Show resolved Hide resolved src/net.cpp Outdated
Show resolved Hide resolved src/net.cpp Outdated
Show resolved Hide resolved src/net.cpp Outdated
Show resolved Hide resolved src/net.cpp Outdated
Show resolved Hide resolved src/net.h Outdated

@MarcoFalke MarcoFalke changed the title Allow connections from misbehavior banned peers. net: Allow connections from misbehavior banned peers Dec 11, 2018

@MarcoFalke MarcoFalke added the P2P label Dec 11, 2018

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Dec 11, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Dec 11, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

Show resolved Hide resolved src/net.cpp Outdated
Show resolved Hide resolved src/net.cpp Outdated
src/net.cpp Outdated
LOCK(cs_setBanned);
for (const auto& it : setBanned) {
CSubNet subNet = it.first;
const CSubNet &subNet = it.first;

This comment has been minimized.

@promag

promag Dec 12, 2018

Member

const CSubNet& subNet 🏃

@gmaxwell gmaxwell force-pushed the gmaxwell:201812-softbanmisbehaving branch 4 times, most recently Dec 12, 2018

@bitcoin bitcoin deleted a comment from DrahtBot Dec 14, 2018

@gmaxwell

This comment has been minimized.

Copy link
Member Author

gmaxwell commented Dec 31, 2018

can someone at least give me a concept ack?

@sdaftuar

This comment has been minimized.

Copy link
Member

sdaftuar commented Dec 31, 2018

Yep, Concept ACK.

src/net.h Outdated
@@ -671,6 +672,7 @@ class CNode
// the network or wire types and the cleaned string used when displayed or logged.
std::string strSubVer GUARDED_BY(cs_SubVer), cleanSubVer GUARDED_BY(cs_SubVer);
CCriticalSection cs_SubVer; // used for both cleanSubVer and strSubVer
bool m_prefer_evict; // This peer is preferred for eviction.

This comment has been minimized.

@laanwj

laanwj Jan 2, 2019

Member

One idea I had would be to make this an 'eviction priority' integer instead of a boolean, then choose one of the peers with lowest priority when an eviction is needed. But not necessary unless this is going to be used for other things, as well.

This comment has been minimized.

@gmaxwell

gmaxwell Jan 3, 2019

Author Member

I thought of that too! but thought it was simpler to make it a bool until we had something else to trigger it on.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jan 2, 2019

Concept ACK

@sdaftuar

This comment has been minimized.

Copy link
Member

sdaftuar commented Jan 7, 2019

Perhaps we should go further and not bother disconnecting inbound peers who misbehave, and just increase their likelihood of eviction? One of the changes I've been meaning to propose is to not disconnect inbound peers for relaying an invalid block or header, so that in the event of a future softfork, old peers don't get disconnected for not knowing about the new rules (which risks network partition).

The only benefit to disconnecting that I can think of is if we're using disconnection as a rate limit on the bad behavior itself -- but that seems like a poor remedy for limiting a determined adversary that we assume has access to an ~infinite IP range.

On further thought: this seems like it would be a more involved change to make than I realized (with all the associated testing changes), so we can postpone discussion to a separate PR.

@sdaftuar
Copy link
Member

sdaftuar left a comment

Looks reasonable, will test.

@@ -1034,6 +1058,12 @@ bool CConnman::AttemptToEvictConnection()

if (vEvictionCandidates.empty()) return false;

// If any remaining peers are preferred for eviction consider only them.

This comment has been minimized.

@sdaftuar

sdaftuar Jan 7, 2019

Member

It's not entirely clear to me why we might allow preferred eviction peers to be protected under one of the above criteria -- can you elaborate on that design choice in this comment?

This comment has been minimized.

@gmaxwell

gmaxwell Jan 7, 2019

Author Member

My thought was along the lines of "we don't ever want to disconnect the last inbound peer to give us a block we accepted, regardless of who it is"-- beyond that, I didn't think it mattered much if they got protected under other criteria. I'm fully open to changing it, if you have a suggestion for a better behaviour.

This comment has been minimized.

@sdaftuar

sdaftuar Jan 7, 2019

Member

Seems like fine reasoning, maybe just leave a comment to that effect so that someone could refine this later without wondering why it’s done the current way?

src/net.cpp Outdated

// Don't accept connections from banned peers, but if our inbound slots aren't almost full, accept
// if the only banning reason was an automatic misbehavior ban.
if (!whitelisted && bannedlevel > (nInbound + 1 < nMaxInbound))

This comment has been minimized.

@sdaftuar

sdaftuar Jan 7, 2019

Member

nit: bannedlevel > (nInbound + 1 < nMaxInbound) is not the most readable code, but I do appreciate the comment.

@gmaxwell

This comment has been minimized.

Copy link
Member Author

gmaxwell commented Jan 7, 2019

@sdaftuar I think in the long run we should move in that direction, but it's a bigger change that requires more careful consideration.

Consider: disconnecting on an invalid block / header also potentially helps protect the peer when we're the one that is in the wrong (e.g. rejecting blocks due to corrupt state). Not a reason to not do it, but perhaps it's a reason to be first really confident that our outbound management of consensus rule incompatibilities is really good. Likewise, we might want to implement more holistic rate limiting or prioritized message processing (e.g. queue messages from peers, handle messages from helpful, outbound, peers first).

@sdaftuar

This comment has been minimized.

Copy link
Member

sdaftuar commented Jan 9, 2019

ACK

My only observation from testing is that the interaction between whitelisting and m_prefer_evict is not entirely obvious, but I think this is mostly due to the pre-existing not-well-defined behavior of whitelisting (for instance, I think a whitelisted peer should probably be exempt from eviction, which if I read correctly is not currently the case? But instead after this PR a peer could conceivably be whitelisted and have the prefer_evict flag set -- though that would only be in a strange situation where a such a peer received an automatic misbehavior ban prior to being whitelisted).

@gmaxwell

This comment has been minimized.

Copy link
Member Author

gmaxwell commented Jan 10, 2019

@sdaftuar pre-existing code unconditionally exempts whitelisted peers from consideration for eviction: https://github.com/bitcoin/bitcoin/blob/006a07de4e5c3de36ef84d186315a4a5f23da158/src/net.cpp#L1025

I think the remaining interaction isn't that weird, like if a peer gets eviction pref then gets whitelisted it won't get evicted while whitelisted, but if the whitelisting is removed, the remaining pref will still apply.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jan 11, 2019

utACK 006a07de4e5c3de36ef84d186315a4a5f23da158

src/net.cpp Outdated
@@ -2733,6 +2768,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
addrName = addrNameIn == "" ? addr.ToStringIPPort() : addrNameIn;
nVersion = 0;
strSubVer = "";
m_prefer_evict = false;

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 14, 2019

Member

nit: Should use a C++11 default member initializer instead (That will also solve the merge conflict)

This comment has been minimized.

@gmaxwell

gmaxwell Jan 15, 2019

Author Member

@MarcoFalke Please see: github.com//pull/14929/#discussion_r240818366 (sorry github screws up any effort I make to directly link the comment)

This comment has been minimized.

@gmaxwell

gmaxwell Jan 15, 2019

Author Member

oh I see, it was conflicted by a PR that went through and changed initialization, though, strangely, incompletely (e.g. strSubVer)

@gmaxwell gmaxwell force-pushed the gmaxwell:201812-softbanmisbehaving branch Jan 15, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 15, 2019

@naumenkogs

This comment has been minimized.

Copy link
Contributor

naumenkogs commented Jan 16, 2019

Won't this be a good opportunity to use the same variable for banned and whitelisted? Say
IsBannedLevel == -1 for whitelisted, or better some other naming

@gmaxwell

This comment has been minimized.

Copy link
Member Author

gmaxwell commented Jan 16, 2019

@naumenkogs I don't think there is any conceptual conflict with having a peer which is both banned and whitelisted.

@naumenkogs

This comment has been minimized.

Copy link
Contributor

naumenkogs commented Jan 16, 2019

@gmaxwell but being whitelisted just disables all bans, isn't it?

It always makes me a bit confusing and I have to go look in the code how exactly these 2 work together.
Perhaps enforcing a choice would make it more straightforward, not sure.

Allow connections from misbehavior banned peers.
This allows incoming connections from peers which are only banned
 due to an automatic misbehavior ban if doing so won't fill inbound.

These peers are preferred for eviction when inbound fills, but may
 still be kept if they fall into the protected classes.  This
 eviction preference lasts the entire life of the connection even
 if the ban expires.

If they misbehave again they'll still get disconnected.

The main purpose of banning on misbehavior is to prevent our
 connections from being wasted on unhelpful peers such as ones
 running incompatible consensus rules.  For inbound peers this
 can be better accomplished with eviction preferences.

A secondary purpose was to reduce resource waste from repeated
 abuse but virtually any attacker can get a nearly unlimited
 supply of addresses, so disconnection is about the best we can
 do.

@gmaxwell gmaxwell force-pushed the gmaxwell:201812-softbanmisbehaving branch to 0297be6 Jan 22, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 22, 2019

@gmaxwell

This comment has been minimized.

Copy link
Member Author

gmaxwell commented Jan 24, 2019

@naumenkogs It disables bans applying, but it doesn't disable being banned, and shouldn't -- unwhitelisting should reveal the underlying banned (or not) state, because of this I don't think they should share state.

@laanwj laanwj added this to Blockers in High-priority for review Jan 24, 2019

@sdaftuar

This comment has been minimized.

Copy link
Member

sdaftuar commented Jan 24, 2019

utACK 0297be6

@Sjors
Copy link
Member

Sjors left a comment

Concept ACK

// Returns the most severe level of banning that applies to this address.
// 0 - Not banned
// 1 - Automatic misbehavior ban
// 2 - Any other ban

This comment has been minimized.

@Sjors

Sjors Jan 24, 2019

Member

Nit: maybe swap 1 and 2 so we can add other / more detailed specific reasons later?

This comment has been minimized.

@gmaxwell

gmaxwell Jan 27, 2019

Author Member

meh. Trivial to do if there is a need. Without a need otherwise the current order makes more sense to me.

This comment has been minimized.

@Sjors

Sjors Jan 29, 2019

Member

This data is ephemeral, right? Not saved into banlist.dat (which would make it more tedious to change later).

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jan 24, 2019

utACK 0297be6
A functional test-script would be nice.

@naumenkogs

This comment has been minimized.

Copy link
Contributor

naumenkogs commented Jan 24, 2019

utACK 0297be6

@jonasschnelli jonasschnelli merged commit 0297be6 into bitcoin:master Jan 29, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Jan 29, 2019

Merge #14929: net: Allow connections from misbehavior banned peers
0297be6 Allow connections from misbehavior banned peers. (Gregory Maxwell)

Pull request description:

  This allows incoming connections from peers which are only banned
   due to an automatic misbehavior ban if doing so won't fill inbound.

  These peers are preferred for eviction when inbound fills, but may
   still be kept if they fall into the protected classes.  This
   eviction preference lasts the entire life of the connection even
   if the ban expires.

  If they misbehave again they'll still get disconnected.

  The main purpose of banning on misbehavior is to prevent our
   connections from being wasted on unhelpful peers such as ones
   running incompatible consensus rules.  For inbound peers this
   can be better accomplished with eviction preferences.

  A secondary purpose was to reduce resource waste from repeated
   abuse but virtually any attacker can get a nearly unlimited
   supply of addresses, so disconnection is about the best we can
   do.

  This can reduce the potential from negative impact due to incorrect misbehaviour bans.

Tree-SHA512: 03bc8ec8bae365cc437daf70000c8f2edc512e37db821bc4e0fafa6cf56cc185e9ab40453aa02445f48d6a2e3e7268767ca2017655aca5383108416f1e2cf20f

@jnewbery jnewbery removed this from Blockers in High-priority for review Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment