assumeutxo state and locking cleanup

ryanofsky commented at 10:01 pm on October 6, 2023: contributor

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 (review), #27746 (review), #24232 (review) 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 commented at 10:01 pm on October 6, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

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.

DrahtBot added the label CI failed on Oct 6, 2023

cacrowley approved

fjahr commented at 7:56 am on October 7, 2023: contributor

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

fanquake added this to the milestone 26.0 on Oct 7, 2023

hebasto commented at 10:59 am on October 7, 2023: member

Drop from 26.0 milestone?

fanquake commented at 12:32 pm on October 7, 2023: member

Drop from 26.0 milestone?

Why?

hebasto commented at 12:36 pm on October 7, 2023: member

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 commented at 12:38 pm on October 7, 2023: member

Sorry about the noise.

DrahtBot added the label Needs rebase on Oct 8, 2023

in src/net_processing.cpp:5972 in 12b3a6b205 outdated

5943                 TryDownloadingHistoricalBlocks(
5944                     *peer,
5945                     get_inflight_budget(),
5946-                    vToDownload, m_chainman.GetBackgroundSyncTip(),
5947-                    Assert(m_chainman.GetSnapshotBaseBlock()));
5948+                    vToDownload, historical_blocks->first, historical_blocks->second);

Sjors commented at 12:22 pm on October 9, 2023:

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

Sjors commented at 12:29 pm on October 9, 2023: member

Concept ACK

ryanofsky force-pushed on Oct 10, 2023

ryanofsky force-pushed on Oct 11, 2023

achow101 removed this from the milestone 26.0 on Oct 12, 2023

ryanofsky force-pushed on Oct 12, 2023

DrahtBot removed the label Needs rebase on Oct 12, 2023

maflcko commented at 8:05 pm on October 12, 2023: member

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

ryanofsky force-pushed on Oct 12, 2023

DrahtBot removed the label CI failed on Oct 12, 2023

ryanofsky commented at 4:00 pm on October 18, 2023: contributor

Update on this PR: The state cleanup part of this PR is implemented. I think it is a nice change which should make code easier to understand by making chainstate objects self contained and removing assumeutxo specific special cases in validation code. It also net removes 140 lines of code.

The locking changes (not yet pushed) are a mess, though. As mentioned in the PR description, I’m trying to delete the background chainstate after snapshot validation without waiting for bitcoind to be restarted. This requires a mutex to protect chainstates from being deleted while they are in use, so I added a simple m_chainstates_mutex to guard the m_chainstates vector, but this has snowballed into a much bigger change because of lock ordering conflicts. Some parts of the code like Chainstate::ActivateBestChain need to lock m_chainstates_mutex before cs_main so cs_main can be released to let notifications be processed. But most code wants to lock cs_main first, and only needs to access ChainstateManager object briefly to get a reference to the active chain, so it more naturally locks cs_main before m_chainstates_mutex. I was able to make cleanups in validation.cpp, moving things between Chainstate and ChainstateManager to acquire m_chainstates_mutex before cs_main, or relying on Chainstate members and avoiding the need to lock m_chainstates_mutex at all, and I think these changes are good, but after doing this (3b5c646790c55c01c318bce7d029a8d9e67147bc) it turns out the lock order inconsistency is much worse in net_processing than validation, and it would not be good for performance or code complexity to try lock m_chainstates_mutex before cs_main there.

I’m thinking of taking a different approach of using a shared mutex instead of an exclusive mutex to protect access to Chainstate instances and prevent them from being deleted while in use. Then code could freely obtain shared locks to m_chainstates_mutex and exclusive locks to cs_main in either order. In order to prevent deadlocks, the small amount of code that creates and deletes chainstates and needs exclusive access to both locks would just need to be careful to acquire both locks atomically and never hold one exclusive lock while it is waiting for the other. This idea is conceptually pretty simple, and it could introduce support for shared locking that could be useful in other contexts. But I’m not exactly sure what approach to take with the implementation, and I’m worried it may run into problems with compiler thread safety analysis. I’m also wondering if it might make sense to pause working on locking and instead work on splitting up the current PR into commits to make it more reviewable.

DrahtBot added the label Needs rebase on Oct 20, 2023

fjahr commented at 9:15 am on October 24, 2023: contributor

