Merge bitcoin#28562: AssumeUTXO follow-ups

5d227a6 rpc: Use Ensure(Any)Chainman in assumeutxo related RPCs (Fabian Jahr)
710e5db doc: Drop references to assumevalid in assumeutxo docs (Fabian Jahr)
1ff1c34 test: Rename wait_until_helper to wait_until_helper_internal (Fabian Jahr)
a482f86 chain: Rename HaveTxsDownloaded to HaveNumChainTxs (Fabian Jahr)
82e48d2 blockstorage: Let FlushChainstateBlockFile return true in case of missing cursor (Fabian Jahr)
73700fb validation, test: Improve and document nChainTx check for testability (Fabian Jahr)
2c9354f doc: Add snapshot chainstate removal warning to reindexing documentation (Fabian Jahr)
4e915e9 test: Improvements of feature_assumeutxo (Fabian Jahr)
a47fbe7 doc: Add and edit some comments around assumeutxo (Fabian Jahr)
0a39b8c validation: remove unused mempool param in DetectSnapshotChainstate (Fabian Jahr)

Pull request description:

  Addressing what I consider to be non- or not-too-controversial comments from bitcoin#27596.

  Let me know if I missed anything among the many comments that can be easily included here.

ACKs for top commit:
  ryanofsky:
    Code review ACK 5d227a6. Just suggested doc change and new EnsureChainman RPC cleanup commit since last review.

Tree-SHA512: 6f7c762100e18f82946b881676db23e67da7dc3a8bf04e4999a183e90b4f150a0b1202bcb95920ba937a358867bbf2eca300bd84b9b1776c7c490410e707c267

Author: fanquake

doc/design/assumeutxo.md

@@ -1,7 +1,7 @@
 # assumeutxo
 
 Assumeutxo is a feature that allows fast bootstrapping of a validating bitcoind
-instance with a very similar security model to assumevalid.
+instance.
 
 The RPC commands `dumptxoutset` and `loadtxoutset` are used to
 respectively generate and load UTXO snapshots. The utility script

doc/release-notes-27596.md

@@ -12,7 +12,7 @@ RPC
 `loadtxoutset` has been added, which allows loading a UTXO snapshot of the format
 generated by `dumptxoutset`. Once this snapshot is loaded, its contents will be
 deserialized into a second chainstate data structure, which is then used to sync to
-the network's tip under a security model very much like `assumevalid`.
+the network's tip.
 
 Meanwhile, the original chainstate will complete the initial block download process in
 the background, eventually validating up to the block that the snapshot is based upon.

src/chain.h

@@ -280,10 +280,8 @@ class CBlockIndex
      * Note that this will be true for the snapshot base block, if one is loaded (and
      * all subsequent assumed-valid blocks) since its nChainTx value will have been set
      * manually based on the related AssumeutxoData entry.
-     *
-     * TODO: potentially change the name of this based on the fact above.
      */
