Merge bitcoin/bitcoin#24138: index: Commit MuHash and best block together for coinstatsindex

691d45fdc83ec14f87a400f548553168ac70263f Add coinstatsindex_unclean_shutdown test (Ryan Ofsky)
eb6cc05da32c5bde122725a0bc907d3767a791cd index: Commit DB_MUHASH and DB_BEST_BLOCK to disk together (Martin Zumsande)

Pull request description:

  Fixes #24076

  Coinstatsindex currently writes the MuHash (`DB_MUHASH`) to disk in `CoinStatsIndex::WriteBlock()` and `CoinStatsIndex::ReverseBlock()`, but the best synced block is written in `BaseIndex::Commit()`. These are called at different points in time, both during the ThreadSync phase, and also after the initial sync is finished and validation callbacks (`BlockConnected()` vs `ChainStateFlushed()`) perform the syncing.

  As a result, the index DB is temporarily in an inconsistent state, and if bitcoind is terminated uncleanly (so that there is no time to call `Commit()` by receiving an interrupt or by flushing the chainstate) this leads to problems:
  On the next startup, `Init()` will read the best block and a MuHash that corresponds to a different (higher) block. Indexing will  be picked up at the the best block processing some blocks again, but since MuHash is a rolling hash, it will process some utxos twice and the muhashes for all future blocks will be wrong, as was observed in #24076.

  Fix this by always committing `DB_MUHASH` together with `DB_BEST_BLOCK`.

  Note that the block data for the index is still written at different times, but this does not corrupt the index - at worst, these entries will be processed another time and overwritten after an unclean shutdown and restart.

ACKs for top commit:
  ryanofsky:
    Code review ACK 691d45fdc83ec14f87a400f548553168ac70263f. Only change since last review is adding test
  fjahr:
    ACK 691d45fdc83ec14f87a400f548553168ac70263f

Tree-SHA512: e1c3b5f06fa4baacd1b070abb0f8111fe2ea4a001ca8b8bf892e96597cf8b5d5ea10fa8fb837cfbf46648f052c742d912add4ce26d4406294fc5fc20809a0e1b

6 files changed, 83 insertions, 12 deletions

diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp
index a1c8a5937c..386eb67ce9 100644
--- a/src/index/coinstatsindex.cpp
+++ b/src/index/coinstatsindex.cpp
@@ -228,10 +228,9 @@ bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
     m_muhash.Finalize(out);
     value.second.muhash = out;
 
-    CDBBatch batch(*m_db);
-    batch.Write(DBHeightKey(pindex->nHeight), value);
-    batch.Write(DB_MUHASH, m_muhash);
-    return m_db->WriteBatch(batch);
+    // Intentionally do not update DB_MUHASH here so it stays in sync with
+    // DB_BEST_BLOCK, and the index is not corrupted if there is an unclean shutdown.
+    return m_db->Write(DBHeightKey(pindex->nHeight), value);
 }
 
 static bool CopyHeightIndexToHashIndex(CDBIterator& db_it, CDBBatch& batch,
@@ -388,6 +387,14 @@ bool CoinStatsIndex::Init()
     return true;
 }
 
+bool CoinStatsIndex::CommitInternal(CDBBatch& batch)
+{
+    // DB_MUHASH should always be committed in a batch together with DB_BEST_BLOCK
+    // to prevent an inconsistent state of the DB.
+    batch.Write(DB_MUHASH, m_muhash);
+    return BaseIndex::CommitInternal(batch);
+}
+
 // Reverse a single block as part of a reorg
 bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex)
 {
@@ -489,5 +496,5 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
     Assert(m_total_unspendables_scripts == read_out.second.total_unspendables_scripts);
     Assert(m_total_unspendables_unclaimed_rewards == read_out.second.total_unspendables_unclaimed_rewards);
 
-    return m_db->Write(DB_MUHASH, m_muhash);
+    return true;
 }
diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h
index d2a6c9c964..24190ac137 100644
--- a/src/index/coinstatsindex.h
+++ b/src/index/coinstatsindex.h
@@ -39,6 +39,8 @@ private:
 protected:
     bool Init() override;
 
+    bool CommitInternal(CDBBatch& batch) override;
+
     bool WriteBlock(const CBlock& block, const CBlockIndex* pindex) override;
 
     bool Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip) override;
diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp
index 92de4ec7ba..5b73481bc1 100644
--- a/src/test/coinstatsindex_tests.cpp
+++ b/src/test/coinstatsindex_tests.cpp
@@ -2,8 +2,10 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <chainparams.h>
 #include <index/coinstatsindex.h>
 #include <test/util/setup_common.h>
+#include <test/util/validation.h>
 #include <util/time.h>
 #include <validation.h>
 
@@ -16,6 +18,17 @@ using node::CoinStatsHashType;
 
 BOOST_AUTO_TEST_SUITE(coinstatsindex_tests)
 
