New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enforce block index invariants in chainstate #1708

Merged

ImplOfAnImpl merged 27 commits into master from enforce_block_index_invariants

Apr 15, 2024

Contributor

ImplOfAnImpl commented Apr 3, 2024

is_persistent flag was added to BlockIndex, which means that this BlockIndex will never be deleted (until we implement purging, that is). In practice, a BlockIndex is persistent iff the corresponding Block is in the chainstate db.
In ChainstateInterface, get_block_index/get_gen_block_index were replaced with get_persistent_block_index/get_any_block_index and get_persistent_gen_block_index/get_any_gen_block_index; the get_persistent variants only return a BlockIndex is it's marked as "persistent".
Some block index related consistency checks were added to TestFramework.
Chainstate consistency checks were added, which are performed once at startup and then after a block is processed or invalidated; they are enabled by default for regtest only (but can be enabled via a command-line argument for mainnet and testnet too).

ImplOfAnImpl requested review from erubboli, iljakuklic, TheQuantumPhysicist and azarovh as code owners

April 3, 2024 19:02

ImplOfAnImpl commented

View reviewed changes

chainstate/src/detail/chainstateref/consistency_checker.rs Show resolved Hide resolved

TheQuantumPhysicist reviewed

View reviewed changes

chainstate/storage/src/lib.rs Show resolved Hide resolved

TheQuantumPhysicist reviewed

View reviewed changes

chainstate/src/detail/chainstateref/consistency_checker.rs Outdated Show resolved Hide resolved

TheQuantumPhysicist reviewed

View reviewed changes

chainstate/src/detail/chainstateref/consistency_checker.rs Show resolved Hide resolved

TheQuantumPhysicist reviewed

View reviewed changes

chainstate/src/detail/query.rs Outdated Show resolved Hide resolved

TheQuantumPhysicist reviewed

View reviewed changes

chainstate/src/interface/chainstate_interface.rs Outdated Show resolved Hide resolved

TheQuantumPhysicist reviewed

View reviewed changes

chainstate/types/src/block_index.rs Outdated Show resolved Hide resolved

TheQuantumPhysicist reviewed

View reviewed changes

chainstate/types/src/gen_block_index.rs Outdated Show resolved Hide resolved

TheQuantumPhysicist reviewed

View reviewed changes

Collaborator

TheQuantumPhysicist left a comment

First round done. I'll review again tomorrow.

ImplOfAnImpl mentioned this pull request

Add more consistency checks for the chainstate db #1710

Open

TheQuantumPhysicist reviewed

View reviewed changes

chainstate/types/src/gen_block_index.rs Outdated Show resolved Hide resolved

ImplOfAnImpl added 15 commits

April 4, 2024 20:21


          Add block_exists function to chainstateref

39bd9af


          Add is_persistent flag to BlockIndex

5333bbd


          Rename get_block_index to get_any_block_index in the query interface;…

15b171d

… replace get_block_index in ChainstateInterface with get_any_block_index/get_persistent_block_index and use the latter in production whenever possible


          Some cleanup

bc0248e


          More block index related checks in TestFramework plus some cleanup


          More block index related checks in TestFramework

e61f4c4


          Add a stub for check_block_index_consistency and basic tests

68ce059


          Move "heavy checks"-related tests to framework_tests.rs

0e68279


          Implement basic checks in check_block_index_consistency

ea306c4


          Remove/apply FIXMEs

2309ee6


          More checks in check_block_index_consistency

9a2e217


          More checks in check_block_index_consistency

b51eaef


          Move consistency checks to a separate file

e810416


          One more consistency check; appease clippy

4e850bf


          Minor cleanup

4777fba

ImplOfAnImpl added 6 commits

April 4, 2024 20:21


          Update h2 to appease "cargo deny"

192fe7e


          Apply review comments

812ab75


          Replace Eq/PartialEq implementations for BlockIndex/GenBlockIndex wit…

ef26fb4

…h a custom function


          Small improvement in check_block_index_consistency

ab2e048


          Change of terminology: "persisting block index" -> "persisted block"

0f0382b


          Forbid implementing Eq and PartialEq for BlockIndex and GenBlockIndex…

af0aead

… via assert_not_impl_any.

azarovh approved these changes

View reviewed changes

Member

azarovh left a comment

Looks good. Though I'm not a fan of new functions in BlockchainStorageRead and it feels like they belong to chainstate/storage/src/internal/expensive.rs. But I don't see the way to actually do that so let it be.


          Minor cleanup

1484d16

ImplOfAnImpl force-pushed the enforce_block_index_invariants branch from aa59061 to 1484d16 Compare

April 4, 2024 18:13

iljakuklic reviewed

View reviewed changes

chainstate/test-suite/src/tests/syncing_tests.rs Outdated Show resolved Hide resolved

chainstate/src/detail/chainstateref/mod.rs Outdated Show resolved Hide resolved

chainstate/src/detail/chainstateref/consistency_checker.rs Outdated Show resolved Hide resolved

chainstate/src/detail/chainstateref/consistency_checker.rs Show resolved Hide resolved

ImplOfAnImpl added 3 commits

April 5, 2024 12:13


          Add 'with_heavy_checks_enabled' function to ChainstateConfig; remove …

61fea41

…redundant "with_chainstate_config" calls for TestFramework.


          Implement debug_assert_or_log; use it in del_block_index_of_non_persi…

da7a115

…sted_block


          Add more (release-only) checks to chainstate consistency checker

c318011

TheQuantumPhysicist reviewed

View reviewed changes

utils/src/debug_panic.rs Outdated Show resolved Hide resolved

TheQuantumPhysicist reviewed

View reviewed changes

chainstate/src/detail/chainstateref/consistency_checker.rs Outdated Show resolved Hide resolved

ImplOfAnImpl added 2 commits

April 6, 2024 00:51


          Unify the "CRITICAL" log error messages

e5c012d


          Don't disable a slow check in debug builds; disable all checks for a …

53f5d0e

…lengthy test instead.

iljakuklic approved these changes

View reviewed changes

TheQuantumPhysicist approved these changes

View reviewed changes

ImplOfAnImpl merged commit cf4d058 into master

23 checks passed

ImplOfAnImpl deleted the enforce_block_index_invariants branch

April 15, 2024 14:40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment