Merge #6084: fix: backport bitcoin#26909, allow for silent overwriting of inconsistent peers.dat

adba60924c addrman: allow for silent overwriting of inconsistent peers.dat (Kittywhiskers Van Gogh)
fbb2b51d75 merge bitcoin#26909: prevent peers.dat corruptions by only serializing once (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  [bitcoin#22762](https://github.com/bitcoin/bitcoin/pull/22762) (backported as part of [dash#6043](https://github.com/dashpay/dash/pull/6043)) did away with then-existing behaviour of overwriting `peers.dat` silently if found corrupt with the rationale of preventing situations where the wrong file is pointed at or the file is written by a higher version of Core. Alongside a change in behaviour, refactoring also took place and further changes were built on top of them.

  Since then, there have been reports of an increasing number of "Corrupt data. Consistency check failed with code -5: iostream error" errors from builds based on `develop`. Reverting the pull request that introduced this change in behaviour is non-trivial due to the number of backports that build on top of the refactoring brought along with it.

  Nor were any other error messages found except for the one mentioned above. The tendency for `peers.dat` to corrupt itself has also been documented upstream ([bitcoin#26599](https://github.com/bitcoin/bitcoin/issues/26599)), with the issue marked as closed with the merger of [bitcoin#26909](https://github.com/bitcoin/bitcoin/pull/26909).

  Therefore, to remedy the above problem, alongside backporting [bitcoin#26909](https://github.com/bitcoin/bitcoin/pull/26909), to avoid inconvenience, instead of reverting all progress made from backporting (as the benefits of not overwriting `peers.dat` for having the wrong magic altogether, for example, is something that doesn't need to be reverted), only inconsistent `peers.dat` files will be overwritten and the action logged with no user intervention required.

  ## Breaking Changes

  None expected.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK adba60924c
  knst:
    utACK adba60924c
  PastaPastaPasta:
    utACK adba60924c

Tree-SHA512: 3e09e7a77c82cce333fe9f02f137485e362f7816c450aef3d18950b9fd57276b4a21cbd1fe90b3eefd62ede0947970ed367c5917930a0656833bc38c0629e408

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());
+        HashedSourceWriter hashwriter{stream};
-        stream << Params().MessageStart() << data;
+        hashwriter << Params().MessageStart() << data;
-        hasher << Params().MessageStart() << data;
+        stream << hashwriter.GetHash();
-        stream << hasher.GetHash();
     } catch (const std::exception& e) {
         return error("%s: Serialize or I/O error - %s", __func__, e.what());
     }
@@ -197,6 +196,15 @@ std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const A
         addrman = std::make_unique<AddrMan>(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<AddrMan>(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."),

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));
     }
@@ -1168,8 +1168,7 @@ void AddrMan::Unserialize(Stream& s_)
 }
 
 // explicit instantiation
-template void AddrMan::Serialize(CHashWriter& s) const;
+template void AddrMan::Serialize(HashedSourceWriter<CAutoFile>& s) const;
-template void AddrMan::Serialize(CAutoFile& s) const;
 template void AddrMan::Serialize(CDataStream& s) const;
 template void AddrMan::Unserialize(CAutoFile& s);
 template void AddrMan::Unserialize(CHashVerifier<CAutoFile>& s);

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:

src/hash.h

@@ -201,6 +201,30 @@ public:
     }
 };
 
+/** Writes data to an underlying source stream, while hashing the written data. */
+template <typename Source>
+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<const std::byte> src)
+    {
+        m_source.write(src);
+        CHashWriter::write(src);
+    }
+
+    template <typename T>
+    HashedSourceWriter& operator<<(const T& obj)
+    {
+        ::Serialize(*this, obj);
+        return *this;
+    }
+};
+
 /** Compute the 256-bit hash of an object's serialization. */
 template<typename T>
 uint256 SerializeHash(const T& obj, int nType=SER_GETHASH, int nVersion=PROTOCOL_VERSION)

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()

test/functional/feature_addrman.py

@@ -123,10 +123,8 @@ class AddrmanTest(BitcoinTestFramework):
         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(
+        with self.nodes[0].assert_debug_log(['Creating peers.dat because of invalid or corrupt file']):
-            expected_msg=init_error("Corrupt data. Consistency check failed with code -16: .*"),
+            self.start_node(0)
-            match=ErrorMatch.FULL_REGEX,
-        )
 
         self.log.info("Check that missing addrman is recreated")
         self.stop_node(0)