Introduce BanMan #11457

theuni · 2017-10-05T23:14:47Z

BanMan is not a bad Man, he's just a different class of Man. He's an ex-ConnMan Man, looking to take a different path and make a new namespace for himself.

Beware BanMan's banhammer as he banishes bandits with the bandwidth to jam him. Not standing for non-canon, he’ll ban and he’ll ban. Some heroes wear capes, but BanMan? A Bandana.

--

Despite the diff size, this is mostly move-only. It breaks the ban/unban functions out of CConnman and into a new class because, while logically bans are tied to connections, they're really just entries in a database. Like CConnman, a global is still required due to RPC (and qt). I plan to address this along with CAddrMan, which I'll be breaking out next.

This also makes testing easier as different implementations can be dropped in.

There are a few small behavioral changes here, which are pretty insignificant:

Banning no longer implies disconnecting. If you want both, you need to call both.
For simplicity, The ban db is read in the constructor, meaning that it happens in init rather than net.
RPC returns RPC_DATABASE_ERROR if the bandb is not loaded. This should not be possible now, but the idea is for future changes to allow us to disable p2p but still interact with the ban commands.

practicalswift · 2017-10-06T04:32:01Z

src/banman.h

What about adding GUARDED_BY(…) (see #10866 and #11226) annotations here?

Like this:

CCriticalSection m_cs_banned; banmap_t m_banned GUARDED_BY(m_cs_banned); bool m_is_dirty GUARDED_BY(m_cs_banned);

Nit: The lock was renamed from cs_setBanned to m_cs_banned. All CCriticalSection:s in the code base except m_cs_callbacks_pending are named with the cs_ prefix (as opposed to m_cs_). Is m_cs_ the recommended prefix going forward? More specifically, what prefix is recommended for my work in #10866? :-)

I think there is a point in having consistent naming for the locks in order to allow for easy grepping of the locks. Personally I use grepping frequently (e.g. git grep ' cs_[a-zA-Z0-9]\+;' -- "*.h" "*.cpp") in the scripts that keep tracks of the locks to cover in #10866).

See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md:

Class member variables have a m_ prefix.

So yes, m_cs_foo is the recommended syntax going forward for member mutexes, I suppose.

What about adding GUARDED_BY(…) (see #10866 and #11226) annotations here?

Will do. Additionally, this actually needs another mutex for the db. I've been debating whether it should be fixed as part of this PR, but event-driven db flushes can actually collide with the one on a timer. addrman has the same issue, and that may be the cause of some addrman corruption we've seen lately (#11252, #11409, #11454).

practicalswift · 2017-10-06T04:33:36Z

Concept ACK modulo GUARDED_BY(…) addition :-)

JeremyRubin · 2017-10-06T14:59:54Z

Concept Ack!

jimpo

Concept ACK

jimpo · 2017-10-06T19:59:51Z

src/addrdb.h

Make explicit?

jimpo · 2017-10-06T20:01:30Z

src/banman.h

Should this be a class-level comment? (Like placed right above class BanMan)

BanMan still needs class-level comment.

TheBlueMatt

Looking godd, will give it a full review when it quiets down.

TheBlueMatt · 2017-10-06T19:57:37Z

src/rpc/net.cpp

Shouldnt these technically be in the opposite order? Isnt there otherwise a (supidly rare) race where a new connection can come in and slip behind the ban?

Yes, good point. Will fix.

Inverting opens another dumb case: ban -> (unban -> connect) -> disconnect 🙄 Do you think DisconnectNode() should be called inside Ban() while the lock is held?

I don't see how this could reasonably happen. Anyway, it's not a departure from the current behavior.

promag

Partial review. Concept ACK.

promag · 2017-10-06T21:47:03Z

src/banman.h

#endif // BITCOIN_BANMAN_H

promag · 2017-10-06T21:49:13Z

src/init.cpp

Use std::make_unique.

that's c++14 :(

A substitute for C++14 std::make_unique in the form of MakeUnique is added to util.h as part of #11043 (see 27a54ea) which will hopefully be merged soon :-)

promag · 2017-10-06T22:09:14Z

src/rpc/net.cpp

Inverting opens another dumb case: ban -> (unban -> connect) -> disconnect 🙄 Do you think DisconnectNode() should be called inside Ban() while the lock is held?

promag · 2017-10-06T22:10:28Z

src/test/DoS_tests.cpp

Use std::make_unique.

Use std::make_unique.

It's not available in c++11, but we do have MakeUnique in src/util.h

jonasschnelli · 2017-10-07T04:28:53Z

Nice!
Concept ACK,.. will review soon.

Sjors · 2017-10-07T09:50:54Z

src/banman.h

Would it make sense to move ClearBanned() to a subclass that's only used by the test suite?

This is a misleading comment that needs to be dropped. This is used by the clearbanned rpc as well.

theuni · 2017-10-18T04:40:35Z

Rebased and addressed most of the feedback here.

sdaftuar

Concept ACK. Left a few comments/questions, but looks pretty good. I still need to verify the move-only-ness of a758cebfc66e0a694506291d925f2ef156661654.

sdaftuar · 2017-10-18T12:33:50Z

src/qt/rpcconsole.cpp

I think we need to invoke the new DisconnectNode() method that takes a CNetAddr, rather than just the nodeid, in order to keep the behavior the same (ie disconnect all peers on the same subnet)?

When accepting an ip as a string from the user I agree. But in this case the request is for a specific node, so I would argue that the id is more in line with what the user would expect.

I don't care hugely either way though.

As an aside, what we really need here is kick/ban/kick+ban options. But I don't think it's worth adding the complexity?

sdaftuar · 2017-10-18T12:45:12Z

src/net.cpp

Is this a place where a constexpr is preferred over just const?

Yep, will change.

sdaftuar · 2017-10-18T12:47:32Z

src/init.cpp

(same question as before, about whether constexpr should be preferred?)

sdaftuar · 2017-10-18T12:52:28Z

src/init.cpp

Mostly an aside, since we're already doing it this way with connman, but I'm curious why we're not using shared pointers here instead of this unique_ptr.get() stuff?

Because these aren't intended to be actual globals. We just don't (yet) have the infrastructure to pass them into RPC and qt. Once banman/addrman are split out, I'll be PRing a set of changes that make RPC instantiated. With that done, the ui (rpc/qt/whatever else) will have these interfaces passed in, and they can assume that the pointers are good for their lifetime.

sdaftuar · 2017-10-18T12:56:05Z

src/init.cpp

Is there a circumstance where g_banman would not exist here? If so a comment would be nice; if not then I'm wondering if we should assert instead, or otherwise enforce a shutdown.

My original thinking was that if we add a -nop2p option (which I'm hoping we can do soon), the bandb would be disabled as well. But thinking about it now, one doesn't exclude the other. So yes, an assert makes more sense here. Will do.

sdaftuar · 2017-10-18T13:03:12Z

src/net.cpp

Why do we need this, if the BanMan already dumps its banlist in its destructor?

Good point, will remove.

sdaftuar · 2017-10-18T15:44:24Z

src/rpc/net.cpp

nit: Does this rpc error change require a release note mention? If so, we should track this in an issue.

I didn't bother with docs because as far as I know, it's been impossible to receive this error in practice because g_connman always exists. It won't be until we have a -nop2p that it's relevant.

sdaftuar · 2017-10-18T15:45:19Z

src/qt/rpcconsole.cpp

Same comment as before about using nodeid rather than address

Same response :)

Note that these comments are a bit stale. We used to track down the entry by looking through all node addresses, but now we use the id directly. I'll update those comments independently.

Also, see the behavior of disconnectSelectedNode above. I think it makes sense to be consistent with that?

For the sake of not over-complicating this, It may make sense to just keep the old behavior for this PR and switch to id in a follow-up.

sdaftuar · 2017-10-18T15:49:39Z

src/rpc/net.cpp

nit: Consider adding a release-note to do for the rpc error change in this rpc call as well.

Same as above. These could be asserts for now, but imo we're less likely to forget about these if errors are hooked up in advance.

sdaftuar · 2017-10-18T16:01:18Z

src/init.cpp

I think we need to pass gArgs.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME) here? Or else I'm missing how the command line argument gets used after this commit.

Very much so, thanks for catching that.

theuni · 2017-10-19T18:02:31Z

Addressed @sdaftuar's feedback. I went ahead and changed the Disconnect() in qt/rpcconsole back to using IP rather than nodeid to avoid adding unnecessary behavioral changes in a PR that should otherwise be a code refactor.

TheBlueMatt · 2017-11-13T18:59:47Z

Concept ACK, needs rebase.

theuni · 2017-11-14T21:31:42Z

rebased.

promag

In commit net: Break disconnecting out of Ban(),

Missing call DisconnectNode() after:

bitcoin/src/net_processing.cpp

Line 2844 in 3bdf242

connman->Ban(pnode->addr, BanReasonNodeMisbehaving);

promag · 2017-11-14T22:42:05Z

src/net.cpp

In commit net: Break disconnecting out of Ban(),

Why bool if the return value is not used?

Just to be consistent with the others.

promag · 2017-11-14T22:42:42Z

src/qt/rpcconsole.cpp

In commit net: Break disconnecting out of Ban(),

Nit, add space.

theuni · 2017-11-14T23:51:06Z

In commit net: Break disconnecting out of Ban(),

Missing call DisconnectNode() after:

This is handled by setting fDisconnect directly above.

TheBlueMatt

Nice.

TheBlueMatt · 2017-12-14T16:52:14Z

src/net.cpp

This is a behavior change when banning a remote IP in net_processing - we no longer disconnect any other connections from the same peer, only block new ones and disconnect the specific connection that made us ban them. Don't know that its a big deal, but might be nice to disconnect-by-ip there, too.

Good point. Disconnecting the single node seems more correct to me, but I'll revert to the current behavior here to avoid the change.

#11457 (comment)

In commit "net: Break disconnecting out of Ban()"

This is a behavior change

Seems to be fixed by the new change: #11457 (comment), but it would be clearer if commit message also said this commit does not change current behavior.

TheBlueMatt · 2017-12-14T16:57:12Z

src/init.cpp

Hmm, it seems like our trend is to move things like this into the constructor/init logic for the subsystem/module itself instead of shoving everything in init. I rather liked that trend....

Well the schedule in this case is bitcoind specific. There's no reason for BanMan to know about it, and it's not exclusive to net. Throwing in an app-specific schedule would kinda defeat the purpose of breaking it out.

Ugh, I mean the other annoying bit is that init.o is, by far, our largest compilation module. Any excuse to pull stuff out of init and put knowledge of a module in that module instead of init seems like the right direction to me. Also because we're telling the scheduler to schedule calling a function in that module based on a timer set in that module's header just seems...weird to me. If we want to ensure its easy to add unit tests later you can easily make the scheduler an optional arg and skip dumping if one isn't provided to the constructor.

#11457 (comment)

In commit "banman: create and split out banman"

Well the schedule in this case is bitcoind specific. There's no reason for BanMan to know about it, and it's not exclusive to net. Throwing in an app-specific schedule would kinda defeat the purpose of breaking it out.

I'm surprised the scheduler would be considered "bitcoind specific" / "app-specific" but BanMan would not be. How could that be true? On the surface, it seems reasonable for BanMan to use the scheduler since it relies on a scheduler to function, and would seem to simplify init.cpp and anything else using BanMan.

@ryanofsky BanMan doesn't rely on a scheduler to function any more than the wallet does, that's a bitcoind thing. Banman just loads/unloads the bandb when it's told.

Imagine a small util to dump/edit the bandb, (which would be completely reasonable). The scheduler would just be a needless dependency.

@TheBlueMatt It's time to create an app context class (and .h/cpp) anyway, that holds these types of objects for the life of bitcoind. I figured one of us would get around to that once enough stuff had piled up in init.cpp :p .

TheBlueMatt · 2017-12-14T16:59:21Z

src/net.cpp

This seems like a dead store?

Yes, will remove.

TheBlueMatt · 2017-12-14T17:04:57Z

src/rpc/net.cpp

Is it really worth checking? We already check for g_banman further up, are we really anticipating having a g_banman but not a g_connman?

Yea, I think it may be useful in the future to run with -p2p=0 in order to view/cleanup your bandb.

TheBlueMatt · 2017-12-14T17:07:42Z

src/net.h

I cant say I really understand the desire to avoid the argparsing dep, when you include <util.h> either way. Seems like useless indirection.

So that we can test independent of the cmdline args.

theuni · 2017-12-19T01:41:08Z

Addressed @TheBlueMatt's review and squashed. Diff from before (minus the rebase to master):

diff --git a/src/net.cpp b/src/net.cpp
index 78c08b1..46e7783 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2125,4 +2125,2 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
 
-    nStart = GetTimeMillis();
-
     uiInterface.InitMessage(_("Starting network threads..."));
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 2981413..e8d731c 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2852,3 +2852,2 @@ bool PeerLogicValidation::SendRejectsAndCheckIfBanned(CNode* pnode)
         else {
-            pnode->fDisconnect = true;
             if (pnode->addr.IsLocal())
@@ -2861,2 +2860,4 @@ bool PeerLogicValidation::SendRejectsAndCheckIfBanned(CNode* pnode)
             }
+            // Disconnect all nodes with this address
+            connman->DisconnectNode(pnode->addr);
         }

