kernel: Flush in ChainstateManager destructor #31382

pull TheCharlatan wants to merge 6 commits into bitcoin:master from TheCharlatan:chainman_flush_destructor changing 14 files +70 −87
  1. TheCharlatan commented at 1:18 pm on November 27, 2024: contributor

    This simplifies the shutdown code a bit and allows for getting rid of the goto in bitcoin-chainstate as well as making it a bit easier to use for future users of the kernel library.

    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

  2. DrahtBot commented at 1:18 pm on November 27, 2024: contributor

    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

    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:

    • #30610 (validation: do not wipe utxo cache for stats/scans/snapshots by sipa)

    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.

  3. DrahtBot added the label Validation on Nov 27, 2024
  4. TheCharlatan marked this as a draft on Nov 27, 2024
  5. DrahtBot added the label CI failed on Nov 27, 2024
  6. DrahtBot commented at 2:04 pm on November 27, 2024: contributor

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

    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.

  7. in src/bitcoin-chainstate.cpp:69 in b17201e7d2 outdated
    68-    // things instantiated so far requires running the epilogue to be torn down
    69-    // properly
    70-    assert(kernel::SanityChecks(kernel_context));
    71+    if (!kernel::SanityChecks(kernel_context)) {
    72+        std::cerr << "Failed sanity check.";
    73+        return 0;
    


    l0rinc commented at 2:09 pm on November 27, 2024:
    nit: this being a console app, it might make sense to return a non-zero error code when cerr is used

    TheCharlatan commented at 2:22 pm on November 27, 2024:
    Yeah, done.
  8. l0rinc commented at 2:15 pm on November 27, 2024: contributor

    Concept ACK

    Closed #31362 in favor of this

  9. TheCharlatan force-pushed on Nov 27, 2024
  10. TheCharlatan force-pushed on Nov 27, 2024
  11. TheCharlatan commented at 3:01 pm on November 27, 2024: contributor
    Seems like I am running into this here: #25073 (comment), maybe time to introduce a replace mempool method like Carl suggested there?
  12. shutdown: Tear down mempool after chainman
    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.
    fa078572f8
  13. TheCharlatan force-pushed on Nov 27, 2024
  14. TheCharlatan force-pushed on Nov 27, 2024
  15. tests: Add replace mempool method
    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.
    93049689f3
  16. kernel: Flush in ChainstateManager dtor
    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.
    7a37f20083
  17. tests: Use chainman dtor logic to simulate restart
    Also remove the reference to the destructed chainman, which could cause
    UB once the chainman is reset.
    32dc5732ee
  18. validation: Remove ResetCoinsViews
    Now that its only callsite has been moved to the destructor, let the
    smart pointer do its thing.
    5e3569264b
  19. kernel: Remove epilogue in bitcoin-chainstate
    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 epilogue goto.
    6c3bc478ca
  20. TheCharlatan force-pushed on Nov 27, 2024
  21. DrahtBot removed the label CI failed on Nov 27, 2024
  22. TheCharlatan commented at 8:51 am on November 28, 2024: contributor

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

    Did this now, ready for review.

  23. TheCharlatan marked this as ready for review on Nov 28, 2024

github-metadata-mirror

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-12-03 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me