Introduce BanMan

theuni commented at 11:14 pm on October 5, 2017: member

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.

fanquake added the label P2P on Oct 5, 2017

theuni force-pushed on Oct 6, 2017

in src/banman.h:58 in a006ca20f2 outdated

53+    //!clean unused entries (if bantime has expired)
54+    void SweepBanned();
55+
56+    banmap_t m_banned;
57+    CCriticalSection m_cs_banned;
58+    bool m_is_dirty;

practicalswift commented at 4:32 am on October 6, 2017:

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

Like this:

0    CCriticalSection m_cs_banned;
1    banmap_t m_banned GUARDED_BY(m_cs_banned);
2    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).

theuni commented at 8:15 pm on October 6, 2017:

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.

theuni commented at 8:27 pm on October 6, 2017:

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 commented at 4:33 am on October 6, 2017: contributor

Concept ACK modulo GUARDED_BY(…) addition :-)

JeremyRubin commented at 2:59 pm on October 6, 2017: contributor

Concept Ack!

in src/rpc/net.cpp:545 in ac90cbc5c1 outdated

540@@ -541,7 +541,13 @@ UniValue setban(const JSONRPCRequest& request)
541         if (request.params[3].isTrue())
542             absolute = true;
543 
544-        isSubnet ? g_connman->Ban(subNet, BanReasonManuallyAdded, banTime, absolute) : g_connman->Ban(netAddr, BanReasonManuallyAdded, banTime, absolute);
545+        if (isSubnet) {
546+            g_connman->DisconnectNode(subNet);

TheBlueMatt commented at 7:57 pm on October 6, 2017:

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?

theuni commented at 8:48 pm on October 6, 2017:

Yes, good point. Will fix.

promag commented at 10:09 pm on October 6, 2017:

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

theuni commented at 10:50 pm on October 6, 2017:

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

in src/addrdb.h:97 in a006ca20f2 outdated

93@@ -94,7 +94,7 @@ class CBanDB
94 private:
95     fs::path pathBanlist;
96 public:
97-    CBanDB();
98+    CBanDB(fs::path pathBanlist);

jimpo commented at 7:59 pm on October 6, 2017:

Make explicit?

theuni commented at 8:16 pm on October 6, 2017:

Will do.

in src/banman.h:22 in a006ca20f2 outdated

17+class CSubNet;
18+
19+class BanMan
20+{
21+public:
22+    // Denial-of-service detection/prevention

jimpo commented at 8:01 pm on October 6, 2017:

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

theuni commented at 8:16 pm on October 6, 2017:

Sure

jimpo commented at 4:59 pm on April 2, 2018:

BanMan still needs class-level comment.

jimpo commented at 8:02 pm on October 6, 2017: contributor

Concept ACK

TheBlueMatt commented at 8:37 pm on October 6, 2017: member

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

in src/banman.h:66 in a006ca20f2 outdated

60+    CBanDB m_ban_db;
61+    int64_t m_default_ban_time;
62+};
63+
64+extern std::unique_ptr<BanMan> g_banman;
65+#endif

promag commented at 9:47 pm on October 6, 2017:

0#endif // BITCOIN_BANMAN_H

in src/init.cpp:1276 in a006ca20f2 outdated

1272@@ -1267,11 +1273,13 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1273     // is not yet setup and may end up being set up twice if we
1274     // need to reindex later.
1275 
1276+    assert(!g_banman);
1277+    g_banman = std::unique_ptr<BanMan>(new BanMan(GetDataDir() / "banlist.dat", &uiInterface, DEFAULT_MISBEHAVING_BANTIME));

promag commented at 9:49 pm on October 6, 2017:

Use std::make_unique.

theuni commented at 10:28 pm on October 6, 2017:

that’s c++14 :(

practicalswift commented at 7:29 pm on October 9, 2017:

A substitute for C++14 std::make_unique in the form of MakeUnique is added to util.h as part of #11043 (see https://github.com/bitcoin/bitcoin/pull/11043/commits/27a54eacc07325e9a10cbea71ab03d87a96679b7) which will hopefully be merged soon :-)

in src/test/DoS_tests.cpp:205 in 027b8d3918 outdated

43@@ -44,6 +44,8 @@ BOOST_FIXTURE_TEST_SUITE(DoS_tests, TestingSetup)
44 
45 BOOST_AUTO_TEST_CASE(DoS_banning)
46 {
47+    auto connman = std::unique_ptr<CConnman>(new CConnman(0x1337, 0x1337));

promag commented at 10:10 pm on October 6, 2017:

Use std::make_unique.

ryanofsky commented at 6:20 pm on January 4, 2018:

Use std::make_unique.

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

promag commented at 10:14 pm on October 6, 2017: member

Partial review. Concept ACK.

jonasschnelli commented at 4:28 am on October 7, 2017: contributor

Nice! Concept ACK,.. will review soon.

in src/banman.h:41 in 7a574787c8 outdated

36+public:
37+    ~BanMan();
38+    BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time);
39+    void Ban(const CNetAddr& netAddr, const BanReason& reason, int64_t bantimeoffset = 0, bool sinceUnixEpoch = false);
40+    void Ban(const CSubNet& subNet, const BanReason& reason, int64_t bantimeoffset = 0, bool sinceUnixEpoch = false);
41+    void ClearBanned(); // needed for unit testing

Sjors commented at 9:50 am on October 7, 2017:

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

theuni commented at 8:01 pm on October 9, 2017:

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

theuni force-pushed on Oct 18, 2017

theuni commented at 4:40 am on October 18, 2017: member

Rebased and addressed most of the feedback here.

in src/qt/rpcconsole.cpp:1167 in 5226fd1c0c outdated

1172+
1173+        // Find possible nodes, ban it and clear the selected node
1174+        const CNodeCombinedStats *stats = clientModel->getPeerTableModel()->getNodeStats(detailNodeRow);
1175+        if(stats) {
1176+            g_connman->Ban(stats->nodeStats.addr, BanReasonManuallyAdded, bantime);
1177+            g_connman->DisconnectNode(id);

sdaftuar commented at 12:33 pm on October 18, 2017:

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)?

theuni commented at 4:49 pm on October 18, 2017:

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?

in src/net.cpp:45 in fc8eeeca4b outdated

42-#define DUMP_ADDRESSES_INTERVAL 900
43+// Dump addresses to peers.dat every 15 minutes (900s)
44+static const int DUMP_PEERS_INTERVAL = 60 * 15;
45+
46+// Dump addresses to banlist.dat every 15 minutes (900s)
47+static const int DUMP_BANS_INTERVAL = 60 * 15;

sdaftuar commented at 12:45 pm on October 18, 2017:

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

theuni commented at 4:37 pm on October 18, 2017:

Yep, will change.

in src/init.cpp:77 in b435a7995d outdated

71@@ -72,8 +72,12 @@ static const bool DEFAULT_PROXYRANDOMIZE = true;
72 static const bool DEFAULT_REST_ENABLE = false;
73 static const bool DEFAULT_STOPAFTERBLOCKIMPORT = false;
74 
75+// Dump addresses to banlist.dat every 15 minutes (900s)
76+static const int DUMP_BANS_INTERVAL = 60 * 15;

sdaftuar commented at 12:47 pm on October 18, 2017:

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

in src/init.cpp:1281 in b435a7995d outdated

1276     assert(!g_connman);
1277     g_connman = std::unique_ptr<CConnman>(new CConnman(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max())));
1278     CConnman& connman = *g_connman;
1279 
1280-    peerLogic.reset(new PeerLogicValidation(&connman));
1281+    peerLogic.reset(new PeerLogicValidation(g_connman.get(), g_banman.get()));

sdaftuar commented at 12:52 pm on October 18, 2017:

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?

theuni commented at 4:35 pm on October 18, 2017:

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.

in src/init.cpp:1733 in b435a7995d outdated

1729@@ -1722,5 +1730,11 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1730     StartWallets(scheduler);
1731 #endif
1732 
1733+    if (g_banman) {

sdaftuar commented at 12:56 pm on October 18, 2017:

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.

theuni commented at 4:37 pm on October 18, 2017:

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.

in src/net.cpp:2231 in b435a7995d outdated

2412@@ -2405,7 +2413,9 @@ void CConnman::Stop()
2413     if (fAddressesInitialized)
2414     {
2415         DumpAddresses();
2416-        DumpBanlist();
2417+        if (m_banman) {
2418+            m_banman->DumpBanlist();

sdaftuar commented at 1:03 pm on October 18, 2017:

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

theuni commented at 4:38 pm on October 18, 2017:

Good point, will remove.

in src/rpc/net.cpp:514 in b435a7995d outdated

508@@ -509,8 +509,8 @@ UniValue setban(const JSONRPCRequest& request)
509                             + HelpExampleCli("setban", "\"192.168.0.0/24\" \"add\"")
510                             + HelpExampleRpc("setban", "\"192.168.0.6\", \"add\", 86400")
511                             );
512-    if(!g_connman)
513-        throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
514+    if(!g_banman)
515+        throw JSONRPCError(RPC_DATABASE_ERROR, "Error: Ban database not loaded");

sdaftuar commented at 3:44 pm on October 18, 2017:

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

theuni commented at 4:59 pm on October 18, 2017:

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.

in src/qt/rpcconsole.cpp:1170 in b435a7995d outdated

1167-            g_connman->DisconnectNode(id);
1168+            if (g_banman) {
1169+                g_banman->Ban(stats->nodeStats.addr, BanReasonManuallyAdded, bantime);
1170+            }
1171+            if (g_connman) {
1172+                g_connman->DisconnectNode(id);

sdaftuar commented at 3:45 pm on October 18, 2017:

Same comment as before about using nodeid rather than address

theuni commented at 4:56 pm on October 18, 2017:

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.

in src/rpc/net.cpp:579 in b435a7995d outdated

573@@ -570,11 +574,11 @@ UniValue listbanned(const JSONRPCRequest& request)
574                             + HelpExampleRpc("listbanned", "")
575                             );
576 
577-    if(!g_connman)
578-        throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
579+    if(!g_banman)
580+        throw JSONRPCError(RPC_DATABASE_ERROR, "Error: Ban database not loaded");

sdaftuar commented at 3:49 pm on October 18, 2017:

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

theuni commented at 5:00 pm on October 18, 2017:

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.

in src/init.cpp:1275 in 8009f1d587 outdated

1271@@ -1272,7 +1272,7 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1272     // need to reindex later.
1273 
1274     assert(!g_banman);
1275-    g_banman = std::unique_ptr<BanMan>(new BanMan(GetDataDir() / "banlist.dat", &uiInterface));
1276+    g_banman = std::unique_ptr<BanMan>(new BanMan(GetDataDir() / "banlist.dat", &uiInterface, DEFAULT_MISBEHAVING_BANTIME));

sdaftuar commented at 4:01 pm on October 18, 2017:

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.

theuni commented at 4:40 pm on October 18, 2017:

Very much so, thanks for catching that.

sdaftuar commented at 4:14 pm on October 18, 2017: member

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

theuni force-pushed on Oct 19, 2017

theuni commented at 6:02 pm on October 19, 2017: member

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.

theuni force-pushed on Oct 19, 2017

TheBlueMatt commented at 6:59 pm on November 13, 2017: member

Concept ACK, needs rebase.

theuni force-pushed on Nov 14, 2017

theuni commented at 9:31 pm on November 14, 2017: member

rebased.

in src/net.cpp:2369 in 5f25e6ac15 outdated

2552@@ -2559,6 +2553,25 @@ bool CConnman::DisconnectNode(const std::string& strNode)
2553     }
2554     return false;
2555 }
2556+
2557+bool CConnman::DisconnectNode(const CSubNet& subnet)

promag commented at 10:42 pm on November 14, 2017:

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

Why bool if the return value is not used?

theuni commented at 11:41 pm on November 14, 2017:

Just to be consistent with the others.

in src/qt/rpcconsole.cpp:1148 in 5f25e6ac15 outdated

1153-	if(stats) {
1154-	    g_connman->Ban(stats->nodeStats.addr, BanReasonManuallyAdded, bantime);
1155-	}
1156+        // Get currently selected peer address
1157+        int detailNodeRow = clientModel->getPeerTableModel()->getRowByNodeId(id);
1158+        if(detailNodeRow < 0)

promag commented at 10:42 pm on November 14, 2017:

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

Nit, add space.

theuni commented at 11:50 pm on November 14, 2017:

Fixed

promag commented at 10:51 pm on November 14, 2017: member

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

Missing call DisconnectNode() after:

https://github.com/bitcoin/bitcoin/blob/3bdf242fc68a8d767932c6214455d4d413effbc9/src/net_processing.cpp#L2844

theuni force-pushed on Nov 14, 2017

theuni commented at 11:51 pm on November 14, 2017: member

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

Missing call DisconnectNode() after:

This is handled by setting fDisconnect directly above.

TheBlueMatt commented at 4:59 pm on November 16, 2017: member

Heh, rebase needed for the include change.

theuni force-pushed on Nov 16, 2017

fanquake added this to the "In progress" column in a project

laanwj added this to the "Blockers" column in a project

laanwj commented at 8:19 am on November 28, 2017: member

I’ve added this to high-priority for review, would be nice to move this forward and this blocks further work in this direction.

theuni commented at 9:35 pm on November 28, 2017: member

@laanwj Thanks!

TheBlueMatt commented at 5:06 pm on December 13, 2017: member

Needs rebase.

theuni force-pushed on Dec 13, 2017

in src/net.cpp:559 in 04e692c529 outdated

561-        for (CNode* pnode : vNodes) {
562-            if (subNet.Match((CNetAddr)pnode->addr))
563-                pnode->fDisconnect = true;
564-        }
565-    }
566+

