banman: save the banlist in a JSON format on disk

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). Still read `banlist.dat` (if it exists) and merge its contents with `banlist.json` (if it exists). This is because we need to read `banlist.dat` at least once in order to convert it and it is easier to read it every time (not just once). Supersedes bitcoin#20904 Resolves bitcoin#19748
vasild · Jan 19, 2021 · 790727e · 790727e
1 parent 7acda55
commit 790727e
Show file tree

Hide file tree

Showing 9 changed files with 142 additions and 24 deletions.
diff --git a/doc/files.md b/doc/files.md
@@ -55,7 +55,8 @@ Subdirectory       | File(s)               | Description
 `indexes/blockfilter/basic/`    | `fltrNNNNN.dat`<sup>[\[2\]](#note2)</sup> | Blockfilter index filters for the basic filtertype; *optional*, used if `-blockfilterindex=basic`
 `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). Bitcoin Core will not save the banlist to it. It would only read it on startup if it is present and merge its contents with the contents of `banlist.json`.
+`./`               | `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

diff --git a/src/addrdb.cpp b/src/addrdb.cpp
@@ -11,13 +11,70 @@
 #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 {
 
+/**
+ * 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& ban_entry : bans) {
+        UniValue j = ban_entry.second.ToJson();
+        j.pushKV("address", ban_entry.first.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.
+ * @return true if the conversion was successful.
+ * @throws std::runtime_error if the JSON does not have the expected fields.
+ */
+bool 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["address"].get_str();
+        if (!LookupSubNet(subnet_str, subnet)) {
+            error("%s: Cannot parse banned address or subnet: %s", __func__, subnet_str);
+            return false;
+        }
+        bans.insert_or_assign(subnet, CBanEntry{ban_entry_json});
+    }
+
+    return true;
+}
+
 template <typename Stream, typename Data>
 bool SerializeDB(Stream& stream, const Data& data)
 {
@@ -106,31 +163,68 @@ bool DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true)
 }
 
 template <typename Data>
-bool DeserializeFileDB(const fs::path& path, Data& data)
+bool DeserializeFileDB(const fs::path& path, Data& data, bool log_missing_file = true)
 {
     // open input file, and associate with CAutoFile
     FILE *file = fsbridge::fopen(path, "rb");
     CAutoFile filein(file, SER_DISK, CLIENT_VERSION);
-    if (filein.IsNull())
-        return error("%s: Failed to open file %s", __func__, path.string());
+    if (filein.IsNull()) {
+        if (log_missing_file) {
+            error("%s: Failed to open file %s", __func__, path.string());
+        }
+        return false;
+    }
 
     return DeserializeDB(filein, data);
 }
 
 }
 
-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);
+    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: %s", __func__, err);
+    }
+    return false;
 }
 
 bool CBanDB::Read(banmap_t& banSet)
 {
-    return DeserializeFileDB(m_ban_list_path, banSet);
+    const bool dat_loaded = DeserializeFileDB(m_banlist_dat, banSet, false);
+
+    std::map<std::string, util::SettingsValue> settings;
+    std::vector<std::string> errors;
+
+    if (!fs::exists(m_banlist_json)) {
+        return dat_loaded;
+    }
+
+    if (!util::ReadSettings(m_banlist_json, settings, errors)) {
+        for (const auto& err : errors) {
+            error("%s: %s", __func__, err);
+        }
+        return dat_loaded;
+    }
+
+    if (settings.find(JSON_KEY) == settings.end()) {
+        error("%s: Cannot parse %s: missing key %s", __func__, m_banlist_json.string(), JSON_KEY);
+        return dat_loaded;
+    }
+
+    const bool json_loaded = BanMapFromJson(settings[JSON_KEY], banSet);
+
+    return dat_loaded || json_loaded;
 }
 
 CAddrDB::CAddrDB()

diff --git a/src/addrdb.h b/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,11 +76,17 @@ 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);

diff --git a/src/banman.cpp b/src/banman.cpp
@@ -25,10 +25,10 @@ BanMan::BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t
         SetBannedSetDirty(false); // no need to write down, just read data
         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("Invalid or missing banlist.dat; recreating\n");
+        LogPrintf("Invalid or missing banlist database; recreating\n");
         SetBannedSetDirty(true); // force write
         DumpBanlist();
     }
@@ -53,8 +53,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()
@@ -188,7 +188,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, "%s: Removed banned node address/subnet: %s\n", __func__, sub_net.ToString());
             } else
                 ++it;
         }

diff --git a/src/banman.h b/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

diff --git a/src/init.cpp b/src/init.cpp
@@ -1392,7 +1392,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
     const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)};
 
     assert(!node.banman);
-    node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
+    node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
     assert(!node.connman);
     node.connman = MakeUnique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), args.GetBoolArg("-networkactive", true));
 

diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
@@ -222,7 +222,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
 BOOST_AUTO_TEST_CASE(peer_discouragement)
 {
     const CChainParams& chainparams = Params();
-    auto banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
+    auto banman = MakeUnique<BanMan>(GetDataDir() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
     auto connman = MakeUnique<CConnman>(0x1337, 0x1337);
     auto peerLogic = PeerManager::make(chainparams, *connman, banman.get(), *m_node.scheduler,
                                        *m_node.chainman, *m_node.mempool, false);
@@ -269,7 +269,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
 BOOST_AUTO_TEST_CASE(DoS_bantime)
 {
     const CChainParams& chainparams = Params();
-    auto banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
+    auto banman = MakeUnique<BanMan>(GetDataDir() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
     auto connman = MakeUnique<CConnman>(0x1337, 0x1337);
     auto peerLogic = PeerManager::make(chainparams, *connman, banman.get(), *m_node.scheduler,
                                        *m_node.chainman, *m_node.mempool, false);

diff --git a/src/test/fuzz/banman.cpp b/src/test/fuzz/banman.cpp
@@ -33,8 +33,9 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
 {
     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);
+    const fs::path banlist_file = GetDataDir() / "fuzzed_banlist";
+    fs::remove(banlist_file.string() + ".dat");
+    fs::remove(banlist_file.string() + ".json");
     {
         BanMan ban_man{banlist_file, nullptr, ConsumeBanTimeOffset(fuzzed_data_provider)};
         while (fuzzed_data_provider.ConsumeBool()) {
@@ -77,5 +78,6 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
                 });
         }
     }
-    fs::remove(banlist_file);
+    fs::remove(banlist_file.string() + ".dat");
+    fs::remove(banlist_file.string() + ".json");
 }
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
@@ -190,7 +190,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
         throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString()));
     }
 
-    m_node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
+    m_node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
     m_node.connman = MakeUnique<CConnman>(0x1337, 0x1337); // Deterministic randomness for tests.
     m_node.peerman = PeerManager::make(chainparams, *m_node.connman, m_node.banman.get(),
                                        *m_node.scheduler, *m_node.chainman, *m_node.mempool,