jimpo · 2018-01-04T19:18:34Z

src/net_processing.cpp

Commit [banman: create and split out banman].

I feel like this should be checked outside the surrounding if (pnode->addr.IsLocal()) statement. Doesn't make sense to log "not banning local peer" if it wouldn't have been banned anyway.

good point, will fix.

jimpo · 2018-01-04T19:23:04Z

src/qt/rpcconsole.cpp

Commit [banman: create and split out banman].

g_banman is guaranteed to exist given the check at the top of the function.

Yep, will fix.

jimpo · 2018-01-04T19:25:29Z

src/rpc/net.cpp

Commit [banman: create and split out banman].

Perhaps pull the common DisconnectNode code out of this if/else statement?

One is a subnet, the other is an address. CSubNet is a subclass of CNetAddr, so we could potentially store a generic one, but imo duplicating the disconnect is more straightforward.

ryanofsky

utACK 8c91ffe777ffcd2724f576490068abcbb95d60c0

ryanofsky · 2018-01-04T17:14:20Z

src/net.cpp

In commit "net: Break disconnecting out of Ban()"

Maybe drop this C cast. It doesn't do anything and could cause bugs if class definitions change.

ryanofsky · 2018-01-04T18:00:11Z

src/net.cpp

#11457 (comment)

In commit "net: Break disconnecting out of Ban()"