@ryanofsky I think it would be great if you could split out the cleanup parts into a self-contained PR and take them out of draft status to attract more serious review. To me, it’s clear they would be a nice improvement on their own and they are addressing several comments in #27596. While that is in review you will probably have time to go back to figuring out the locking approach.

assumeutxo test: Add RPC test for fake nTx and nChainTx values

The fake values will be removed in the next commit, so it is useful to have
test coverage confirming the change in behavior.

Also add ubsan suppressions for integer overflows in the getchaintxstats RPC.
The overflows existed previously but were not covered by previous tests. The
getchaintxstats RPC could be changed to prevent overflows in the future, but
for now the suppressions are needed to avoid CI errors in the PR:
https://cirrus-ci.com/task/5549867297144832?logs=ci#L2930

32481c30a5

validation: Check GuessVerificationProgress is not called with disconnected block

Use Assume macro as suggested https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1479427801

cecf618b68

assumeutxo: Get rid of faked nTx and nChainTx values

The `PopulateAndValidateSnapshot` function introduced in
f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake
`nTx` and `nChainTx` values that can show up in RPC results (see #29328) and
make `CBlockIndex` state hard to reason about, because it is difficult to know
whether the values are real or fake.

Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the
values are unknown, instead of faking them.

This commit fixes an assert failure in the (pindex->nChainTx == pindex->nTx +
prev_chain_tx) check that would previously happen if a snapshot was loaded, and
a block was submitted which forked from the chain before the snapshot block and
after the last downloaded background chain block. This block would not be
marked assumed-valid because it would not be an ancestor of the snapshot, and
it would have nTx set, nChainTx unset, and prev->nChainTx set with a fake
value, so the assert would fail. After this commit, prev->nChainTx is unset
instead of being set to a fake value, so the assert succeeds. A test which
submits a block like this and previously crashed the node has been added in
feature_assumeutxo.py. It was written and posted by maflcko in
https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1918947945

Compatibility note: This change could result in -checkblockindex failures if a
snapshot was loaded by a previous version of Bitcoin Core and not fully
validated, because fake nTx values will have been saved to the block index. It
would be pretty easy to avoid these failures by adding some compatibility code
to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake
(when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a
little simpler not to worry about being compatible in this case.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

594336ae8a

assumeutxo: Remove BLOCK_ASSUMED_VALID flag

Flag adds complexity and is not currently used for anything.

8afcd99435

Merge remote-tracking branch 'origin/pull/29370/head' 302d69c422

refactor: Replace ChainstateManager IBD and snapshot chainstates with flat list of chainstates

Also add m_validity and m_target_block members to Chainstate class it is
possible to determine chainstate properties directly from Chainstate objects
and it is not neccessary for validation code make calls to ChainstateManager
and decide what to do by looking at assumeutxo download state.

Goal is to remove hardcoded logic handling assumeutxo snapshots from most
validation code. Also to make it easy to fix locking issues like the fact that
cs_main is currently held unnecessarily validating snapshots, and the fact that
background chainstate is not immediately deleted when it is no longer used,
wasting disk space and adding a long startup delay next time the node is
restarted.

This follows up on some previous discussions:

https://github.com/bitcoin/bitcoin/pull/24232#discussion_r835355848
https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1321872262
https://github.com/bitcoin/bitcoin/pull/28562#discussion_r1344824078

35d0519c47

broken 0bb0db780f

maflcko commented at 5:55 pm on February 22, 2024: member

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 commented at 7:54 pm on February 22, 2024: contributor

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.

ryanofsky force-pushed on Feb 23, 2024

DrahtBot commented at 5:04 pm on February 23, 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.

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

DrahtBot added the label CI failed on Feb 23, 2024

DrahtBot removed the label Needs rebase on Feb 23, 2024

DrahtBot commented at 2:13 am on March 9, 2024: contributor

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

DrahtBot added the label Needs rebase on Mar 9, 2024

DrahtBot commented at 3:18 am on June 6, 2024: contributor

⌛ 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 commented at 0:47 am on September 3, 2024: contributor

⌛ 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 commented at 1:44 am on December 9, 2024: contributor

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

assumeutxo state and locking cleanup #28608

Code Coverage

Reviews

Conflicts