ryanofsky
commented at 2:53 pm on May 31, 2024:
contributor
This PR contains the first part of #28608, which tries to make assumeutxo code more maintainable, and improve it by not locking cs_main for 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 Chainstate objects self-contained, so for example, it is possible to determine what blocks to connect to a chainstate without querying ChainstateManager, and to determine whether a Chainstate is validated without basing it on inferences like &cs != &ActiveChainstate() or GetAll().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.
DrahtBot
commented at 2:53 pm on May 31, 2024:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)
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.
DrahtBot added the label
Refactoring
on May 31, 2024
DrahtBot added the label
CI failed
on May 31, 2024
DrahtBot
commented at 7:56 pm on May 31, 2024:
contributor
🚧 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.
The following test code never checked anything because the if statement was
always false:
if (cs != &chainman_restarted.ActiveChainstate()) {
BOOST_CHECK_EQUAL(cs->m_chain.Height(), 109);
}
Also, the height of the background chainstate it was intending to check is 110,
not 109. Fix both problems by rewriting the check.
499c4b6562
refactor: Add Chainstate::m_target_blockhash member
Make Chainstate objects aware of what block they are targeting. This makes
Chainstate objects more self contained, so it's possible to determine what
blocks should be downloaded and connected to them without needing to query
ChainstateManager or look at other Chainstate objects.
The motivation for this change is to make code more readable, so it only
requires knowing about Chainstate targets, not reasoning about the assumeutxo
snapshot validation sequence. This change also enables simplifications to the
ChainstateManager interface in subsequent commits, and could make it easier to
implement new features like creating new Chainstate objects to generate UTXO
snapshots or index UTXO data.
b8fa4bae49
refactor: Add Chainstate m_validity and m_target_blockhash members
Get rid of m_disabled/IsUsable members. Instead of marking chains disabled for
different reasons, record chainstate validity and validation progress
explicitly and use that information directly.
cdc44e878a
refactor: Deduplicate Chainstate activation code
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")
7c745ac427
refactor: Pass chainstate parameters to MaybeCompleteSnapshotValidation
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.
34b199f59e
refactor: Add Chainstate::StoragePath() method
Use to simplify code determining the chainstate leveldb paths.
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.
40c0eddf57
refactor: Convert ChainstateRole enum to struct
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 validity 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.
ca2c7e8406
refactor: Add ChainstateManager::m_chainstates member
Use to replace m_active_chainstate, m_ibd_chainstate, and m_snapshot_chainstate
members.
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-06-29 07:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