This is a behavior change

Seems to be fixed by the new change: #11457 (comment), but it would be clearer if commit message also said this commit does not change current behavior.

ryanofsky · 2018-01-04T18:09:51Z

src/net_processing.cpp

In commit "net: Break disconnecting out of Ban()"

Is this a behavior change in the IsLocal case? It would be helpful if commit message clarified whether this is supposed be a pure refactoring or a behavior change.

I'm not sure what the behavior change would be, so I guess not :)

ryanofsky · 2018-01-04T18:16:49Z

src/test/DoS_tests.cpp

In commit "tests: remove member connman/peerLogic in TestingSetup"

Could abbreviate auto connman = MakeUnique<CConnman>(0x1337, 0x1337);

ryanofsky · 2018-01-04T18:20:55Z

src/test/DoS_tests.cpp

Use std::make_unique.

It's not available in c++11, but we do have MakeUnique in src/util.h

ryanofsky · 2018-01-04T18:36:23Z

src/init.cpp

#11457 (comment)

In commit "banman: create and split out banman"

Well the schedule in this case is bitcoind specific. There's no reason for BanMan to know about it, and it's not exclusive to net. Throwing in an app-specific schedule would kinda defeat the purpose of breaking it out.

I'm surprised the scheduler would be considered "bitcoind specific" / "app-specific" but BanMan would not be. How could that be true? On the surface, it seems reasonable for BanMan to use the scheduler since it relies on a scheduler to function, and would seem to simplify init.cpp and anything else using BanMan.

