From fbb2b51d75f281921fd6b7b3423afd25f73a324d Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 15 Jan 2023 20:18:11 -0500 Subject: [PATCH 1/2] merge bitcoin#26909: prevent peers.dat corruptions by only serializing once --- src/addrdb.cpp | 7 +++---- src/addrman.cpp | 3 +-- src/hash.h | 24 ++++++++++++++++++++++++ src/test/streams_tests.cpp | 14 ++++++++++++++ 4 files changed, 42 insertions(+), 6 deletions(-) diff --git a/src/addrdb.cpp b/src/addrdb.cpp index 940f03193ca5..4de03a8e1e4f 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -33,10 +33,9 @@ bool SerializeDB(Stream& stream, const Data& data) { // Write and commit header, data try { - CHashWriter hasher(stream.GetType(), stream.GetVersion()); - stream << Params().MessageStart() << data; - hasher << Params().MessageStart() << data; - stream << hasher.GetHash(); + HashedSourceWriter hashwriter{stream}; + hashwriter << Params().MessageStart() << data; + stream << hashwriter.GetHash(); } catch (const std::exception& e) { return error("%s: Serialize or I/O error - %s", __func__, e.what()); } diff --git a/src/addrman.cpp b/src/addrman.cpp index f756be4bc3f0..951c71841bf2 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -1168,8 +1168,7 @@ void AddrMan::Unserialize(Stream& s_) } // explicit instantiation -template void AddrMan::Serialize(CHashWriter& s) const; -template void AddrMan::Serialize(CAutoFile& s) const; +template void AddrMan::Serialize(HashedSourceWriter& s) const; template void AddrMan::Serialize(CDataStream& s) const; template void AddrMan::Unserialize(CAutoFile& s); template void AddrMan::Unserialize(CHashVerifier& s); diff --git a/src/hash.h b/src/hash.h index 8e1b73707c1d..8c491ef3bcbf 100644 --- a/src/hash.h +++ b/src/hash.h @@ -201,6 +201,30 @@ class CHashVerifier : public CHashWriter } }; +/** Writes data to an underlying source stream, while hashing the written data. */ +template +class HashedSourceWriter : public CHashWriter +{ +private: + Source& m_source; + +public: + explicit HashedSourceWriter(Source& source LIFETIMEBOUND) : CHashWriter{source.GetType(), source.GetVersion()}, m_source{source} {} + + void write(Span src) + { + m_source.write(src); + CHashWriter::write(src); + } + + template + HashedSourceWriter& operator<<(const T& obj) + { + ::Serialize(*this, obj); + return *this; + } +}; + /** Compute the 256-bit hash of an object's serialization. */ template uint256 SerializeHash(const T& obj, int nType=SER_GETHASH, int nVersion=PROTOCOL_VERSION) diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp index 1c604f52f649..d569a042076b 100644 --- a/src/test/streams_tests.cpp +++ b/src/test/streams_tests.cpp @@ -437,4 +437,18 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_rand) fs::remove("streams_test_tmp"); } +BOOST_AUTO_TEST_CASE(streams_hashed) +{ + CDataStream stream(SER_NETWORK, INIT_PROTO_VERSION); + HashedSourceWriter hash_writer{stream}; + const std::string data{"bitcoin"}; + hash_writer << data; + + CHashVerifier hash_verifier{&stream}; + std::string result; + hash_verifier >> result; + BOOST_CHECK_EQUAL(data, result); + BOOST_CHECK_EQUAL(hash_writer.GetHash(), hash_verifier.GetHash()); +} + BOOST_AUTO_TEST_SUITE_END() From adba60924c2bed51b802aa9faf898faa7d62bdf9 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 27 Jun 2024 06:09:30 +0000 Subject: [PATCH 2/2] addrman: allow for silent overwriting of inconsistent peers.dat --- src/addrdb.cpp | 9 +++++++++ src/addrman.cpp | 2 +- src/addrman.h | 10 ++++++++++ test/functional/feature_addrman.py | 6 ++---- 4 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/addrdb.cpp b/src/addrdb.cpp index 4de03a8e1e4f..5c4015567b90 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -196,6 +196,15 @@ std::optional LoadAddrman(const std::vector& asmap, const A addrman = std::make_unique(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 DbInconsistentError& e) { + // Addrman has shown a tendency to corrupt itself even with graceful shutdowns on known-good + // hardware. As the user would have to delete and recreate a new database regardless to cope + // with frequent corruption, we are restoring old behaviour that does the same, silently. + // + // TODO: Evaluate cause and fix, revert this change at some point. + addrman = std::make_unique(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman); + LogPrintf("Creating peers.dat because of invalid or corrupt file (%s)\n", e.what()); + 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."), diff --git a/src/addrman.cpp b/src/addrman.cpp index 951c71841bf2..aa05b7a09877 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -391,7 +391,7 @@ void AddrManImpl::Unserialize(Stream& s_) const int check_code{ForceCheckAddrman()}; if (check_code != 0) { - throw std::ios_base::failure(strprintf( + throw DbInconsistentError(strprintf( "Corrupt data. Consistency check failed with code %s", check_code)); } diff --git a/src/addrman.h b/src/addrman.h index 7b36d8ddd9e0..2d5579eee8b6 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -23,6 +23,16 @@ class AddrManImpl; /** Default for -checkaddrman */ static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0}; +class DbInconsistentError : public std::exception +{ + using std::exception::exception; + const std::string error; + +public: + explicit DbInconsistentError(const std::string _error) : error{_error} {} + const char* what() const noexcept override { return error.c_str(); } +}; + /** Stochastic address manager * * Design goals: diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py index 1dc1bfae519d..06efac8abb5d 100755 --- a/test/functional/feature_addrman.py +++ b/test/functional/feature_addrman.py @@ -123,10 +123,8 @@ def run_test(self): self.log.info("Check that corrupt addrman cannot be read (failed check)") self.stop_node(0) write_addrman(peers_dat, bucket_key=0) - self.nodes[0].assert_start_raises_init_error( - expected_msg=init_error("Corrupt data. Consistency check failed with code -16: .*"), - match=ErrorMatch.FULL_REGEX, - ) + with self.nodes[0].assert_debug_log(['Creating peers.dat because of invalid or corrupt file']): + self.start_node(0) self.log.info("Check that missing addrman is recreated") self.stop_node(0)