Ignore banlist.dat #22570

All calls to LogPrintf() and LogPrint() should be terminated with \n

Thanks, fixed.

I think this should read 22.0 instead of 23.0 because already in 22.0 we ignore banlist.dat if banlist.json is present during read and only write to banlist.json.

`banlist.dat`  | Stores the addresses/subnets of banned nodes; superseded by `banlist.json` in 22.0 and completely ignored in 23.0 | [PR [#20966](/bitcoin-bitcoin/20966/)](https://github.com/bitcoin/bitcoin/pull/20966), [PR [#22570](/bitcoin-bitcoin/22570/)](https://github.com/bitcoin/bitcoin/pull/22570)

The dirty argument can be removed.

There is just one caller of this method and if this method returns false, then the caller would ignore the value set in Read() and would set the dirty flag to true regardless. The only reason for the dirty flag was if Read() returns true and would set dirty to true. This was done to force json creation if only banlist.dat existed and was read successfully.

The dirty argument is used to not write the json file again if it was read successfully and nothing needed to be swept. Happy to drop it, as it simplifies and writing the json once per startup shouldn't cost too much.

Yes, since Read() does not do any sweeping its return value is enough to tell whether the read was successful or not. Or, in other words - with this PR it is not possible Read() to return true and set its second argument (dirty) to true.

Something like this should work (not tested):

diff --git i/src/addrdb.cpp w/src/addrdb.cpp
index fbffd5c959..c3e224ee83 100644
--- i/src/addrdb.cpp
+++ w/src/addrdb.cpp
@@ -194,25 +194,22 @@ bool CBanDB::Write(const banmap_t& banSet)
     for (const auto& err : errors) {
         error("%s", err);
     }
     return false;
 }
 
-bool CBanDB::Read(banmap_t& banSet, bool& dirty)
+bool CBanDB::Read(banmap_t& banSet)
 {
     if (fs::exists(m_banlist_dat)) {
         LogPrintf("banlist.dat ignored because it can only be read by " PACKAGE_NAME " version 22.x. Remove %s to silence this warning.\n", m_banlist_dat);
     }
     // If the JSON banlist does not exist, then recreate it
     if (!fs::exists(m_banlist_json)) {
-        dirty = true;
         return false;
     }
 
-    dirty = false;
-
     std::map<std::string, util::SettingsValue> settings;
     std::vector<std::string> errors;
 
     if (!util::ReadSettings(m_banlist_json, settings, errors)) {
         for (const auto& err : errors) {
             LogPrintf("Cannot load banlist %s: %s\n", m_banlist_json.string(), err);
diff --git i/src/addrdb.h w/src/addrdb.h
index 0213a14af5..1e0ccb1f60 100644
--- i/src/addrdb.h
+++ w/src/addrdb.h
@@ -92,17 +92,15 @@ public:
     bool Write(const banmap_t& banSet);
 
     /**
      * Read the banlist from disk.
      * [@param](/bitcoin-bitcoin/contributor/param/)[out] banSet The loaded list. Set if `true` is returned, otherwise it is left
      * in an undefined state.
-     * [@param](/bitcoin-bitcoin/contributor/param/)[out] dirty Indicates whether the loaded list needs flushing to disk. Set if
-     * `true` is returned, otherwise it is left in an undefined state.
      * [@return](/bitcoin-bitcoin/contributor/return/) true on success
      */
-    bool Read(banmap_t& banSet, bool& dirty);
+    bool Read(banmap_t& banSet);
 };
 
 /**
  * Dump the anchor IP address database (anchors.dat)
  *
  * Anchors are last known outgoing block-relay-only peers that are
diff --git i/src/banman.cpp w/src/banman.cpp
index d2437e6733..c64a48a05a 100644
--- i/src/banman.cpp
+++ w/src/banman.cpp
@@ -15,13 +15,13 @@
 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)
 {
     if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…").translated);
 
     int64_t n_start = GetTimeMillis();
-    if (m_ban_db.Read(m_banned, m_is_dirty)) {
+    if (m_ban_db.Read(m_banned)) {
         SweepBanned(); // sweep out unused entries
 
         LogPrint(BCLog::NET, "Loaded %d banned node addresses/subnets  %dms\n", m_banned.size(),
                  GetTimeMillis() - n_start);
     } else {
         LogPrintf("Recreating the banlist database\n");

Thanks, will pick the patch. It is almost correct 🥲:

==4031966== Conditional jump or move depends on uninitialised value(s)
==4031966==    at 0x46011B: BanMan::DumpBanlist() (banman.cpp:44)
==4031966==    by 0x45E962: BanMan::BanMan(boost::filesystem::path, CClientUIInterface*, long) (banman.cpp:32)
==4031966==    by 0x15C119: make_unique<BanMan, boost::filesystem::path, CClientUIInterface *, long> (unique_ptr.h:962)
==4031966==    by 0x15C119: AppInitMain(NodeContext&, interfaces::BlockAndHeaderTipInfo*) (init.cpp:1164)
==4031966==    by 0x141388: AppInit (bitcoind.cpp:226)
==4031966==    by 0x141388: main (bitcoind.cpp:269)
==4031966== 

Oh, why did you had to run valgrind on this? It was perfectly safe before that...

nit:

        LogPrintf("%s ignored because it can only be read by " PACKAGE_NAME " version 22.x or older. Remove %s to silence this warning.\n", m_banlist_dat, m_banlist_dat);

Or maybe remove CBanDB::m_banlist_dat and hardcode the string "banlist.dat" here since it is only used here.

I need the full path for fs::exists(m_banlist_dat), so I am going to keep as is for now.

This is the whitespace fixup that was dropped as a standalone change in #22562. Here it is combined with some other changes that touch those files.

netaddress.h is not fully clang-formatted. Running clang-format -i netaddress.h produces the following diff:

<details> <summary>format diff</summary>

diff --git i/src/netaddress.h w/src/netaddress.h
index 77e1c0464e..eb35ed3fac 100644
--- i/src/netaddress.h
+++ w/src/netaddress.h
@@ -39,14 +39,13 @@ static constexpr int ADDRV2_FORMAT = 0x20000000;
  * belongs to both `NET_UNROUTABLE` and `NET_IPV4`.
  * Keep these sequential starting from 0 and `NET_MAX` as the last entry.
  * We have loops like `for (int i = 0; i < NET_MAX; ++i)` that expect to iterate
  * over all enum values and also `GetExtNetwork()` "extends" this enum by
  * introducing standalone constants starting from `NET_MAX`.
  */
-enum Network
-{
+enum Network {
     /// Addresses from these networks are not publicly routable on the global Internet.
     NET_UNROUTABLE = 0,
 
     /// IPv4
     NET_IPV4,
 
@@ -70,22 +69,20 @@ enum Network
     NET_MAX,
 };
 
 /// Prefix of an IPv6 address when it contains an embedded IPv4 address.
 /// Used when (un)serializing addresses in ADDRv1 format (pre-BIP155).
 static const std::array<uint8_t, 12> IPV4_IN_IPV6_PREFIX{
-    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xFF, 0xFF
-};
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xFF, 0xFF};
 
 /// Prefix of an IPv6 address when it contains an embedded TORv2 address.
 /// Used when (un)serializing addresses in ADDRv1 format (pre-BIP155).
 /// Such dummy IPv6 addresses are guaranteed to not be publicly routable as they
 /// fall under RFC4193's fc00::/7 subnet allocated to unique-local addresses.
 static const std::array<uint8_t, 6> TORV2_IN_IPV6_PREFIX{
-    0xFD, 0x87, 0xD8, 0x7E, 0xEB, 0x43
-};
+    0xFD, 0x87, 0xD8, 0x7E, 0xEB, 0x43};
 
 /// Prefix of an IPv6 address when it contains an embedded "internal" address.
 /// Used when (un)serializing addresses in ADDRv1 format (pre-BIP155).
 /// The prefix comes from 0xFD + SHA256("bitcoin")[0:5].
 /// Such dummy IPv6 addresses are guaranteed to not be publicly routable as they
 /// fall under RFC4193's fc00::/7 subnet allocated to unique-local addresses.

</details>

I guess a scripted diff that fully formats the files would be even easier to verify and more complete.

Thanks, taken. Couldn't get a scripted-diff working, but happy to take if you find one.

commit f8a3771957cb1d4d202cd7e3173edfbbeed2657e
Parent: faaf9e930d94328c6be42516beab817d45d847f1
Author:     Vasil Dimov <vd@FreeBSD.org>
AuthorDate: Thu Jul 29 12:39:49 2021 +0200
Commit:     Vasil Dimov <vd@FreeBSD.org>
CommitDate: Thu Jul 29 12:39:49 2021 +0200


    scripted-diff: violate guidelines
    
    -BEGIN VERIFY SCRIPT-
    clang-format -i src/netaddress.h src/test/fuzz/deserialize.cpp
    -END VERIFY SCRIPT-

diff --git a/src/netaddress.h b/src/netaddress.h
index c81f9fd5d8..eb35ed3fac 100644
--- a/src/netaddress.h
+++ b/src/netaddress.h
...
same diff as in fa81dae3db
...

Verify:

$ test/lint/commit-script-check.sh origin/master..HEAD
Running script for: f8a3771957cb1d4d202cd7e3173edfbbeed2657e
clang-format -i src/netaddress.h src/test/fuzz/deserialize.cpp
OK