Merge bitcoin/bitcoin#22362: Drop only invalid entries when reading banlist.json

faa6c3d44c861c0486c1369e1d098b7645ab07cd net: Drop only invalid entries when reading banlist.json (MarcoFalke)

Pull request description:

  All entries will be dropped when there is at least one invalid one in `banlist.json`. Fix this by only dropping invalid ones.

  Also suggested in https://github.com/bitcoin/bitcoin/pull/20966#issuecomment-861150204

ACKs for top commit:
  laanwj:
    Re-ACK faa6c3d44c861c0486c1369e1d098b7645ab07cd

Tree-SHA512: 5a58e7f1dcabf78d0c65d8c6d5d997063af1efeaa50ca7730fc00056fda7e0061b6f7a38907ea045fe667c9f61d392e01e556b425a95e6b126e3c41cd33deb83

3 files changed, 57 insertions, 4 deletions

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index 06d195aaaf..326409c078 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -69,6 +69,7 @@ BITCOIN_TESTS =\
   test/allocator_tests.cpp \
   test/amount_tests.cpp \
   test/arith_uint256_tests.cpp \
+  test/banman_tests.cpp \
   test/base32_tests.cpp \
   test/base58_tests.cpp \
   test/base64_tests.cpp \
diff --git a/src/net_types.cpp b/src/net_types.cpp
index c8f57fe6c6..e4101a9876 100644
--- a/src/net_types.cpp
+++ b/src/net_types.cpp
@@ -4,12 +4,16 @@
 
 #include <net_types.h>
 
+#include <logging.h>
 #include <netaddress.h>
 #include <netbase.h>
 #include <univalue.h>
 
+static const char* BANMAN_JSON_VERSION_KEY{"version"};
+
 CBanEntry::CBanEntry(const UniValue& json)
-    : nVersion(json["version"].get_int()), nCreateTime(json["ban_created"].get_int64()),
+    : nVersion(json[BANMAN_JSON_VERSION_KEY].get_int()),
+      nCreateTime(json["ban_created"].get_int64()),
       nBanUntil(json["banned_until"].get_int64())
 {
 }
@@ -17,7 +21,7 @@ CBanEntry::CBanEntry(const UniValue& json)
 UniValue CBanEntry::ToJson() const
 {
     UniValue json(UniValue::VOBJ);
-    json.pushKV("version", nVersion);
+    json.pushKV(BANMAN_JSON_VERSION_KEY, nVersion);
     json.pushKV("ban_created", nCreateTime);
     json.pushKV("banned_until", nBanUntil);
     return json;
@@ -54,11 +58,16 @@ UniValue BanMapToJson(const banmap_t& bans)
 void BanMapFromJson(const UniValue& bans_json, banmap_t& bans)
 {
     for (const auto& ban_entry_json : bans_json.getValues()) {
+        const int version{ban_entry_json[BANMAN_JSON_VERSION_KEY].get_int()};
+        if (version != CBanEntry::CURRENT_VERSION) {
+            LogPrintf("Dropping entry with unknown version (%s) from ban list\n", version);
+            continue;
+        }
         CSubNet subnet;
         const auto& subnet_str = ban_entry_json[BANMAN_JSON_ADDR_KEY].get_str();
         if (!LookupSubNet(subnet_str, subnet)) {
-            throw std::runtime_error(
-                strprintf("Cannot parse banned address or subnet: %s", subnet_str));
+            LogPrintf("Dropping entry with unparseable address or subnet (%s) from ban list\n", subnet_str);
+            continue;
         }
         bans.insert_or_assign(subnet, CBanEntry{ban_entry_json});
     }
diff --git a/src/test/banman_tests.cpp b/src/test/banman_tests.cpp
new file mode 100644
index 0000000000..27ce9ad638
--- /dev/null
+++ b/src/test/banman_tests.cpp
@@ -0,0 +1,43 @@
+// Copyright (c) 2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <banman.h>
+#include <chainparams.h>
+#include <netbase.h>
+#include <streams.h>
+#include <test/util/logging.h>
+#include <test/util/setup_common.h>
+#include <util/readwritefile.h>
+
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_FIXTURE_TEST_SUITE(banman_tests, BasicTestingSetup)
+
+BOOST_AUTO_TEST_CASE(file)
+{
+    SetMockTime(777s);
+    const fs::path banlist_path{m_args.GetDataDirBase() / "banlist_test"};
+    {
+        const std::string entries_write{
+            "{ \"banned_nets\": ["
+            "  { \"version\": 1, \"ban_created\": 0, \"banned_until\": 778, \"address\": \"aaaaaaaaa\" },"
+            "  { \"version\": 2, \"ban_created\": 0, \"banned_until\": 778, \"address\": \"bbbbbbbbb\" },"
+            "  { \"version\": 1, \"ban_created\": 0, \"banned_until\": 778, \"address\": \"1.0.0.0/8\" }"
+            "] }",
+        };
+        assert(WriteBinaryFile(banlist_path + ".json", entries_write));
+        {
+            // The invalid entries will be dropped, but the valid one remains
+            ASSERT_DEBUG_LOG("Dropping entry with unparseable address or subnet (aaaaaaaaa) from ban list");
+            ASSERT_DEBUG_LOG("Dropping entry with unknown version (2) from ban list");
+            BanMan banman{banlist_path, /*client_interface=*/nullptr, /*default_ban_time=*/0};
+            banmap_t entries_read;
+            banman.GetBanned(entries_read);
+            assert(entries_read.size() == 1);
+        }
+    }
+}
+
+BOOST_AUTO_TEST_SUITE_END()