Ignore banlist.dat

MarcoFalke commented at 5:27 pm on July 28, 2021: member

The code to read banlist.dat should be removed eventually. The major release (22.x) can be used to translate a banlist.dat into a banlist.json. Thus, it is now possible to remove the reading code.

MarcoFalke added the label P2P on Jul 28, 2021

in src/addrdb.cpp:203 in fab2cd8287 outdated

198@@ -199,11 +199,13 @@ bool CBanDB::Write(const banmap_t& banSet)
199 
200 bool CBanDB::Read(banmap_t& banSet, bool& dirty)
201 {
202-    // If the JSON banlist does not exist, then try to read the non-upgraded banlist.dat.
203+    if (fs::exists(m_banlist_dat)) {
204+        LogPrintf("banlist.dat ignored because it can only be read by " PACKAGE_NAME " version 22.x. Remove %s to silence this warning.", m_banlist_dat);

fanquake commented at 0:16 am on July 29, 2021:

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

MarcoFalke commented at 6:35 am on July 29, 2021:

Thanks, fixed.

Zero-1729 commented at 1:02 am on July 29, 2021: contributor

Concept ACK, but agree with @fanquake’s comment regarding LogPrintf() and adding a terminal newline.

DrahtBot commented at 3:39 am on July 29, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22375 by MarcoFalke
#22362 by MarcoFalke
#22140 by jonatack
#21283 by achow101
#20233 by jnewbery

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.

MarcoFalke force-pushed on Jul 29, 2021

in doc/files.md:116 in fa718eab70 outdated

112@@ -114,6 +113,7 @@ These subdirectories and files are no longer used by Bitcoin Core:
113 
114 Path           | Description | Repository notes
115 ---------------|-------------|-----------------
116+`banlist.dat`  | Stores the addresses/subnets of banned nodes; replaced by `banlist.json` in 23.0 | [PR #22570](https://github.com/bitcoin/bitcoin/pull/22570)

vasild commented at 7:09 am on July 29, 2021:

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.

0`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)

in src/addrdb.cpp:200 in fa718eab70 outdated

198@@ -199,11 +199,13 @@ bool CBanDB::Write(const banmap_t& banSet)
199 
200 bool CBanDB::Read(banmap_t& banSet, bool& dirty)

vasild commented at 7:59 am on July 29, 2021:

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.

MarcoFalke commented at 10:16 am on July 29, 2021:

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.

vasild commented at 10:28 am on July 29, 2021:

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

 0diff --git i/src/addrdb.cpp w/src/addrdb.cpp
 1index fbffd5c959..c3e224ee83 100644
 2--- i/src/addrdb.cpp
 3+++ w/src/addrdb.cpp
 4@@ -194,25 +194,22 @@ bool CBanDB::Write(const banmap_t& banSet)
 5     for (const auto& err : errors) {
 6         error("%s", err);
 7     }
 8     return false;
 9 }
10 
11-bool CBanDB::Read(banmap_t& banSet, bool& dirty)
12+bool CBanDB::Read(banmap_t& banSet)
13 {
14     if (fs::exists(m_banlist_dat)) {
15         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);
16     }
17     // If the JSON banlist does not exist, then recreate it
18     if (!fs::exists(m_banlist_json)) {
19-        dirty = true;
20         return false;
21     }
22 
23-    dirty = false;
24-
25     std::map<std::string, util::SettingsValue> settings;
26     std::vector<std::string> errors;
27 
28     if (!util::ReadSettings(m_banlist_json, settings, errors)) {
29         for (const auto& err : errors) {
30             LogPrintf("Cannot load banlist %s: %s\n", m_banlist_json.string(), err);
31diff --git i/src/addrdb.h w/src/addrdb.h
32index 0213a14af5..1e0ccb1f60 100644
33--- i/src/addrdb.h
34+++ w/src/addrdb.h
35@@ -92,17 +92,15 @@ public:
36     bool Write(const banmap_t& banSet);
37 
38     /**
39      * Read the banlist from disk.
40      * [@param](/bitcoin-bitcoin/contributor/param/)[out] banSet The loaded list. Set if `true` is returned, otherwise it is left
41      * in an undefined state.
42-     * [@param](/bitcoin-bitcoin/contributor/param/)[out] dirty Indicates whether the loaded list needs flushing to disk. Set if
43-     * `true` is returned, otherwise it is left in an undefined state.
44      * [@return](/bitcoin-bitcoin/contributor/return/) true on success
45      */
46-    bool Read(banmap_t& banSet, bool& dirty);
47+    bool Read(banmap_t& banSet);
48 };
49 
50 /**
51  * Dump the anchor IP address database (anchors.dat)
52  *
53  * Anchors are last known outgoing block-relay-only peers that are
54diff --git i/src/banman.cpp w/src/banman.cpp
55index d2437e6733..c64a48a05a 100644
56--- i/src/banman.cpp
57+++ w/src/banman.cpp
58@@ -15,13 +15,13 @@
59 BanMan::BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time)
60     : m_client_interface(client_interface), m_ban_db(std::move(ban_file)), m_default_ban_time(default_ban_time)
61 {
62     if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…").translated);
63 
64     int64_t n_start = GetTimeMillis();
65-    if (m_ban_db.Read(m_banned, m_is_dirty)) {
66+    if (m_ban_db.Read(m_banned)) {
67         SweepBanned(); // sweep out unused entries
68 
69         LogPrint(BCLog::NET, "Loaded %d banned node addresses/subnets  %dms\n", m_banned.size(),
70                  GetTimeMillis() - n_start);
71     } else {
72         LogPrintf("Recreating the banlist database\n");

MarcoFalke commented at 10:44 am on July 29, 2021:

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

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

vasild commented at 10:53 am on July 29, 2021:

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

in src/addrdb.cpp:203 in fa718eab70 outdated

198@@ -199,11 +199,13 @@ bool CBanDB::Write(const banmap_t& banSet)
199 
200 bool CBanDB::Read(banmap_t& banSet, bool& dirty)
201 {
202-    // If the JSON banlist does not exist, then try to read the non-upgraded banlist.dat.
203+    if (fs::exists(m_banlist_dat)) {
204+        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);

vasild commented at 8:02 am on July 29, 2021:

nit:

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

MarcoFalke commented at 10:13 am on July 29, 2021:

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

practicalswift commented at 8:41 am on July 29, 2021: contributor

Concept ACK

in src/netaddress.h:123 in fa718eab70 outdated

119@@ -120,354 +120,354 @@ static constexpr uint16_t I2P_SAM31_PORT{0};
120  */
121 class CNetAddr
122 {
123-    protected:

vasild commented at 9:48 am on July 29, 2021:

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:

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

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

MarcoFalke commented at 10:27 am on July 29, 2021:

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

vasild commented at 10:50 am on July 29, 2021:

 0commit f8a3771957cb1d4d202cd7e3173edfbbeed2657e
 1Parent: faaf9e930d94328c6be42516beab817d45d847f1
 2Author:     Vasil Dimov <vd@FreeBSD.org>
 3AuthorDate: Thu Jul 29 12:39:49 2021 +0200
 4Commit:     Vasil Dimov <vd@FreeBSD.org>
 5CommitDate: Thu Jul 29 12:39:49 2021 +0200
 6
 7
 8    scripted-diff: violate guidelines
 9    
10    -BEGIN VERIFY SCRIPT-
11    clang-format -i src/netaddress.h src/test/fuzz/deserialize.cpp
12    -END VERIFY SCRIPT-
13
14diff --git a/src/netaddress.h b/src/netaddress.h
15index c81f9fd5d8..eb35ed3fac 100644
16--- a/src/netaddress.h
17+++ b/src/netaddress.h
18...
19same diff as in fa81dae3db
20...

Verify:

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

vasild commented at 9:49 am on July 29, 2021: member

Approach ACK

Just to confirm that my understanding is correct: if this is merged now in master, it will be included in 23.0 but not in 22.1, right?

MarcoFalke commented at 10:08 am on July 29, 2021: member

Just to confirm that my understanding is correct: if this is merged now in master, it will be included in 23.0 but not in 22.1, right?

Yes, the pull request is to the master branch. 22.x is a different branch: https://github.com/bitcoin/bitcoin/tree/22.x

MarcoFalke force-pushed on Jul 29, 2021

vasild approved

vasild commented at 3:06 pm on July 29, 2021: member

ACK fac51e94ff2781de70ad4e8d3b4868a2d148d237

Zero-1729 approved

Zero-1729 commented at 7:37 pm on July 29, 2021: contributor

ACK fac51e94ff2781de70ad4e8d3b4868a2d148d237

vasild commented at 8:47 am on July 30, 2021: member

Further cleanup is possible:

https://github.com/bitcoin/bitcoin/blob/787296eb6744b15ab693c053e4030ff68dfc95e0/src/test/fuzz/banman.cpp#L55

https://github.com/bitcoin/bitcoin/blob/787296eb6744b15ab693c053e4030ff68dfc95e0/src/test/fuzz/banman.cpp#L117

Ignore banlist.dat

This also allows to remove the "dirty" argument, which can now be
deduced from the return value of Read().

fa384fdd0b

Remove unused CSubNet serialize code fa4e6afdae

Fix whitespace in touched files

Leaving the incorrect indentation would be frustrating because:
* Some editor may fix up the whitespace when editing a file, so before
  commiting the whitespace changes need to be undone.
* It makes it harder to use clang-format-diff on a change.

Can be trivially reviewed with --word-diff-regex=. --ignore-all-space

fa1eddb1a3

MarcoFalke force-pushed on Jul 30, 2021

Zero-1729 commented at 9:30 am on July 30, 2021: contributor

re-ACK fa1eddb1a3d1319ddc3643b6f34fe2014de32764

vasild approved

vasild commented at 10:58 am on July 30, 2021: member

ACK fa1eddb1a3d1319ddc3643b6f34fe2014de32764

jonatack commented at 11:57 am on July 30, 2021: member

Light code review utACK fa1eddb1a3d1319ddc3643b6f34fe2014de32764

Zero-1729 approved

laanwj commented at 10:43 am on August 2, 2021: member

Not a big fan of the huge whitespace change in the last commit, but otherwise concept and code review ACK fa1eddb1a3d1319ddc3643b6f34fe2014de32764

laanwj merged this on Aug 2, 2021

laanwj closed this on Aug 2, 2021

MarcoFalke deleted the branch on Aug 2, 2021

MarcoFalke commented at 2:20 pm on August 2, 2021: member

Release notes for 23.0 added in #22603

sidhujag referenced this in commit 1273a5204d on Aug 4, 2021

MarcoFalke referenced this in commit 2b06af1747 on Aug 4, 2021

sidhujag referenced this in commit 6cb8e022bc on Aug 4, 2021

MarcoFalke commented at 8:23 am on August 7, 2021: member

Looks like we can re-enable the disabled assert again due to the removal of the code here: #22657

MarcoFalke referenced this in commit 2c6707be8b on Sep 6, 2021

DrahtBot locked this on Aug 16, 2022

Ignore banlist.dat #22570

Conflicts