Merge bitcoin/bitcoin#22762: Raise InitError when peers.dat is invalid or corrupted

fa55c3d Raise InitError when peers.dat is invalid or corrupted (MarcoFalke)
fa4e2cc Inline ReadPeerAddresses (MarcoFalke)
fa5aeec Move LoadAddrman from init to addrdb (MarcoFalke)

Pull request description:

  peers.dat is silently erased when it can not be parsed or when it appears corrupted. Fix that by notifying the user. This might help in the following examples:

  * The user provided the database, but picked the wrong one.
  * A future version of Bitcoin Core wrote the file and it can't be read.
  * The file was corrupted by a logic bug in Bitcoin Core.
  * The file was corrupted by a disk failure.

ACKs for top commit:
  jonatack:
    Code review re-ACK fa55c3d per `git range-diff eb1f570 fa59c6d fa55c3` and verified the new tests fail on master, except "Check mocked addrman is valid", as expected
  prayank23:
    tACK bitcoin/bitcoin@fa55c3d
  vasild:
    ACK fa55c3d

Tree-SHA512: 78264a78ee570a3c3262cf9c8542b5ffaffa5f52da1eef66c8c381f346989272967cfe1769c573502d9d7d3f7ad68c3ac3b2ec734185d2e4e7595b7122b14196

Author: merge-script

src/addrdb.cpp

@@ -18,8 +18,15 @@
 #include <univalue.h>
 #include <util/settings.h>
 #include <util/system.h>
+#include <util/translation.h>
 
 namespace {
+
+class DbNotFoundError : public std::exception
+{
+    using std::exception::exception;
+};
+
 template <typename Stream, typename Data>
 bool SerializeDB(Stream& stream, const Data& data)
 {
@@ -77,47 +84,40 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
 }
 
 template <typename Stream, typename Data>
-bool DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true)
+void DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true)
 {
-    try {
-        CHashVerifier<Stream> verifier(&stream);
-        // de-serialize file header (network specific magic number) and ..
-        unsigned char pchMsgTmp[4];
-        verifier >> pchMsgTmp;
-        // ... verify the network matches ours
-        if (memcmp(pchMsgTmp, Params().MessageStart(), sizeof(pchMsgTmp)))
-            return error("%s: Invalid network magic number", __func__);
-
-        // de-serialize data
-        verifier >> data;
-
-        // verify checksum
-        if (fCheckSum) {
-            uint256 hashTmp;
-            stream >> hashTmp;
-            if (hashTmp != verifier.GetHash()) {
-                return error("%s: Checksum mismatch, data corrupted", __func__);
-            }
-        }
-    }
-    catch (const std::exception& e) {
-        return error("%s: Deserialize or I/O error - %s", __func__, e.what());
+    CHashVerifier<Stream> verifier(&stream);
+    // de-serialize file header (network specific magic number) and ..
+    unsigned char pchMsgTmp[4];
+    verifier >> pchMsgTmp;
+    // ... verify the network matches ours
+    if (memcmp(pchMsgTmp, Params().MessageStart(), sizeof(pchMsgTmp))) {
+        throw std::runtime_error{"Invalid network magic number"};
     }
 
-    return true;
+    // de-serialize data
+    verifier >> data;
+
+    // verify checksum
+    if (fCheckSum) {
+        uint256 hashTmp;
+        stream >> hashTmp;
+        if (hashTmp != verifier.GetHash()) {
+            throw std::runtime_error{"Checksum mismatch, data corrupted"};
+        }
+    }
 }
 
 template <typename Data>
-bool DeserializeFileDB(const fs::path& path, Data& data, int version)
+void DeserializeFileDB(const fs::path& path, Data& data, int version)
 {
     // open input file, and associate with CAutoFile
     FILE* file = fsbridge::fopen(path, "rb");
     CAutoFile filein(file, SER_DISK, version);
     if (filein.IsNull()) {
-        LogPrintf("Missing or invalid file %s\n", path.string());
-        return false;
+        throw DbNotFoundError{};
     }
-    return DeserializeDB(filein, data);
+    DeserializeDB(filein, data);
 }
 } // namespace
 
@@ -176,15 +176,32 @@ bool DumpPeerAddresses(const ArgsManager& args, const CAddrMan& addr)
     return SerializeFileDB("peers", pathAddr, addr, CLIENT_VERSION);
 }
 