+static void IndexWaitSynced(BaseIndex& index)
+{
+    // Allow the CoinStatsIndex to catch up with the block index that is syncing
+    // in a background thread.
+    const auto timeout = GetTime<std::chrono::seconds>() + 120s;
+    while (!index.BlockUntilSyncedToCurrentChain()) {
+        BOOST_REQUIRE(timeout > GetTime<std::chrono::milliseconds>());
+        UninterruptibleSleep(100ms);
+    }
+}
+
 BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
 {
     CoinStatsIndex coin_stats_index{1 << 20, true};
@@ -36,13 +49,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
 
     BOOST_REQUIRE(coin_stats_index.Start(m_node.chainman->ActiveChainstate()));
 
-    // Allow the CoinStatsIndex to catch up with the block index that is syncing
-    // in a background thread.
-    const auto timeout = GetTime<std::chrono::seconds>() + 120s;
-    while (!coin_stats_index.BlockUntilSyncedToCurrentChain()) {
-        BOOST_REQUIRE(timeout > GetTime<std::chrono::milliseconds>());
-        UninterruptibleSleep(100ms);
-    }
+    IndexWaitSynced(coin_stats_index);
 
     // Check that CoinStatsIndex works for genesis block.
     const CBlockIndex* genesis_block_index;
@@ -78,4 +85,44 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
     // Rest of shutdown sequence and destructors happen in ~TestingSetup()
 }
 
+// Test shutdown between BlockConnected and ChainStateFlushed notifications,
+// make sure index is not corrupted and is able to reload.
+BOOST_FIXTURE_TEST_CASE(coinstatsindex_unclean_shutdown, TestChain100Setup)
+{
+    CChainState& chainstate = Assert(m_node.chainman)->ActiveChainstate();
+    const CChainParams& params = Params();
+    {
+        CoinStatsIndex index{1 << 20};
+        BOOST_REQUIRE(index.Start(chainstate));
+        IndexWaitSynced(index);
+        std::shared_ptr<const CBlock> new_block;
+        CBlockIndex* new_block_index = nullptr;
+        {
+            const CScript script_pub_key{CScript() << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG};
+            const CBlock block = this->CreateBlock({}, script_pub_key, chainstate);
+
+            new_block = std::make_shared<CBlock>(block);
+
+            LOCK(cs_main);
+            BlockValidationState state;
+            BOOST_CHECK(CheckBlock(block, state, params.GetConsensus()));
+            BOOST_CHECK(chainstate.AcceptBlock(new_block, state, &new_block_index, true, nullptr, nullptr));
+            CCoinsViewCache view(&chainstate.CoinsTip());
+            BOOST_CHECK(chainstate.ConnectBlock(block, state, new_block_index, view));
+        }
+        // Send block connected notification, then stop the index without
+        // sending a chainstate flushed notification. Prior to #24138, this
+        // would cause the index to be corrupted and fail to reload.
+        ValidationInterfaceTest::BlockConnected(index, new_block, new_block_index);
+        index.Stop();
+    }
+
+    {
+        CoinStatsIndex index{1 << 20};
+        // Make sure the index can be loaded.
+        BOOST_REQUIRE(index.Start(chainstate));
+        index.Stop();
+    }
+}
+
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/test/util/validation.cpp b/src/test/util/validation.cpp
index 1aed492c3c..49535855f9 100644
--- a/src/test/util/validation.cpp
+++ b/src/test/util/validation.cpp
@@ -7,6 +7,7 @@
 #include <util/check.h>
 #include <util/time.h>
 #include <validation.h>
+#include <validationinterface.h>
 
 void TestChainState::ResetIbd()
 {
@@ -20,3 +21,8 @@ void TestChainState::JumpOutOfIbd()
     m_cached_finished_ibd = true;
     Assert(!IsInitialBlockDownload());
 }
+
+void ValidationInterfaceTest::BlockConnected(CValidationInterface& obj, const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex)
+{
+    obj.BlockConnected(block, pindex);
+}
diff --git a/src/test/util/validation.h b/src/test/util/validation.h
index b13aa0be60..b0bc717b6c 100644
--- a/src/test/util/validation.h
+++ b/src/test/util/validation.h
@@ -7,6 +7,8 @@
 
 #include <validation.h>
 
+class CValidationInterface;
+
 struct TestChainState : public CChainState {
     /** Reset the ibd cache to its initial state */
     void ResetIbd();
@@ -14,4 +16,10 @@ struct TestChainState : public CChainState {
     void JumpOutOfIbd();
 };
 
+class ValidationInterfaceTest
+{
+public:
+    static void BlockConnected(CValidationInterface& obj, const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex);
+};
+
 #endif // BITCOIN_TEST_UTIL_VALIDATION_H
diff --git a/src/validationinterface.h b/src/validationinterface.h
index 7c3ce00fbc..ac62f8b467 100644
--- a/src/validationinterface.h
+++ b/src/validationinterface.h
@@ -174,6 +174,7 @@ protected:
      * has been received and connected to the headers tree, though not validated yet */
     virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& block) {};
     friend class CMainSignals;
+    friend class ValidationInterfaceTest;
 };
 
 struct MainSignalsInstance;