assumeutxo: Get rid of faked nTx and nChainTx values

The `PopulateAndValidateSnapshot` function introduced in
f6e2da5 from bitcoin#19806 has been setting fake
`nTx` and `nChainTx` values that can show up in RPC results (see bitcoin#29328) and
make `CBlockIndex` state hard to reason about, because it is difficult to know
whether the values are real or fake.

Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the
values are unknown, instead of faking them.

This commit fixes an assert failure in the (pindex->nChainTx == pindex->nTx +
prev_chain_tx) check that would previously happen if a snapshot was loaded, and
a block was submitted which forked from the chain before the snapshot block and
after the last downloaded background chain block. This block would not be
marked assumed-valid because it would not be an ancestor of the snapshot, and
it would have nTx set, nChainTx unset, and prev->nChainTx set with a fake
value, so the assert would fail. After this commit, prev->nChainTx is unset
instead of being set to a fake value, so the assert succeeds. A test which
submits a block like this and previously crashed the node has been added in
feature_assumeutxo.py. It was written and posted by maflcko in
bitcoin#29261 (comment)

Compatibility note: This change could result in -checkblockindex failures if a
snapshot was loaded by a previous version of Bitcoin Core and not fully
validated, because fake nTx values will have been saved to the block index. It
would be pretty easy to avoid these failures by adding some compatibility code
to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake
(when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a
little simpler not to worry about being compatible in this case.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

src/chain.h

@@ -98,16 +98,20 @@ enum BlockStatus : uint32_t {
 
  /**
  * Only first tx is coinbase, 2 <= coinbase input script length <= 100, transactions valid, no duplicate txids,
- * sigops, size, merkle root. Implies all parents are at least TREE but not necessarily TRANSACTIONS. When all
- * parent blocks also have TRANSACTIONS, CBlockIndex::nChainTx will be set.
+ * sigops, size, merkle root. Implies all parents are at least TREE but not necessarily TRANSACTIONS.
+ *
+ * If a block's validity is at least VALID_TRANSACTIONS, CBlockIndex::nTx will be set. If a block and all previous
+ * blocks back to the genesis block or an assumeutxo snapshot block are at least VALID_TRANSACTIONS,
+ * CBlockIndex::nChainTx will be set.
  */
  BLOCK_VALID_TRANSACTIONS = 3,
 
  //! Outputs do not overspend inputs, no double spends, coinbase output ok, no immature coinbase spends, BIP30.
- //! Implies all parents are either at least VALID_CHAIN, or are ASSUMED_VALID
+ //! Implies all previous blocks back to the genesis block or an assumeutxo snapshot block are at least VALID_CHAIN.
  BLOCK_VALID_CHAIN = 4,
 
- //! Scripts & signatures ok. Implies all parents are either at least VALID_SCRIPTS, or are ASSUMED_VALID.
+ //! Scripts & signatures ok. Implies all previous blocks back to the genesis block or an assumeutxo snapshot block
+ //! are at least VALID_SCRIPTS.
  BLOCK_VALID_SCRIPTS = 5,
 
  //! All validity bits.
@@ -173,21 +177,16 @@ class CBlockIndex
  //! (memory only) Total amount of work (expected number of hashes) in the chain up to and including this block
  arith_uint256 nChainWork{};
 
- //! Number of transactions in this block.
+ //! Number of transactions in this block. This will be nonzero if the block
+ //! reached the VALID_TRANSACTIONS level, and zero otherwise.
  //! Note: in a potential headers-first mode, this number cannot be relied upon
- //! Note: this value is faked during UTXO snapshot load to ensure that
- //! LoadBlockIndex() will load index entries for blocks that we lack data for.
- //! @sa ActivateSnapshot
  unsigned int nTx{0};
 
  //! (memory only) Number of transactions in the chain up to and including this block.
- //! This value will be non-zero only if and only if transactions for this block and all its parents are available.
+ //! This value will be non-zero if this block and all previous blocks back
+ //! to the genesis block or an assumeutxo snapshot block have reached the
+ //! VALID_TRANSACTIONS level.
  //! Change to 64-bit type before 2024 (assuming worst case of 60 byte transactions).
- //!
- //! Note: this value is faked during use of a UTXO snapshot because we don't
- //! have the underlying block data available during snapshot load.
- //! @sa AssumeutxoData
- //! @sa ActivateSnapshot
  unsigned int nChainTx{0};
 
  //! Verification status of this block. See enum BlockStatus
@@ -262,15 +261,14 @@ class CBlockIndex
  }
 
  /**
- * Check whether this block's and all previous blocks' transactions have been
- * downloaded (and stored to disk) at some point.
+ * Check whether this block and all previous blocks back to the genesis block or an assumeutxo snapshot block have
+ * reached VALID_TRANSACTIONS and had transactions downloaded (and stored to disk) at some point.
  *
  * Does not imply the transactions are consensus-valid (ConnectTip might fail)
  * Does not imply the transactions are still stored on disk. (IsBlockPruned might return true)
  *
- * 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.
+ * Note that this will be true for the snapshot base block, if one is loaded, since its nChainTx value will have
+ * been set manually based on the related AssumeutxoData entry.
  */
  bool HaveNumChainTxs() const { return nChainTx != 0; }
 

src/test/util/chainstate.h

@@ -91,13 +91,16 @@ CreateAndActivateUTXOSnapshot(
  // these blocks instead
  CBlockIndex *pindex = orig_tip;
  while (pindex && pindex != chain.m_chain.Tip()) {
- pindex->nStatus &= ~BLOCK_HAVE_DATA;
- pindex->nStatus &= ~BLOCK_HAVE_UNDO;
- // We have to set the ASSUMED_VALID flag, because otherwise it
- // would not be possible to have a block index entry without HAVE_DATA
- // and with nTx > 0 (since we aren't setting the pruned flag);
- // see CheckBlockIndex().
- pindex->nStatus |= BLOCK_ASSUMED_VALID;
+ // Remove all data and validity flags by just setting
+ // BLOCK_VALID_TREE. Also reset transaction counts and sequence
+ // ids that are set when blocks are received, to make test setup
+ // more realistic and satisfy consistency checks in
+ // CheckBlockIndex().
+ assert(pindex->IsValid(BlockStatus::BLOCK_VALID_TREE));
+ pindex->nStatus = BlockStatus::BLOCK_VALID_TREE;
+ pindex->nTx = 0;
+ pindex->nChainTx = 0;
+ pindex->nSequenceId = 0;
  pindex = pindex->pprev;
  }
  }

src/test/validation_chainstatemanager_tests.cpp

@@ -463,6 +463,8 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
  // Blocks with heights in range [91, 110] are marked ASSUMED_VALID
  if (i < last_assumed_valid_idx && i >= assumed_valid_start_idx) {
  index->nStatus = BlockStatus::BLOCK_VALID_TREE | BlockStatus::BLOCK_ASSUMED_VALID;
+ index->nTx = 0;
+ index->nChainTx = 0;
  }
 
  ++num_indexes;