net: Allow connections from misbehavior banned peers #14929

gmaxwell · 2018-12-11T21:14:26Z

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 · 2018-12-11T21:16:15Z

@pstratem @TheBlueMatt

promag

The obvious code review..

src/net.cpp

src/net.h

DrahtBot · 2018-12-11T23:22:21Z

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

Conflicts

No conflicts as of last run.

src/net.cpp

promag · 2018-12-12T16:15:49Z

src/net.cpp

    LOCK(cs_setBanned);
    for (const auto& it : setBanned) {
-        CSubNet subNet = it.first;
+        const CSubNet &subNet = it.first;


const CSubNet& subNet 🏃

gmaxwell · 2018-12-31T21:21:31Z

can someone at least give me a concept ack?

sdaftuar · 2018-12-31T21:26:31Z

Yep, Concept ACK.

laanwj · 2019-01-02T11:30:30Z

src/net.h

@@ -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.


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.

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 · 2019-01-02T11:30:39Z

Concept ACK

sdaftuar · 2019-01-07T19:13:45Z

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

Looks reasonable, will test.

sdaftuar · 2019-01-07T20:10:44Z

src/net.cpp

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

    if (vEvictionCandidates.empty()) return false;

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


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?

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.

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?

sdaftuar · 2019-01-07T20:17:55Z

src/net.cpp

+
+    // 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))


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

gmaxwell · 2019-01-07T20:23:19Z

@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 · 2019-01-09T16:30:56Z

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 · 2019-01-10T20:20:11Z

@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 · 2019-01-11T20:29:24Z

utACK 006a07de4e5c3de36ef84d186315a4a5f23da158

maflcko · 2019-01-14T15:40:38Z

src/net.cpp

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


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

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

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

naumenkogs · 2019-01-16T17:46:45Z

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 · 2019-01-16T18:43:28Z

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

naumenkogs · 2019-01-16T19:04:59Z

@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.

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 · 2019-01-24T19:40:49Z

@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.

sdaftuar · 2019-01-24T19:50:12Z

utACK 0297be6

Sjors

Concept ACK

Sjors · 2019-01-24T19:58:00Z

src/banman.cpp

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


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

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

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

jonasschnelli · 2019-01-24T19:59:40Z

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

naumenkogs · 2019-01-24T20:00:05Z

utACK 0297be6

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

Summary: 0297be61a 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. --- This is a backport from Core PR14929 (bitcoin/bitcoin#14929) Test Plan: ninja check cmake --build . --config Release --target check-functional -- -j 6 Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, nakihito Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, nakihito Subscribers: nakihito Differential Revision: https://reviews.bitcoinabc.org/D4759

2ad5838 Clean up separated ban/discourage interface (Pieter Wuille) b691f2d Replace automatic bans with discouragement filter (Pieter Wuille) Pull request description: This patch improves performance and resource usage around IP addresses that are banned for misbehavior. They're already not actually banned since #14929, as connections from them are still allowed, but they are preferred for eviction if the inbound connection slots are full. Stop treating these like manually banned IP ranges, and instead just keep them in a rolling Bloom filter of misbehaving nodes, which isn't persisted to disk or exposed through the ban framework. The effect remains the same: preferred for eviction, avoided for outgoing connections, and not relayed to other peers. Also change the name of this mechanism to "discouraged" to better reflect reality. ACKs for top commit: naumenkogs: utACK 2ad5838 amitiuttarwar: code review ACK 2ad5838 jonatack: ACK 2ad5838 per changes since last review `git range-diff 3276c14 1f7e0ca 2ad5838` jnewbery: Code review ACK 2ad5838 Tree-SHA512: 5dedef401d9cbfa026812651303e6286223563dbeed7a10766ed536ac9e3f29ed4bd0df29cc6deadceeb35cbe9f066346add14ef0833958ca9f93d123fe7aab5

2ad5838 Clean up separated ban/discourage interface (Pieter Wuille) b691f2d Replace automatic bans with discouragement filter (Pieter Wuille) Pull request description: This patch improves performance and resource usage around IP addresses that are banned for misbehavior. They're already not actually banned since bitcoin#14929, as connections from them are still allowed, but they are preferred for eviction if the inbound connection slots are full. Stop treating these like manually banned IP ranges, and instead just keep them in a rolling Bloom filter of misbehaving nodes, which isn't persisted to disk or exposed through the ban framework. The effect remains the same: preferred for eviction, avoided for outgoing connections, and not relayed to other peers. Also change the name of this mechanism to "discouraged" to better reflect reality. ACKs for top commit: naumenkogs: utACK 2ad5838 amitiuttarwar: code review ACK 2ad5838 jonatack: ACK 2ad5838 per changes since last review `git range-diff 3276c14 1f7e0ca 2ad5838` jnewbery: Code review ACK 2ad5838 Tree-SHA512: 5dedef401d9cbfa026812651303e6286223563dbeed7a10766ed536ac9e3f29ed4bd0df29cc6deadceeb35cbe9f066346add14ef0833958ca9f93d123fe7aab5

…eers 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

promag reviewed Dec 11, 2018

View reviewed changes

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 Show resolved Hide resolved

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

maflcko added the P2P label Dec 11, 2018

maflcko added this to the 0.18.0 milestone Dec 11, 2018

maflcko added the Needs gitian build label Dec 12, 2018

practicalswift reviewed Dec 12, 2018

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

promag reviewed Dec 12, 2018

View reviewed changes

DrahtBot removed the Needs gitian build label Dec 14, 2018

bitcoin deleted a comment from DrahtBot Dec 14, 2018

laanwj reviewed Jan 2, 2019

View reviewed changes

sdaftuar reviewed Jan 7, 2019

View reviewed changes

DrahtBot added the Needs rebase label Jan 14, 2019

maflcko reviewed Jan 14, 2019

View reviewed changes

DrahtBot removed the Needs rebase label Jan 15, 2019

DrahtBot added the Needs rebase label Jan 21, 2019

DrahtBot removed the Needs rebase label Jan 22, 2019

Sjors reviewed Jan 24, 2019

View reviewed changes

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

sipa mentioned this pull request Jun 9, 2020

Replace automatic bans with discouragement filter #19219

Merged

str4d mentioned this pull request Nov 16, 2020

banlist.dat: store banlist on disk zcash/zcash#2812

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

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

		if (vEvictionCandidates.empty()) return false;

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

net: Allow connections from misbehavior banned peers #14929

net: Allow connections from misbehavior banned peers #14929

Uh oh!

Conversation

gmaxwell commented Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented Dec 11, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell commented Dec 31, 2018

Uh oh!

sdaftuar commented Dec 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jan 2, 2019

Uh oh!

sdaftuar commented Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdaftuar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell commented Jan 7, 2019

Uh oh!

sdaftuar commented Jan 9, 2019

Uh oh!

gmaxwell commented Jan 10, 2019

Uh oh!

sipa commented Jan 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naumenkogs commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented Jan 16, 2019

Uh oh!

naumenkogs commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented Jan 24, 2019

Uh oh!

sdaftuar commented Jan 24, 2019

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

gmaxwell commented Dec 11, 2018 •

edited

Loading

DrahtBot commented Dec 11, 2018 •

edited

Loading

sdaftuar commented Jan 7, 2019 •

edited

Loading

gmaxwell Jan 15, 2019 •

edited

Loading

naumenkogs commented Jan 16, 2019 •

edited

Loading

naumenkogs commented Jan 16, 2019 •

edited

Loading