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

ryanofsky · 2024-02-02T16:18:52Z

The PopulateAndValidateSnapshot function introduced in f6e2da5 from #19806 has been setting fake nTx and nChainTx values that can show up in RPC results (#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.

DrahtBot · 2024-02-02T16:18:55Z

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	Sjors, mzumsande, maflcko, achow101
Concept ACK	fjahr

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29656 (chainparams: Change nChainTx type to uint64_t by fjahr)
#29617 (test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply by jrakibi)
#29553 (assumeutxo: Add dumptxoutset height param, remove shell scripts by fjahr)
#28339 (validation: improve performance of CheckBlockIndex by mzumsande)
#27006 (reduce cs_main scope, guard block index 'nFile' under a local mutex by furszy)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko · 2024-02-02T16:47:18Z

Backwards compatibility code in LoadBlockIndex to override nTx == 1 values set by previous code when they are fake (when (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS)

Not sure. This can only happen on test-chains, so I think it would be cleaner to just require the developers, if there are any, to finish their background sync, or to wipe their chainstate, or the blocksdir, or the whole test data dir.

DrahtBot · 2024-02-02T17:16:09Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/21160915955}

ryanofsky · 2024-02-02T18:44:52Z

Not sure. This can only happen on test-chains, so I think it would be cleaner to just require the developers, if there are any, to finish their background sync, or to wipe their chainstate, or the blocksdir, or the whole test data dir.

It makes sense that having backwards compatibility may not be worth it, but I don't see why test chains would set nTx to a real value without also setting BLOCK_VALID_TRANSACTIONS. It seems like we should be able to use BLOCK_VALID_TRANSACTIONS to know if nTx is real or fake, as long as we are checking for BLOCK_VALID_TRANSACTIONS, directly, and not using the IsValid() helper which is also influenced by BLOCK_FAILED flags.

Updated 4c70d99 -> a3228f0 (pr/nofake.1 -> pr/nofake.2, compare) to fix broken validation_block_tests due to a buggy check, also making some minor cleanups

maflcko

lgtm

src/chain.h

src/test/validation_chainstatemanager_tests.cpp

src/test/util/chainstate.h

src/test/validation_chainstatemanager_tests.cpp

src/validation.cpp

maflcko · 2024-02-05T15:00:52Z

Also, it would be good to include the test to ensure the crash no longer happens.

#29261 (comment)

ryanofsky

Thanks for the close review! Will work on your suggestions

src/chain.h

src/test/util/chainstate.h

src/test/validation_chainstatemanager_tests.cpp

src/validation.cpp

ryanofsky

Updated a3228f0 -> 3bdbc3f (pr/nofake.2 -> pr/nofake.3, compare) implementing all suggestions, adding test coverage for the RPCs, and adding a new commit that drops the no longer needed BLOCK_ASSUMED_VALID flag.
Updated 3bdbc3f -> 603a92c (pr/nofake.3 -> pr/nofake.4, compare) removing more BLOCK_ASSUMED_VALID references

src/test/util/chainstate.h

src/test/validation_chainstatemanager_tests.cpp

src/validation.cpp

src/chain.h

src/validation.cpp

test/functional/feature_assumeutxo.py

maflcko · 2024-02-06T09:04:07Z

Needs rebase

ryanofsky · 2024-02-07T01:07:56Z

Needs rebase

Yes, thanks for recognizing the CI failures.

Rebased 603a92c -> 3f0b860 (pr/nofake.4 -> pr/nofake.5, compare) with test fixes needed after silent conflict with #29354

maflcko · 2024-02-07T08:25:29Z

Could include the test #29370 (comment) , or did you want to leave that for later?

ryanofsky

Rebased 345ac98 -> 9d9a745 (pr/nofake.20 -> pr/nofake.21, compare) due to conflict with #29478

ryanofsky · 2024-03-18T18:10:28Z

src/test/util/chainstate.h