-bool ReadPeerAddresses(const ArgsManager& args, CAddrMan& addr)
+void ReadFromStream(CAddrMan& addr, CDataStream& ssPeers)
 {
-    const auto pathAddr = args.GetDataDirNet() / "peers.dat";
-    return DeserializeFileDB(pathAddr, addr, CLIENT_VERSION);
+    DeserializeDB(ssPeers, addr, false);
 }
 
-bool ReadFromStream(CAddrMan& addr, CDataStream& ssPeers)
+std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const ArgsManager& args, std::unique_ptr<CAddrMan>& addrman)
 {
-    return DeserializeDB(ssPeers, addr, false);
+    auto check_addrman = std::clamp<int32_t>(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
+    addrman = std::make_unique<CAddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
+
+    int64_t nStart = GetTimeMillis();
+    const auto path_addr{args.GetDataDirNet() / "peers.dat"};
+    try {
+        DeserializeFileDB(path_addr, *addrman, CLIENT_VERSION);
+        LogPrintf("Loaded %i addresses from peers.dat  %dms\n", addrman->size(), GetTimeMillis() - nStart);
+    } catch (const DbNotFoundError&) {
+        // Addrman can be in an inconsistent state after failure, reset it
+        addrman = std::make_unique<CAddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
+        LogPrintf("Creating peers.dat because the file was not found (%s)\n", path_addr);
+        DumpPeerAddresses(args, *addrman);
+    } catch (const std::exception& e) {
+        addrman = nullptr;
+        return strprintf(_("Invalid or corrupt peers.dat (%s). If you believe this is a bug, please report it to %s. As a workaround, you can move the file (%s) out of the way (rename, move, or delete) to have a new one created on the next start."),
+                         e.what(), PACKAGE_BUGREPORT, path_addr);
+    }
+    return std::nullopt;
 }
 
 void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& anchors)
