kernel: Flush in ChainstateManager destructor by sedited · Pull Request #31382 · bitcoin/bitcoin

sedited · 2024-11-27T13:18:50Z

This simplifies the shutdown code a bit and reduces the amount of extra logic required in kernel/bitcoinkernel.cpp to tear down the chainman.

Moving the second flush out of the Shutdown function into the ChainstateManager destructor, should be safe since there should be no new events created after the first flush. Tweak the chainman tests to actually exercise this new tear down behaviour.

This PR is part of the libbitcoinkernel project

DrahtBot · 2024-11-27T13:18:53Z

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/31382.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	l0rinc, stickies-v, mzumsande

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33421 (node: add BlockTemplateCache by ismaelsadeeq)
#32427 ((RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store by sedited)

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 · 2024-11-27T14:04:17Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/33601554977}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

l0rinc

Concept ACK

Closed #31362 in favor of this

sedited · 2024-11-27T15:01:23Z

Seems like I am running into this here: #25073 (comment), maybe time to introduce a replace mempool method like Carl suggested there?

sedited · 2024-11-28T08:51:39Z

maybe time to introduce a replace mempool method like Carl suggested there?

Did this now, ready for review.

stickies-v

Concept ACK

Simplifies the shutdown logic, and makes ChainstateManager more self contained. I'll need to think a bit more about how to test or verify the safety of changes such as in 7a37f20.

stickies-v · 2025-01-02T09:28:16Z

+    m_node.mempool = std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node), error);
+    auto& dummy_chainstate{static_cast<DummyChainState&>(m_node.chainman->ActiveChainstate())};
+    dummy_chainstate.SetMempool(m_node.mempool.get());


