bitcoin: 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 --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
About this issue
- Original URL
- State: closed
- Created 5 months ago
- Reactions: 1
- Comments: 19 (19 by maintainers)
Commits related to this issue
- Merge bitcoin/bitcoin#29299: validation: fix misleading checkblockindex comments 9819db4ccaa03519a78d4d9ecce9f89f5be669e5 validation: move nChainTx assert down in CheckBlockIndex (Martin Zumsande) 03... — committed to fanquake/bitcoin by glozow 5 months ago
- assumeutxo: Get rid of faked nTx and nChainTx values The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake `nTx` and `nCh... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- assumeutxo: Get rid of faked nTx and nChainTx values The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake `nTx` and `nCh... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- assumeutxo: Get rid of faked nTx and nChainTx values The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake `nTx` and `nCh... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- assumeutxo: Get rid of faked nTx and nChainTx values The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake `nTx` and `nCh... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- assumeutxo: Get rid of faked nTx and nChainTx values The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake `nTx` and `nCh... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- assumeutxo: Get rid of faked nTx and nChainTx values The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake `nTx` and `nCh... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- assumeutxo: Get rid of faked nTx and nChainTx values The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake `nTx` and `nCh... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- assumeutxo: Get rid of faked nTx and nChainTx values The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake `nTx` and `nCh... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- assumeutxo: Get rid of faked nTx and nChainTx values The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake `nTx` and `nCh... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- assumeutxo: Get rid of faked nTx and nChainTx values The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake `nTx` and `nCh... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- test: assumeutxo stale block CheckBlockIndex crash test Add a test for a CheckBlockIndex crash that would happen before previous "assumeutxo: Get rid of faked nTx and nChainTx values" commit. The cr... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- test: assumeutxo stale block CheckBlockIndex crash test Add a test for a CheckBlockIndex crash that would happen before previous "assumeutxo: Get rid of faked nTx and nChainTx values" commit. The cr... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- test: assumeutxo stale block CheckBlockIndex crash test Add a test for a CheckBlockIndex crash that would happen before previous "assumeutxo: Get rid of faked nTx and nChainTx values" commit. The cr... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- test: assumeutxo stale block CheckBlockIndex crash test Add a test for a CheckBlockIndex crash that would happen before previous "assumeutxo: Get rid of faked nTx and nChainTx values" commit. The cr... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- test: assumeutxo stale block CheckBlockIndex crash test Add a test for a CheckBlockIndex crash that would happen before previous "assumeutxo: Get rid of faked nTx and nChainTx values" commit. The cr... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- test: assumeutxo stale block CheckBlockIndex crash test Add a test for a CheckBlockIndex crash that would happen before previous "assumeutxo: Get rid of faked nTx and nChainTx values" commit. The cr... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- test: assumeutxo stale block CheckBlockIndex crash test Add a test for a CheckBlockIndex crash that would happen before previous "assumeutxo: Get rid of faked nTx and nChainTx values" commit. The cr... — committed to ryanofsky/bitcoin by ryanofsky 5 months ago
- test: assumeutxo stale block CheckBlockIndex crash test Add a test for a CheckBlockIndex crash that would happen before previous "assumeutxo: Get rid of faked nTx and nChainTx values" commit. The cr... — committed to mzumsande/bitcoin by ryanofsky 5 months ago
- test: assumeutxo stale block CheckBlockIndex crash test Add a test for a CheckBlockIndex crash that would happen before previous "assumeutxo: Get rid of faked nTx and nChainTx values" commit. The cr... — committed to BitgesellOfficial/bitgesell by ryanofsky 5 months ago
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 toBLOCK_VALID_SCRIPTS
. However,nTx
andnChainTx
are updated from their fake to their actual values inAcceptBlock
/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 updatednTx
and fakenTx
ones, so that the condition could fail everywhere in the range of assumed-valid blocks.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).Good idea.
On top of https://github.com/bitcoin/bitcoin/pull/29354 , the following diff should crash the node:
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
andnChainTx
values as described in https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1914922108 “In the long run…”.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. ThennChainTx
must be prevnChainTx
+nTx
.~ ~2. Case wherenTx
is 0. ThennChainTx
must be 0 because the block doesn’t have transactions yet.~ ~3. Case where prevnChainTx
is 0. ThennChainTx
must be 0 because some ancestor block doesn’t have transactions yet.~ ~4. Case whereIsAssumedValid()
is true. Probably the only meaningful thing to check in this case is thatnChainTx
is greater than prevnChainTx
. This should always be true except if prevIsValid()
is true and this is not the snapshot block. In that case, there will be a discontinuity andnChainTx
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:
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.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 callingReceivedBlockTransactions
and therefore skipped setting anChainTx
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.