-                // and with nTx > 0 (since we aren't setting the pruned flag);
-                // see CheckBlockIndex().
-                pindex->nStatus |= BLOCK_ASSUMED_VALID;
+                // Remove all data and validity flags by just setting


re: #29370 (comment)

The comment above (Reset the HAVE_DATA flags...) could be removed or at least changed to match the changed code here.

The comment above is still accurate and still describes the main thing this is trying to do. Now just other flags are reset as well. Happy to change any of the comments if you have a specific suggestion though.

Sjors · 2024-03-19T10:42:06Z

re-ACK 9d9a745

I also tried generating and loading a signet snapshot with this PR combined with #29612.

mzumsande

Tested ACK 9d9a745

I reviewed the code and tested this running with checkblockindex on signet.

maflcko · 2024-03-20T09:56:54Z

src/validation.cpp

+                    // and the transactions were not pruned (pindexFirstMissing
+                    // is null), it is a potential candidate. The check
+                    // excludes pruned blocks, because if any blocks were
+                    // pruned between pindex the current chain tip, pindex will


nit: 9b97d5b: pindex *and* the current

src/validation.cpp

maflcko

ACK 9d9a745 🎯

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 9d9a7458a2570f7db56ab626b22010591089c312 🎯
m+XWEuj+TMxTAm2vqwQ2KlIlqiGQf/ADm9CzC8dphRCYKKQo4VacrJ9sy4MIZjiOABr94CzwXsoVSmSPlrElBw==

maflcko · 2024-03-20T11:35:41Z

test/functional/feature_assumeutxo.py

+        # setting and testing nChainTx values, and it exposed previous bugs.
+        snapshot_hash = n0.getblockhash(SNAPSHOT_BASE_HEIGHT)
+        snapshot_block = n0.getblock(snapshot_hash, 0)
+        n1.submitblock(snapshot_block)


nit in ef174e9:

I wonder if it would be better to randomly either submit or not submit the block, because the test should be passing in both cases.

If a crash were to happen, it would be intermittent. However, it would still be deterministic, because the randomness seed is printed/logged.

achow101 · 2024-03-20T16:49:08Z

ACK 9d9a745

fjahr · 2024-03-20T19:11:15Z

Post merge utACK 9d9a745

…nected block Use Assume macro as suggested bitcoin/bitcoin#29370 (comment)

