Merge bitcoin/bitcoin#22734: addrman: Avoid crash on corrupt data, Force Check after deserialize

fa3669f72f fuzz: Move all addrman fuzz targets to one file (MarcoFalke)
fa7a883f5a addrman: Replace assert with throw on corrupt data (MarcoFalke)
fa298971e6 Refactor: Turn the internal addrman check helper into a forced check (MarcoFalke)
fae5c633dc move-only: Move CAddrMan::Check to cpp file (MarcoFalke)

Pull request description:

  Assert should only be used for program internal logic errors, not to sanitize external user input.

  The assert was introduced via the debug-only runtime option `-checkaddrman` in commit 803ef70fd9, thus won't need a backport.

  Also, it doesn't really make sense to continue when the deserialized addrman doesn't pass the sanity check.

  For example, if `nLastSuccess` is negative, it would  later result in integer overflows. Thus, this patch fixes #22931.

  Also,
  Fixes #22503
  Fixes #22504
  Fixes #22519

  Closes #22498

  Steps to test:

  ```
  mkdir -p /tmp/test_235/regtest/
  echo 'H4sIAAAAAAAAA/u1f+stZmUGYgELgwPRakfBKBgFo2AUjIJRMApGwSgYBaNgFIyCUTBswdyGpFnLjUKjP9e0bvjYusl6b+L2e7Vs2dd6N//Pua0/xQUALJAn93IQAAA=' | base64 --decode | zcat > /tmp/test_235/regtest/peers.dat
  ./src/qt/bitcoin-qt -regtest -datadir=/tmp/test_235/ -checkaddrman=1 -printtoconsole | grep -A2 'Loading P2P addresses'
  ```

  Output before:
  ```
  2021-09-10T11:28:37Z init message: Loading P2P addresses…
  2021-09-10T11:28:37Z ADDRMAN CONSISTENCY CHECK FAILED!!! err=-16
  bitcoin-qt: addrman.cpp:765: void CAddrMan::Check() const: Assertion `false' failed.

  (program crashes)
  ```

  Output after:
  ```
  2021-09-10T11:26:00Z init message: Loading P2P addresses…
  2021-09-10T11:26:00Z Error: Invalid or corrupt peers.dat (Corrupt data. Consistency check failed with code -16: iostream error). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/tmp/test_235/regtest/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.

  (program exits)
  ```

ACKs for top commit:
  naumenkogs:
    ACK fa3669f72f
  jnewbery:
    Code review ACK fa3669f72f
  vasild:
    ACK fa3669f72f

Tree-SHA512: 687e4a4765bbc66495152fa7a49d28ee84b405dc5370ba87b4016b5593e45f54c4ce5cae579e4d433e0e082d20fc263969fa602679c911accef0adb2d6213bd6

src/Makefile.test.include

@@ -232,7 +232,6 @@ test_fuzz_fuzz_SOURCES = \
  test/fuzz/crypto_hkdf_hmac_sha256_l32.cpp \
  test/fuzz/crypto_poly1305.cpp \
  test/fuzz/cuckoocache.cpp \
- test/fuzz/data_stream.cpp \
  test/fuzz/decode_tx.cpp \
  test/fuzz/descriptor_parse.cpp \
  test/fuzz/deserialize.cpp \

src/addrman.cpp

@@ -386,7 +386,12 @@ void CAddrMan::Unserialize(Stream& s_)
         LogPrint(BCLog::ADDRMAN, "addrman lost %i new and %i tried addresses due to collisions or invalid addresses\n", nLostUnk, nLost);
     }
 
-    Check();
+    const int check_code{ForceCheckAddrman()};
+    if (check_code != 0) {
+        throw std::ios_base::failure(strprintf(
+            "Corrupt data. Consistency check failed with code %s",
+            check_code));
+    }
 }
 
 // explicit instantiation
@@ -743,13 +748,24 @@ CAddrInfo CAddrMan::Select_(bool newOnly) const
     }
 }
 
-int CAddrMan::Check_() const
+void CAddrMan::Check() const
 {
     AssertLockHeld(cs);
 
     // Run consistency checks 1 in m_consistency_check_ratio times if enabled
-    if (m_consistency_check_ratio == 0) return 0;
-    if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return 0;
+    if (m_consistency_check_ratio == 0) return;
+    if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return;
+
+    const int err{ForceCheckAddrman()};
+    if (err) {
+        LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
+        assert(false);
+    }
+}
+
+int CAddrMan::ForceCheckAddrman() const
+{
+    AssertLockHeld(cs);
 
     LogPrint(BCLog::ADDRMAN, "Addrman checks started: new %i, tried %i, total %u\n", nNew, nTried, vRandom.size());
 

src/addrman.h

@@ -391,20 +391,12 @@ private:
     //! Return a random to-be-evicted tried table address.
     CAddrInfo SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs);
 
-    //! Consistency check
-    void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs)
-    {
-        AssertLockHeld(cs);
+    //! Consistency check, taking into account m_consistency_check_ratio. Will std::abort if an inconsistency is detected.
+    void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
-        const int err = Check_();
-        if (err) {
-            LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
-            assert(false);
-        }
-    }
-
-    //! Perform consistency check. Returns an error code or zero.
-    int Check_() const EXCLUSIVE_LOCKS_REQUIRED(cs);
+    //! Perform consistency check, regardless of m_consistency_check_ratio.
+    //! @returns an error code or zero.
+    int ForceCheckAddrman() const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     /**
      * Return all or many randomly selected addresses, optionally by network.

src/test/fuzz/addrman.cpp

@@ -23,6 +23,17 @@ void initialize_addrman()
     SelectParams(CBaseChainParams::REGTEST);
 }
 
+FUZZ_TARGET_INIT(data_stream_addr_man, initialize_addrman)
+{
+    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);
+    try {
+        ReadFromStream(addr_man, data_stream);
+    } catch (const std::exception&) {
+    }
+}
+
 class CAddrManDeterministic : public CAddrMan
 {
 public:

src/test/fuzz/data_stream.cpp

@@ -1,30 +0,0 @@
-// Copyright (c) 2020-2021 The Bitcoin Core developers
-// Distributed under the MIT software license, see the accompanying
-// file COPYING or http://www.opensource.org/licenses/mit-license.php.
-
-#include <addrdb.h>
-#include <addrman.h>
-#include <net.h>
-#include <test/fuzz/FuzzedDataProvider.h>
-#include <test/fuzz/fuzz.h>
-#include <test/fuzz/util.h>
-#include <test/util/setup_common.h>
-
-#include <cstdint>
-#include <vector>
-
-void initialize_data_stream_addr_man()
-{
-    static const auto testing_setup = MakeNoLogFileContext<>();
-}
-
-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);
-    try {
-        ReadFromStream(addr_man, data_stream);
-    } catch (const std::exception&) {
-    }
-}

test/functional/feature_addrman.py

@@ -19,6 +19,7 @@ def serialize_addrman(
     format=1,
     lowest_compatible=3,
     net_magic="regtest",
+    bucket_key=1,
     len_new=None,
     len_tried=None,
     mock_checksum=None,
@@ -29,7 +30,7 @@ def serialize_addrman(
     r = MAGIC_BYTES[net_magic]
     r += struct.pack("B", format)
     r += struct.pack("B", INCOMPATIBILITY_BASE + lowest_compatible)
-    r += ser_uint256(1)
+    r += ser_uint256(bucket_key)
     r += struct.pack("i", len_new or len(new))
     r += struct.pack("i", len_tried or len(tried))
     ADDRMAN_NEW_BUCKET_COUNT = 1 << 10
@@ -119,6 +120,14 @@ class AddrmanTest(BitcoinTestFramework):
             match=ErrorMatch.FULL_REGEX,
         )
 
+        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,
+        )
+
         self.log.info("Check that missing addrman is recreated")
         self.stop_node(0)
         os.remove(peers_dat)