Merge bitcoin/bitcoin#29370: assumeutxo: Get rid of faked nTx and nChainTx values

9d9a7458a2570f7db56ab626b22010591089c312 assumeutxo: Remove BLOCK_ASSUMED_VALID flag (Ryan Ofsky)
ef174e9ed21c08f38e5d4b537b6decfd1f646db9 test: assumeutxo snapshot block CheckBlockIndex crash test (Ryan Ofsky)
0391458d767b842a7925785a7053400c0e1cb55a test: assumeutxo stale block CheckBlockIndex crash test (Ryan Ofsky)
ef29c8b662309a438121a83f27fd7bdd1779700c assumeutxo: Get rid of faked nTx and nChainTx values (Ryan Ofsky)
9b97d5bbf980d657a277c85d113c2ae3e870e0ec doc: Improve comments describing setBlockIndexCandidates checks (Ryan Ofsky)
0fd915ee6bef63bb360ccc5c039a3c11676c38e3 validation: Check GuessVerificationProgress is not called with disconnected block (Ryan Ofsky)
63e8fc912c21a2f5b47e8eab10fb13c604afed85 ci: add getchaintxstats ubsan suppressions (Ryan Ofsky)
f252e687ec94b6ccafb5bc44b7df3daeb473fdea assumeutxo test: Add RPC test for fake nTx and nChainTx values (Ryan Ofsky)

Pull request description:

  The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake `nTx` and `nChainTx` values that can show up in RPC results (https://github.com/bitcoin/bitcoin/issues/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. Also drop no-longer needed `BLOCK_ASSUMED_VALID` flag.

  Dropping the faked values also fixes assert failures in the `CheckBlockIndex` `(pindex->nChainTx == pindex->nTx + prev_chain_tx)` check that could happen previously if forked or out-of-order blocks before the snapshot got submitted while the snapshot was being validated. The PR includes two commits adding tests for these failures and describing them in detail.

  Compatibility note: This change could cause new `-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.

ACKs for top commit:
  Sjors:
    re-ACK 9d9a7458a2570f7db56ab626b22010591089c312
  achow101:
    ACK 9d9a7458a2570f7db56ab626b22010591089c312
  mzumsande:
    Tested ACK 9d9a7458a2570f7db56ab626b22010591089c312
  maflcko:
    ACK 9d9a7458a2570f7db56ab626b22010591089c312 🎯

Tree-SHA512: b1e1e2731ec36be30d5f5914042517219378fc31486674030c29d9c7488ed83fb60ba7095600f469dc32f0d8ba79c49ff7706303006507654e1762f26ee416e0

1 files changed, 4 insertions, 3 deletions

diff --git a/src/validation.h b/src/validation.h
index b64ba4dcbc..bcf153719a 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -585,9 +585,10 @@ public:
     const CBlockIndex* SnapshotBase() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     /**
-     * The set of all CBlockIndex entries with either BLOCK_VALID_TRANSACTIONS (for
-     * itself and all ancestors) *or* BLOCK_ASSUMED_VALID (if using background
-     * chainstates) and as good as our current tip or better. Entries may be failed,
+     * The set of all CBlockIndex entries that have as much work as our current
+     * tip or more, and transaction data needed to be validated (with
+     * BLOCK_VALID_TRANSACTIONS for each block and its parents back to the
+     * genesis block or an assumeutxo snapshot block). Entries may be failed,
      * though, and pruning nodes may be missing the data for the block.
      */
     std::set<CBlockIndex*, node::CBlockIndexWorkComparator> setBlockIndexCandidates;