Return of the Banman #14605

pull dongcarl wants to merge 12 commits into bitcoin:master from dongcarl:banman changing 18 files +456 −332
  1. dongcarl commented at 5:39 AM on October 30, 2018: member

    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)
    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)
  2. fanquake added the label P2P on Oct 30, 2018
  3. DrahtBot commented at 6:19 AM on October 30, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #14987 (RPCHelpMan: Pass through Result and Examples by MarcoFalke)
    • #14929 (net: Allow connections from misbehavior banned peers by gmaxwell)
    • #14897 (randomize GETDATA(tx) request order and introduce bias toward outbound by naumenkogs)
    • #14856 (net: remove more CConnman globals (theuni) by dongcarl)
    • #14464 (refactor: make checkqueue manage the threads by itself (also removed some boost dependencies) by ken2812221)
    • #10785 (Serialization improvements by sipa)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  4. fanquake added this to the "In progress" column in a project
  5. in src/qt/rpcconsole.cpp:1231 in af2c654832 outdated 
    1232 | -	// Find possible nodes, ban it and clear the selected node
1233 | -	const CNodeCombinedStats *stats = clientModel->getPeerTableModel()->getNodeStats(detailNodeRow);
1234 | -	if(stats) {
1235 | +    // Find possible nodes, ban it and clear the selected node
1236 | +    const CNodeCombinedStats *stats = clientModel->getPeerTableModel()->getNodeStats(detailNodeRow);
1237 | +    if(stats) {
    MarcoFalke commented at 12:22 PM on October 30, 2018:

    Accidental changes in the first commit?

  6. in src/addrdb.cpp:108 in af2c654832 outdated 
     104 | @@ -105,9 +105,8 @@ bool DeserializeFileDB(const fs::path& path, Data& data)
 105 |  
 106 |  }
 107 |  
 108 | -CBanDB::CBanDB()
 109 | +CBanDB::CBanDB(fs::path ban_file) : pathBanlist(std::move(ban_file))
    practicalswift commented at 2:35 PM on October 30, 2018:

    Make sure parameter name matches between declaration and definition :-)

  7. in src/banman.h:39 in af2c654832 outdated 
      34 | +class BanMan
  35 | +{
  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);
    practicalswift commented at 2:35 PM on October 30, 2018:

    Make sure parameter names match between:

        void Ban(const CNetAddr& netAddr, const BanReason& reason, int64_t bantimeoffset = 0, bool sinceUnixEpoch = false);
    void BanMan::Ban(const CNetAddr& addr, const BanReason &banReason, int64_t bantimeoffset, bool sinceUnixEpoch) {
  8. in src/banman.h:40 in af2c654832 outdated 
      35 | +{
  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);
    practicalswift commented at 2:36 PM on October 30, 2018:

    Make sure these match:

        void Ban(const CSubNet& subNet, const BanReason& reason, int64_t bantimeoffset = 0, bool sinceUnixEpoch = false);
    void BanMan::Ban(const CSubNet& subNet, const BanReason &banReason, int64_t bantimeoffset, bool sinceUnixEpoch) {
  9. in src/banman.h:44 in af2c654832 outdated 
      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();
  42 | +    bool IsBanned(CNetAddr ip);
  43 | +    bool IsBanned(CSubNet subnet);
  44 | +    bool Unban(const CNetAddr &ip);
    practicalswift commented at 2:36 PM on October 30, 2018:

    Make sure these match:

        bool Unban(const CNetAddr &ip);
    bool BanMan::Unban(const CNetAddr &addr) {
  10. in src/banman.h:45 in af2c654832 outdated 
      40 | +    void Ban(const CSubNet& subNet, const BanReason& reason, int64_t bantimeoffset = 0, bool sinceUnixEpoch = false);
  41 | +    void ClearBanned();
  42 | +    bool IsBanned(CNetAddr ip);
  43 | +    bool IsBanned(CSubNet subnet);
  44 | +    bool Unban(const CNetAddr &ip);
  45 | +    bool Unban(const CSubNet &ip);
    practicalswift commented at 2:36 PM on October 30, 2018:

    Same here:

        bool Unban(const CSubNet &ip);
    bool BanMan::Unban(const CSubNet &subNet) {
  11. in src/banman.h:46 in af2c654832 outdated 
      41 | +    void ClearBanned();
  42 | +    bool IsBanned(CNetAddr ip);
  43 | +    bool IsBanned(CSubNet subnet);
  44 | +    bool Unban(const CNetAddr &ip);
  45 | +    bool Unban(const CSubNet &ip);
  46 | +    void GetBanned(banmap_t &banmap);
    practicalswift commented at 2:37 PM on October 30, 2018:

    Same here:

        void GetBanned(banmap_t &banmap);
    void BanMan::GetBanned(banmap_t &banMap)
  12. in src/banman.h:50 in af2c654832 outdated 
      45 | +    bool Unban(const CSubNet &ip);
  46 | +    void GetBanned(banmap_t &banmap);
  47 | +    void DumpBanlist();
  48 | +
  49 | +private:
  50 | +    void SetBanned(const banmap_t &banmap);
    practicalswift commented at 2:37 PM on October 30, 2018:

    And here :-)

        void SetBanned(const banmap_t &banmap);
    void BanMan::SetBanned(const banmap_t &banMap)
  13. in src/banman.cpp:139 in af2c654832 outdated 
     134 | +}
 135 | +
 136 | +bool BanMan::Unban(const CSubNet &subNet) {
 137 | +    {
 138 | +        LOCK(m_cs_banned);
 139 | +        if (!m_banned.erase(subNet))
    practicalswift commented at 2:40 PM on October 30, 2018:

    Nit: if (m_banned.erase(subNet) == 0) to increase readability and avoid implicit conversion?

  14. in src/banman.cpp:68 in af2c654832 outdated 
      63 | +        LOCK(m_cs_banned);
  64 | +        m_banned.clear();
  65 | +        m_is_dirty = true;
  66 | +    }
  67 | +    DumpBanlist(); //store banlist to disk
  68 | +    if(m_client_interface)
    practicalswift commented at 2:47 PM on October 30, 2018:

    Since this is new code - please run all new C++ files through clang-format (using the project local settings) to get the expected formatting.

  15. in src/banman.cpp:111 in af2c654832 outdated 
     106 | +    CBanEntry banEntry(GetTime());
 107 | +    banEntry.banReason = banReason;
 108 | +    if (bantimeoffset <= 0)
 109 | +    {
 110 | +        bantimeoffset = m_default_ban_time;
 111 | +        sinceUnixEpoch = false;
    practicalswift commented at 8:55 PM on October 30, 2018:

    Could this be re-formulated to avoid reassigning parameters?

    dongcarl commented at 9:03 PM on October 31, 2018:

    Reformulated in https://github.com/bitcoin/bitcoin/pull/14605/commits/36e6b241d0ed6f6d6ab503d61dfd95053202ce9b, let me know if you think that's good

    practicalswift commented at 9:07 PM on November 15, 2018:

    @dongcarl Nice! Thanks! :-)

  16. dongcarl force-pushed on Oct 31, 2018
  17. dongcarl force-pushed on Oct 31, 2018
  18. laanwj commented at 5:14 PM on October 31, 2018: member

    thank you for picking this up! We welcome the Ban Man to our kingdom

  19. laanwj added this to the milestone 0.18.0 on Oct 31, 2018
  20. dongcarl force-pushed on Nov 1, 2018
  21. laanwj commented at 1:10 PM on November 6, 2018: member

    No merge conflicts, but it does need rebase [#14555]; current build fails wiith

    /.../bitcoin/src/banman.cpp:10:10: fatal error: util.h: No such file or directory
 #include <util.h>
          ^~~~~~~~
compilation terminated.
  22. dongcarl force-pushed on Nov 6, 2018
  23. dongcarl commented at 4:59 PM on November 6, 2018: member

    Done.

  24. in src/banman.cpp:77 in 157684a341 outdated 
      72 | +bool BanMan::IsBanned(CNetAddr net_addr)
  73 | +{
  74 | +    LOCK(m_cs_banned);
  75 | +    for (const auto& it : m_banned) {
  76 | +        CSubNet sub_net = it.first;
  77 | +        CBanEntry banEntry = it.second;
    kostyantyn commented at 5:25 PM on November 6, 2018:

    since it's a new class and it mostly everywhere follows snake_case notation, WDYS to stick to it and in a few other places?

    theuni commented at 9:56 PM on December 4, 2018:

    @kostyantyn That'd be fine in a follow-up PR, but the primary interest here is renaming the member variables so that they're immediately distinguishable.

  25. DrahtBot added the label Needs rebase on Nov 15, 2018
  26. dongcarl force-pushed on Nov 15, 2018
  27. DrahtBot removed the label Needs rebase on Nov 15, 2018
  28. dongcarl commented at 2:04 AM on November 17, 2018: member

    Rebased, and addressed casing concerns. Additional review welcome.

  29. DrahtBot added the label Needs rebase on Nov 28, 2018
  30. dongcarl force-pushed on Nov 28, 2018
  31. DrahtBot removed the label Needs rebase on Nov 28, 2018
  32. in src/init.cpp:1287 in 701702427e outdated 
    1282 | @@ -1278,11 +1283,12 @@ bool AppInitMain(InitInterfaces& interfaces)
1283 |      // is not yet setup and may end up being set up twice if we
1284 |      // need to reindex later.
1285 |  
1286 | +    assert(!g_banman);
1287 | +    g_banman = std::unique_ptr<BanMan>(new BanMan(&uiInterface));
    theuni commented at 9:27 PM on December 4, 2018:

    This can now use MakeUnique.

  33. ryanofsky commented at 9:35 PM on December 4, 2018: member

    I'd like to review this soon, but is there a description of what high level goal of this PR is? Is it cleanup geared towards making code maintenance easier? Or is it intended to enable a new feature? Is it cleaning up technical debt from a previously added feature? The previous PR #14605 just says 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," which I think justifies the change but doesn't really motivate it.

  34. in src/interfaces/node.cpp:126 in 701702427e outdated 
     119 | @@ -120,24 +120,24 @@ class NodeImpl : public Node
 120 |      }
 121 |      bool getBanned(banmap_t& banmap) override
 122 |      {
 123 | -        if (g_connman) {
 124 | -            g_connman->GetBanned(banmap);
 125 | +        if (g_banman) {
    theuni commented at 9:39 PM on December 4, 2018:

    Note for follow-up PR:

    connman and banman pointers should be passed into NodeImpl's constructor. That would eliminate the use of globals here.

    Sjors commented at 6:06 PM on January 16, 2019:

    cc @ryanofsky

  35. in src/banman.cpp:111 in 5090c8c9fd outdated 
     105 | @@ -106,11 +106,14 @@ void BanMan::Ban(const CSubNet& subNet, const BanReason& banReason, int64_t bant
 106 |  {
 107 |      CBanEntry banEntry(GetTime());
 108 |      banEntry.banReason = banReason;
 109 | +
 110 | +    int64_t normalized_bantimeoffset = bantimeoffset;
 111 | +    int64_t normalized_sinceUnixEpoch = sinceUnixEpoch;
    theuni commented at 10:25 PM on December 4, 2018:

    normalized_sinceUnixEpoch should be a bool.

  36. in src/banman.cpp:28 in 9bad9fa116 outdated 
      24 | @@ -25,7 +25,7 @@ BanMan::BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t
  25 |          SweepBanned();            // sweep out unused entries
  26 |  
  27 |          LogPrint(BCLog::NET, "Loaded %d banned node ips/subnets from banlist.dat  %dms\n",
  28 | -            banmap.size(), GetTimeMillis() - nStart);
    theuni commented at 10:27 PM on December 4, 2018:

    Hungarian or snake_case, pick one :)

    dongcarl commented at 1:56 PM on December 5, 2018:

    Fixed in 9bad9fa1168ec53e92bb8b2df0b5e9d578344a2f, unless this should be squashed in

  37. in src/banman.cpp:108 in 9bad9fa116 outdated 
     123 | -    int64_t normalized_sinceUnixEpoch = sinceUnixEpoch;
 124 | -    if (bantimeoffset <= 0) {
 125 | -        normalized_bantimeoffset = m_default_ban_time;
 126 | -        normalized_sinceUnixEpoch = false;
 127 | +    CBanEntry ban_entry(GetTime());
 128 | +    ban_entry.banReason = ban_reason;
    theuni commented at 10:38 PM on December 4, 2018:

    Please don't use special-cases in scripted-diffs to negate part of a change, as that means that manual review is required.

    Instead, please add a commit before the scripted-diff which renames any necessary conflicts.

    dongcarl commented at 1:54 PM on December 5, 2018:

    Do you mean rename CBanEntry.banReason to CBanEntry.m_ban_reason before the scripted diff?

    theuni commented at 10:27 PM on December 6, 2018:

    How about just first adding a commit to hack around it so that we don't have to use the var by name?

    diff --git a/src/addrdb.h b/src/addrdb.h
index 90eca44..f50442f 100644
--- a/src/addrdb.h
+++ b/src/addrdb.h
@@ -43,6 +43,11 @@ public:
         nCreateTime = nCreateTimeIn;
     }

+    explicit CBanEntry(int64_t nCreateTimeIn, BanReason banReasonIn) : CBanEntry(nCreateTimeIn)
+    {
+        banReason = banReasonIn;
+    }
+
     ADD_SERIALIZE_METHODS;

     template <typename Stream, typename Operation>
diff --git a/src/net.cpp b/src/net.cpp
index fde85b0..db97ae8 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -540,8 +540,7 @@ void CConnman::Ban(const CNetAddr& addr, const BanReason &banReason, int64_t ban
 }

 void CConnman::Ban(const CSubNet& subNet, const BanReason &banReason, int64_t bantimeoffset, bool sinceUnixEpoch) {
-    CBanEntry banEntry(GetTime());
-    banEntry.banReason = banReason;
+    CBanEntry banEntry(GetTime(), banReason);
     if (bantimeoffset <= 0)
     {
         bantimeoffset = gArgs.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME);

    After that, I believe the scripted diff should work without hacks.

  38. theuni commented at 10:46 PM on December 4, 2018: member

    @dongcarl Thanks for picking this up! Added a few nits.

  39. theuni commented at 10:49 PM on December 4, 2018: member

    @ryanofsky This is one step (one of the big ones) closer to instantiated CConnman (self-contained and no-globals), which would allow for all kinds of cool things, primarily running separate net instances against each-other.

    Another nice side-effect of the above is that once self-contained, bitcoind could be run (or even compiled!) without net support.

    Edit: See my comment here as an example of next steps: #14605 (review)

  40. DrahtBot added the label Needs rebase on Dec 13, 2018
  41. dongcarl force-pushed on Jan 3, 2019
  42. dongcarl commented at 3:19 PM on January 3, 2019: member

    Rebased and addressed reviews!

  43. DrahtBot removed the label Needs rebase on Jan 3, 2019
  44. in src/addrdb.h:48 in bc526e9e98 outdated 
      42 | @@ -43,6 +43,11 @@ class CBanEntry
  43 |          nCreateTime = nCreateTimeIn;
  44 |      }
  45 |  
  46 | +    explicit CBanEntry(int64_t n_create_time_in, BanReason ban_reason_in) : CBanEntry(n_create_time_in)
  47 | +    {
  48 | +        banReason = ban_reason_in;
    Empact commented at 10:48 AM on January 9, 2019:

    nit: could be initializer

    dongcarl commented at 7:43 PM on January 15, 2019:

    I don't think this can be initializer as we need to SetNull(), the current one does the delegation thing.

  45. in src/addrdb.h:100 in bc526e9e98 outdated 
      96 | @@ -92,9 +97,9 @@ class CAddrDB
  97 |  class CBanDB
  98 |  {
  99 |  private:
 100 | -    fs::path pathBanlist;
 101 | +    fs::path m_ban_list_path;
    Empact commented at 10:48 AM on January 9, 2019:

    nit: could be const

  46. in src/banman.cpp:14 in bc526e9e98 outdated 
       9 | +#include <ui_interface.h>
  10 | +#include <util/system.h>
  11 | +#include <util/time.h>
  12 | +
  13 | +
  14 | +BanMan::BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time) : m_client_interface(client_interface), m_ban_db(std::move(ban_file)), m_default_ban_time(default_ban_time)
    Empact commented at 10:49 AM on January 9, 2019:

    nit: could wrap this line

  47. in src/banman.cpp:17 in bc526e9e98 outdated 
      12 | +
  13 | +
  14 | +BanMan::BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time) : m_client_interface(client_interface), m_ban_db(std::move(ban_file)), m_default_ban_time(default_ban_time)
  15 | +{
  16 | +    if (m_client_interface)
  17 | +        m_client_interface->InitMessage(_("Loading banlist..."));
    Empact commented at 10:50 AM on January 9, 2019:

    nit: one line or braces, here and a few other places

  48. in src/banman.h:65 in bc526e9e98 outdated 
      57 | +    CCriticalSection m_cs_banned;
  58 | +    banmap_t m_banned GUARDED_BY(m_cs_banned);
  59 | +    bool m_is_dirty GUARDED_BY(m_cs_banned);
  60 | +    CClientUIInterface* m_client_interface = nullptr;
  61 | +    CBanDB m_ban_db;
  62 | +    const int64_t m_default_ban_time;
    Empact commented at 11:04 AM on January 9, 2019:

    nit: #include <cstdint>

  49. in src/banman.h:68 in bc526e9e98 outdated 
      60 | +    CClientUIInterface* m_client_interface = nullptr;
  61 | +    CBanDB m_ban_db;
  62 | +    const int64_t m_default_ban_time;
  63 | +};
  64 | +
  65 | +extern std::unique_ptr<BanMan> g_banman;
    Empact commented at 11:04 AM on January 9, 2019:

    nit: #include <memory>

  50. in src/net.cpp:2482 in bc526e9e98 outdated 
    2477 | +bool CConnman::DisconnectNode(const CSubNet& subnet)
2478 | +{
2479 | +    bool disconnected = false;
2480 | +    LOCK(cs_vNodes);
2481 | +    for (CNode* pnode : vNodes) {
2482 | +        if (subnet.Match(static_cast<CNetAddr>(pnode->addr))) {
    Empact commented at 11:14 AM on January 9, 2019:

    Seems dynamic_cast would be more appropriate here, given the cast to base class.

    Edit: maybe not:

    A downcast can also be performed with static_cast, which avoids the cost of the runtime check, but it's only safe if the program can guarantee (through some other logic) that the object pointed to by expression is definitely Derived. https://en.cppreference.com/w/cpp/language/dynamic_cast

    Maybe a static_assert is in order to enforce?

    dongcarl commented at 8:28 PM on January 15, 2019:

    We are upcasting from CAddress to its base class of CNetAddr here, so it wouldn't need a static_assert I believe.

    Since we are just upcasting, we don't even need casting here, so I'll just remove it unless there are obscure C++ reasons for doing so.

    laanwj commented at 3:18 PM on January 16, 2019:

    I'd advise against using dynamic_cast. It's not used anywhere else in the code and it relies on C++ run-time type information which has at least historically a large overhead. I'm also fairly sure it's unnecessary here.

  51. in src/net_processing.cpp:2967 in bc526e9e98 outdated 
    2969 | -            {
2970 | -                connman->Ban(pnode->addr, BanReasonNodeMisbehaving);
2971 | +        else if (pnode->addr.IsLocal()) {
2972 | +            // Disconnect but don't ban _this_ local node
2973 | +            LogPrintf("Warning: disconnecting but not banning local peer %s!\n", pnode->addr.ToString());
2974 | +            connman->DisconnectNode(pnode->GetId());
    Empact commented at 11:23 AM on January 9, 2019:

    nit: could have a CNode* version of DisconnectNode to avoid the lookup

    dongcarl commented at 9:17 PM on January 15, 2019:

    I'm going to revert to doing

    pnode->fDisconnect = true;

    so as to avoid the lookup, then in a followup PR we can make fDisconnect private, and have a Disconnect method on CNode.

  52. Empact commented at 11:28 AM on January 9, 2019: member

    Concept ACK

  53. DrahtBot added the label Needs rebase on Jan 9, 2019
  54. laanwj commented at 2:53 PM on January 15, 2019: member

    We want to have this in for 0.18.0, there's not that much time anymore. This has had a fair amount of review. Let's rebase, address the nits, do a last review round, and merge it.

  55. dongcarl commented at 6:35 PM on January 15, 2019: member

    @laanwj Yes. I will make it happen this week.

  56. dongcarl force-pushed on Jan 15, 2019
  57. dongcarl commented at 10:53 PM on January 15, 2019: member

    Rebased and addressed nits.

  58. DrahtBot removed the label Needs rebase on Jan 15, 2019
  59. laanwj commented at 12:52 PM on January 16, 2019: member

    Thank you @dongkarl!

  60. laanwj commented at 1:04 PM on January 16, 2019: member

    utACK cb1ee04b6a7d56834a6447711ca5500fc8bd83cd

    Original PR had utACKs by me, @ryanofsky, @jimpo. Please re-review so that we can merge this!

  61. 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.
    7cc2b9f678
  62. tests: remove member connman/peerLogic in TestingSetup 136bd7926c
  63. in src/net_processing.cpp:2971 in b4ca7e2933 outdated 
    2971 | +                // Disconnect but don't ban _this_ local node
2972 | +                LogPrintf("Warning: disconnecting but not banning local peer %s!\n", pnode->addr.ToString());
2973 | +            } else {
2974 | +                // Disconnect and ban all nodes sharing the address
2975 |                  connman->Ban(pnode->addr, BanReasonNodeMisbehaving);
2976 |              }
    promag commented at 4:50 PM on January 16, 2019:

    Commit b4ca7e2

    Discussed with @dongcarl about not changing behavior in this commit, the conclusion is to call conman->DisconnectNode(pnode->addr) here and add pnode->fDisconnect = true; above.

  64. dongcarl force-pushed on Jan 16, 2019
  65. in src/net.cpp:2646 in 7cc2b9f678 outdated 
    2641 | +{
2642 | +    bool disconnected = false;
2643 | +    LOCK(cs_vNodes);
2644 | +    for (CNode* pnode : vNodes) {
2645 | +        if (subnet.Match(pnode->addr)) {
2646 | +            pnode->fDisconnect = true;
    Sjors commented at 5:45 PM on January 16, 2019:

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

    dongcarl commented at 6:54 PM on January 16, 2019:

    I think this belongs in another PR, but happy to do that PR :-)

  66. in src/net.cpp:45 in 9db3a772fc outdated 
      40 | @@ -41,8 +41,11 @@
  41 |  
  42 |  #include <math.h>
  43 |  
  44 | -// Dump addresses to peers.dat and banlist.dat every 15 minutes (900s)
  45 | -#define DUMP_ADDRESSES_INTERVAL 900
  46 | +// Dump addresses to peers.dat every 15 minutes (900s)
  47 | +static constexpr int DUMP_PEERS_INTERVAL = 60 * 15;
    Sjors commented at 6:01 PM on January 16, 2019:

    Nit: 15 * 60 to be consistent with other _INTERVAL constants.

    dongcarl commented at 6:55 PM on January 16, 2019:

    Done!

  67. Sjors approved
  68. Sjors commented at 6:22 PM on January 16, 2019: member

    tACK 95b495b9697502b2b478dda8cece0ae9ffb6818f @ryanofsky wrote:

    is there a description of what high level goal of this PR is? Is it cleanup geared towards making code maintenance easier? [...]

    I think reducing the 3000 lines in net.cpp by 200 is almost worth it by itself :-)

    From the original description in #11457:

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

    So this is a step towards being able to mock network code and e.g. test banning behavior for a large number of simulated nodes.

  69. net: split up addresses/ban dumps in preparation for moving them 83c1ea2e5e
  70. banman: create and split out banman
    Some say he has always been.
    4c0d961eb0
  71. 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.
    2e56702ece
  72. banman: pass in default ban time as a parameter
    Removes the dependency on arg parsing.
    d0469b2e93
  73. net: move BanMan to its own files af3503d903
  74. 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-
    84fc3fbd03
  75. banman: add thread annotations and mark members const where possible
    Also remove misleading comment. ClearBanned is used by rpc as well.
    daae598feb
  76. banman: reformulate nBanUtil calculation
    Avoid reassigning parameters.
    1ffa4ce27d
  77. banman: Add, use CBanEntry ctor that takes ban reason c2e04d37f3
  78. scripted-diff: batch-recase BanMan variables
    -BEGIN VERIFY SCRIPT-
sed -i "s/banMap/banmap/g" src/banman.h src/banman.cpp
sed -i "s/netAddr/net_addr/g" src/banman.h src/banman.cpp
sed -i "s/sinceUnixEpoch/since_unix_epoch/g" src/banman.h src/banman.cpp
sed -i "s/bantimeoffset/ban_time_offset/g" src/banman.h src/banman.cpp
sed -i "s/subNet/sub_net/g" src/banman.h src/banman.cpp
sed -i "s/banReason/ban_reason/g" src/banman.h src/banman.cpp
sed -i "s/notifyUI/notify_ui/g" src/banman.h src/banman.cpp
sed -i "s/banEntry/ban_entry/g" src/banman.h src/banman.cpp
sed -i "s/nStart/n_start/g" src/banman.h src/banman.cpp
-END VERIFY SCRIPT-
    18185b57c3
  79. dongcarl force-pushed on Jan 16, 2019
  80. Sjors commented at 7:10 PM on January 16, 2019: member

    re-tACK 18185b57

  81. laanwj referenced this in commit 978682b9dc on Jan 20, 2019
  82. laanwj commented at 5:58 PM on January 21, 2019: member

    re-utACK 18185b57c32d0a43afeca4c125b9352c692923e9

  83. laanwj merged this on Jan 21, 2019
  84. laanwj closed this on Jan 21, 2019
  85. laanwj referenced this in commit 5baa9092c4 on Jan 21, 2019
  86. fanquake moved this from the "In progress" to the "Done" column in a project
  87. deadalnix referenced this in commit 47f476c061 on Jun 9, 2020
  88. PastaPastaPasta referenced this in commit 38ee2a7a94 on Jul 18, 2021
  89. PastaPastaPasta referenced this in commit 7cf87fe383 on Jul 18, 2021
  90. Munkybooty referenced this in commit 8edd40fcff on Aug 21, 2021
  91. Munkybooty referenced this in commit 27c483e6fd on Aug 23, 2021
  92. Munkybooty referenced this in commit aceea13f9f on Aug 24, 2021
  93. Munkybooty referenced this in commit 9845e85bcd on Aug 24, 2021
  94. Munkybooty referenced this in commit 53a6f62ea4 on Aug 24, 2021
  95. UdjinM6 referenced this in commit 45c54cb89d on Aug 24, 2021
  96. Munkybooty referenced this in commit 30f6319fd0 on Aug 24, 2021
  97. MarcoFalke locked this on Dec 16, 2021
Contributors
dongcarlDrahtBotMarcoFalkepracticalswiftlaanwjkostyantyntheuniryanofskySjorsEmpactpromag
Labels
P2P

Milestone
0.18.0

Linked (view graph)
#11457 Introduce BanMan#15194 Add comment describing fDisconnect behavior
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: 2026-05-04 12:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me