theuni · 2018-03-29T20:06:20Z

Rebased with only trivial changes, with one exception: 2335579.

As @TheBlueMatt and @ryanofsky pointed out, it wasn't clear if any behavioral changes were introduced in SendRejectsAndCheckIfBanned, and it turned out that the new code indeed did not match master's behavior exactly, as pointed out by rpc tests.

That's now fixed and behavior should be exactly as before.

laanwj · 2018-03-29T21:47:00Z

utACK 0950d72

Sjors · 2018-03-30T09:06:35Z

Did some light testing on macOS banning and unbanning unsuspecting testnet nodes. Didn't get in trouble.

These are separate events which need to be carried out by separate subsystems. This also cleans up some whitespace and tabs in qt to avoid getting flagged by the linter. Current behavior is preserved.

Some say he has always been.

There's no need to hard-code the path here. Passing it in means that there are no ordering concerns wrt establishing the datadir.

Removes the dependency on arg parsing.

-BEGIN VERIFY SCRIPT- sed -i "s/clientInterface/m_client_interface/g" src/banman.h src/banman.cpp sed -i "s/setBannedIsDirty/m_is_dirty/g" src/banman.h src/banman.cpp sed -i "s/cs_setBanned/m_cs_banned/g" src/banman.h src/banman.cpp sed -i "s/setBanned/m_banned/g" src/banman.h src/banman.cpp -END VERIFY SCRIPT-