@@ -196,9 +213,10 @@ void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& a
 std::vector<CAddress> ReadAnchors(const fs::path& anchors_db_path)
 {
     std::vector<CAddress> anchors;
-    if (DeserializeFileDB(anchors_db_path, anchors, CLIENT_VERSION | ADDRV2_FORMAT)) {
+    try {
+        DeserializeFileDB(anchors_db_path, anchors, CLIENT_VERSION | ADDRV2_FORMAT);
         LogPrintf("Loaded %i addresses from %s\n", anchors.size(), anchors_db_path.filename());
-    } else {
+    } catch (const std::exception&) {
         anchors.clear();
     }
 

src/addrdb.h

@@ -10,17 +10,18 @@
 #include <net_types.h> // For banmap_t
 #include <univalue.h>
 
+#include <optional>
 #include <vector>
 
 class ArgsManager;
 class CAddrMan;
 class CAddress;
 class CDataStream;
+struct bilingual_str;
 
 bool DumpPeerAddresses(const ArgsManager& args, const CAddrMan& addr);
-bool ReadPeerAddresses(const ArgsManager& args, CAddrMan& addr);
 /** Only used by tests. */
-bool ReadFromStream(CAddrMan& addr, CDataStream& ssPeers);
+void ReadFromStream(CAddrMan& addr, CDataStream& ssPeers);
 
 /** Access to the banlist database (banlist.json) */
 class CBanDB
@@ -46,6 +47,9 @@ class CBanDB
     bool Read(banmap_t& banSet);
 };
 
+/** Returns an error string on failure */
+std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const ArgsManager& args, std::unique_ptr<CAddrMan>& addrman);
+
 /**
  * Dump the anchor IP address database (anchors.dat)
  *

src/init.cpp

@@ -1200,19 +1200,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
             LogPrintf("Using /16 prefix for IP bucketing\n");
         }
 
-        auto check_addrman = std::clamp<int32_t>(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
-        node.addrman = std::make_unique<CAddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
-
-        // Load addresses from peers.dat
         uiInterface.InitMessage(_("Loading P2P addresses…").translated);
-        int64_t nStart = GetTimeMillis();
-        if (ReadPeerAddresses(args, *node.addrman)) {
-            LogPrintf("Loaded %i addresses from peers.dat  %dms\n", node.addrman->size(), GetTimeMillis() - nStart);
-        } else {
-            // Addrman can be in an inconsistent state after failure, reset it
-            node.addrman = std::make_unique<CAddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
-            LogPrintf("Recreating peers.dat\n");
-            DumpPeerAddresses(args, *node.addrman);
+        if (const auto error{LoadAddrman(asmap, args, node.addrman)}) {
+            return InitError(*error);
         }
     }
 

src/test/addrman_tests.cpp

@@ -1043,7 +1043,7 @@ BOOST_AUTO_TEST_CASE(load_addrman)
 
     CAddrMan addrman2(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 100);
     BOOST_CHECK(addrman2.size() == 0);
-    BOOST_CHECK(ReadFromStream(addrman2, ssPeers2));
+    ReadFromStream(addrman2, ssPeers2);
     BOOST_CHECK(addrman2.size() == 3);
 }
 
@@ -1073,7 +1073,7 @@ BOOST_AUTO_TEST_CASE(load_addrman_corrupted)
 
     CAddrMan addrman2(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 100);
     BOOST_CHECK(addrman2.size() == 0);
-    BOOST_CHECK(!ReadFromStream(addrman2, ssPeers2));
+    BOOST_CHECK_THROW(ReadFromStream(addrman2, ssPeers2), std::ios_base::failure);
 }
 
 

src/test/fuzz/data_stream.cpp

@@ -23,5 +23,8 @@ FUZZ_TARGET_INIT(data_stream_addr_man, initialize_data_stream_addr_man)
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
     CDataStream data_stream = ConsumeDataStream(fuzzed_data_provider);
     CAddrMan addr_man(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
-    ReadFromStream(addr_man, data_stream);
+    try {
+        ReadFromStream(addr_man, data_stream);
+    } catch (const std::exception&) {
+    }
 }

test/functional/feature_addrman.py

@@ -10,6 +10,7 @@
 from test_framework.messages import ser_uint256, hash256
 from test_framework.p2p import MAGIC_BYTES
 from test_framework.test_framework import BitcoinTestFramework
+from test_framework.test_node import ErrorMatch
 from test_framework.util import assert_equal
 
 
@@ -43,6 +44,12 @@ def set_test_params(self):
 
     def run_test(self):
         peers_dat = os.path.join(self.nodes[0].datadir, self.chain, "peers.dat")
+        init_error = lambda reason: (
+            f"Error: Invalid or corrupt peers.dat \\({reason}\\). If you believe this "
+            f"is a bug, please report it to {self.config['environment']['PACKAGE_BUGREPORT']}. "
+            f'As a workaround, you can move the file \\("{peers_dat}"\\) out of the way \\(rename, '
+            "move, or delete\\) to have a new one created on the next start."
+        )
 
         self.log.info("Check that mocked addrman is valid")
         self.stop_node(0)
@@ -54,30 +61,29 @@ def run_test(self):
         self.log.info("Check that addrman from future cannot be read")
         self.stop_node(0)
         write_addrman(peers_dat, lowest_compatible=111)
-        with self.nodes[0].assert_debug_log([
-                f'ERROR: DeserializeDB: Deserialize or I/O error - Unsupported format of addrman database: 1. It is compatible with formats >=111, but the maximum supported by this version of {self.config["environment"]["PACKAGE_NAME"]} is 3.',
-                "Recreating peers.dat",
-        ]):
-            self.start_node(0)
-        assert_equal(self.nodes[0].getnodeaddresses(), [])
+        self.nodes[0].assert_start_raises_init_error(
+            expected_msg=init_error(
+                "Unsupported format of addrman database: 1. It is compatible with "
+                "formats >=111, but the maximum supported by this version of "
+                f"{self.config['environment']['PACKAGE_NAME']} is 3.: (.+)"
+            ),
+            match=ErrorMatch.FULL_REGEX,
+        )
 
         self.log.info("Check that corrupt addrman cannot be read")
         self.stop_node(0)
         with open(peers_dat, "wb") as f:
             f.write(serialize_addrman()[:-1])
-        with self.nodes[0].assert_debug_log([
-                "ERROR: DeserializeDB: Deserialize or I/O error - CAutoFile::read: end of file",
-                "Recreating peers.dat",
-        ]):
-            self.start_node(0)
-        assert_equal(self.nodes[0].getnodeaddresses(), [])
+        self.nodes[0].assert_start_raises_init_error(
+            expected_msg=init_error("CAutoFile::read: end of file.*"),
+            match=ErrorMatch.FULL_REGEX,
+        )
 
         self.log.info("Check that missing addrman is recreated")
         self.stop_node(0)
         os.remove(peers_dat)
         with self.nodes[0].assert_debug_log([
-                f"Missing or invalid file {peers_dat}",
-                "Recreating peers.dat",
+                f'Creating peers.dat because the file was not found ("{peers_dat}")',
         ]):
             self.start_node(0)
         assert_equal(self.nodes[0].getnodeaddresses(), [])