Repository: bitcoin/bitcoin (language: cpp)

Issue:
assumeutxo: nTx and nChainTx violations in CheckBlockIndex
When disabling the "test-only" assumptions in CheckBlockIndex, the check fails. This is problematic, because test-only code should not affect the behavior of the program in production.

Current diff:

```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 8c583c586c..00d7ee3736 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4866,9 +4866,9 @@ void ChainstateManager::CheckBlockIndex()
         unsigned int prev_chain_tx = pindex->pprev ? pindex->pprev->nChainTx : 0;
         assert((pindex->nChainTx == pindex->nTx + prev_chain_tx)
                // For testing, allow transaction counts to be completely unset.
-               || (pindex->nChainTx == 0 && pindex->nTx == 0)
+               //|| (pindex->nChainTx == 0 && pindex->nTx == 0)
                // For testing, allow this nChainTx to be unset if previous is also unset.
-               || (pindex->nChainTx == 0 && prev_chain_tx == 0 && pindex->pprev)
+               //|| (pindex->nChainTx == 0 && prev_chain_tx == 0 && pindex->pprev)
                // Transaction counts prior to snapshot are unknown.
                || pindex->IsAssumedValid());
 
```

Steps to reproduce the crash:

```
$ ./src/qt/bitcoin-qt -datadir=/tmp -regtest
> generatetodescriptor 1 raw(ff)
```

Related to https://github.com/bitcoin/bitcoin/pull/28791 and https://github.com/bitcoin/bitcoin/pull/28562#discussion_r1350470707


Discussion / hints:
> `$ ./src/qt/bitcoin-qt -datadir=/tmp -regtest`

Do we not consider regtest "testing"?

ISTM that only "test mode" (regtest) will fail the assertion with the lines removed.
> Do we not consider regtest "testing"?

The comment is vague, but it's really referring to unit tests.

When I suggested adding the two conditions allowing `pindex->nChainTx == 0` "for testing" in https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1323226418, the reason was to avoid crashes in unit tests, because some unit tests skipped calling `ReceivedBlockTransactions` and therefore skipped setting a `nChainTx` value:

https://github.com/bitcoin/bitcoin/blob/651fb034d85eb5db561bfd24b74f7271417defa5/src/validation.cpp#L3613

But outside unit tests the condition `pindex->nChainTx == pindex->nTx + prev_chain_tx` should be true and the assert should succeed.

So there is unexpected behavior here if bitcoin-qt is crashing without these conditions, and maybe there is a real bug. Also this code could probably be improved by updating unit tests to set nChainTx correctly, so the two special case conditions in the assert allowing `pindex->nChainTx == 0` could be dropped.
> So there is unexpected behavior here if bitcoin-qt is crashing without these conditions

This is not specific to `bitcoin-qt`, or assumeutxo. For example, It also happens with `bitcoin-cli -regtest -generate 1` (after creating an empty wallet) on an empty datadir.

I think the comment `// For testing, allow transaction counts to be completely unset.` is wrong. `CheckBlockIndex` traverses the entire block index tree, no matter if we have the block on disk or not. If we don't have it, `ReceivedBlockTransactions`  hasn't been called, and both `nTx` and `nChainTx` are 0.

Therefore, the check `assert((pindex->nChainTx == pindex->nTx + prev_chain_tx)`  cannot be applied to headers for which we never processed the block data, and this has nothing to do with AssumeUtxo or Testing.

I'll open a PR with a fix tomorrow unless someone disagrees.
Oops, sorry I missed that context so my explanation doesn't really describe what the checks are doing (although it does describe what they were intending to do). If the PR is just going to change the comments and delete the words "For testing," that sounds good.
Other notes on improving this assert:

- The final pindex->IsAssumedValid() check added in [#28791](https://github.com/bitcoin/bitcoin/pull/28791) seems a little overbroad. I think the condition `(pindex->nChainTx == pindex->nTx + prev_chain_tx)` should actually be true for all assumed-valid blocks except the first one and the last one, so that check could be tightened up
- It might possible to add a test covering the assert failure that pindex->IsAssumedValid() from [#28791](https://github.com/bitcoin/bitcoin/pull/28791) fixes, since [#28791](https://github.com/bitcoin/bitcoin/pull/28791) didn't seem to include a test
> > `$ ./src/qt/bitcoin-qt -datadir=/tmp -regtest`
> 
> Do we not consider regtest "testing"?

Regtest should have the same properties as `main` for the purposes here. It should be possible to reproduce on main as well, if you want to do the POW.
Ah I see now, thanks. The repro steps threw me off there. I did now also verify that it is hit on (a custom) signet (with `-checkblocks` set).

Fix patch:
diff --git a/src/validation.cpp b/src/validation.cpp
index 8c583c586c204..4dc3a0f512517 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4862,16 +4862,6 @@ void ChainstateManager::CheckBlockIndex()
     CBlockIndex* pindexFirstAssumeValid = nullptr; // Oldest ancestor of pindex which has BLOCK_ASSUMED_VALID
     while (pindex != nullptr) {
         nNodes++;
-        // Make sure nChainTx sum is correctly computed.
-        unsigned int prev_chain_tx = pindex->pprev ? pindex->pprev->nChainTx : 0;
-        assert((pindex->nChainTx == pindex->nTx + prev_chain_tx)
-               // For testing, allow transaction counts to be completely unset.
-               || (pindex->nChainTx == 0 && pindex->nTx == 0)
-               // For testing, allow this nChainTx to be unset if previous is also unset.
-               || (pindex->nChainTx == 0 && prev_chain_tx == 0 && pindex->pprev)
-               // Transaction counts prior to snapshot are unknown.
-               || pindex->IsAssumedValid());
-
         if (pindexFirstAssumeValid == nullptr && pindex->nStatus & BLOCK_ASSUMED_VALID) pindexFirstAssumeValid = pindex;
         if (pindexFirstInvalid == nullptr && pindex->nStatus & BLOCK_FAILED_VALID) pindexFirstInvalid = pindex;
         if (pindexFirstMissing == nullptr && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
@@ -4954,6 +4944,15 @@ void ChainstateManager::CheckBlockIndex()
             // Checks for not-invalid blocks.
             assert((pindex->nStatus & BLOCK_FAILED_MASK) == 0); // The failed mask cannot be set for blocks without invalid parents.
         }
+        // Make sure nChainTx sum is correctly computed.
+        unsigned int prev_chain_tx = pindex->pprev ? pindex->pprev->nChainTx : 0;
+        assert((pindex->nChainTx == pindex->nTx + prev_chain_tx)
+               // Transaction may be completely unset - happens if only the header was accepted but the block hasn't been processed.
+               || (pindex->nChainTx == 0 && pindex->nTx == 0)
+               // nChainTx may be unset, but nTx set (if a block has been accepted, but one of its predecessors hasn't been processed yet)
+               || (pindex->nChainTx == 0 && prev_chain_tx == 0 && pindex->pprev)
+               // Transaction counts prior to snapshot are unknown.
+               || pindex->IsAssumedValid());
         // Chainstate-specific checks on setBlockIndexCandidates
         for (auto c : GetAll()) {
             if (c->m_chain.Tip() == nullptr) continue;


Test patch: