snapshots: don't core dump when running -checkblockindex after loadtxoutset

[maaku]

Transaction counts aren't known for block history loaded from a snapshot. If you start with -checkblockindex after loading a snapshot, the bitcoin daemon will core dump. The test suite does not check for this because all the snapshots have no non-coinbase transactions (all blocks prior to the snapshot are assumed to have nTx = 1).

Recommend for backport to 26.x

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc, fjahr, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

[fanquake]

cc @pablomartin4btc

[pablomartin4btc]

I'll check this soon.

[luke-jr]

Maybe snapshots should include it?

[maflcko]

For reference, this whole assert block is likely wrong, see the thread at #28562 (comment)

[maaku]

Yes, the entire assert block is wrong. When a snapshot is loaded, the pindex->nTx value is set to 1. So I'm not really sure what these exceptions would be allowing through.

But since this issue affects the v26 release, I've tried to keep the code delta as small and minimally impactful as possible.

[pablomartin4btc]

Removing the testing assert conditions, as explained by @maflcko in the comment link provided above, even with this fix, bitcoind still crashes.

2023-11-09T15:54:40Z [dnsseed] dnsseed thread exit
bitcoind: validation.cpp:4880: void ChainstateManager::CheckBlockIndex(): Assertion `(pindex->nChainTx == pindex->nTx + prev_chain_tx) || pindex->IsAssumedValid()' failed.
Aborted (core dumped)

[maaku]

@luke-jr That would require storing an integer for every historical block. That would only add a megabyte or so to the snapshot size, so it would be quite doable. But it seems kinda silly when the field isn't used (except for this assert), and nothing else about the prior block data is validated.

[pablomartin4btc]

tACK cdc6ac4

I've managed to reproduce the issue

• getblockchaininfo - IBD not completed

  ./src/bitcoin-cli -datadir=${AU_DATADIR} getblockchaininfo
  {
    "chain": "main",
    "blocks": 800000,
    "headers": 815917,
    "bestblockhash": "00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054",
    "difficulty": 53911173001054.59,
    "time": 1690168629,
    "mediantime": 1690165851,
    "verificationprogress": 0.9438294771289651,
    "initialblockdownload": true,
    "chainwork": "00000000000000000000000000000000000000004fc85ab3390629e495bf13d5",
    "size_on_disk": 31288815,
    "pruned": true,
    "pruneheight": 800001,
    "automatic_pruning": true,
    "prune_target_size": 20971520000,
    "warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
  }
  
  

• getchainstates - UTXO snapshot at 800 000

  ./src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
  {
    "headers": 815917,
    "chainstates": [
      {
        "blocks": 71968,
        "bestblockhash": "0000000000d485b326ff1f88f2cb66fcc24c4345d1b151fa5a4d00476f74a977",
        "difficulty": 244.2132230923753,
        "verificationprogress": 0.0001072847948972671,
        "coins_db_cache_bytes": 419430,
        "coins_tip_cache_bytes": 784649420,
        "validated": true
      },
      {
        "blocks": 800000,
        "bestblockhash": "00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054",
        "difficulty": 53911173001054.59,
        "verificationprogress": 0.9438294714681589,
        "coins_db_cache_bytes": 7969177,
        "coins_tip_cache_bytes": 14908338995,
        "snapshot_blockhash": "00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054",
        "validated": false
      }
    ]
  }
  

2023-11-09T14:44:55Z [msghand] New block-relay-only v1 peer connected: version: 70016, blocks=815999, peer=1
bitcoind: validation.cpp:4879: void ChainstateManager::CheckBlockIndex(): Assertion `(pindex->nChainTx == pindex->nTx + prev_chain_tx) || (pindex->nChainTx == 0 && pindex->nTx == 0) || (pindex->nChainTx == 0 && prev_chain_tx == 0 && pindex->pprev)' failed.
Aborted (core dumped)

I've verified this PR fixes the problem.

Since we know that loaded snapshots don't know tx count (*), should we not also avoid this by not letting the user call -checkblockindex under these circumstances and return an error? I'd also add this warning in the documentation if that makes sense.

(*) Is this also happening after background validation is completed?

[pablomartin4btc]

Removing the testing assert conditions, as explained by @maflcko in the comment link provided above, even with this fix, bitcoind still crashes.

2023-11-09T15:54:40Z [dnsseed] dnsseed thread exit
bitcoind: validation.cpp:4880: void ChainstateManager::CheckBlockIndex(): Assertion `(pindex->nChainTx == pindex->nTx + prev_chain_tx) || pindex->IsAssumedValid()' failed.
Aborted (core dumped)

[maaku]

No, that would be likely to cause problems in various workflows. It is entirely reasonable that -checkblockindex is set in a bitcoin.conf file, just as it is done by default in the RPC tests. In that case restarting the daemon after loading a snapshot but before validation of the historical blockchain is complete will cause it to be in an unstartable state, even though there is nothing wrong with the block index. checkblockindex is useful for checking the integrity of the database, even before the whole historical blockchain is downloaded.

[fjahr]

utACK cdc6ac4

This seems to be the best solution for this issue.

[achow101]

ACK cdc6ac4

[jamesob]

Post-merge ACK cdc6ac4

[fanquake]

Backported to 26 in #29209.