Skip to content

assumeutxo state and locking cleanup #28608

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
Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

assumeutxo state and locking cleanup #28608

wants to merge 7 commits into from

Conversation

ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Oct 6, 2023 •
edited
Loading

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:

  1. Not locking cs_main while validating assumeutxo snapshots, so the node is responsive when the background chainstate download finishes.
  2. 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
Copy link
Contributor

DrahtBot commented Oct 6, 2023 •
edited
Loading

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.

@ryanofsky ryanofsky mentioned this pull request Oct 6, 2023
Merged
This was referenced Oct 7, 2023
Merged
Draft
Open
@fjahr
Copy link
Contributor

fjahr commented Oct 7, 2023

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

@fanquake fanquake added this to the 26.0 milestone Oct 7, 2023
@hebasto
Copy link
Member

hebasto commented Oct 7, 2023

Drop from 26.0 milestone?

@fanquake
Copy link
Member

fanquake commented Oct 7, 2023

Drop from 26.0 milestone?

Why?

@hebasto
Copy link
Member

hebasto commented Oct 7, 2023 •
edited
Loading

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
Copy link
Member

hebasto commented Oct 7, 2023

Sorry about the noise.

@Sjors Sjors mentioned this pull request Oct 9, 2023
Closed
Sjors
Sjors reviewed Oct 9, 2023
View reviewed changes
Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

src/net_processing.cpp
TryDownloadingHistoricalBlocks(
*peer,
get_inflight_budget(),
vToDownload, m_chainman.GetBackgroundSyncTip(),
Assert(m_chainman.GetSnapshotBaseBlock()));
vToDownload, historical_blocks->first, historical_blocks->second);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Sjors Sjors mentioned this pull request Oct 9, 2023
Merged
@ryanofsky ryanofsky force-pushed the pr/noibd branch 2 times, most recently from f870170 to c6122e4 Compare October 11, 2023 20:14
@achow101 achow101 removed this from the 26.0 milestone Oct 12, 2023
@ryanofsky ryanofsky force-pushed the pr/noibd branch 2 times, most recently from 4ee775e to decf338 Compare October 12, 2023 16:06
@maflcko
Copy link
Member

maflcko commented Oct 12, 2023 

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

This was referenced Oct 12, 2023
Merged
Closed
This was referenced Oct 12, 2023
Merged
Merged
@maflcko
Copy link
Member

maflcko commented Feb 22, 2024

Not sure what the status here is? It would be good to rebase, so that reviewers can take a look, or close, so that it can be grabbed up, if there is need.

@ryanofsky
Copy link
Contributor Author

Not sure what the status here is? It would be good to rebase, so that reviewers can take a look, or close, so that it can be grabbed up, if there is need.

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.

@DrahtBot
Copy link
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.

Debug: https://github.com/bitcoin/bitcoin/runs/21916654389

This was referenced Feb 24, 2024
Closed
Open
Merged
Merged
Merged
Merged
Merged
Draft
Merged
Merged
Merged
@DrahtBot DrahtBot mentioned this pull request Mar 5, 2024
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 9, 2024

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

@fjahr fjahr mentioned this pull request Mar 10, 2024
Closed
32 tasks
@ryanofsky ryanofsky mentioned this pull request May 31, 2024
Open
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 6, 2024

⌛ 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.

4 similar comments
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 3, 2024

⌛ 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
Copy link
Contributor

DrahtBot commented Dec 9, 2024

⌛ 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
Copy link
Contributor

DrahtBot commented Mar 8, 2025

⌛ 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
Copy link
Contributor

DrahtBot commented Jun 5, 2025

⌛ 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
Copy link
Contributor

yuvicc commented Jun 15, 2025

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
Copy link
Contributor Author

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:

  1. 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.

  2. 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
Copy link
Contributor

yuvicc commented Jun 16, 2025

I will look into this and get back here soon!

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

@Sjors Sjors Sjors left review comments

@cacrowley cacrowley cacrowley approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
CI failed Needs rebase
Projects
Assume UTXO
Status: In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@ryanofsky @DrahtBot @fjahr @hebasto @fanquake @maflcko @yuvicc @Sjors @cacrowley @achow101