merge bitcoin#20966: save the banlist in a JSON format on disk

doc/files.md

@@ -56,7 +56,8 @@ Subdirectory       | File(s)               | Description
 `./`               | `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
 `evodb/`         |                       |special txes and quorums database
 `llmq/`          |                       |quorum signatures database
-`./`               | `banlist.dat`         | Stores the IPs/subnets of banned nodes
+`./`               | `banlist.dat`         | Stores the addresses/subnets of banned nodes (deprecated). `dashd` or `dash-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.
 `./`               | `dash.conf`        | User-defined [configuration settings](dash-conf.md) for `dashd` or `dash-qt`. File is not written to by the software and must be created manually. Path can be specified by `-conf` option
 `./`               | `dashd.pid`        | Stores the process ID (PID) of `dashd` or `dash-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 `dashd` or `dash-qt`; can be specified by `-debuglogfile` option
@@ -111,7 +112,7 @@ Subdirectory | File                 | Description
 
 ## Legacy subdirectories and files
 
-These subdirectories and files are no longer used by the Dash Core:
+These subdirectories and files are no longer used by Dash Core:
 
 Path           | Description | Repository notes
 ---------------|-------------|-----------------

src/addrdb.cpp

@@ -10,15 +10,74 @@
 #include <clientversion.h>
 #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>
 
 #include <cstdint>
 
+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)
 {
@@ -120,18 +179,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 @@ public:
         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 @@ public:
         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 @@ public:
     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()
@@ -177,13 +175,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();
@@ -198,7 +189,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
@@ -80,7 +81,6 @@ public:
     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

@@ -1662,7 +1662,7 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc
     assert(!node.addrman);
     node.addrman = std::make_unique<CAddrMan>();
     assert(!node.banman);
-    node.banman = std::make_unique<BanMan>(GetDataDir() / "banlist.dat", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
+    node.banman = std::make_unique<BanMan>(GetDataDir() / "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);
 

src/test/denialofservice_tests.cpp

@@ -226,7 +226,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
 BOOST_AUTO_TEST_CASE(DoS_banning)
 {
     const CChainParams& chainparams = Params();
-    auto banman = std::make_unique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
+    auto banman = std::make_unique<BanMan>(GetDataDir() / "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, *governance, m_node.cj_ctx,
@@ -274,7 +274,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
 BOOST_AUTO_TEST_CASE(DoS_banscore)
 {
     const CChainParams& chainparams = Params();
-    auto banman = std::make_unique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
+    auto banman = std::make_unique<BanMan>(GetDataDir() / "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, *governance, m_node.cj_ctx,
@@ -320,7 +320,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
 BOOST_AUTO_TEST_CASE(DoS_bantime)
 {
     const CChainParams& chainparams = Params();
-    auto banman = std::make_unique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
+    auto banman = std::make_unique<BanMan>(GetDataDir() / "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, *governance, m_node.cj_ctx,

src/test/fuzz/banman.cpp

@@ -8,8 +8,10 @@
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
+#include <util/readwritefile.h>
 #include <util/system.h>
 
+#include <cassert>
 #include <cstdint>
 #include <limits>
 #include <string>
@@ -37,8 +39,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 = GetDataDir() / "fuzzed_banlist.dat";
-    fs::remove(banlist_file);
+    fs::path banlist_file = GetDataDir() / "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()) {
@@ -79,5 +93,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

@@ -261,7 +261,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
         throw std::runtime_error("LoadGenesisBlock failed.");
     }
 
-    m_node.banman = std::make_unique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
+    m_node.banman = std::make_unique<BanMan>(GetDataDir() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
     m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman, m_node.banman.get(),
                                        *m_node.scheduler, *m_node.chainman, *m_node.mempool, *governance,
                                        m_node.cj_ctx, m_node.llmq_ctx, false);

test/functional/rpc_setban.py

@@ -22,7 +22,7 @@ class SetBanTests(BitcoinTestFramework):
         # Node 0 connects to Node 1, check that the noban permission is not granted
         self.connect_nodes(0, 1)
         peerinfo = self.nodes[1].getpeerinfo()[0]
-        assert not 'noban' in peerinfo['permissions']
+        assert not "noban" in peerinfo["permissions"]
 
         # Node 0 get banned by Node 1
         self.nodes[1].setban("127.0.0.1", "add")
@@ -36,27 +36,40 @@ class SetBanTests(BitcoinTestFramework):
         self.restart_node(1, ['-whitelist=127.0.0.1'])
         self.connect_nodes(0, 1)
         peerinfo = self.nodes[1].getpeerinfo()[0]
-        assert 'noban' in peerinfo['permissions']
+        assert "noban" in peerinfo["permissions"]
 
         # If we remove the ban, Node 0 should be able to reconnect even without noban permission
         self.nodes[1].setban("127.0.0.1", "remove")
         self.restart_node(1, [])
         self.connect_nodes(0, 1)
         peerinfo = self.nodes[1].getpeerinfo()[0]
-        assert not 'noban' in peerinfo['permissions']
+        assert not "noban" in peerinfo["permissions"]
 
         self.log.info("Test that a non-IP address can be banned/unbanned")
         node = self.nodes[1]
         tor_addr = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"
         ip_addr = "1.2.3.4"
-        assert(not self.is_banned(node, tor_addr))
-        assert(not self.is_banned(node, ip_addr))
+        assert not self.is_banned(node, tor_addr)
+        assert not self.is_banned(node, ip_addr)
+
         node.setban(tor_addr, "add")
-        assert(self.is_banned(node, tor_addr))
-        assert(not self.is_banned(node, ip_addr))
+        assert self.is_banned(node, tor_addr)
+        assert not self.is_banned(node, ip_addr)
+
+        self.log.info("Test the ban list is preserved through restart")
+
+        self.restart_node(1)
+        assert self.is_banned(node, tor_addr)
+        assert not self.is_banned(node, ip_addr)
+
         node.setban(tor_addr, "remove")
-        assert(not self.is_banned(self.nodes[1], tor_addr))
-        assert(not self.is_banned(node, ip_addr))
+        assert not self.is_banned(self.nodes[1], tor_addr)
+        assert not self.is_banned(node, ip_addr)
+
+        self.restart_node(1)
+        assert not self.is_banned(node, tor_addr)
+        assert not self.is_banned(node, ip_addr)
+
 
 if __name__ == '__main__':
     SetBanTests().main()