bitcoin: assumeutxo: nTx and nChainTx violations in CheckBlockIndex

• The final pindex->IsAssumedValid() check added in #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

After trying this out, I think that this is not the case. The BLOCK_ASSUMED_VALID status is only removed when the block is connected to the chain and raised to BLOCK_VALID_SCRIPTS. However, nTx and nChainTx are updated from their fake to their actual values in AcceptBlock / ReceivedBlockTransactions, which happens before that. So during the period where we receive blocks for the background chain in some, possibly random, order, there will be a mix of blocks with updated nTx and fake nTx ones, so that the condition could fail everywhere in the range of assumed-valid blocks.

• It might possible to add a test covering the assert failure that pindex->IsAssumedValid() from #28791 fixes, since #28791 didn’t seem to include a test

For that, we’d need a chain where some blocks (at least the snapshot block, possibly also others) have additional transactions. It seems not very clean to update the regtest chainparams everytime we want to test a snapshot with a slightly different chain. Not sure if this has been discussed before, but maybe it could make sense to add a RPC that allows us to register an entry to m_assumeutxo_data dynamically (just for regtest of course).

test

Good idea.

On top of https://github.com/bitcoin/bitcoin/pull/29354 , the following diff should crash the node:

diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index 528680f2ca..e9d74ea132 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -165,6 +165,14 @@ class AssumeutxoTest(BitcoinTestFramework):
                 self.mini_wallet.send_self_transfer(from_node=n0)
             self.generate(n0, nblocks=1, sync_fun=self.no_op)
             newblock = n0.getblock(n0.getbestblockhash(), 0)
+            if i == 4:
+                # Create a stale block
+                temp_invalid = n0.getbestblockhash()
+                n0.invalidateblock(temp_invalid)
+                stale_hash = self.generateblock(n0, output="raw(aaaa)", transactions=[], sync_fun=self.no_op)["hash"]
+                n0.invalidateblock(stale_hash)
+                n0.reconsiderblock(temp_invalid)
+                stale_block = n0.getblock(stale_hash, 0)
 
             # make n1 aware of the new header, but don't give it the block.
             n1.submitheader(newblock)
@@ -215,6 +223,12 @@ class AssumeutxoTest(BitcoinTestFramework):
 
         assert_equal(n1.getblockchaininfo()["blocks"], SNAPSHOT_BASE_HEIGHT)
 
+        self.log.info("Sumbit a stale block")
+        n1.submitblock(stale_block)
+        n1.getchaintips()
+        n1.getblock(stale_hash)
+        #assert False
+
         self.log.info("Submit a spending transaction for a snapshot chainstate coin to the mempool")
         # spend the coinbase output of the first block that is not available on node1
         spend_coin_blockhash = n1.getblockhash(START_HEIGHT + 1)

Let me know if I should reopen this issue?

It’s good to close. Extending the assert with https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1908513362 would only be a marginal improvement. I think a better long term fix would be to get rid of fake nTx and nChainTx values as described in https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1914922108 “In the long run…”.

So during the period where we receive blocks for the background chain in some, possibly random, order, there will be a mix of blocks with updated nTx and fake nTx ones, so that the condition could fail everywhere in the range of assumed-valid blocks.

Thanks for figuring this out and trying this. ~Trying to put this all together it seems like these are the cases:~

~1. Case where IsValid() is true. Then nChainTx must be prev nChainTx + nTx.~ ~2. Case where nTx is 0. Then nChainTx must be 0 because the block doesn’t have transactions yet.~ ~3. Case where prev nChainTx is 0. Then nChainTx must be 0 because some ancestor block doesn’t have transactions yet.~ ~4. Case where IsAssumedValid() is true. Probably the only meaningful thing to check in this case is that nChainTx is greater than prev nChainTx. This should always be true except if prev IsValid() is true and this is not the snapshot block. In that case, there will be a discontinuity and nChainTx will decrease because it is a bogus value following a real value.~

EDIT: There are a number of mistakes in the suggestion above. It would be good to check for increasing nChainTx values, but the breakdown above is not correct. The following checks do seem to work, though:

if (!pindex->pprev) {
    // If there's no previous block, nChainTx should be set to the number
    // of transactions in this block.
    assert(pindex->nChainTx == pindex->nTx);
} else if (pindex->IsAssumedValid()) {
    // If block is assumed valid, nChainTx could be faked, but should at
    // least be increasing between blocks. The only exception is
    // assumed-valid blocks that are not the snapshot block and don't
    // have transactions, directly following blocks that do have
    // transactions. The faked nChainTx values in these will probably be
    // less than the non-faked values in the previous blocks.
    assert(pindex->nChainTx > pindex->pprev->nChainTx || (!pindex->IsValid() && pindex->pprev->IsValid() && pindex != GetSnapshotBaseBlock()));
} else if (pindex->nTx == 0 || pindex->pprev->nChainTx == 0) {
    // If block is missing transactions, or any parent block is,
    // nChainTx should be unset.
    assert(pindex->nChainTx == 0);
} else {
    // For normal blocks, nChainTx should be set to the sum of
    // transactions in this and previous blocks.
    assert(pindex->nChainTx == pindex->pprev->nChainTx + pindex->nTx);
}

$ ./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.

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.