assumeutxo state and locking cleanup #28608

ryanofsky · 2023-10-06T22:01:03Z

This is based on #29370. The non-base commits are:

This is a draft PR to follow up on comments about simplifying assumetxo state representation #28562 (comment), #27746 (comment), #24232 (comment) so validation code is less complicated, and each chainstate is handled independently without references to other assumeutxo chainstates everywhere.

Implementation is not done, but the plan is also for this PR to make two functional improvements:

Not locking cs_main while validating assumeutxo snapshots, so the node is responsive when the background chainstate download finishes.
Deleting the background chainstate right away when it is no longer needed, instead of waiting until the next restart, which takes up extra disk space and slows down the next startup.

DrahtBot · 2023-10-06T22:01:05Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28608.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	fjahr, Sjors

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:

#29553 (assumeutxo: Add dumptxoutset height param, remove shell scripts by fjahr)
#29478 (test: Test new header sync behavior in loadtxoutsetinfo by fjahr)
#29370 (assumeutxo: Get rid of faked nTx and nChainTx values by ryanofsky)
#29236 (log: Nuke error(...) by maflcko)
#29039 (versionbits refactoring by ajtowns)
#28960 (kernel: Remove dependency on CScheduler by TheCharlatan)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#28616 (Show transactions as not fully confirmed during background validation by Sjors)
#28339 (validation: improve performance of CheckBlockIndex by mzumsande)
#26022 (Add util::ResultPtr class by ryanofsky)
#25972 (build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set by fanquake)
#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.

fjahr · 2023-10-07T07:56:26Z

Concept ACK, aside from the linked discussions, this would also resolve a few more review comments in #27596.

hebasto · 2023-10-07T10:59:50Z

Drop from 26.0 milestone?

fanquake · 2023-10-07T12:32:33Z

Drop from 26.0 milestone?

Why?

hebasto · 2023-10-07T12:36:45Z

Drop from 26.0 milestone?

Why?

~~Because it is still drafted and conflicting a day before the feature freeze?~~

nm. I missed the context.

hebasto · 2023-10-07T12:38:47Z

Sorry about the noise.

Sjors

Concept ACK

Sjors · 2023-10-09T12:22:15Z

src/net_processing.cpp

                    get_inflight_budget(),
-                    vToDownload, m_chainman.GetBackgroundSyncTip(),
-                    Assert(m_chainman.GetSnapshotBaseBlock()));
+                    vToDownload, historical_blocks->first, historical_blocks->second);


If practical, it would be nice to have this change in a seperate commit from the (simpler) changes above.

maflcko · 2023-10-12T20:05:49Z

A new circular dependency in the form of "interfaces/chain.h -> kernel/chain -> interfaces/chain.h" appears to have been introduced.

DrahtBot · 2024-03-09T02:13:54Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot · 2024-06-06T03:18:46Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2024-09-03T00:47:06Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2024-12-09T01:44:19Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2025-03-08T00:30:38Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2025-06-05T01:53:23Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

yuvicc · 2025-06-15T07:11:59Z

I've just been working on other things but I want to rebase this and split it up, probably next week I think. Of course if anyone wants to work on this or some smaller part of it, I'd welcome that and be very happy to help.

Hey hi, I can work on this given that you have been stuck with other commitments. Should I work on this or open a separate pr, either way works for me?

ryanofsky · 2025-06-16T15:35:39Z

re: #28608 (comment)

Hey hi, I can work on this given that you have been stuck with other commitments. Should I work on this or open a separate pr, either way works for me?

Hi, feel free to work on anything here and open a separate PR. Note that this PR is trying to do two things:

Refactor assumeutxo implementation so it is more self-contained and assumeutxo terms and special cases are not all over validation and networking code. I wound up opening a separate PR refactor: Improve assumeutxo state representation #30214 to do this, which is still not merged but work is ongoing.
Improving assumeutxo implementation. Specifically when background chainstate finishes downloading, not keeping cs_main locked while validating the snapshot, and deleting the background chainstate right away instead of waiting until the next restart.

I'm not sure which of these you may be more interested in but progress has been very slow on the first and nonexistent on the second. I think different approaches are possible and the two things above are not tied together so a lot could be done here depending on your goals.

yuvicc · 2025-06-16T19:30:06Z

I will look into this and get back here soon!

DrahtBot · 2025-09-13T02:00:34Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

