Page MenuHomePhabricator

[backport] net: Allow connections from misbehavior banned peers
ClosedPublic

Authored by majcosta on Dec 18 2019, 19:00.

Details

Reviewers
deadalnix
nakihito
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABCf4ccfea4ad20: [backport] net: Allow connections from misbehavior banned peers
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 (https://github.com/bitcoin/bitcoin/pull/14929)

Test Plan
ninja check
cmake --build . --config Release --target check-functional -- -j 6

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

majcosta created this revision.Dec 18 2019, 19:00
Owners added a reviewer: Restricted Owners Package.Dec 18 2019, 19:00
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 18 2019, 19:00
nakihito requested changes to this revision.Dec 19 2019, 19:30
nakihito added a subscriber: nakihito.

Some nits.

src/banman.cpp
95 ↗(On Diff #14971)

Formatting should be:

if() {
  dostuff();
}
src/net.cpp
918 ↗(On Diff #14971)

There's an extra space at the beginning of this comment line you can remove.

This revision now requires changes to proceed.Dec 19 2019, 19:30
majcosta updated this revision to Diff 14997.Dec 19 2019, 20:10

Changed the if statement to obey coding style and removed spurious comment whitespace

nakihito accepted this revision.Dec 19 2019, 20:45
deadalnix accepted this revision.Dec 19 2019, 21:37

I love how this crap pretty much never comes with a test...

This revision is now accepted and ready to land.Dec 19 2019, 21:37

Just FYI, @Fabien and I talked a bit about this -- initially we both wondered whether we really want this (what's the point of banning, if they can just come back), but we both eventually figured it's probably good to do. As Greg says, fresh IP addrs are by no means in short supply...