-    bool HaveTxsDownloaded() const { return nChainTx != 0; }
+    bool HaveNumChainTxs() const { return nChainTx != 0; }
 
     NodeSeconds Time() const
     {

src/init.cpp

@@ -462,8 +462,8 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-prune=<n>", strprintf("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex. "
             "Warning: Reverting this setting requires re-downloading the entire blockchain. "
             "(default: 0 = disable pruning blocks, 1 = allow manual pruning via RPC, >=%u = automatically prune block files to stay under the specified target size in MiB)", MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-    argsman.AddArg("-reindex", "Rebuild chain state and block index from the blk*.dat files on disk. This will also rebuild active optional indexes.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-    argsman.AddArg("-reindex-chainstate", "Rebuild chain state from the currently indexed blocks. When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-reindex", "If enabled, wipe chain state and block index, and rebuild them from blk*.dat files on disk. Also wipe and rebuild other optional indexes that are active. If an assumeutxo snapshot was loaded, its chainstate will be wiped as well. The snapshot can then be reloaded via RPC.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-reindex-chainstate", "If enabled, wipe chain state, and rebuild it from blk*.dat files on disk. If an assumeutxo snapshot was loaded, its chainstate will be wiped as well. The snapshot can then be reloaded via RPC.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-settings=<file>", strprintf("Specify path to dynamic settings data file. Can be disabled with -nosettings. File is written at runtime and not meant to be edited by users (use %s instead for custom settings). Relative paths will be prefixed by datadir location. (default: %s)", BITCOIN_CONF_FILENAME, BITCOIN_SETTINGS_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 #if HAVE_SYSTEM
     argsman.AddArg("-startupnotify=<cmd>", "Execute command on startup.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

src/kernel/chainparams.cpp

@@ -495,7 +495,7 @@ class CRegTestParams : public CChainParams
             {
                 .height = 110,
                 .hash_serialized = AssumeutxoHash{uint256S("0x1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618")},
-                .nChainTx = 110,
+                .nChainTx = 111,
                 .blockhash = uint256S("0x696e92821f65549c7ee134edceeeeaaa4105647a3c4fd9f298c0aec0ab50425c")
             },
             {

src/net_processing.cpp

@@ -1448,7 +1448,7 @@ void PeerManagerImpl::FindNextBlocks(std::vector<const CBlockIndex*>& vBlocks, c
                 return;
             }
             if (pindex->nStatus & BLOCK_HAVE_DATA || (activeChain && activeChain->Contains(pindex))) {
-                if (activeChain && pindex->HaveTxsDownloaded())
+                if (activeChain && pindex->HaveNumChainTxs())
                     state->pindexLastCommonBlock = pindex;
             } else if (!IsBlockRequested(pindex->GetBlockHash())) {
                 // The block is not already downloaded, and not yet in flight.
@@ -1937,6 +1937,8 @@ void PeerManagerImpl::BlockConnected(
         }
     }
 
+    // The following task can be skipped since we don't maintain a mempool for
+    // the ibd/background chainstate.
     if (role == ChainstateRole::BACKGROUND) {
         return;
     }
@@ -2231,7 +2233,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
         LOCK(cs_main);
         const CBlockIndex* pindex = m_chainman.m_blockman.LookupBlockIndex(inv.hash);
         if (pindex) {
-            if (pindex->HaveTxsDownloaded() && !pindex->IsValid(BLOCK_VALID_SCRIPTS) &&
+            if (pindex->HaveNumChainTxs() && !pindex->IsValid(BLOCK_VALID_SCRIPTS) &&
                     pindex->IsValid(BLOCK_VALID_TREE)) {
                 // If we have the block and all of its parents, but have not yet validated it,
                 // we might be in the middle of connecting it (ie in the unlock of cs_main

src/node/blockstorage.cpp

@@ -761,12 +761,14 @@ bool BlockManager::FlushChainstateBlockFile(int tip_height)
 {
     LOCK(cs_LastBlockFile);
     auto& cursor = m_blockfile_cursors[BlockfileTypeForHeight(tip_height)];
+    // If the cursor does not exist, it means an assumeutxo snapshot is loaded,
+    // but no blocks past the snapshot height have been written yet, so there
+    // is no data associated with the chainstate, and it is safe not to flush.
     if (cursor) {
-        // The cursor may not exist after a snapshot has been loaded but before any
-        // blocks have been downloaded.
         return FlushBlockFile(cursor->file_num, /*fFinalize=*/false, /*finalize_undo=*/false);
     }
-    return false;
+    // No need to log warnings in this case.
+    return true;
 }
 
 uint64_t BlockManager::CalculateCurrentUsage()

src/node/chainstate.cpp

@@ -185,7 +185,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
     chainman.InitializeChainstate(options.mempool);
 
     // Load a chain created from a UTXO snapshot, if any exist.
-    bool has_snapshot = chainman.DetectSnapshotChainstate(options.mempool);
+    bool has_snapshot = chainman.DetectSnapshotChainstate();
 
     if (has_snapshot && (options.reindex || options.reindex_chainstate)) {
         LogPrintf("[snapshot] deleting snapshot chainstate due to reindexing\n");

src/rpc/blockchain.cpp

@@ -1455,7 +1455,7 @@ static RPCHelpMan getchaintips()
         } else if (block->nStatus & BLOCK_FAILED_MASK) {
             // This block or one of its ancestors is invalid.
             status = "invalid";
-        } else if (!block->HaveTxsDownloaded()) {
+        } else if (!block->HaveNumChainTxs()) {
             // This block cannot be connected because full block data for it or one of its parents is missing.
             status = "headers-only";
         } else if (block->IsValid(BLOCK_VALID_SCRIPTS)) {
@@ -2707,7 +2707,7 @@ static RPCHelpMan loadtxoutset()
         "Load the serialized UTXO set from disk.\n"
         "Once this snapshot is loaded, its contents will be "
         "deserialized into a second chainstate data structure, which is then used to sync to "
-        "the network's tip under a security model very much like `assumevalid`. "
+        "the network's tip. "
         "Meanwhile, the original chainstate will complete the initial block download process in "
         "the background, eventually validating up to the block that the snapshot is based upon.\n\n"
 
@@ -2759,7 +2759,7 @@ static RPCHelpMan loadtxoutset()
     LogPrintf("[snapshot] waiting to see blockheader %s in headers chain before snapshot activation\n",
         base_blockhash.ToString());
 
-    ChainstateManager& chainman = *node.chainman;
+    ChainstateManager& chainman = EnsureChainman(node);
 
     while (max_secs_to_wait_for_headers > 0) {
         snapshot_start_block = WITH_LOCK(::cs_main,
@@ -2831,8 +2831,7 @@ return RPCHelpMan{
     LOCK(cs_main);
     UniValue obj(UniValue::VOBJ);
 
-    NodeContext& node = EnsureAnyNodeContext(request.context);
-    ChainstateManager& chainman = *node.chainman;
+    ChainstateManager& chainman = EnsureAnyChainman(request.context);
 
     auto make_chain_data = [&](const Chainstate& cs, bool validated) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
         AssertLockHeld(::cs_main);

src/test/fuzz/chain.cpp

@@ -29,7 +29,7 @@ FUZZ_TARGET(chain)
         (void)disk_block_index->GetBlockTimeMax();
         (void)disk_block_index->GetMedianTimePast();
         (void)disk_block_index->GetUndoPos();
-        (void)disk_block_index->HaveTxsDownloaded();
+        (void)disk_block_index->HaveNumChainTxs();
         (void)disk_block_index->IsValid();
     }