The checks are changing slightly in the next commit, so try to explains the ones that exist to avoid confusion (bitcoin/bitcoin#29370 (comment))

Add a test for a CheckBlockIndex crash that would happen before previous "assumeutxo: Get rid of faked nTx and nChainTx values" commit. The crash was an assert failure in the (pindex->nChainTx == pindex->nTx + prev_chain_tx) check that would previously happen if the snapshot block was submitted after loading the snapshot and downloading a few blocks after the snapshot. In that case ReceivedBlockTransactions() previously would overwrite the nChainTx value of the submitted snapshot block with a fake value based on the previous block, so the (pindex->nChainTx == pindex->nTx + prev_chain_tx) check would later fail on the first block after the snapshot. This test was originally posted by Martin Zumsande <mzumsande@gmail.com> in bitcoin/bitcoin#29370 (comment) Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

de71d4d fuzz: improve utxo_snapshot target (Martin Zumsande) Pull request description: Add the possibility of giving more guidance to the creation of the metadata and/or coins, so that the fuzzer gets the chance to reach more error conditions in ActivateSnapshot and sometimes successfully creates a valid snapshot. This also changes the asserts for the success case that were outdated (after #29370) and only didn't result in a crash because the fuzzer wasn't able to reach this code before. ACKs for top commit: maflcko: re-ACK de71d4d 🎆 fjahr: utACK de71d4d TheCharlatan: ACK de71d4d Tree-SHA512: 346974d594164544d8cd3df7d8362c905fd93116215e9f5df308dfdac55bab04d727bfd7fd001cf11318682d11ee329b4b4a43308124c04d64b67840ab8a58a0

This was fixed in commit b50554b. Also, address the typo nits from: * bitcoin#29370 (comment) * bitcoin#30598 (comment)

fa04511 doc: Remove outdated nTx faking comment (MarcoFalke) Pull request description: This problematic `nTx` "faking" was removed in commit b50554b. So fix the wrong comment. Also, address the typo nits from: * #29370 (comment) * #30598 (comment) ACKs for top commit: fjahr: ACK fa04511 Tree-SHA512: c918f0b9274be9c347a37d6221915688977a858fb8d2146035718bf17df0bd3b51d67ef12b971556851c0f69f46d26f557c35a5461abeb0683b538b9dc48f5b6

af9f987 doc: update NeedsRedownload() comment (Sjors Provoost) Pull request description: Noticed two outdated comments while reviewing #29370. Since #21009 we no longer roll back the chain, when a user updates a pre-segwit node to a modern node. In this unlikely scenario we tell the user to `-reindex`. This PR updates a comment in `PopulateAndValidateSnapshot` to reflect that change. Ditto for the description of `nStatus` in `chain.h`. ACKs for top commit: maflcko: re-ACK af9f987 fjahr: ACK af9f987 Tree-SHA512: d590f1cff6823297764c863753ed5478b8933d503c43933902d50b449dfd852a02aeb318c072ad25d02e4c2583d7026cd176a10b0584292d6bbe381a063f5c45

This was fixed in commit b50554babdddf452acaa51bac757736766c70e81. Also, address the typo nits from: * bitcoin/bitcoin#29370 (comment) * bitcoin/bitcoin#30598 (comment)

…ove wallet RPC errors 9d2d9f7 rpc: Include assumeutxo as a failure reason of rescanblockchain (Fabian Jahr) 595edee test, assumeutxo: import descriptors during background sync (Alfonso Roman Zubeldia) d73ae60 rpc: Improve importdescriptor RPC error messages (Fabian Jahr) 27f99b6 validation: Don't assume m_chain_tx_count in GuessVerificationProgress (Fabian Jahr) 42d5d53 interfaces: Add helper function for wallet on pruning (Fabian Jahr) Pull request description: A test that is added as part of #30455 uncovered this issue: The `GuessVerificationProgress` function is used during during descriptor import and relies on `m_chain_tx_count`. In #29370 an [`Assume` was added](0fd915e) expecting the `m_chaint_tx_count` to be set. However, as the test uncovered, `GuessVerificationProgress` is called with background sync blocks that have `m_chaint_tx_count = 0` when they have not been downloaded and processed yet. The simple fix is to remove the `Assume`. Users should not be thrown off by the `Internal bug detected` error. The behavior of `importdescriptor` is kept consistent with the behavior for blocks missing due to pruning. The test by alfonsoromanz is cherry-picked here to show that the [CI errors](https://cirrus-ci.com/task/5110045812195328?logs=ci#L2535) should be fixed by this change. This PR also improves error messages returned by the `importdescriptors` and `rescanblockchain` RPCs. The error message now changes depending on the situation of the node, i.e. if pruning is happening or an assumutxo backgroundsync is active. ACKs for top commit: achow101: ACK 9d2d9f7 mzumsande: Code Review ACK 9d2d9f7 furszy: Code review ACK 9d2d9f7 Tree-SHA512: b841a9b371e5eb8eb3bfebca35645ff2fdded7a3e5e06308d46a33a51ca42cc4c258028c9958fbbb6cda9bb990e07ab8d8504dd9ec6705ef78afe0435912b365

ryanofsky mentioned this pull request Feb 2, 2024

assumeutxo: nTx and nChainTx in RPC results are off #29328

Closed

DrahtBot added the CI failed label Feb 2, 2024

ryanofsky force-pushed the pr/nofake branch from 4c70d99 to a3228f0 Compare February 2, 2024 18:36

This was referenced Feb 2, 2024

doc: Assert that assumed-valid blocks are not fully valid in CheckBlockIndex() #29355

Closed

refactor: Remove nChainTx from consensus #29349

Closed

DrahtBot mentioned this pull request Feb 2, 2024

redeclare nChainTx to use uint64_t #29331

Closed

DrahtBot removed the CI failed label Feb 2, 2024

maflcko approved these changes Feb 5, 2024

View reviewed changes

maflcko reviewed Feb 5, 2024

View reviewed changes

src/validation.cpp Show resolved Hide resolved

ryanofsky commented Feb 5, 2024

View reviewed changes

ryanofsky force-pushed the pr/nofake branch from a3228f0 to 3bdbc3f Compare February 5, 2024 22:14

ryanofsky commented Feb 5, 2024

View reviewed changes

ryanofsky force-pushed the pr/nofake branch from 3bdbc3f to 603a92c Compare February 5, 2024 22:43

DrahtBot added the CI failed label Feb 5, 2024

DrahtBot mentioned this pull request Feb 6, 2024

validation: improve performance of CheckBlockIndex #28339

Merged

mzumsande reviewed Feb 6, 2024

View reviewed changes

src/validation.cpp Show resolved Hide resolved

maflcko reviewed Feb 6, 2024

View reviewed changes

test/functional/feature_assumeutxo.py Outdated Show resolved Hide resolved

ryanofsky mentioned this pull request Feb 6, 2024

Update nChainTx to 64bit type #29258

Closed

ryanofsky force-pushed the pr/nofake branch from 603a92c to 3f0b860 Compare February 7, 2024 01:03

DrahtBot removed the CI failed label Feb 7, 2024

ryanofsky force-pushed the pr/nofake branch from 3f0b860 to 2f4e7e8 Compare February 7, 2024 19:08

ryanofsky mentioned this pull request Feb 7, 2024

assumeutxo: nTx and nChainTx violations in CheckBlockIndex #29261

Closed

ryanofsky commented Mar 18, 2024

View reviewed changes

DrahtBot removed the Needs rebase label Mar 18, 2024

DrahtBot mentioned this pull request Mar 19, 2024

chainparams: Change nChainTx type to uint64_t #29656

Merged

DrahtBot requested a review from fjahr March 19, 2024 10:42

mzumsande reviewed Mar 19, 2024

View reviewed changes

maflcko reviewed Mar 20, 2024

View reviewed changes

DrahtBot requested a review from maflcko March 20, 2024 10:07

maflcko approved these changes Mar 20, 2024

View reviewed changes

achow101 self-assigned this Mar 20, 2024

achow101 merged commit b50554b into bitcoin:master Mar 20, 2024

janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Apr 6, 2024

validation: Check GuessVerificationProgress is not called with discon…

877976a

…nected block Use Assume macro as suggested bitcoin/bitcoin#29370 (comment)

mzumsande mentioned this pull request Jun 24, 2024

fuzz: improve utxo_snapshot target #30329

Merged

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Aug 10, 2024

doc: Remove outdated nTx faking comment

fa04511

This was fixed in commit b50554b. Also, address the typo nits from: * bitcoin#29370 (comment) * bitcoin#30598 (comment)

maflcko mentioned this pull request Aug 10, 2024

doc: Remove outdated nTx faking comment #30624

Merged

This was referenced Sep 16, 2024

test: assumeutxo: add missing tests in wallet_assumeutxo.py #30455

Merged

wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors #30909

Merged

bitcoin locked and limited conversation to collaborators Mar 20, 2025

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

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

Uh oh!

Conversation

ryanofsky commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

maflcko commented Feb 2, 2024

Uh oh!

DrahtBot commented Feb 2, 2024

Uh oh!

ryanofsky commented Feb 2, 2024

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Feb 5, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Feb 6, 2024

Uh oh!

ryanofsky commented Feb 7, 2024

Uh oh!

maflcko commented Feb 7, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

Sjors commented Mar 19, 2024

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 20, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 20, 2024

Choose a reason for hiding this comment

Uh oh!

achow101 commented Mar 20, 2024

ryanofsky commented Feb 2, 2024 •

edited

Loading

DrahtBot commented Feb 2, 2024 •

edited

Loading

ryanofsky left a comment •

edited

Loading