Save the banlist in banlist.json instead of banlist.dat.
This makes it possible to store Tor v3 entries in the banlist on disk (and any other addresses that cannot be serialized in addrv1 format).
Only read banlist.dat if it exists and banlist.json does not exist (first start after an upgrade).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
110+
111+- The list of banned hosts and networks (via `setban` RPC) is now saved on disk
112+ in JSON format in `banlist.json` instead of `banlist.dat`. Both files are
113+ read on startup and contents merged. Updateds are only written to the new
114+ `banlist.json`. A future version of Bitcoin Core may completely ignore
115+ `banlist.dat`. (#20966)
Please add the release notes to https://github.com/bitcoin-core/bitcoin-devwiki/wiki/%2320852-Release-notes-snippet
Otherwise they will have to be removed here after merge, then added to the 0.21 branch. (Release notes are only added to the point releases, not the next major version that also includes those fixes)
doc/release-notes.md
Notes:
I guess it would be possible to write the ban list to settings.json instead of in banlist.json (as originally proposed in #19748), but the periodic dump may create some complications and also it is unclear what to do if we are running with -nosettings.
The current patch would write banlist.json on the first change of the ban list. This means that a node that upgrades from before this PR with only banlist.dat on disk may not create banlist.json for some time. I think this is ok.
Concept ACK.
Should try to read JSON first, and if it exists then skip old format? Or am I missing something?
Personally I wouldn’t add from/to JSON in CBanEntry, BanMapFromJson and BanMapToJson seem enough. Also, there should be some validation in BanMapFromJson?
fd751850e…e74196741: read banlist.dat only if banlist.json is not present and catch all exceptions that could come from an invalid JSON.
@promag
Should try to read JSON first, and if it exists then skip old format?
I made it to read both if present so that if the user temporary downgrades and makes some changes to banlist.dat those changes would be picked up by the new version. However this has the deficiency that if an entry is deleted from banlist.json it will keep being loaded from banlist.dat. Changed to what you suggest above.
Personally I wouldn’t add from/to JSON in
CBanEntry,BanMapFromJsonandBanMapToJsonseem enough.
CBanEntry being able to ser/unser itself to/from JSON improves its encapsulation.
Also, there should be some validation in
BanMapFromJson?
Right! Added try/catch to handle invalid JSONs.
e74196741...7493bbe38: rebase due to conflicts
7493bbe38...1310b5460: rebase due to conflicts
1310b5460...469da8ca3: rebase due to conflicts
43+ * passing to `BanMapFromJson()`.
44+ */
45+UniValue BanMapToJson(const banmap_t& bans)
46+{
47+ UniValue bans_json(UniValue::VARR);
48+ for (const auto& [address, ban_entry] : bans) {
8372aad9cc01774e981edee1dd8822a31ef6ea18
is the goal still to backport this? If yes, it could make sense to use C++11
My understanding is that this deserves a backport because it would allow storing torv3 addresses in the banlist on disk. Not super important though.
Switched to C++11.
64+{
65+ for (const auto& ban_entry_json : bans_json.getValues()) {
66+ CSubNet subnet;
67+ const auto& subnet_str = ban_entry_json["address"].get_str();
68+ if (!LookupSubNet(subnet_str, subnet)) {
69+ return error("%s: Cannot parse banned address or subnet: %s", __func__, subnet_str);
8372aad9cc01774e981edee1dd8822a31ef6ea18
error here, but std::runtime_error if a field is missing?
You probably mean -logsourcelocations instead of -logfunctionnames.
Removed __func__. Changed this function to throw an exception for all errors and return void. Now it does not do any logging - just bubble up a detailed error message to the callers via the exception.
189+ if (util::WriteSettings(m_banlist_json, {{JSON_KEY, BanMapToJson(banSet)}}, errors)) {
190+ return true;
191+ }
192+
193+ for (const auto& err : errors) {
194+ error("%s: %s", __func__, err);
same commit:
__func__
The caller of CBanDB::Write() does not ignore its return code? If you mean the return code of error() itself - this is because we want to log all of the errors (possibly more than one) before returning false.
Removed __func__.
206+ std::map<std::string, util::SettingsValue> settings;
207+ std::vector<std::string> errors;
208+
209+ if (!util::ReadSettings(m_banlist_json, settings, errors)) {
210+ for (const auto& err : errors) {
211+ error("%s: %s", __func__, err);
same commit:
__func__ ;)
I think it is ok to see something like the following in the log:
0ERROR: ... banlist foo ...
1ERROR: ... banlist bar ...
2Recreating the banlist database
Removed __func__.
ERROR. For example, http://www.erisian.com.au/bitcoin-core-dev/log-2021-02-13.html#l-296
I changed it to avoid the word “error”. I don’t think those errors should be hushed - those are errors from loading the file and we already checked that the file exists. So may include something like “json parse error”, “io error”, “permission denied”.
In general I don’t think we should avoid using “error” or use it just for cases where we have detected a bug in the program. Messages like “error: permission denied” or “error: connection reset” are to be expected.
error() too much.
214+ }
215+
216+ try {
217+ return BanMapFromJson(settings[JSON_KEY], banSet);
218+ } catch (const std::runtime_error& e) {
219+ return error("%s: Cannot parse %s: %s", __func__, m_banlist_json.string(), e.what());
__func__.
33@@ -34,8 +34,9 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
34 {
35 FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
36 SetMockTime(ConsumeTime(fuzzed_data_provider));
37- const fs::path banlist_file = GetDataDir() / "fuzzed_banlist.dat";
38- fs::remove(banlist_file);
39+ const fs::path banlist_file = GetDataDir() / "fuzzed_banlist";
40+ fs::remove(banlist_file.string() + ".dat");
same commit:
I don’t understand this line. There shouldn’t be any reason this instance will ever write a banlist.dat file.
Also, to make the fuzz test more useful, it should probably create a banlist.dat file. Extra points for creating a corrupted one. With extra bonus points for passing one in without disk access.
fs::remove calls and replaced them with a creation of a corrupted or inaccessible banlist file.
review ACK 469da8ca3f566041972a87e847e7ecf44ef2306b 🎐
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK 469da8ca3f566041972a87e847e7ecf44ef2306b 🎐
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUiXDwv9E2HLRr4rmJmOP/mO+47lhG6gEcqx6EAYuIjlBklEnDK9PhDo43/t3AzN
8suyNHfsJ9mGGD/QXoZxUYge2dfqaH1U1y/BdrlVZw02r/HXqvOGzJ4YMjV4ysw04
9vVh0U1osD7w3ypYLR0BhcAj4SVR1NDT3SYMKf8qlfPFtXfrn6W9i0kG6siwdV6ZE
10mw/IqbisqDq92azMw+26z07Mu7uXv4Z1zWiz0qD7KBsJ/pinDdXCNi45gde7G091
119tMF7KOJYyv/6uY/ibXoq/MYSDvkwye0SgGkNh6kku1MBjscBZG2KF/B6YIZMOn7
122GTl+vRyUWMmsmANB7ELHyJs0wlh30bNGpjUErWfYwH4fs7KKLiQfvohms3UFOI4
13oLeAsqpDo8HX8X0SzMmIOib/bhg6+X5yxIJW113pLfuohc35bBKEHATvZlzmYjSZ
14lXFVLKY38YKsbxCBoXr3ISL7OeKKn6AwFVFZLjA8BlbogASWI9bunmJyHuC/o1n2
15n2ha9e1V
16=sn/g
17-----END PGP SIGNATURE-----
Timestamp of file with hash 4f0238a21835acd2f6c1095be832c4be7733461a6b3f0bf1498f5bab2c98df7d -
469da8ca3...4efa5e73b: address review suggestions, remove the release notes that were removed before but sneaked back in one of the updates
git rebase --skip
187@@ -188,7 +188,7 @@ void BanMan::SweepBanned()
188 m_banned.erase(it++);
189 m_is_dirty = true;
190 notify_ui = true;
191- LogPrint(BCLog::NET, "%s: Removed banned node ip/subnet from banlist.dat: %s\n", __func__, sub_net.ToString());
192+ LogPrint(BCLog::NET, "%s: Removed banned node address/subnet: %s\n", __func__, sub_net.ToString());
__func__ while touching this line?
03239a3ab...aa90a7d46: address suggestions
50@@ -51,12 +51,22 @@ def run_test(self):
51 ip_addr = "1.2.3.4"
52 assert(not self.is_banned(node, tor_addr))
53 assert(not self.is_banned(node, ip_addr))
54+
55 node.setban(tor_addr, "add")
56 assert(self.is_banned(node, tor_addr))
57 assert(not self.is_banned(node, ip_addr))
58+
59+ self.restart_node(1)
aa90a7d46a506b26a6016d1171f7ddae809e9c99
nit, here and below, add a comment on why the check is repeated after the restart.
175@@ -119,18 +176,49 @@ bool DeserializeFileDB(const fs::path& path, Data& data)
176 }
177 } // namespace
178
179-CBanDB::CBanDB(fs::path ban_list_path) : m_ban_list_path(std::move(ban_list_path))
180+CBanDB::CBanDB(fs::path ban_list_path)
181+ : m_banlist_dat(ban_list_path.string() + ".dat"),
51909ded1e8b26158f8d36cfe323ac72feac1d78
nit, fs::path(ban_list_path).replace_extension("dat").
replace_extension() a bit confusing in this case because we know that there is no extension, so we are not replacing anything (I know it will do the right thing, but still).
198 }
199
200 bool CBanDB::Read(banmap_t& banSet)
201 {
202- return DeserializeFileDB(m_ban_list_path, banSet);
203+ if (!fs::exists(m_banlist_json)) {
51909ded1e8b26158f8d36cfe323ac72feac1d78
A comment here would be nice. Or maybe just take the next suggestion so it’s clear what this is doing.
199
200 bool CBanDB::Read(banmap_t& banSet)
201 {
202- return DeserializeFileDB(m_ban_list_path, banSet);
203+ if (!fs::exists(m_banlist_json)) {
204+ return DeserializeFileDB(m_banlist_dat, banSet);
51909ded1e8b26158f8d36cfe323ac72feac1d78
Maybe migrate on 1st run?
0 bool CBanDB::Read(banmap_t& banSet)
1 {
2 if (!fs::exists(m_banlist_json)) {
3- return DeserializeFileDB(m_banlist_dat, banSet);
4+ if (!DeserializeFileDB(m_banlist_dat, banSet)) {
5+ return false;
6+ }
7+ // log something
8+ Write(banSet);
9+ return true;
10 }
Read() calling Write() seems like some anti-pattern to me. Instead I signal from Read() that the list needs re-flushing, so that the caller takes care of it.
CBanDB::MigrateAndRead where calling Write() isn’t so bad.
aa90a7d46...7e399e2de: rebase due to conflicts
7e399e2de...08f4f98ea: address some suggestions; force saving on disk if we loaded just the old banlist.dat so that we create the new banlist.json this way, without waiting for mods to the list in order to flush it to disk.
54@@ -55,7 +55,8 @@ Subdirectory | File(s) | Description
55 `indexes/blockfilter/basic/` | `fltrNNNNN.dat`<sup>[\[2\]](#note2)</sup> | Blockfilter index filters for the basic filtertype; *optional*, used if `-blockfilterindex=basic`
56 `wallets/` | | [Contains wallets](#multi-wallet-environment); can be specified by `-walletdir` option; if `wallets/` subdirectory does not exist, wallets reside in the [data directory](#data-directory-location)
57 `./` | `anchors.dat` | Anchor IP address database, created on shutdown and deleted at startup. Anchors are last known outgoing block-relay-only peers that are tried to re-connect to on startup
58-`./` | `banlist.dat` | Stores the IPs/subnets of banned nodes
59+`./` | `banlist.dat` | Stores the addresses/subnets of banned nodes (deprecated). Bitcoin Core will not save the banlist to it. It would only read it on startup if `banlist.json` is not present.
s/would/will/
Suggested version all in present tense:
0`./` | `banlist.dat` | Stores the addresses/subnets of banned nodes (deprecated); `bitcoind` or `bitcoin-qt` no longer save the banlist to this file, but read it on startup if `banlist.json` is not present.
if you retouch, in the same commit and file would you please touch up:
0 ## Legacy subdirectories and files
1
2-These subdirectories and files are no longer used by the Bitcoin Core:
3+These subdirectories and files are no longer used by Bitcoin Core:
66 assert(not self.is_banned(self.nodes[1], tor_addr))
67 assert(not self.is_banned(node, ip_addr))
68
69+ self.restart_node(1)
70+ assert(not self.is_banned(self.nodes[1], tor_addr))
71+ assert(not self.is_banned(node, ip_addr))
08f4f98ea nit, drop the outer parens on the 4 added asserts and add a second newline at line 72 (per pycodestyle, “E305 expected 2 blank lines after class or function definition, found 1”)
(the black linter, e.g. apt install black && black test/functional/rpc_setban.py will do both of these for you)
assert() I added the new code with assert() for consistency and then removed all () from asserts in the entire file in a separate commit.
banlist.json file for torv3 and i2p addresses. As expected, attempting to remove any previously incorrectly saved addrv2 addresses fails to recognize them unless the “new” address is stipulated, but subsequent setban add/remove operations work correctly. This testing was done by simultaneously looking at the banlist files and running setban/listbanned, shutting down, sometimes rm-ing the banlist.json file, restarting on master and with this patch, and repeating the testing.
08f4f98ea...fe4aafda5: apply doc and style suggestions
… remove any leftover (e.g. “::/0”) addresses from mis-saved addrv2 ban entries when creating the json file …
Hmm. Could there be real, manually added, no-mistake, ban entries like ::/0? Somebody has banned deliberately the entire IPv6 network?
21@@ -22,7 +22,7 @@ def run_test(self):
22 # Node 0 connects to Node 1, check that the noban permission is not granted
23 self.connect_nodes(0, 1)
24 peerinfo = self.nodes[1].getpeerinfo()[0]
25- assert(not 'noban' in peerinfo['permissions'])
26+ assert not "noban" in peerinfo["permissions"]
fe4aafda512483e863b23cfad403509b1604555c drive-by nit, while touching lines 25 and 46 in this commit (maybe just make it a PEP8/black linter commit)
$ pycodestyle test/functional/rpc_setban.py
test/functional/rpc_setban.py:25:16: E713 test for membership should be 'not in'
test/functional/rpc_setban.py:46:16: E713 test for membership should be 'not in'
doc/. Don’t want to bother other reviewers with too big and unrelated changes to this PR.
Ok. For info, pycodestyle is PEP8 that we follow per doc/test/functional/README.md
lint-python.sh is happy with the current rpc_setban.py:
0$ ./test/lint/lint-python.sh
1test/functional/test_runner.py:42: error: Module has no attribute "getwindowsversion"
2Found 1 error in 1 file (checked 199 source files)
3$
assert x not in y being easier than not assert x in y
… remove any leftover (e.g. “::/0”) addresses from mis-saved addrv2 ban entries when creating the json file …
Hmm. Could there be real, manually added, no-mistake, ban entries like
::/0? Somebody has banned deliberately the entire IPv6 network?
Good question. Would anyone do that. OTOH, have people been banning tor v3 addresses yet. Possible. IDK if it’s an edge case worth worrying about.
185
186 bool CBanDB::Write(const banmap_t& banSet)
187 {
188- return SerializeFileDB("banlist", m_ban_list_path, banSet);
189+ std::vector<std::string> errors;
190+ if (util::WriteSettings(m_banlist_json, {{JSON_KEY, BanMapToJson(banSet)}}, errors)) {
(Read)WriteSettings() to something like (Read)WriteJsonObjFile()?
47+ UniValue bans_json(UniValue::VARR);
48+ for (const auto& it : bans) {
49+ const auto& address = it.first;
50+ const auto& ban_entry = it.second;
51+ UniValue j = ban_entry.ToJson();
52+ j.pushKV("address", address.ToString());
const and re-used in BanMapFromJson()?
65+void BanMapFromJson(const UniValue& bans_json, banmap_t& bans)
66+{
67+ for (const auto& ban_entry_json : bans_json.getValues()) {
68+ CSubNet subnet;
69+ const auto& subnet_str = ban_entry_json["address"].get_str();
70+ if (!LookupSubNet(subnet_str, subnet)) {
version and banned_until as well?
I am not sure. version and banned_until are not sanity-checked when reading banlist.dat, so I do the same with banlist.json as the purpose of this PR is to switch the format, not introduce new checks.
Maybe worth considering separately.
96+
97+ /**
98+ * Read the banlist from disk.
99+ * @param[out] banSet The loaded list, set if `true` is returned, otherwise it is left
100+ * in an undefined state.
101+ * @param[out] dirty Indicates whether the loaded list needs flushing to disk, only set
only set if
trueis returned.
I think it’s possible for it to be set and yet the function return false if DeserializeFileDB returns false due to an invalid or missing file.
186 bool CBanDB::Write(const banmap_t& banSet)
187 {
188- return SerializeFileDB("banlist", m_ban_list_path, banSet);
189+ std::vector<std::string> errors;
190+ if (util::WriteSettings(m_banlist_json, {{JSON_KEY, BanMapToJson(banSet)}}, errors)) {
191+ return true;
banlist.dat if it does exist at this point? Or a log to inform the user that they can remove it? Might be confusing to have two files both claiming to be banlists.
I deliberately avoid removing banlist.dat automatically because that would prevent downgrading to an older version (and keeping the ban list).
Something like this may be a good idea:
0--- i/src/addrdb.cpp
1+++ w/src/addrdb.cpp
2@@ -185,12 +185,18 @@ CBanDB::CBanDB(fs::path ban_list_path)
3 }
4
5 bool CBanDB::Write(const banmap_t& banSet)
6 {
7 std::vector<std::string> errors;
8 if (util::WriteSettings(m_banlist_json, {{JSON_KEY, BanMapToJson(banSet)}}, errors)) {
9+ if (fs::exists(m_banlist_dat)) {
10+ LogPrintf("Successfully saved %s to disk. %s can be removed manually now if "
11+ "downgrading to an older version is not planned.\n",
12+ m_banlist_json.string(),
13+ m_banlist_dat.string());
14+ }
15 return true;
16 }
17
18 for (const auto& err : errors) {
19 error("%s", err);
20 }
but it would be printed every time which would be annoying. There is some doc added to doc/files.md which should clarify things if the user wonders why there are two files. Maybe leave it as is?
We can add an automatic removal of banlist.dat one major version after this change is released.
banlist.dat one major version later seems like a fair choice!
16@@ -17,7 +17,8 @@
17
18 // NOTE: When adjusting this, update rpcnet:setban's help ("24h")
19 static constexpr unsigned int DEFAULT_MISBEHAVING_BANTIME = 60 * 60 * 24; // Default 24-hour ban
20-// How often to dump addresses to banlist.dat
21+
22+/// How often to dump banned addresses/subnets to disk.
35@@ -34,8 +36,20 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
36 {
37 FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
38 SetMockTime(ConsumeTime(fuzzed_data_provider));
39- const fs::path banlist_file = GetDataDir() / "fuzzed_banlist.dat";
40- fs::remove(banlist_file);
41+ fs::path banlist_file = GetDataDir() / "fuzzed_banlist";
banlist_file and only after that execute the actual test.
ACK fe4aafd
Code reviewed and tested locally. Thank you for the patch @vasild ! I especially like that node operators will have an option to share banlists as a human-readable json file instead of rpc commands 🚀
To verify things work as intended:
0$ git pull --rebase master
1$ make -j 9
2$ mkdir -p node0 node1
3
4# Run node 0
5$ src/bitcoind -regtest -datadir=`pwd`/node0 -rpccookiefile=.cookie -port=2222 -rpcport=3333
6
7# Run node 1
8$ src/bitcoind -regtest -datadir=`pwd`/node1 -rpccookiefile=.cookie -port=2223 -rpcport=3334
9
10# Node 0 adds Node 1 as peer
11$ src/bitcoin-cli -regtest -datadir=`pwd`/node0 -rpccookiefile=.cookie -rpcport=3333 addnode "127.0.0.1:2223" "onetry"
12
13# Node 1 sees the peer
14$ src/bitcoin-cli -regtest -datadir=`pwd`/node1 -rpccookiefile=.cookie -rpcport=3334 getpeerinfo
15
16#Node 1 bans Node 0
17$ src/bitcoin-cli -regtest -datadir=`pwd`/node1 -rpccookiefile=.cookie -rpcport=3334 setban "127.0.0.1" "add"
18$ src/bitcoin-cli -regtest -datadir=`pwd`/node1 -rpccookiefile=.cookie -rpcport=3334 listbanned
19
20# Node 1 eliminates the peer
21$ src/bitcoin-cli -regtest -datadir=`pwd`/node1 -rpccookiefile=.cookie -rpcport=3334 getpeerinfo
22
23# Stop both nodes manually
24
25# Banlist files on each node
26$ ls -lan node0/regtest | grep banlist
27-rw------- 1 501 20 37 May 14 11:58 banlist.dat
28
29$ ls -lan node1/regtest | grep banlist
30-rw------- 1 501 20 91 May 14 12:08 banlist.dat
31
32$ git checkout -b pr-20966 vasild/json_bans
33$ make -j 9
34
35# Run node 0
36$ src/bitcoind -regtest -datadir=`pwd`/node0 -rpccookiefile=.cookie -port=2222 -rpcport=3333
37
38# Run node 1
39$ src/bitcoind -regtest -datadir=`pwd`/node1 -rpccookiefile=.cookie -port=2223 -rpcport=3334
40
41# Banlist files on each node
42$ ls -lan node0/regtest | grep banlist
43-rw------- 1 501 20 37 May 14 11:58 banlist.dat
44-rw------- 1 501 20 34 May 14 12:21 banlist.json
45
46$ ls -lan node1/regtest | grep banlist
47-rw------- 1 501 20 91 May 14 12:08 banlist.dat
48-rw------- 1 501 20 167 May 14 12:21 banlist.json
49
50$ cat node0/regtest/banlist.json
51{
52 "banned_nets": [
53 ]
54 }
55
56$ cat node1/regtest/banlist.json
57{
58 "banned_nets": [
59 {
60 "version": 1,
61 "ban_created": 1621019336,
62 "banned_until": 1621105736,
63 "address": "127.0.0.1/32"
64 }
65 ]
66 }
67
68# Unban node 0 from node 1
69$ src/bitcoin-cli -regtest -datadir=`pwd`/node1 -rpccookiefile=.cookie -rpcport=3334 setban "127.0.0.1" "remove"
70
71# Verify banlist.json updated
72$ cat node1/regtest/banlist.json
73{
74 "banned_nets": [
75 ]
76 }
77
78# Manually restart node1
79# Verify unchanged banlist.json
80$ cat node1/regtest/banlist.json
81{
82 "banned_nets": [
83 ]
84 }
However, this is the behavior I’d like to point to and make sure it’s ok:
0# Manually stop node1
1# Node 1 deletes banlist.json to clear all bans
2$ rm node1/regtest/banlist.json
3# Restart node 1
4
5# The ban comes back from banlist.dat
6$ src/bitcoin-cli -regtest -datadir=`pwd`/node1 -rpccookiefile=.cookie -rpcport=3334 listbanned
7[
8 {
9 "address": "127.0.0.1/32",
10 "ban_created": 1621019336,
11 "banned_until": 1621105736,
12 "ban_duration": 86400,
13 "time_remaining": 85186
14 }
15]
16
17$ cat node1/regtest/banlist.json
18{
19 "banned_nets": [
20 {
21 "version": 1,
22 "ban_created": 1621019336,
23 "banned_until": 1621105736,
24 "address": "127.0.0.1/32"
25 }
26 ]
27 }
Some other suggestions below.
fe4aafda51...13c681161c: address review suggestions
this is the behavior I’d like to point to and make sure it’s ok … Node 1 deletes banlist.json to clear all bans … The ban comes back from banlist.dat
Yes, this is how it is supposed to work. The operator should have seen that there are two files banlist.dat and banlist.json and deleted both or consulted doc/files.md. We don’t automatically delete banlist.dat after creating banlist.json because that would create issues if the user decides to downgrade to the previous version which only recognizes banlist.dat.
13c6811
Diff-review re-ACK 13c681161c4a3f2a2acd6d812c1360e0ad94019c per git diff 08f4f98 13c6811 and git range-diff d4c409c 08f4f98 13c6811
Only non-showstopping mini-regret is the Python “not in” not updated while re-touching to conform to PEP8 per our functional test guidelines.
13c681161c...88b843e49c: rebase due to conflicts
0--- a/doc/release-notes.md
1+++ b/doc/release-notes.md
2@@ -182,12 +182,21 @@ RPC
3 - `verifymessage` now returns RPC_TYPE_ERROR (-3) if the passed signature
4 is malformed. Previously returned RPC_INVALID_ADDRESS_OR_KEY (-5).
5
6 Tests
7 -----
8
9+Files
10+-----
11+
12+- The list of banned hosts and networks (via `setban` RPC) is now saved on disk
13+ in JSON format in `banlist.json` instead of `banlist.dat`. `banlist.dat` is
14+ only read on startup if `banlist.json` is not present. Changes are only
15+ written to the new `banlist.json`. A future version of Bitcoin Core may
16+ completely ignore `banlist.dat`. (#20966)
17+
18 Credits
19 =======
74+ assert not self.is_banned(self.nodes[1], tor_addr)
75+ assert not self.is_banned(node, ip_addr)
76+
77+ self.restart_node(1)
78+ assert not self.is_banned(self.nodes[1], tor_addr)
79+ assert not self.is_banned(node, ip_addr)
f371273 could be done in a follow-up along with not_in, but if you retouch
0--- a/test/functional/rpc_setban.py
1+++ b/test/functional/rpc_setban.py
2@@ -56,18 +56,17 @@ class SetBanTests(BitcoinTestFramework):
3 assert self.is_banned(node, tor_addr)
4 assert not self.is_banned(node, ip_addr)
5
6- # Test that the ban list is preserved through restart.
7-
8+ self.log.info("Test the ban list is preserved through restart")
9 self.restart_node(1)
10 assert self.is_banned(node, tor_addr)
11 assert not self.is_banned(node, ip_addr)
12
13 node.setban(tor_addr, "remove")
14- assert not self.is_banned(self.nodes[1], tor_addr)
15+ assert not self.is_banned(node, tor_addr)
16 assert not self.is_banned(node, ip_addr)
17
18 self.restart_node(1)
19- assert not self.is_banned(self.nodes[1], tor_addr)
20+ assert not self.is_banned(node, tor_addr)
21 assert not self.is_banned(node, ip_addr)
node in the second one, but left the first one as that line is not modified by this PR.
git range-diff ce4a852 13c6811 88b843e
55@@ -56,7 +56,8 @@ Subdirectory | File(s) | Description
56 `indexes/coinstats/db/` | LevelDB database | Coinstats index; *optional*, used if `-coinstatsindex=1`
57 `wallets/` | | [Contains wallets](#multi-wallet-environment); can be specified by `-walletdir` option; if `wallets/` subdirectory does not exist, wallets reside in the [data directory](#data-directory-location)
58 `./` | `anchors.dat` | Anchor IP address database, created on shutdown and deleted at startup. Anchors are last known outgoing block-relay-only peers that are tried to re-connect to on startup
59-`./` | `banlist.dat` | Stores the IPs/subnets of banned nodes
60+`./` | `banlist.dat` | Stores the addresses/subnets of banned nodes (deprecated). `bitcoind` or `bitcoin-qt` no longer save the banlist to this file, but read it on startup if `banlist.json` is not present.
61+`./` | `banlist.json` | Stores the addresses/subnets of banned nodes.
banlist.dat?
73- assert(not self.is_banned(node, ip_addr))
74+ assert not self.is_banned(self.nodes[1], tor_addr)
75+ assert not self.is_banned(node, ip_addr)
76+
77+ self.restart_node(1)
78+ assert not self.is_banned(self.nodes[1], tor_addr)
what are reasons to use self.nodes[1]? Did I miss something?
88b843e49c...4b52c7234f: rebase due to conflicts
release notes in #20966 (comment)
git range-diff 5c4f0c4 88b843e 4b52c72, rebase only, debug build clean, test passes, repeatedly tested banning/unbanning peers on signet while inspecting banlist.json in my editor including through restarts. However, there are gotchas when downgrading/upgrading; it may be a good idea to warn and suggest best practices about that in the release note.
- The list of banned hosts and networks (via
setbanRPC) is now saved on disk in JSON format inbanlist.jsoninstead ofbanlist.dat.banlist.datis only read on startup ifbanlist.jsonis not present. Changes are only written to the newbanlist.json. A future version of Bitcoin Core may completely ignorebanlist.dat. (#20966)
Regarding the release note, peers may be banned and unbanned via the GUI peers window in addition to the setban RPC. As mentioned in my previous comment, it may be a good idea to describe user gotchas and best practices for upgrading and downgrading.
utACK 4b52c7234f3c2e3067d7d6c6fd7ebf2b96bb8a45
I think it would be useful to make the code ignore future version numbers, so it will not completely wipe banlists on downgrading (from future versions that introduce additional features to this format), but that can be done later.
Save the banlist in `banlist.json` instead of `banlist.dat`.
This makes it possible to store Tor v3 entries in the banlist on disk
(and any other addresses that cannot be serialized in addrv1 format).
Only read `banlist.dat` if it exists and `banlist.json` does not
exist (first start after an upgrade).
Supersedes https://github.com/bitcoin/bitcoin/pull/20904
Resolves https://github.com/bitcoin/bitcoin/issues/19748
With `banlist.dat` (being written in addrv1 format) if we would try to
write a Tor v3 subnet, it would serialize as a dummy-all-0s IPv6
address and subsequently, when deserialized will not result in the same
subnet.
This problem does not exist with `banlist.json` where the data is saved
in textual, human-readable form.
4b52c7234f...5fcc96457d: rebase due to conflicts
5fcc96457d...bb719a08db: minor tweaks in rpc_setban.py - log a message and use node instead of self.nodes[1]
git range-diff 6a67366 4b52c72 bb719a0
