Merge bitcoin/bitcoin#20966: banman: save the banlist in a JSON format on disk

bb719a0 style: remove () from assert in rpc_setban.py (Vasil Dimov)
24b10eb doc: fix grammar in doc/files.md (Vasil Dimov)
dd4e957 test: ensure banlist can be read from disk after restart (Vasil Dimov)
d197977 banman: save the banlist in a JSON format on disk (Vasil Dimov)

Pull request description:

  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 bitcoin/bitcoin#20904
  Resolves bitcoin/bitcoin#19748

ACKs for top commit:
  jonatack:
    Code review re-ACK bb719a0 per `git range-diff 6a67366 4b52c72 bb719a0`
  achow101:
    Code Review ACK bb719a0

Tree-SHA512: fc135c3a1fe20bcf5d008ce6bea251b4135e56c78bf8f750b4bd8144c095b81ffe165133cdc7e4715875eec7e7c4e13ad9f5d2450b21102af063d7c8abf716b6

Author: MarcoFalke

doc/files.md

@@ -56,7 +56,8 @@ Subdirectory       | File(s)               | Description
 `indexes/coinstats/db/` | LevelDB database | Coinstats index; *optional*, used if `-coinstatsindex=1`
 `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)
 `./`               | `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
-`./`               | `banlist.dat`         | Stores the IPs/subnets of banned nodes
+`./`               | `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.
+`./`               | `banlist.json`        | Stores the addresses/subnets of banned nodes.
 `./`               | `bitcoin.conf`        | User-defined [configuration settings](bitcoin-conf.md) for `bitcoind` or `bitcoin-qt`. File is not written to by the software and must be created manually. Path can be specified by `-conf` option
 `./`               | `bitcoind.pid`        | Stores the process ID (PID) of `bitcoind` or `bitcoin-qt` while running; created at start and deleted on shutdown; can be specified by `-pid` option
 `./`               | `debug.log`           | Contains debug information and general logging generated by `bitcoind` or `bitcoin-qt`; can be specified by `-debuglogfile` option
@@ -109,7 +110,7 @@ Subdirectory | File                 | Description
 
 ## Legacy subdirectories and files
 
-These subdirectories and files are no longer used by the Bitcoin Core:
+These subdirectories and files are no longer used by Bitcoin Core:
 
 Path           | Description | Repository notes
 ---------------|-------------|-----------------

src/addrdb.cpp

@@ -11,13 +11,72 @@
 #include <cstdint>
 #include <hash.h>
 #include <logging/timer.h>
+#include <netbase.h>
 #include <random.h>
 #include <streams.h>
 #include <tinyformat.h>
+#include <univalue.h>
+#include <util/settings.h>
 #include <util/system.h>
 
+CBanEntry::CBanEntry(const UniValue& json)
+    : nVersion(json["version"].get_int()), nCreateTime(json["ban_created"].get_int64()),
+      nBanUntil(json["banned_until"].get_int64())
+{
+}
+
+UniValue CBanEntry::ToJson() const
+{
+    UniValue json(UniValue::VOBJ);
+    json.pushKV("version", nVersion);
+    json.pushKV("ban_created", nCreateTime);
+    json.pushKV("banned_until", nBanUntil);
+    return json;
+}
+
 namespace {
 
+static const char* BANMAN_JSON_ADDR_KEY = "address";
+
+/**
+ * Convert a `banmap_t` object to a JSON array.
+ * @param[in] bans Bans list to convert.
+ * @return a JSON array, similar to the one returned by the `listbanned` RPC. Suitable for
+ * passing to `BanMapFromJson()`.
+ */
+UniValue BanMapToJson(const banmap_t& bans)
+{
+    UniValue bans_json(UniValue::VARR);
+    for (const auto& it : bans) {
+        const auto& address = it.first;
+        const auto& ban_entry = it.second;
+        UniValue j = ban_entry.ToJson();
+        j.pushKV(BANMAN_JSON_ADDR_KEY, address.ToString());
+        bans_json.push_back(j);
+    }
+    return bans_json;
+}
+
+/**
+ * Convert a JSON array to a `banmap_t` object.
+ * @param[in] bans_json JSON to convert, must be as returned by `BanMapToJson()`.
+ * @param[out] bans Bans list to create from the JSON.
+ * @throws std::runtime_error if the JSON does not have the expected fields or they contain
+ * unparsable values.
+ */
+void BanMapFromJson(const UniValue& bans_json, banmap_t& bans)
+{
+    for (const auto& ban_entry_json : bans_json.getValues()) {
+        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));
+        }
+        bans.insert_or_assign(subnet, CBanEntry{ban_entry_json});
+    }
+}
+
 template <typename Stream, typename Data>
 bool SerializeDB(Stream& stream, const Data& data)
 {
@@ -119,18 +178,54 @@ bool DeserializeFileDB(const fs::path& path, Data& data, int version)
 }
 } // namespace
 
-CBanDB::CBanDB(fs::path ban_list_path) : m_ban_list_path(std::move(ban_list_path))
+CBanDB::CBanDB(fs::path ban_list_path)
+    : m_banlist_dat(ban_list_path.string() + ".dat"),
+      m_banlist_json(ban_list_path.string() + ".json")
 {
 }
 
 bool CBanDB::Write(const banmap_t& banSet)
 {
-    return SerializeFileDB("banlist", m_ban_list_path, banSet, CLIENT_VERSION);
+    std::vector<std::string> errors;
+    if (util::WriteSettings(m_banlist_json, {{JSON_KEY, BanMapToJson(banSet)}}, errors)) {
+        return true;
+    }
+
+    for (const auto& err : errors) {
+        error("%s", err);
+    }
+    return false;
 }
 
-bool CBanDB::Read(banmap_t& banSet)
+bool CBanDB::Read(banmap_t& banSet, bool& dirty)
 {
-    return DeserializeFileDB(m_ban_list_path, banSet, CLIENT_VERSION);
+    // If the JSON banlist does not exist, then try to read the non-upgraded banlist.dat.
+    if (!fs::exists(m_banlist_json)) {
+        // If this succeeds then we need to flush to disk in order to create the JSON banlist.
+        dirty = true;
+        return DeserializeFileDB(m_banlist_dat, banSet, CLIENT_VERSION);
+    }
+
+    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);
+        }
+        return false;
+    }
+
+    try {
+        BanMapFromJson(settings[JSON_KEY], banSet);
+    } catch (const std::runtime_error& e) {
+        LogPrintf("Cannot parse banlist %s: %s\n", m_banlist_json.string(), e.what());
+        return false;
+    }
+
+    return true;
 }
 
 CAddrDB::CAddrDB()

src/addrdb.h

@@ -9,6 +9,7 @@
 #include <fs.h>
 #include <net_types.h> // For banmap_t
 #include <serialize.h>
+#include <univalue.h>
 
 #include <string>
 #include <vector>
@@ -36,6 +37,13 @@ class CBanEntry
         nCreateTime = nCreateTimeIn;
     }
 
+    /**
+     * Create a ban entry from JSON.
+     * @param[in] json A JSON representation of a ban entry, as created by `ToJson()`.
+     * @throw std::runtime_error if the JSON does not have the expected fields.
+     */
+    explicit CBanEntry(const UniValue& json);
+
     SERIALIZE_METHODS(CBanEntry, obj)
     {
         uint8_t ban_reason = 2; //! For backward compatibility
@@ -48,6 +56,12 @@ class CBanEntry
         nCreateTime = 0;
         nBanUntil = 0;
     }
+
+    /**
+     * Generate a JSON representation of this ban entry.
+     * @return JSON suitable for passing to the `CBanEntry(const UniValue&)` constructor.
+     */
+    UniValue ToJson() const;
 };
 
 /** Access to the (IP) address database (peers.dat) */
@@ -62,15 +76,30 @@ class CAddrDB
     static bool Read(CAddrMan& addr, CDataStream& ssPeers);
 };
 
-/** Access to the banlist database (banlist.dat) */
+/** Access to the banlist databases (banlist.json and banlist.dat) */
 class CBanDB
 {
 private:
-    const fs::path m_ban_list_path;
+    /**
+     * JSON key under which the data is stored in the json database.
+     */
+    static constexpr const char* JSON_KEY = "banned_nets";
+
+    const fs::path m_banlist_dat;
+    const fs::path m_banlist_json;
 public:
     explicit CBanDB(fs::path ban_list_path);
     bool Write(const banmap_t& banSet);
-    bool Read(banmap_t& banSet);
+
+    /**
+     * Read the banlist from disk.
+     * @param[out] banSet The loaded list. Set if `true` is returned, otherwise it is left
+     * in an undefined state.
+     * @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 true on success
+     */
+    bool Read(banmap_t& banSet, bool& dirty);
 };
 
 /**

src/banman.cpp

@@ -18,20 +18,18 @@ BanMan::BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t
     if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…").translated);
 
     int64_t n_start = GetTimeMillis();
-    m_is_dirty = false;
-    banmap_t banmap;
-    if (m_ban_db.Read(banmap)) {
-        SetBanned(banmap);        // thread save setter
-        SetBannedSetDirty(false); // no need to write down, just read data
-        SweepBanned();            // sweep out unused entries
+    if (m_ban_db.Read(m_banned, m_is_dirty)) {
+        SweepBanned(); // sweep out unused entries
 
-        LogPrint(BCLog::NET, "Loaded %d banned node ips/subnets from banlist.dat  %dms\n",
-            m_banned.size(), GetTimeMillis() - n_start);
+        LogPrint(BCLog::NET, "Loaded %d banned node addresses/subnets  %dms\n", m_banned.size(),
+                 GetTimeMillis() - n_start);
     } else {
-        LogPrintf("Recreating banlist.dat\n");
-        SetBannedSetDirty(true); // force write
-        DumpBanlist();
+        LogPrintf("Recreating the banlist database\n");
+        m_banned = {};
+        m_is_dirty = true;
     }
+
+    DumpBanlist();
 }
 
 BanMan::~BanMan()
@@ -53,8 +51,8 @@ void BanMan::DumpBanlist()
         SetBannedSetDirty(false);
     }
 
-    LogPrint(BCLog::NET, "Flushed %d banned node ips/subnets to banlist.dat  %dms\n",
-        banmap.size(), GetTimeMillis() - n_start);
+    LogPrint(BCLog::NET, "Flushed %d banned node addresses/subnets to disk  %dms\n", banmap.size(),
+             GetTimeMillis() - n_start);
 }
 
 void BanMan::ClearBanned()
@@ -167,13 +165,6 @@ void BanMan::GetBanned(banmap_t& banmap)
     banmap = m_banned; //create a thread safe copy
 }
 
-void BanMan::SetBanned(const banmap_t& banmap)
-{
-    LOCK(m_cs_banned);
-    m_banned = banmap;
-    m_is_dirty = true;
-}
-
 void BanMan::SweepBanned()
 {
     int64_t now = GetTime();
@@ -188,7 +179,7 @@ void BanMan::SweepBanned()
                 m_banned.erase(it++);
                 m_is_dirty = true;
                 notify_ui = true;
-                LogPrint(BCLog::NET, "%s: Removed banned node ip/subnet from banlist.dat: %s\n", __func__, sub_net.ToString());
+                LogPrint(BCLog::NET, "Removed banned node address/subnet: %s\n", sub_net.ToString());
             } else
                 ++it;
         }

src/banman.h

@@ -17,7 +17,8 @@
 
 // NOTE: When adjusting this, update rpcnet:setban's help ("24h")
 static constexpr unsigned int DEFAULT_MISBEHAVING_BANTIME = 60 * 60 * 24; // Default 24-hour ban
-// How often to dump addresses to banlist.dat
+
+/// How often to dump banned addresses/subnets to disk.
 static constexpr std::chrono::minutes DUMP_BANS_INTERVAL{15};
 
 class CClientUIInterface;
@@ -30,7 +31,7 @@ class CSubNet;
 // If an address or subnet is banned, we never accept incoming connections from
 // it and never create outgoing connections to it. We won't gossip its address
 // to other peers in addr messages. Banned addresses and subnets are stored to
-// banlist.dat on shutdown and reloaded on startup. Banning can be used to
+// disk on shutdown and reloaded on startup. Banning can be used to
 // prevent connections with spy nodes or other griefers.
 //
 // 2. Discouragement. If a peer misbehaves enough (see Misbehaving() in
@@ -79,7 +80,6 @@ class BanMan
     void DumpBanlist();
 
 private:
-    void SetBanned(const banmap_t& banmap);
     bool BannedSetIsDirty();
     //!set the "dirty" flag for the banlist
     void SetBannedSetDirty(bool dirty = true);

src/init.cpp

@@ -1161,7 +1161,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     assert(!node.addrman);
     node.addrman = std::make_unique<CAddrMan>();
     assert(!node.banman);
-    node.banman = std::make_unique<BanMan>(gArgs.GetDataDirNet() / "banlist.dat", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
+    node.banman = std::make_unique<BanMan>(gArgs.GetDataDirNet() / "banlist", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
     assert(!node.connman);
     node.connman = std::make_unique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), *node.addrman, args.GetBoolArg("-networkactive", true));
 

src/test/denialofservice_tests.cpp

@@ -191,7 +191,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
 BOOST_AUTO_TEST_CASE(peer_discouragement)
 {
     const CChainParams& chainparams = Params();
-    auto banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
+    auto banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
     auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman);
     auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, banman.get(),
                                        *m_node.scheduler, *m_node.chainman, *m_node.mempool, false);
@@ -285,7 +285,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
 BOOST_AUTO_TEST_CASE(DoS_bantime)
 {
     const CChainParams& chainparams = Params();
-    auto banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
+    auto banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
     auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman);
     auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, banman.get(),
                                        *m_node.scheduler, *m_node.chainman, *m_node.mempool, false);

src/test/fuzz/banman.cpp

@@ -9,8 +9,10 @@
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
 #include <test/util/setup_common.h>
+#include <util/readwritefile.h>
 #include <util/system.h>
 
+#include <cassert>
 #include <cstdint>
 #include <limits>
 #include <string>
@@ -38,8 +40,20 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
     int limit_max_ops{300};
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
     SetMockTime(ConsumeTime(fuzzed_data_provider));
-    const fs::path banlist_file = gArgs.GetDataDirNet() / "fuzzed_banlist.dat";
-    fs::remove(banlist_file);
+    fs::path banlist_file = gArgs.GetDataDirNet() / "fuzzed_banlist";
+
+    const bool start_with_corrupted_banlist{fuzzed_data_provider.ConsumeBool()};
+    if (start_with_corrupted_banlist) {
+        const std::string sfx{fuzzed_data_provider.ConsumeBool() ? ".dat" : ".json"};
+        assert(WriteBinaryFile(banlist_file.string() + sfx,
+                               fuzzed_data_provider.ConsumeRandomLengthString()));
+    } else {
+        const bool force_read_and_write_to_err{fuzzed_data_provider.ConsumeBool()};
+        if (force_read_and_write_to_err) {
+            banlist_file = fs::path{"path"} / "to" / "inaccessible" / "fuzzed_banlist";
+        }
+    }
+
     {
         BanMan ban_man{banlist_file, nullptr, ConsumeBanTimeOffset(fuzzed_data_provider)};
         while (--limit_max_ops >= 0 && fuzzed_data_provider.ConsumeBool()) {
@@ -80,5 +94,6 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
                 });
         }
     }
-    fs::remove(banlist_file);
+    fs::remove(banlist_file.string() + ".dat");
+    fs::remove(banlist_file.string() + ".json");
 }

src/test/util/setup_common.cpp

@@ -196,7 +196,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
     }
 
     m_node.addrman = std::make_unique<CAddrMan>();
-    m_node.banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
+    m_node.banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
     m_node.connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests.
     m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman,
                                        m_node.banman.get(), *m_node.scheduler, *m_node.chainman,