I wonder if it would make sense to (probably in a separate PR, I suspect it'll be quite a big change) remove the Chainstate::m_mempool member. It feels like a footgunny shortcut (but need to investigate more), conceptually doesn't make a lot of sense to me, and not having it would avoid having to do e.g. theDummyChainState gymnastics. Is this something you've explored already?

I could not come up with a satisfactory alternative yet, suggestions welcome!

stickies-v · 2025-01-02T11:25:56Z

            m_node.chainman.reset();
+            // Process all callbacks referring to the old manager before creating a
+            // new one.
+            m_node.validation_signals->SyncWithValidationInterfaceQueue();


Should we do this before calling m_node.chainman.reset();? I'm not sure if any of the callbacks actually depend on the chainman (it doesn't look like it, but I'll need to investigate more), but I think it's a bit more logical at least?

The chainman reset may trigger validation interface notifications, so I think it makes sense to sync with the queue afterwards? Unlike the shutdown procedure, we keep the validation interface around beyond the restarts there.

The chainman reset may trigger validation interface notifications

Ah, right, ChainStateFlushed would be called. Yeah, I think with that knowledge doing it afterwards makes more sense. Also, callbacks are processed regardless of SyncWithValidationInterfaceQueue() being called, so I suppose this doesn't really change anything wrt callbacks assuming m_node.chainman exists.

stickies-v · 2025-01-02T11:26:44Z

-            for (Chainstate* cs : chainman.GetAll()) {
-                LOCK(::cs_main);
-                cs->ForceFlushStateToDisk();
-            }
-            // Process all callbacks referring to the old manager before wiping it.
-            m_node.validation_signals->SyncWithValidationInterfaceQueue();
-            LOCK(::cs_main);
-            chainman.ResetChainstates();


nit: the docstring is now a bit out of date:

// Simulate a restart of the node by flushing all state to disk, clearing the // existing ChainstateManager, and unloading the block index.

This seems fine as is? I'd prefer not to document these deeper behaviours, that the caller can expect to "just work".

Sorry, I meant to to remove the flushing reference since that's now automatically handled by clearing chainman, so I agree with your comment.

Simulate a restart of the node by clearing the existing ChainstateManager, and unloading the block index.

I'll do that next time I have to push.

sedited · 2025-01-03T20:54:34Z

Thank you for the review @stickies-v,

Updated 6c3bc47 -> 8e86e77 (chainman_flush_destructor_0 -> chainman_flush_destructor_1, compare)

Addressed @stickies-v's comment, moved Assert checking that there is no mempool construction error to right after constructing the mempool.
Addressed @stickies-v's comment, consolidated teardown notification comment into a single block.

mzumsande

Concept ACK

mzumsande · 2025-01-31T21:57:49Z

    LOCK(::cs_main);
+    for (Chainstate* chainstate : GetAll()) {
+        if (chainstate->CanFlushToDisk()) {
+            chainstate->ForceFlushStateToDisk();


Not too familiar with the shutdown sequence - I don't fully understand why we need to flush the chainstates twice.
Do you know any examples of events that result in the second flush on destruction doing any actual work in bitcoind?

AFAIU the second flush is the original one, while the first flush was introduced in 3192975 to help cleanly process all remaining callbacks. Maybe it could have removed the second flush in that commit?

That said I think it is good practice to be defensive here and keep both.

DrahtBot · 2025-02-27T23:35:03Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/35129562635}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

sedited · 2025-11-25T17:05:47Z

Rebased d9cc26d -> ba29aaf (chainman_flush_destructor_8 -> chainman_flush_destructor_9, compare)

maflcko · 2025-11-25T17:28:27Z

allows for getting rid of the goto in bitcoin-chainstate

I don't think this is accurate?

sedited · 2025-11-25T20:36:00Z

Re #31382 (comment)

I don't think this is accurate?

Thanks, updated.

Next to being more consistent, since tearing down the mempool first leaves a dangling pointer in the chainman, this also enables the changes in the next commits, since flushing calls GetCoinsCacheSizeState, which would access this dangling pointer otherwise.

This is done in preparation for the next commit to also change the mempool pointer within the chainstate once the old mempool is reset. Without this the old, dangling pointer will be reused when destructing the chainstate manager.

Moving the second flush out of the Shutdown function into the ChainstateManager destructor, should be safe since there should be no new events created after the first flush. This simplifies the shutdown code a bit and allows for getting rid of the `goto` in bitcoin-chainstate.

Also remove the reference to the destructed chainman, which could cause UB once the chainman is reset.

Now that its only callsite has been moved to the destructor, let the smart pointer do its thing.

There is no need to flush the background callbacks, since the immediate task runner is used for the validation signals, which does not have any pending callbacks. This allows for removal of the code in the destructor.

sedited · 2025-12-16T16:18:33Z

Rebased ba29aaf -> 8ad5e19 (chainman_flush_destructor_9 -> chainman_flush_destructor_10, compare)

Fixed conflict with refactor: Improve assumeutxo state representation #30214

sedited · 2026-02-20T10:02:37Z

Closing this, since it has not received review in a year, and it is not a pressing change.

DrahtBot added the Validation label Nov 27, 2024

sedited marked this pull request as draft November 27, 2024 13:34

DrahtBot added the CI failed label Nov 27, 2024

l0rinc mentioned this pull request Nov 27, 2024

kernel, refactor: Replace goto with RAII in bitcoin-chainstate #31362

Closed

l0rinc reviewed Nov 27, 2024

View reviewed changes

Comment thread src/bitcoin-chainstate.cpp Outdated

sedited force-pushed the chainman_flush_destructor branch 2 times, most recently from 6d3ec7a to 90f5a20 Compare November 27, 2024 14:21

sedited force-pushed the chainman_flush_destructor branch 3 times, most recently from 9425bf5 to 6c3bc47 Compare November 27, 2024 21:10

DrahtBot removed the CI failed label Nov 27, 2024

DrahtBot mentioned this pull request Nov 28, 2024

validation: do not wipe utxo cache for stats/scans/snapshots #30610

Closed

sedited marked this pull request as ready for review November 28, 2024 08:51

This was referenced Dec 7, 2024

kernel, refactor: return error status on all fatal errors #29700

Draft

kernel, logging: Pass Logger instances to kernel objects #30342

Draft

stickies-v reviewed Jan 2, 2025

View reviewed changes

sedited force-pushed the chainman_flush_destructor branch from 6c3bc47 to 8e86e77 Compare January 3, 2025 20:54

This was referenced Jan 16, 2025

Fees: add Fee rate Forecaster Manager #31664

Closed

Fee Estimation via Fee rate Forecasters #30157

Closed

DrahtBot mentioned this pull request Jan 27, 2025

Cluster mempool implementation #28676

Closed

8 tasks

mzumsande reviewed Jan 31, 2025

View reviewed changes

DrahtBot mentioned this pull request Feb 2, 2025

kernel: Introduce C header API #30595

Merged

DrahtBot added the CI failed label Feb 27, 2025

sedited force-pushed the chainman_flush_destructor branch from 8e86e77 to fe7d09b Compare March 4, 2025 20:42

sedited force-pushed the chainman_flush_destructor branch 3 times, most recently from acc96a4 to d9cc26d Compare November 7, 2025 09:19

DrahtBot removed the CI failed label Nov 7, 2025

DrahtBot mentioned this pull request Nov 18, 2025

policy: allow <minrelay txns in package context if paid for by cpfp #33892

Merged

DrahtBot added the Needs rebase label Nov 25, 2025

DrahtBot mentioned this pull request Nov 25, 2025

Replace cluster linearization algorithm with SFL #32545

Merged

sedited force-pushed the chainman_flush_destructor branch from d9cc26d to ba29aaf Compare November 25, 2025 17:05

DrahtBot removed the Needs rebase label Nov 25, 2025

sedited moved this from API Development to Architecture & Performance in The libbitcoinkernel Project Dec 14, 2025

DrahtBot mentioned this pull request Dec 16, 2025

fees: Introduce Mempool Based Fee Estimation to reduce overestimation #34075

Open

DrahtBot added the Needs rebase label Dec 16, 2025

sedited added 4 commits December 16, 2025 16:44

tests: Add replace mempool method

1ccae91

This is done in preparation for the next commit to also change the mempool pointer within the chainstate once the old mempool is reset. Without this the old, dangling pointer will be reused when destructing the chainstate manager.

tests: Use chainman dtor logic to simulate restart

251645e

Also remove the reference to the destructed chainman, which could cause UB once the chainman is reset.

kevkevinpal mentioned this pull request Dec 16, 2025

refactor: Improve assumeutxo state representation kevkevinpal/bitcoin#231

Closed

sedited added 2 commits December 16, 2025 16:59

validation: Remove ResetCoinsViews

34e9e2b

Now that its only callsite has been moved to the destructor, let the smart pointer do its thing.

kernel: Remove chainman destructor in bitcoinkernel.cpp

8ad5e19

There is no need to flush the background callbacks, since the immediate task runner is used for the validation signals, which does not have any pending callbacks. This allows for removal of the code in the destructor.

sedited force-pushed the chainman_flush_destructor branch from ba29aaf to 8ad5e19 Compare December 16, 2025 16:18

DrahtBot removed the Needs rebase label Dec 16, 2025

DrahtBot mentioned this pull request Dec 21, 2025

coins: drop error catcher, centralize fatal read handling #34132

Open

sedited added the Up for grabs label Feb 20, 2026

sedited closed this Feb 20, 2026

github-project-automation Bot moved this from Architecture & Performance to Done or Closed or Rethinking in The libbitcoinkernel Project Feb 20, 2026

Uh oh!

Conversation

sedited commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Nov 27, 2024

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sedited commented Nov 27, 2024

Uh oh!

sedited commented Nov 28, 2024

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sedited commented Jan 3, 2025

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sedited Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Feb 27, 2025

Uh oh!

sedited commented Nov 25, 2025

Uh oh!

maflcko commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sedited commented Nov 25, 2025

Uh oh!

sedited commented Dec 16, 2025

Uh oh!

sedited commented Feb 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sedited commented Nov 27, 2024 •

edited

Loading

DrahtBot commented Nov 27, 2024 •

edited

Loading

sedited Feb 14, 2025 •

edited

Loading

maflcko commented Nov 25, 2025 •

edited

Loading