TheBlueMatt commented at 4:52 pm on December 14, 2017:

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.

theuni commented at 0:34 am on December 19, 2017:

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

ryanofsky commented at 6:00 pm on January 4, 2018:

#11457 (review)

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.

in src/init.cpp:1769 in 04d0b51af1 outdated

1733@@ -1726,5 +1734,9 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1734     StartWallets(scheduler);
1735 #endif
1736 
1737+    scheduler.scheduleEvery([]{

TheBlueMatt commented at 4:57 pm on December 14, 2017:

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

theuni commented at 0:52 am on December 19, 2017:

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.

TheBlueMatt commented at 3:32 am on December 19, 2017:

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.

ryanofsky commented at 6:36 pm on January 4, 2018:

#11457 (review)

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 commented at 3:46 am on January 10, 2018:

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

in src/net.cpp:2283 in 04d0b51af1 outdated

2295-    } else {
2296-        LogPrintf("Invalid or missing banlist.dat; recreating\n");
2297-        SetBannedSetDirty(true); // force write
2298-        DumpBanlist();
2299-    }
2300+    nStart = GetTimeMillis();

TheBlueMatt commented at 4:59 pm on December 14, 2017:

This seems like a dead store?

theuni commented at 0:36 am on December 19, 2017:

Yes, will remove.

in src/rpc/net.cpp:549 in 04d0b51af1 outdated

543@@ -544,16 +544,20 @@ UniValue setban(const JSONRPCRequest& request)
544             absolute = true;
545 
546         if (isSubnet) {
547-            g_connman->Ban(subNet, BanReasonManuallyAdded, banTime, absolute);
548-            g_connman->DisconnectNode(subNet);
549+            g_banman->Ban(subNet, BanReasonManuallyAdded, banTime, absolute);
550+            if (g_connman) {

TheBlueMatt commented at 5:04 pm on December 14, 2017:

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?

theuni commented at 0:40 am on December 19, 2017:

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

in src/net.h:135 in c4c300849c outdated

131@@ -132,7 +132,7 @@ class BanMan
132     // between nodes running old code and nodes running
133     // new code.
134     ~BanMan();
135-    BanMan(fs::path ban_file, CClientUIInterface* client_interface);
136+    BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time);

TheBlueMatt commented at 5:07 pm on December 14, 2017:

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

theuni commented at 0:38 am on December 19, 2017:

So that we can test independent of the cmdline args.

TheBlueMatt commented at 5:07 pm on December 14, 2017: member

Nice.

theuni force-pushed on Dec 19, 2017

theuni commented at 1:41 am on December 19, 2017: member

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

 0diff --git a/src/net.cpp b/src/net.cpp
 1index 78c08b1..46e7783 100644
 2--- a/src/net.cpp
 3+++ b/src/net.cpp
 4@@ -2125,4 +2125,2 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
 5 
 6-    nStart = GetTimeMillis();
 7-
 8     uiInterface.InitMessage(_("Starting network threads..."));
 9diff --git a/src/net_processing.cpp b/src/net_processing.cpp
10index 2981413..e8d731c 100644
11--- a/src/net_processing.cpp
12+++ b/src/net_processing.cpp
13@@ -2852,3 +2852,2 @@ bool PeerLogicValidation::SendRejectsAndCheckIfBanned(CNode* pnode)
14         else {
15-            pnode->fDisconnect = true;
16             if (pnode->addr.IsLocal())
17@@ -2861,2 +2860,4 @@ bool PeerLogicValidation::SendRejectsAndCheckIfBanned(CNode* pnode)
18             }
19+            // Disconnect all nodes with this address
20+            connman->DisconnectNode(pnode->addr);
21         }

in src/net.cpp:2559 in b8d5dd5751 outdated

2554+bool CConnman::DisconnectNode(const CSubNet& subnet)
2555+{
2556+    bool disconnected = false;
2557+    LOCK(cs_vNodes);
2558+    for (CNode* pnode : vNodes) {
2559+        if (subnet.Match((CNetAddr)pnode->addr)) {

ryanofsky commented at 5:14 pm on January 4, 2018:

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.

theuni commented at 3:49 am on January 10, 2018:

sure

in src/net_processing.cpp:2922 in b8d5dd5751 outdated

2855             else
2856             {
2857                 connman->Ban(pnode->addr, BanReasonNodeMisbehaving);
2858             }
2859+            // Disconnect all nodes with this address
2860+            connman->DisconnectNode(pnode->addr);

ryanofsky commented at 6:09 pm on January 4, 2018:

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.

theuni commented at 4:06 am on January 10, 2018:

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

in src/test/DoS_tests.cpp:75 in 25c27594bf outdated

67@@ -54,6 +68,9 @@ BOOST_FIXTURE_TEST_SUITE(DoS_tests, TestingSetup)
68 // work.
69 BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
70 {
71+    auto connman = std::unique_ptr<CConnman>(new CConnman(0x1337, 0x1337));

ryanofsky commented at 6:16 pm on January 4, 2018:

In commit “tests: remove member connman/peerLogic in TestingSetup”

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

theuni commented at 4:12 am on January 10, 2018:

will do

in src/net_processing.cpp:2856 in 8500946d62 outdated

2852@@ -2853,7 +2853,9 @@ static bool SendRejectsAndCheckIfBanned(CNode* pnode, CConnman* connman)
2853                 LogPrintf("Warning: not banning local peer %s!\n", pnode->addr.ToString());
2854             else
2855             {
2856-                connman->Ban(pnode->addr, BanReasonNodeMisbehaving);
2857+                if (m_banman) {

jimpo commented at 7:18 pm on January 4, 2018:

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.

theuni commented at 3:53 am on January 10, 2018:

good point, will fix.

in src/qt/rpcconsole.cpp:1247 in 8500946d62 outdated

1203@@ -1200,9 +1204,9 @@ void RPCConsole::unbanSelectedNode()
1204         CSubNet possibleSubnet;
1205 
1206         LookupSubNet(strNode.toStdString().c_str(), possibleSubnet);
1207-        if (possibleSubnet.IsValid() && g_connman)
1208+        if (possibleSubnet.IsValid() && g_banman)

jimpo commented at 7:23 pm on January 4, 2018:

Commit [banman: create and split out banman].

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

theuni commented at 4:07 am on January 10, 2018:

Yep, will fix.

in src/rpc/net.cpp:554 in 8500946d62 outdated

552+            }
553         } else {
554-            g_connman->Ban(netAddr, BanReasonManuallyAdded, banTime, absolute);
555-            g_connman->DisconnectNode(netAddr);
556+            g_banman->Ban(netAddr, BanReasonManuallyAdded, banTime, absolute);
557+            if (g_connman) {

jimpo commented at 7:25 pm on January 4, 2018:

Commit [banman: create and split out banman].

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

theuni commented at 4:11 am on January 10, 2018:

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 commented at 7:57 pm on January 4, 2018: member

utACK 8c91ffe777ffcd2724f576490068abcbb95d60c0

laanwj removed this from the "Blockers" column in a project

MarcoFalke deleted a comment on Mar 19, 2018

theuni force-pushed on Mar 29, 2018

theuni commented at 8:06 pm on March 29, 2018: member

Rebased with only trivial changes, with one exception: https://github.com/bitcoin/bitcoin/pull/11457/commits/2335579f842607557a4742d2ec0749e459270bd0.

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 commented at 9:47 pm on March 29, 2018: member

utACK 0950d72

Sjors commented at 9:06 am on March 30, 2018: member

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

net: Break disconnecting out of Ban()

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.

95e4772be1

tests: remove member connman/peerLogic in TestingSetup fc95465852

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

banman: create and split out banman

Some say he has always been.

5b5327f93c

banman: pass the banfile path in

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

020daff1e4

banman: pass in default ban time as a parameter

Removes the dependency on arg parsing.

03c483e550

net: move BanMan to its own files 7b6ec1bdcf

scripted-diff: batch-rename BanMan members

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

dd3ba250ef

banman: add thread annotations and mark members const where possible

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

521ebf37f4

theuni force-pushed on Apr 19, 2018

theuni commented at 11:55 pm on April 19, 2018: member

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

jimpo commented at 8:26 am on April 20, 2018: contributor

utACK 521ebf3

MarcoFalke added the label Needs rebase on Jun 6, 2018

fanquake closed this on Oct 30, 2018

fanquake removed this from the "In progress" column in a project

laanwj referenced this in commit 5baa9092c4 on Jan 21, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

PastaPastaPasta referenced this in commit 38ee2a7a94 on Jul 18, 2021

PastaPastaPasta referenced this in commit 7cf87fe383 on Jul 18, 2021

MarcoFalke locked this on Dec 16, 2021

Introduce BanMan #11457