82be652 doc: Improve ChainstateManager documentation, use consistent terms (Ryan Ofsky) af455dc refactor: Simplify pruning functions (TheCharlatan) ae85c49 refactor: Delete ChainstateManager::GetAll() method (Ryan Ofsky) 6a572db refactor: Add ChainstateManager::ActivateBestChains() method (Ryan Ofsky) 491d827 refactor: Add ChainstateManager::m_chainstates member (Ryan Ofsky) e514fe6 refactor: Delete ChainstateManager::SnapshotBlockhash() method (Ryan Ofsky) ee35250 refactor: Delete ChainstateManager::IsSnapshotValidated() method (Ryan Ofsky) d9e8229 refactor: Delete ChainstateManager::IsSnapshotActive() method (Ryan Ofsky) 4dfe383 refactor: Convert ChainstateRole enum to struct (Ryan Ofsky) 352ad27 refactor: Add ChainstateManager::ValidatedChainstate() method (Ryan Ofsky) a229cb9 refactor: Add ChainstateManager::CurrentChainstate() method (Ryan Ofsky) a9b7f56 refactor: Add Chainstate::StoragePath() method (Ryan Ofsky) 840bd2e refactor: Pass chainstate parameters to MaybeCompleteSnapshotValidation (Ryan Ofsky) 1598a15 refactor: Deduplicate Chainstate activation code (Ryan Ofsky) 9fe927b refactor: Add Chainstate m_assumeutxo and m_target_utxohash members (Ryan Ofsky) 6082c84 refactor: Add Chainstate::m_target_blockhash member (Ryan Ofsky) de00e87 test: Fix broken chainstatemanager_snapshot_init check (Ryan Ofsky) Pull request description: 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. ACKs for top commit: maflcko: re-review ACK 82be652 🕍 fjahr: re-ACK 82be652 sedited: Re-ACK 82be652 Tree-SHA512: 81c67abba9fc5bb170e32b7bf8a1e4f7b5592315b4ef720be916d5f1f5a7088c0c59cfb697744dd385552f58aa31ee36176bae6a6e465723e65861089a1252e5

ryanofsky mentioned this pull request Oct 6, 2023

AssumeUTXO follow-ups #28562

Merged

DrahtBot added the CI failed label Oct 6, 2023

cacrowley approved these changes Oct 7, 2023

View reviewed changes

This was referenced Oct 7, 2023

bugfix: Make CCheckQueue RAII-styled (attempt 2) #26762

Merged

refactor: Use util::Result class for wallet loading #25722

Draft

refactor: Add util::Result failure types and ability to merge result values #25665

Open

fanquake added this to the 26.0 milestone Oct 7, 2023

DrahtBot added the Needs rebase label Oct 8, 2023

Sjors mentioned this pull request Oct 9, 2023

Show transactions as not fully confirmed during background validation #28616

Closed

Sjors reviewed Oct 9, 2023

View reviewed changes

Sjors mentioned this pull request Oct 9, 2023

assumeutxo (2) #27596

Merged

ryanofsky force-pushed the pr/noibd branch 2 times, most recently from f870170 to c6122e4 Compare October 11, 2023 20:14

achow101 removed this from the 26.0 milestone Oct 12, 2023

ryanofsky force-pushed the pr/noibd branch 2 times, most recently from 4ee775e to decf338 Compare October 12, 2023 16:06

DrahtBot removed the Needs rebase label Oct 12, 2023

ryanofsky force-pushed the pr/noibd branch from decf338 to 70484b7 Compare October 12, 2023 20:07

This was referenced Oct 12, 2023

log: Don't log cache rebalancing in absense of a snapshot chainstate #28569

Merged

Covenant tools softfork #28550

Closed

DrahtBot removed the CI failed label Oct 12, 2023

This was referenced Oct 12, 2023

Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly #28051

Merged

Fee Estimator updates from Validation Interface/CScheduler thread #28368

Merged

DrahtBot added CI failed and removed Needs rebase labels Feb 23, 2024

DrahtBot mentioned this pull request Mar 5, 2024

assumeutxo: Add dumptxoutset height param, remove shell scripts #29553

Merged

DrahtBot added the Needs rebase label Mar 9, 2024

fjahr mentioned this pull request Mar 10, 2024

AssumeUTXO Mainnet Readiness Tracking #29616

Closed

32 tasks

ryanofsky mentioned this pull request May 31, 2024

refactor: Improve assumeutxo state representation #30214

Merged

maflcko removed the CI failed label Sep 26, 2025

achow101 closed this Oct 22, 2025

assumeutxo state and locking cleanup #28608

assumeutxo state and locking cleanup #28608

Uh oh!

Conversation

ryanofsky commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

fjahr commented Oct 7, 2023

Uh oh!

hebasto commented Oct 7, 2023

Uh oh!

fanquake commented Oct 7, 2023

Uh oh!

hebasto commented Oct 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Oct 7, 2023

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 12, 2023

Uh oh!

DrahtBot commented Mar 9, 2024

Uh oh!

DrahtBot commented Jun 6, 2024

Uh oh!

DrahtBot commented Sep 3, 2024

Uh oh!

DrahtBot commented Dec 9, 2024

Uh oh!

DrahtBot commented Mar 8, 2025

Uh oh!

DrahtBot commented Jun 5, 2025

Uh oh!

yuvicc commented Jun 15, 2025

Uh oh!

ryanofsky commented Jun 16, 2025

Uh oh!

yuvicc commented Jun 16, 2025

Uh oh!

DrahtBot commented Sep 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

ryanofsky commented Oct 6, 2023 •

edited

Loading

DrahtBot commented Oct 6, 2023 •

edited

Loading

hebasto commented Oct 7, 2023 •

edited

Loading