Also remove misleading comment. ClearBanned is used by rpc as well.

theuni · 2018-04-19T23:55:12Z

rebased. I just shoved g_banman into NodeImpl for now, but obviously we'll want it broken out as a next step.

jimpo · 2018-04-20T08:26:28Z

utACK 521ebf3

@theuni

18185b5 scripted-diff: batch-recase BanMan variables (Carl Dong) c2e04d3 banman: Add, use CBanEntry ctor that takes ban reason (Carl Dong) 1ffa4ce banman: reformulate nBanUtil calculation (Carl Dong) daae598 banman: add thread annotations and mark members const where possible (Cory Fields) 84fc3fb scripted-diff: batch-rename BanMan members (Cory Fields) af3503d net: move BanMan to its own files (Cory Fields) d0469b2 banman: pass in default ban time as a parameter (Cory Fields) 2e56702 banman: pass the banfile path in (Cory Fields) 4c0d961 banman: create and split out banman (Cory Fields) 83c1ea2 net: split up addresses/ban dumps in preparation for moving them (Cory Fields) 136bd79 tests: remove member connman/peerLogic in TestingSetup (Cory Fields) 7cc2b9f net: Break disconnecting out of Ban() (Cory Fields) Pull request description: **Old English à la Beowulf** ``` Banman wæs bréme --blaéd wíde sprang-- Connmanes eafera Coreum in. aéglaéca léodum forstandan Swá bealdode bearn Connmanes guma gúðum cúð gódum daédum· dréah æfter dóme· nealles druncne slóg ``` **Modern English Translation** ``` Banman was famed --his renown spread wide-- Conman's hier, in Core-land. against the evil creature defend the people Thus he was bold, the son of Connman man famed in war, for good deeds; he led his life for glory, never, having drunk, slew ``` -- With @theuni's blessing, here is Banman, rebased. Original PR: #11457 -- Followup PRs: 1. Give `CNode` a `Disconnect` method ([source](#14605 (comment))) 2. Add a comment to `std::atomic_bool fDisconnect` in `net.h` that setting this to true will cause the node to be disconnected the next time `DisconnectNodes()` runs ([source](#14605 (comment))) Tree-SHA512: 9c207edbf577415c22c9811113e393322d936a843d4ff265186728152a67c057779ac4d4f27b895de9729f7a53e870f828b9ebc8bcdab757520c2aebe1e9be35

@theuni

18185b5 scripted-diff: batch-recase BanMan variables (Carl Dong) c2e04d3 banman: Add, use CBanEntry ctor that takes ban reason (Carl Dong) 1ffa4ce banman: reformulate nBanUtil calculation (Carl Dong) daae598 banman: add thread annotations and mark members const where possible (Cory Fields) 84fc3fb scripted-diff: batch-rename BanMan members (Cory Fields) af3503d net: move BanMan to its own files (Cory Fields) d0469b2 banman: pass in default ban time as a parameter (Cory Fields) 2e56702 banman: pass the banfile path in (Cory Fields) 4c0d961 banman: create and split out banman (Cory Fields) 83c1ea2 net: split up addresses/ban dumps in preparation for moving them (Cory Fields) 136bd79 tests: remove member connman/peerLogic in TestingSetup (Cory Fields) 7cc2b9f net: Break disconnecting out of Ban() (Cory Fields) Pull request description: **Old English à la Beowulf** ``` Banman wæs bréme --blaéd wíde sprang-- Connmanes eafera Coreum in. aéglaéca léodum forstandan Swá bealdode bearn Connmanes guma gúðum cúð gódum daédum· dréah æfter dóme· nealles druncne slóg ``` **Modern English Translation** ``` Banman was famed --his renown spread wide-- Conman's hier, in Core-land. against the evil creature defend the people Thus he was bold, the son of Connman man famed in war, for good deeds; he led his life for glory, never, having drunk, slew ``` -- With @theuni's blessing, here is Banman, rebased. Original PR: bitcoin#11457 -- Followup PRs: 1. Give `CNode` a `Disconnect` method ([source](bitcoin#14605 (comment))) 2. Add a comment to `std::atomic_bool fDisconnect` in `net.h` that setting this to true will cause the node to be disconnected the next time `DisconnectNodes()` runs ([source](bitcoin#14605 (comment))) Tree-SHA512: 9c207edbf577415c22c9811113e393322d936a843d4ff265186728152a67c057779ac4d4f27b895de9729f7a53e870f828b9ebc8bcdab757520c2aebe1e9be35

