refactor: Improve assumeutxo state representation - #30214
Merged
Conversation
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30214. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Contributor
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
This was referenced Jun 1, 2024
This was referenced Jun 6, 2024
Contributor
|
Concept ACK |
32 tasks
Get rid of m_disabled/IsUsable members. Instead of marking chains disabled for different reasons, store chainstate assumeutxo status explicitly and use that information to determine how chains should be treated.
Move duplicate code from ChainstateManager::ActivateSnapshot and
ChainstateManager::ActivateExistingSnapshot methods to a new
ChainstateManager::AddChainstate method.
The "AddChainstate" method name doesn't mention snapshots even though it is
only used to add snapshot chainstates now, because it becomes more generalized
in a later commit in this PR ("refactor: Add ChainstateManager::m_chainstates
member")
Remove hardcoded references to m_ibd_chainstate and m_snapshot_chainstate so MaybeCompleteSnapshotValidation function can be simpler and focus on validating the snapshot without dealing with internal ChainstateManager states. This is a step towards being able to validate the snapshot outside of ActivateBestChain loop so cs_main is not locked for minutes when the snapshot block is connected.
Use to simplify code determining the chainstate leveldb paths. New method is the now the only code that needs to figure out the storage path, so the path doesn't need to be constructed multiple places and backed out of leveldb.
CurrentChainstate() is basically the same as ActiveChainstate() except it requires cs_main to be locked when it is called, instead of locking cs_main internally. The name "current" should also be less confusing than "active" because multiple chainstates can be active, and CurrentChainstate() returns the chainstate targeting the current network tip, regardless of what chainstates are being downloaded or how they are used.
ValidatedChainstate() accessor replaces GetChainstateForIndexing() with no change in behavior.
Change ChainstateRole parameter passed to wallets and indexes. Wallets and indexes need to know whether chainstate is historical and whether it is fully validated. They should not be aware of the assumeutxo snapshot validation process.
IsSnapshotActive() method is only called one place outside of tests and asserts, and is confusing because it returns true even after the snapshot is fully validated. The documentation which said this "implies that a background validation chainstate is also in use" is also incorrect, because after the snapshot is validated, the background chainstate gets disabled and IsUsable() would return false.
IsSnapshotValidated() is only called one place outside of tests, and is use redundantly in some tests, asserting that a snapshot is not validated when a snapshot chainstate does not even exist. Simplify by dropping the method and checking Chainstate m_assumeutxo field directly.
SnapshotBlockhash() is only called two places outside of tests, and is used redundantly in some tests, checking the same field as other checks. Simplify by dropping the method and using the m_from_snapshot_blockhash field directly.
Use to replace m_active_chainstate, m_ibd_chainstate, and m_snapshot_chainstate
members. This has several benefits:
- Ensures ChainstateManager treats chainstates instances equally, making
distinctions based on their attributes, not having special cases and making
assumptions based on their identities.
- Normalizes ChainstateManager representation so states that should be
impossible to reach and validation code has no handling for (like
m_snapshot_chainstate being set and m_ibd_chainstate being unset, or both
being set but m_active_chainstate pointing to the m_ibd_chainstate) can no
longer be represented.
- Makes ChainstateManager more extensible so new chainstates can be added for
different purposes, like indexing or generating and validating assumeutxo
snapshots without interrupting regular node operations. With the
m_chainstates member, new chainstates can be added and handled without needing
to make changes all over validation code or to copy/paste/modify the existing
code that's been already been written to handle m_ibd_chainstate and
m_snapshot_chainstate.
- Avoids terms that are confusing and misleading:
- The term "active chainstate" term is confusing because multiple chainstates
will be active and in use at the same time. Before a snapshot is validated,
wallet code will use the snapshot chainstate, while indexes will use the IBD
chainstate, and netorking code will use both chainstates, downloading
snapshot blocks at higher priority, but also IBD blocks simultaneously.
- The term "snapshot chainstate" is ambiguous because it could refer either
to the chainstate originally loaded from a snapshot, or to the chainstate
being used to validate a snapshot that was loaded, or to a chainstate being
used to produce a snapshot, but it is arbitrary used to refer the first
thing. The terms "most-work chainstate" or "assumed-valid chainstate" should
be less ambiguous ways to refer to chainstates loaded from snapshots.
- The term "IBD chainstate" is not just ambiguous but actively confusing
because technically IBD ends and the node is considered synced when the
snapshot chainstate finishes syncing, so in practice the IBD chainstate
will mostly by synced after IBD is complete. The term "fully-validated" is
a better way of describing the characteristics and purpose of this
chainstate.
Deduplicate code looping over chainstate objects and calling ActivateBestChain() and avoid need for code outside ChainstateManager to use the GetAll() method.
Just use m_chainstates array instead.
Move GetPruneRange from ChainstateManager to Chainstate.
Member
|
Only change is rebase and fix of two LLM nits. re-review ACK 82be652 🕍 Show signatureSignature: |
ryanofsky
commented
Dec 12, 2025
ryanofsky
left a comment
ryanofsky
left a comment
Contributor
Author
There was a problem hiding this comment.
Thanks for the reviews!
Rebased 5dd7cbf -> 82be652 (pr/cstate.23 -> pr/cstate.24, compare) fixing conflict #32414 and also adding suggested arg name comments
Comment on lines
+395
to
+396
| //! returns true even after the snapshot is validated, until the next node | ||
| //! restart. |
Contributor
Author
There was a problem hiding this comment.
re: #30214 (comment)
q in cb3e7af: I wonder if this should be fixed in a follow-up, because the comment in
AttachChainsays that the loop called whenhasAssumedValidChainis true may be slow.
Yes it could be nice to change this and make loading old wallets on non-pruned nodes faster, even though it's a little bit of an edge case
| Assert(chain.LoadGenesisBlock()); | ||
| // These cache values will be corrected shortly in `MaybeRebalanceCaches`. | ||
| chain.InitCoinsDB(1 << 20, true, false, ""); | ||
| chain.InitCoinsDB(1 << 20, true, false); |
Contributor
Author
There was a problem hiding this comment.
| snapshot_chainstate->InitCoinsDB( | ||
| static_cast<size_t>(current_coinsdb_cache_size * SNAPSHOT_CACHE_PERC), | ||
| in_memory, false, "chainstate"); | ||
| in_memory, false); |
Contributor
Author
There was a problem hiding this comment.
| validated_cs.m_assumeutxo != Assumeutxo::VALIDATED || | ||
| !validated_cs.m_chain.Tip() || | ||
| // Or the validated chainstate is not targeting the snapshot block... | ||
| !validated_cs.m_target_blockhash || |
Contributor
Author
There was a problem hiding this comment.
re: #30214 (comment)
i'd say assumes are fine to detect logic bugs or storage corruption, but no strong opinion, if the goal is to somehow make this more flexible for future changes
Yeah I think it needs to be up to callers to check their own expected states or to check the return status of this function. Having this function make unnecessary assumptions about particular states it thinks it will be called in seems like it could be easily break if any outside code changes
Contributor
|
re-ACK 82be652 Confirmed that changes since last review were addressing nits and rebasing. |
This was referenced Dec 14, 2025
sedited
approved these changes
Dec 16, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR contains the first part of #28608, which tries to make assumeutxo code more maintainable, and improve it by not locking
cs_mainfor a long time when the snapshot block is connected, and by deleting the snapshot validation chainstate when it is no longer used, instead of waiting until the next restart.The changes in this PR are just refactoring. They make
Chainstateobjects self-contained, so for example, it is possible to determine what blocks to connect to a chainstate without queryingChainstateManager, and to determine whether a Chainstate is validated without basing it on inferences like&cs != &ActiveChainstate()orGetAll().size() == 1.The PR also tries to make assumeutxo terminology less confusing, using "current chainstate" to refer to the chainstate targeting the current network tip, and "historical chainstate" to refer to the chainstate downloading old blocks and validating the assumeutxo snapshot. It removes uses of the terms "active chainstate," "usable chainstate," "disabled chainstate," "ibd chainstate," and "snapshot chainstate" which are confusing for various reasons.