Introduce BanMan #11457

pull theuni wants to merge 9 commits into bitcoin:master from theuni:move-bandb changing 18 files +443 −324
  1. 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.
  2. fanquake added the label P2P on Oct 5, 2017
  3. theuni force-pushed on Oct 6, 2017
  4. theuni force-pushed on Oct 6, 2017
  5. theuni force-pushed on Oct 6, 2017
  6. 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).

  7. practicalswift commented at 4:33 am on October 6, 2017: contributor
    Concept ACK modulo GUARDED_BY(…) addition :-)
  8. JeremyRubin commented at 2:59 pm on October 6, 2017: contributor
    Concept Ack!
  9. 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.
  10. 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.
  11. 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.
  12. jimpo commented at 8:02 pm on October 6, 2017: contributor
    Concept ACK
  13. TheBlueMatt commented at 8:37 pm on October 6, 2017: member
    Looking godd, will give it a full review when it quiets down.
  14. 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
    
  15. 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 :-)
  16. 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

  17. promag commented at 10:14 pm on October 6, 2017: member
    Partial review. Concept ACK.
  18. jonasschnelli commented at 4:28 am on October 7, 2017: contributor
    Nice! Concept ACK,.. will review soon.
  19. 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.
  20. theuni force-pushed on Oct 18, 2017
  21. theuni commented at 4:40 am on October 18, 2017: member
    Rebased and addressed most of the feedback here.
  22. 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?

  23. 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.
  24. 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?)
  25. 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.
  26. 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.
  27. 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.
  28. 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.
  29. 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.

  30. 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.
  31. 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.
  32. 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.
  33. theuni force-pushed on Oct 19, 2017
  34. theuni force-pushed on Oct 19, 2017
  35. theuni force-pushed on Oct 19, 2017
  36. 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.
  37. theuni force-pushed on Oct 19, 2017
  38. TheBlueMatt commented at 6:59 pm on November 13, 2017: member
    Concept ACK, needs rebase.
  39. theuni force-pushed on Nov 14, 2017
  40. theuni commented at 9:31 pm on November 14, 2017: member
    rebased.
  41. 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.
  42. 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
  43. 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

  44. theuni force-pushed on Nov 14, 2017
  45. 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.

  46. TheBlueMatt commented at 4:59 pm on November 16, 2017: member
    Heh, rebase needed for the include change.
  47. theuni force-pushed on Nov 16, 2017
  48. fanquake added this to the "In progress" column in a project

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

  50. 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.
  51. theuni commented at 9:35 pm on November 28, 2017: member
    @laanwj Thanks!
  52. TheBlueMatt commented at 5:06 pm on December 13, 2017: member
    Needs rebase.
  53. theuni force-pushed on Dec 13, 2017
  54. 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.

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

  56. 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.
  57. 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.
  58. 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.
  59. TheBlueMatt commented at 5:07 pm on December 14, 2017: member
    Nice.
  60. theuni force-pushed on Dec 19, 2017
  61. 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         }
    
  62. 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
  63. 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 :)
  64. 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
  65. 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.
  66. 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.
  67. 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.
  68. ryanofsky commented at 7:57 pm on January 4, 2018: member
    utACK 8c91ffe777ffcd2724f576490068abcbb95d60c0
  69. laanwj removed this from the "Blockers" column in a project

  70. MarcoFalke deleted a comment on Mar 19, 2018
  71. theuni force-pushed on Mar 29, 2018
  72. 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.

  73. laanwj commented at 9:47 pm on March 29, 2018: member
    utACK 0950d72
  74. 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.
  75. 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
  76. tests: remove member connman/peerLogic in TestingSetup fc95465852
  77. net: split up addresses/ban dumps in preparation for moving them 1cd6d79fa4
  78. banman: create and split out banman
    Some say he has always been.
    5b5327f93c
  79. 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
  80. banman: pass in default ban time as a parameter
    Removes the dependency on arg parsing.
    03c483e550
  81. net: move BanMan to its own files 7b6ec1bdcf
  82. 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
  83. banman: add thread annotations and mark members const where possible
    Also remove misleading comment. ClearBanned is used by rpc as well.
    521ebf37f4
  84. theuni force-pushed on Apr 19, 2018
  85. 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.
  86. jimpo commented at 8:26 am on April 20, 2018: contributor
    utACK 521ebf3
  87. MarcoFalke added the label Needs rebase on Jun 6, 2018
  88. fanquake closed this on Oct 30, 2018

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

  90. laanwj referenced this in commit 5baa9092c4 on Jan 21, 2019
  91. laanwj removed the label Needs rebase on Oct 24, 2019
  92. PastaPastaPasta referenced this in commit 38ee2a7a94 on Jul 18, 2021
  93. PastaPastaPasta referenced this in commit 7cf87fe383 on Jul 18, 2021
  94. MarcoFalke locked this on Dec 16, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-24 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me