theuni mentioned this pull request Oct 5, 2017

Add m_bantime to Connman options #11434

Closed

fanquake added the P2P label Oct 5, 2017

theuni force-pushed the move-bandb branch 3 times, most recently from 8f30300 to a006ca2 Compare October 6, 2017 01:32

practicalswift reviewed Oct 6, 2017

View reviewed changes

jimpo reviewed Oct 6, 2017

View reviewed changes

TheBlueMatt reviewed Oct 6, 2017

View reviewed changes

promag reviewed Oct 6, 2017

View reviewed changes

Sjors reviewed Oct 7, 2017

View reviewed changes

theuni force-pushed the move-bandb branch from 7a57478 to 7c7446a Compare October 18, 2017 03:44

sdaftuar reviewed Oct 18, 2017

View reviewed changes

theuni force-pushed the move-bandb branch 3 times, most recently from e335678 to 7a0452f Compare October 19, 2017 18:01

theuni force-pushed the move-bandb branch from 7a0452f to 94aa56f Compare October 19, 2017 18:10

This was referenced Oct 24, 2017

Evict outbound peers if tip is stale #11534

Closed

Disconnect outbound peers on invalid chains #11568

Merged

TheBlueMatt mentioned this pull request Nov 6, 2017

Peer is still banned, allthough ban endtime has been reached #11612

Closed

theuni force-pushed the move-bandb branch from 94aa56f to 1b5e70c Compare November 14, 2017 21:30

promag reviewed Nov 14, 2017

View reviewed changes

theuni force-pushed the move-bandb branch from 1b5e70c to 8fbcfd4 Compare November 14, 2017 23:50

TheBlueMatt reviewed Dec 14, 2017

View reviewed changes

theuni force-pushed the move-bandb branch from 5438a9b to 8c91ffe Compare December 19, 2017 01:40

jimpo reviewed Jan 4, 2018

View reviewed changes

ryanofsky reviewed Jan 4, 2018

View reviewed changes

bitcoin deleted a comment from mizhang Mar 19, 2018

theuni force-pushed the move-bandb branch from 8c91ffe to 0950d72 Compare March 29, 2018 19:43

theuni added 9 commits April 19, 2018 19:25

net: Break disconnecting out of Ban()

95e4772

These are separate events which need to be carried out by separate subsystems. This also cleans up some whitespace and tabs in qt to avoid getting flagged by the linter. Current behavior is preserved.

tests: remove member connman/peerLogic in TestingSetup

fc95465

net: split up addresses/ban dumps in preparation for moving them

1cd6d79

banman: create and split out banman

5b5327f

Some say he has always been.

banman: pass the banfile path in

020daff

There's no need to hard-code the path here. Passing it in means that there are no ordering concerns wrt establishing the datadir.

banman: pass in default ban time as a parameter

03c483e

Removes the dependency on arg parsing.

net: move BanMan to its own files

7b6ec1b

banman: add thread annotations and mark members const where possible

521ebf3

Also remove misleading comment. ClearBanned is used by rpc as well.

theuni force-pushed the move-bandb branch from 0950d72 to 521ebf3 Compare April 19, 2018 23:53

maflcko added the Needs rebase label Jun 6, 2018

dongcarl mentioned this pull request Oct 30, 2018

Return of the Banman #14605

Merged

fanquake closed this Oct 30, 2018

laanwj removed the Needs rebase label Oct 24, 2019

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

Introduce BanMan #11457

Introduce BanMan #11457

Uh oh!

Conversation

theuni commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift Oct 6, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Oct 6, 2017

Uh oh!

JeremyRubin commented Oct 6, 2017

Uh oh!

jimpo 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!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt 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!

promag 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!

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!

jonasschnelli commented Oct 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Oct 18, 2017

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!

Choose a reason for hiding this comment

theuni commented Oct 5, 2017 •

edited

Loading

practicalswift Oct 6, 2017 •

edited

Loading