refactor: Improve assumeutxo state representation #30214

pull ryanofsky wants to merge 17 commits into bitcoin:master from ryanofsky:pr/cstate changing 40 files +725 −768
  1. ryanofsky commented at 2:53 pm on May 31, 2024: contributor

    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.

  2. DrahtBot commented at 2:53 pm on May 31, 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/30214.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko, fjahr
    Concept ACK mzumsande
    Stale ACK sedited

    If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #34004 (Implementation of SwiftSync by rustaceanrob)
    • #33854 (fix assumevalid is ignored during reindex by Eunovo)
    • #33817 (validation: reduce persisted UTXO set size by prioritizing positive lookups (RFC) by l0rinc)
    • #33779 (ci, iwyu: Fix warnings in src/kernel and treat them as errors by hebasto)
    • #33728 (test: Add bitcoin-chainstate test for assumeutxo functionality by stringintech)
    • #33680 (validation: do not wipe utxo cache for stats/scans/snapshots by l0rinc)
    • #33604 (p2p: Allow block downloads from peers without snapshot block after assumeutxo validation by stringintech)
    • #33259 (rpc, logging: add backgroundvalidation to getblockchaininfo by polespinasa)
    • #32966 (Silent Payments: Receiving by Eunovo)
    • #32950 (validation: remove BLOCK_FAILED_CHILD by stratospher)
    • #32843 (validation: invalid block handling followups by mzumsande)
    • #32414 (validation: periodically flush dbcache during reindex-chainstate by andrewtoth)
    • #31533 (fuzz: Add fuzz target for block index tree and related validation events by mzumsande)
    • #31382 (kernel: Flush in ChainstateManager destructor 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.

  3. DrahtBot added the label Refactoring on May 31, 2024
  4. DrahtBot added the label CI failed on May 31, 2024
  5. DrahtBot commented at 7:56 pm on May 31, 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/25656143496

  6. ryanofsky force-pushed on Jun 3, 2024
  7. DrahtBot removed the label CI failed on Jun 4, 2024
  8. fjahr commented at 9:21 am on June 8, 2024: contributor
    Concept ACK
  9. DrahtBot added the label Needs rebase on Jun 10, 2024
  10. ryanofsky force-pushed on Jun 17, 2024
  11. DrahtBot removed the label Needs rebase on Jun 17, 2024
  12. DrahtBot added the label Needs rebase on Jun 17, 2024
  13. ryanofsky force-pushed on Jun 17, 2024
  14. DrahtBot removed the label Needs rebase on Jun 17, 2024
  15. DrahtBot added the label Needs rebase on Jun 25, 2024
  16. in src/validation.h:556 in cdc44e878a outdated 
    551+    {
552+        return m_validity;
553+    }
554+
555+    //! Return true if chainstate fully validated or is assumed valid.
556+    bool IsValid() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
    fjahr commented at 9:49 am on July 15, 2024:
    nit: In other places similar checks are inlined without a helper method. That might be actually a bit more clear as well.
    ryanofsky commented at 5:43 pm on July 18, 2024:

    re: #30214 (review)

    nit: In other places similar checks are inlined without a helper method. That might be actually a bit more clear as well.

    Good point, at least at first glance this looks reasonable to inline.

    ryanofsky commented at 9:48 pm on July 19, 2024:

    re: #30214 (review)

    nit: In other places similar checks are inlined without a helper method. That might be actually a bit more clear as well.

    You’re right, that is clearer. Updated different places where this was used.

  17. in src/test/validation_chainstatemanager_tests.cpp:32 in ce02d68008 outdated 
    28@@ -29,6 +29,17 @@ using node::BlockManager;
29 using node::KernelNotifications;
30 using node::SnapshotMetadata;
31 
32+static std::vector<Chainstate*> GetAll(const ChainstateManager& chainman)
    fjahr commented at 10:13 am on July 15, 2024:
    Why do you add this function around here instead of using m_chainstates as well?
    ryanofsky commented at 5:43 pm on July 18, 2024:

    Why do you add this function around here instead of using m_chainstates as well?

    I need to double-check but IIRC, the issue is that m_chainstates requires a lock to access, and it would require larger changes and more verbosity in the tests if they had to be changed to access m_chainstates directly. Will follow up and add a comment describing purpose of this function.

    ryanofsky commented at 9:47 pm on July 19, 2024:

    re: #30214 (review)

    Why do you add this function around here instead of using m_chainstates as well?

    Dropped this function now, it was not hard to get rid of.

  18. in src/validation.h:572 in 493d3844cf outdated 
    551     //! @sa ChainstateRole
552-    ChainstateRole GetRole() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
553+    kernel::ChainstateRole GetRole() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
554+
555+    //! Return whether chain is fully validated, assumed valid, or invalid.
556+    ChainValidity Validity() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
    fjahr commented at 10:51 am on July 15, 2024:
    Not sure why we need this when in another commit we remove GetAll() and instead use m_chainstates. Seems kind of inconsistent but maybe I am missing something.
    ryanofsky commented at 5:43 pm on July 18, 2024:

    re: #30214 (review)

    Not sure why we need this when in another commit we remove GetAll() and instead use m_chainstates. Seems kind of inconsistent but maybe I am missing something.

    Is this comment about the Validity() method? This is just supposed to be a public accessor for code that cannot access the private m_validity member/

    fjahr commented at 11:59 pm on May 16, 2025:
    This can be marked as resolved. Not sure why this comment landed in this spot here, pretty weird, but it was going in the same direction as @mzumsande ’s comment here: https://github.com/bitcoin/bitcoin/pull/30214/files#r2040321756
    ryanofsky commented at 2:20 pm on June 17, 2025:

    re: #30214 (review)

    Thanks and in case that comment disappears from the diff view, another link is #30214 (review)

  19. fjahr commented at 11:11 am on July 15, 2024: contributor

    Approach ACK

    I think pretty much everything here is a worth while improvement and should set up the following improvements well. I am not so deep into the current state of the kernel work though, so I can not comment on that part, for example the newly introduced typing file. I guess @TheCharlatan could give his nod to that here.

    It removes uses of the terms “active chainstate,” “usable chainstate,” “disabled chainstate,” “ibd chainstate,” and “snapshot chainstate” which are confusing for various reasons.

    I think this is a bit confusing because it sounds like you are getting rid of this completely but it’s actually just avoided where you touch the code, right? Cleaning the terms up everywhere can be a follow-up, I would like to try to rewrite the design doc a bit more anyway. But I think (selfishly) would like to break it up into design and usage docs because IMO that makes it way more manageable, i.e. the last commit of #29553.

  20. ryanofsky commented at 6:41 pm on July 18, 2024: contributor

    re: #30214#pullrequestreview-2177291599

    Thanks for reviewing! Will rebase and try to address comments. I’m especially interested if you have ideas on splitting up documentation and code changes into separate PRs.

    I am not so deep into the current state of the kernel work though, so I can not comment on that part, for example the newly introduced typing file. I guess @TheCharlatan could give his nod to that here.

    Sure, for background kernel/types.h is analogous to node/types.h, wallet/types.h, and common/types.h. The types files are used when libraries want to define shared struct and enum types that can be used by code NOT directly depending on the library. For example the gui code uses the wallet::isminetype enum type, but the gui should not depend on the wallet library, so the enum is defined in a shared header wallet/types.h. In this PR, wallet code uses the kernel::ChainstateRole struct type, but the wallet code should not depend on the kernel library, so the struct is defined in a shared header kernel/types.h. This pattern started with the wallet library, but has been useful for node and common libraries as well, so it is natural to extend to the kernel.

    A more stricter separation between libraries would be possible with a more complicated structure, but having shared types files has been a reasonable way to separate the libraries while having a 1 library = 1 directory = 1 namespace structure.

    It removes uses of the terms “active chainstate,” “usable chainstate,” “disabled chainstate,” “ibd chainstate,” and “snapshot chainstate” which are confusing for various reasons.

    I think this is a bit confusing because it sounds like you are getting rid of this completely but it’s actually just avoided where you touch the code, right? Cleaning the terms up everywhere can be a follow-up, I would like to try to rewrite the design doc a bit more anyway. But I think (selfishly) would like to break it up into design and usage docs because IMO that makes it way more manageable, i.e. the last commit of #29553.

    Yeah this description was written aspirationally before I implemented this PR and I did not get around to removing all the old language. I’d be interested if you have specific ideas on how to break this PR up better. I definitely think it could make sense to move documentation / comment changes to a separate PR, and I hope #29553 can be reviewed more and merged soon, before this PR. (This is a good reminder for me to re-ack #29553)

  21. ryanofsky force-pushed on Jul 19, 2024
  22. ryanofsky commented at 9:55 pm on July 19, 2024: contributor
    Rebased 493d3844cfc8628fd63184fa7a657126d9b5dc95 -> 2c1d8b426fa1832fa464c12f3cae7e2976584430 (pr/cstate.4 -> pr/cstate.5, compare) due to conflicts with various prs including #30267, #30388, #30395, #29996, #30406, #30320. Also made suggested simplifications. Rebased 2c1d8b426fa1832fa464c12f3cae7e2976584430 -> 4c995842e3689974364c8c82ee220bfaa30f7f79 (pr/cstate.5 -> pr/cstate.6, compare) due to conflict with #30111 Rebased 4c995842e3689974364c8c82ee220bfaa30f7f79 -> 7fd7960389254c7a2ba50614e1cbdf8911ef6a7d (pr/cstate.6 -> pr/cstate.7, compare) due to conflict with #29656
  23. DrahtBot removed the label Needs rebase on Jul 19, 2024
  24. DrahtBot added the label Needs rebase on Jul 24, 2024
  25. ryanofsky force-pushed on Jul 24, 2024
  26. DrahtBot removed the label Needs rebase on Jul 24, 2024
  27. DrahtBot added the label Needs rebase on Aug 5, 2024
  28. ryanofsky force-pushed on Aug 7, 2024
  29. DrahtBot removed the label Needs rebase on Aug 7, 2024
  30. DrahtBot added the label Needs rebase on Aug 9, 2024
  31. fjahr commented at 8:20 pm on August 25, 2024: contributor

    Thanks for addressing my feedback @ryanofsky ! I hope we can merge #29553 soon and then I will get back to this with another full review. But it does look very good to me already.

    I’m especially interested if you have ideas on splitting up documentation and code changes into separate PRs.

    I’m unsure if it’s really needed because I find clean refactoring commits like those here can be reviewed quicker than other changes, so I would be fine to keep it as is just based on volume of the change. But I did think about it and if that is feasible maybe splitting along the lines of everything that touches ChainstateRole and the rest (in whatever order you think works better) could be an idea. If you feel like splitting it that’s fine for me but I think I would prefer to keep it as one PR as I have already passed everything so I am biased towards fewer fundamental changes ;)

    Ah, and since the first is a fix you could just open that as a separate PR now. That should be an easy review and merge that doesn’t conflict with anything else. The docs changes it feels like it’s better to keep them along the code since you need to get your head into the topic either way. I often feel like reviewing docs and code side by side challenges your understanding and might lead to higher quality review.

  32. sedited commented at 9:09 pm on October 9, 2024: contributor
    Concept ACK
  33. in src/validation.cpp:3480 in 7fd7960389 outdated 
    3489-    if (WITH_LOCK(::cs_main, return m_disabled)) {
3490-        LogPrintf("m_disabled is set - this chainstate should not be in operation. "
3491+    // Belt-and-suspenders check that we aren't attempting to advance the
3492+    // chainstate past the target block.
3493+    if (WITH_LOCK(::cs_main, return m_target_utxohash)) {
3494+        LogPrintf("m_target_utxohash is set - this chainstate should not be in operation. "
    sedited commented at 12:57 pm on March 9, 2025:
    I’ve always been a bit puzzled of this check. Is it really required? If so, why don’t we need to check it on every iteration of the do loop further down? What if it is actually reached, will validation on that chain just always be stuck in an unrecoverable state? Wouldn’t a fatal error be more appropriate then?
    ryanofsky commented at 3:53 pm on March 10, 2025:

    #30214 (review)

    I’ve always been a bit puzzled of this check. Is it really required? If so, why don’t we need to check it on every iteration of the do loop further down? What if it is actually reached, will validation on that chain just always be stuck in an unrecoverable state? Wouldn’t a fatal error be more appropriate then?

    This is labeled as “Belt-and-suspenders check” so it should be safe to assume that it is not required. Sometimes it is useful to have checks for unexpected states that should never happen, so it is possible to detect bugs that may otherwise go unnoticed, or to provide better debug information if the unexpected state does lead to a problem later.

    Sometimes unexpected states should trigger asserts or fatal errors, but sometimes they don’t need to be fatal and can just trigger warnings like this one. In this case the unexpected state may cause the current snapshot to never get fully validated (if the background chainstate is disabled), but since the node will still be functional, and the user might be able to recover from it by loading another snapshot, maybe it is not worth causing a fatal error. Also it might make sense to have this check at the top of this function not in the loop below because it is checking a precondition: looking for an invalid state set by previous code, not invalid state set by this code. It may be possible to improve this by adding other checks, though. And it would probably be good to use if (Assume(...)) here so if this condition is reached it will be detected in fuzz and debug builds.

  34. sedited commented at 4:01 pm on March 12, 2025: contributor
    Reading through commit a35a2a05887202de8c49727b6ea9166bf5f1a6f2 I am asking myself if with the addition of m_target_blockhash, the state of m_validity can just be computed on the fly without having to lug around and assert its state. For tracking the change in validity to INVALID once the tip reaches the target block the historical chainstate, could the presence of the m_target_utxo_hash (similar to how the belt-and-suspenders check is done later on) be checked instead? And for the current chainstate could the ASSUMED_VALID state be tracked with the presence of the snapshot base block and then the transition to the VALIDATED state by checking if the blocks up to the snapshot base block have BLOCK_VALID_MASK?
  35. ryanofsky commented at 4:42 pm on March 12, 2025: contributor

    Reading through commit a35a2a0 I am asking myself if with the addition of m_target_blockhash, the state of m_validity can just be computed on the fly without having to lug around and assert its state.

    I think it could probably work but two reasons not to do it might be:

    (1) to avoid performance regressions in parts of the code that are assuming it is possible to determine whether a chainstate is valid or valid instantly without without iterating over the chain (2) to make intent explicit and make code general and future proof

    To explain (2) maybe it is safe right now to assume that any chain that is not invalid and not fully validated must be assumed-valid. But it’s not clear that this should always be the case. Maybe a potential libbitcoinkernel application could be a fork-monitoring client that tracks multiple chains for some other reason and doesn’t want to mark any of them as assume-valid. Having an explicit assume-valid state could make purpose of the chain more explicit and could let code dealing with multiple chainstates use chains based on how they are intended to be used without trying to infer intent from other attributes.

    Maybe what you are are suggesting could be an improvement, through, and it’s been a while since I thought about this and reasons I listed above are speculative. Could try implementing the suggestion and see if it simplifies the code.

  36. sedited commented at 11:18 am on March 13, 2025: contributor

    Maybe what you are are suggesting could be an improvement, through, and it’s been a while since I thought about this and reasons I listed above are speculative. Could try implementing the suggestion and see if it simplifies the code.

    This is what I had in mind: https://github.com/TheCharlatan/bitcoin/commit/9c89548c670e7b807a969e13408ff03df15143a2 (also rebased this branch to get cmake) . It is very broken in its current state though. There are a few problems with it, but I guess the fundamental problem is that the current application of a snapshot is done in relation to an existing chain and its state, and not against the block index and its state. E.g. the check for not loading a snapshot with less work only looks at the chain state, not the block index. If the chain is behind the block index, but the block index already has more fully-validated work, the snapshot application passes, but my new function would think it is already fully validated. The tests heavily rely on the current behaviour, where the best work chain is taken from the existing chainstate.

    Thinking outside of this proposed change, is this really the desired behaviour though? Shouldn’t the block index be our fundamental source of truth? I can think of a few scenarios outside of the tests where the chain being behind the fully-validated block index can be encountered, for example during a chainstate reindex, but I don’t think they are something where snapshots should be supported.

    (2) to make intent explicit and make code general and future proof

    I would argue that adding this validity state could also make it more complicated to implement, or compose from existing functionality, parallel validation, or similar IBD speedups that might partition the chain even more.

  37. ryanofsky force-pushed on Mar 13, 2025
  38. ryanofsky commented at 5:14 pm on March 13, 2025: contributor

    Rebased 7fd7960389254c7a2ba50614e1cbdf8911ef6a7d -> 58f62c2221d82e22e7c2d9cccedd58e4390ace5a (pr/cstate.7 -> pr/cstate.8, compare) due to various conflicts. Updated 58f62c2221d82e22e7c2d9cccedd58e4390ace5a -> d09b82156ced5d543d08226e8d7b8fda2e0ec532 (pr/cstate.8 -> pr/cstate.9, compare) to fix “error: private field ’m_is_memory’ is not used” https://github.com/bitcoin/bitcoin/pull/30214/checks?check_run_id=38726768015

    re: #30214 (comment)

    Shouldn’t the block index be our fundamental source of truth?

    I’m open to this idea but not sure the block index has enough information in this case to provide information efficiently since you can’t know if a particular block is fully validated without iterating over the whole chain back to genesis. Also, if mulltiple chainstates might exist for different purposes, it may be useful to store information about the purpose of each chainstate and what assumptions it was created under in the chainstate itself not in blocks that it is pointing to. I think the Chainstate::m_validity member in this PR does both of these things in a pretty straightforward way, but if we want to replace it with something better, hopefully that should not be too hard since it is a private member, so the public Chainstate interface would not have change.

    One thing this PR is trying to accomplish is making it possible determine status of Chainstate objects independently, without having to look at ChainstateManager objects and other Chainstate objects. But it should be fine for Chainstate methods to use BlockIndex flags to return their status.

    I would argue that adding this validity state could also make it more complicated to implement, or compose from existing functionality, parallel validation, or similar IBD speedups that might partition the chain even more.

    I think if multiple chainstates will exist for different purposes, the chainstate objects will need to have some field describing their purpose and controlling their behavior. I can easily see the m_validity field not being general enough to do that, but it’s more easy for me to imagine it being replaced by another field than being eliminated entirely. But again I think the fact that it’s a private field should make it easier to change or eliminate.

    Should also point out the that m_validity member added here is not really new. It’s just replacing the previous m_disabled member with something more positive and less ambiguous.

  39. ryanofsky force-pushed on Mar 13, 2025
  40. DrahtBot added the label CI failed on Mar 13, 2025
  41. DrahtBot commented at 5:34 pm on March 13, 2025: contributor

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

    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.

  42. DrahtBot removed the label Needs rebase on Mar 13, 2025
  43. DrahtBot removed the label CI failed on Mar 13, 2025
  44. sedited commented at 8:39 pm on March 13, 2025: contributor

    I think if multiple chainstates will exist for different purposes, the chainstate objects will need to have some field describing their purpose and controlling their behavior.

    Yes, I think I have come to a similar conclusion, and I think the introduced reliance here on the m_target_blockhash and m_from_snapshot_blockhash are very good changes. Having some kind of “start” and “stop” blocks for a chainstate seems like something that is easy to understand and might also be useful beyond assumeutxo.

    since you can’t know if a particular block is fully validated without iterating over the whole chain back to genesis

    Isn’t that only true when iterating over the snapshot block or when you don’t know if you might come across it? My understanding is that is also what the docs say about BLOCK_VALID_CHAIN and BLOCK_VALID_SCRIPTS and is also checked in CheckBlockIndex.

    I think if multiple chainstates will exist for different purposes, the chainstate objects will need to have some field describing their purpose and controlling their behavior.

    I would also like to see a future with more diverse modes of validation, but I feel like we already have the ChainstateManager class for controlling the behaviour of a Chainstate. In my mind a Chainstate should not have to know all the details of what it is being used for, or have rich descriptive state about its own behaviour. I feel like many of the self-describing behaviours, like passing the ChainstateRole through a notification, should actually be done by the ChainstateManager. I don’t think the changes in this PR go diametrically against that goal, but I am also not sure yet if the changes here really make it easier to eventually go into that direction.

  45. ryanofsky commented at 11:08 pm on March 13, 2025: contributor

    I think I agree with all that, and think chainstate objects should just contain information for chainstatemanager to deal with them, not to have “rich” data or application specific data. I also think depending on what you want to do with chainstates, block status flags may provide enough information, but they are not equivalent to the old Chainstate::m_disabled field or the new Chainstate::m_validity field replacing it, since block flags only tell you about validity back to the last snapshot block, not back to genesis block.

    I guess I’m not really sure what problems you see with chainstates knowing their role or validity, but it is true they continue to know some of that information after this PR just like they knew it before. If you want to move that information elsewhere like chainstate manager or derive it from block validity flags I don’t think this PR should get in the way of that, and it probably even helps a little because it narrows the definition of ChainstateRole to not indicate assumeutxo status but only indicate basic information about whether the chainstate is validated and what block it is targeting. The PR should also help more if you are interested in using chainstate objects for applications other than assumeutxo, because it is dropping assumeutxo-specific assumptions many places in validation and networking code and adding a generic ChainstateManager::m_chainstates vector that can hold arbitrary chainstates.

    I’d also point out that the Chainstate::m_validity field only gets written in 2 places: in the Chainstate constructor and in the ChainstateManager::MaybeCompleteSnapshotValidation method, so I don’t think it adds much maintenance burden even if you think other fields might be more useful.

  46. DrahtBot added the label Needs rebase on Mar 14, 2025
  47. in src/validation.cpp:6063 in 22bfd948f0 outdated 
    6146     }
6147     const int snapshot_tip_height = this->ActiveHeight();
6148     const int snapshot_base_height = *Assert(this->GetSnapshotBaseHeight());
6149     const CBlockIndex& index_new = *Assert(m_ibd_chainstate->m_chain.Tip());
6150-
6151-    if (index_new.nHeight < snapshot_base_height) {
    sedited commented at 12:52 pm on March 14, 2025:
    In commit 22bfd948f0d45083748724a304fa2c8482464028: Just a note: The removal of this particular check in this commit seems a bit random with all the following kind of redundant assertions, but the code does get cleaned up in later commits.
    ryanofsky commented at 4:06 pm on March 17, 2025:

    re: #30214 (review)

    In commit 22bfd94: Just a note: The removal of this particular check in this commit seems a bit random with all the following kind of redundant assertions, but the code does get cleaned up in later commits.

    We might be noticing different things here so let me know if you have suggestions for improving this, but I don’t think this change is random. This is replacing the index_new.nHeight < snapshot_base_height check with a ReachedTarget() check, and combining the return SKIPPED it triggers with the earlier one with a more complete comment about when SKIPPED is returned.

    The other changes to this function that happen in later commit don’t really affect these things, they just pass new parameters into this function so it can be more independent of ChainstateManager.

    sedited commented at 3:12 pm on March 27, 2025:

    Missed this in my previous round, but is this not changing behaviour significantly? As far as I can tell we now just check that the target blockhash is reached at some point, while this check that is removed here only skips blocks up to the snapshot height and tries to complete validation with the rest once the snapshot base height is reached. What if the snapshot block is never reached? Does background validation then just continue on indefinitely?

    I might be missing something, but the way I am reading this we also don’t have any tests for exercising what previously was previously the BASE_BLOCKHASH_MISMATCH failure case.

    ryanofsky commented at 3:22 pm on March 27, 2025:

    re: #30214 (review)

    Will look more closely, but I think both the old code and new code are assuming that the chainstate tip is an ancestor of the chainstate target block, or points directly to it. If that’s not true I think you are right that the behavior of this function before and after this change would be different, but a lot of other things would be broken too.

    ryanofsky commented at 4:03 pm on March 27, 2025:

    re: #30214 (comment)

    What if the snapshot block is never reached? Does background validation then just continue on indefinitely?

    I think you are right and there is an improvement that can be made here. The case where the validation chainstate tip somehow points to a higher block than than target block (snapshot block) is supposed to be impossible and should never occur. That case used to return BASE_BLOCKHASH_MISMATCH, but was changed here to trigger an Assert instead, so the code would be simpler and so we would know if a bug occurred. However, because the Assert is place on line 6173 below the SKIPPED check on line 6126 I think the assert would never trigger, and SKIPPED would always be returned instead.

    There are two callers MaybeCompleteSnapshotValidation, one is in ConnectTip which was ignoring the return value so would not be affected, but the other call in LoadChainstate would be affected and could cause the error to be ignored, instead of causing a startup failure.

    I think it would probably be good to add an assert to the top of this function to make sure the chain tip is really is an ancestor of the target block, to document the assumption make make it for likely for bugs to be caught.

    sedited commented at 9:40 pm on March 27, 2025:

    I find it a bit hard to check this, but wouldn’t the assertion then be triggerable if an alternative block at that height is received? How about adding a ReachedTargetHeight, and use that in the if block? Something like:

     0diff --git a/src/validation.cpp b/src/validation.cpp
 1index 2d19ce45e5..da9efae00f 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -6091 +6091 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation(Chai
 5-            !validated_chainstate.ReachedTarget()) {
 6+            !validated_chainstate.ReachedTargetHeight()) {
 7@@ -6096,0 +6097 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation(Chai
 8+    if (!validated_chainstate.ReachedTarget()) return SnapshotCompletionResult::BASE_BLOCKHASH_MISMATCH;
 9diff --git a/src/validation.h b/src/validation.h
10index 1b872c5af6..2d66995440 100644
11--- a/src/validation.h
12+++ b/src/validation.h
13@@ -643,0 +644,7 @@ public:
14+    //! Return true if chainstate reached target block height.
15+    bool ReachedTargetHeight() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
16+    {
17+        const CBlockIndex* target_block{TargetBlock()};
18+        return target_block && target_block->nHeight <= m_chain.Height();
19+    }
20+
21@@ -865,0 +873,2 @@ enum class SnapshotCompletionResult {
22+    BASE_BLOCKHASH_MISMATCH,
    ryanofsky commented at 10:05 pm on April 7, 2025:

    re: #30214 (review)

    Since we’re talking about a case that should be impossible and indicates a bug I think the best thing to do is just to add asserts where convenient to make sure it does not happen, so I updated ReachedTarget() to have a new assert checking that if a target block is set the chain tip must be an ancestor of it. Your suggestion to add a ReachedTargetHeight() method could also work to trigger the existing assert on line 6173, but I don’t think it’s as appealing because any check based on height rather than hashes will be less reliable.

    sedited commented at 8:01 am on April 8, 2025:
    Thanks for considering! I think that your change is good too. I was trying to write a functional test to see what happens when the snapshot block is re-organized while the historical chainstate is still catching up. It seems like we already shut down the node with a fatal error in that case, because DisconnectTip will fail to read the historical blocks that are not synced yet. In a way that is similar behaviour to when a re-org beyond the prune height occurs. Since that is the case I agree that reaching such a state on the historical chainstate is a programming error. Should we document this fatal error case with a functional test?
    ryanofsky commented at 4:05 pm on April 8, 2025:

    re: #30214 (review)

    Should we document this fatal error case with a functional test?

    It could be good to have a test or improve handling of that case, but I don’t think there is much of a connection between that case and the changes in this PR.

    It seems to me like implicit assumption than snapshot block will not get reorged is a different than the assumption that !TargetBlock() || TargetBlock()->GetAncestor(m_chain.Height()) == m_chain.Tip() will be true for any chainstate. If we would like to gracefully handle the case where the snapshot block gets reorged (which would be nice IMO), I think the best way to do that would be to call SetTargetBlock(nullptr) to reset the target block of the validation chainstate and turn it back into a normal chainstate.

  48. in src/validation.cpp:6108 in 22bfd948f0 outdated 
    6181@@ -6161,14 +6182,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation()
6182         GetNotifications().fatalError(user_error);
6183     };
6184 
6185-    if (index_new.GetBlockHash() != snapshot_blockhash) {
6186-        LogPrintf("[snapshot] supposed base block %s does not match the "
6187-          "snapshot base block %s (height %d). Snapshot is not valid.\n",
6188-          index_new.ToString(), snapshot_blockhash.ToString(), snapshot_base_height);
6189-        handle_invalid_snapshot();
6190-        return SnapshotCompletionResult::BASE_BLOCKHASH_MISMATCH;
    sedited commented at 12:56 pm on March 14, 2025:
    I think this SnapshotcompletionResult variant can be removed, no?
    ryanofsky commented at 4:04 pm on March 17, 2025:

    re: #30214 (review)

    I think this SnapshotcompletionResult variant can be removed, no?

    Good catch, removed.

  49. in src/validation.h:1142 in 9853c8384e outdated 
    1138@@ -1136,7 +1139,7 @@ class ChainstateManager
1139     Chainstate* HistoricalChainstate() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex())
1140     {
1141         auto* cs{m_ibd_chainstate.get()};
1142-        return IsUsable(cs) && cs->m_target_blockhash ? cs : nullptr;
1143+        return cs && cs->Validity() != ChainValidity::INVALID && cs->m_target_blockhash && !cs->m_target_utxohash ? cs : nullptr;
    sedited commented at 1:37 pm on March 14, 2025:
    In commit 9853c8384e1df9ad14c2801e780cc189ac20c6d0: Is the validity check required? AFAICT the historic chainstate is never invalid.
    ryanofsky commented at 6:35 pm on March 17, 2025:

    re: #30214 (review)

    In commit 9853c83: Is the validity check required? AFAICT the historic chainstate is never invalid.

    That’s true. This anticipates a later change but there is no reason to add that complexity here. Dropped this condition.

  50. in src/kernel/types.h:1 in 7e9ee9345a outdated 
    0@@ -0,0 +1,30 @@
1+// Copyright (c) 2024 The Bitcoin Core developers
    sedited commented at 4:52 pm on March 14, 2025:
    Nit: Are we still doing -present now? I thought I saw some discussion about that recently, but can’t find it anymore.
    ryanofsky commented at 7:04 pm on March 17, 2025:

    re: #30214 (review)

    Nit: Are we still doing -present now? I thought I saw some discussion about that recently, but can’t find it anymore.

    The change to “present” never made sense to me because I think the only thing “present” could mean is “when this line was written”, not what we might want it to mean like “whenever the code here was last modified” or “whenever the code here was published.”

    Since “present” just seems like a more ambiguous way of writing the current year, I’d be inclined to stick with a more a conventional approach. But I’m happy to change if the project wants to make a wider switch to “present” or someone can explain why writing “present” is better than writing the current year and never updating it. I couldn’t find much discussion about this topic, but https://opensource.stackexchange.com/questions/4885/copyright-until-the-present-year-in-bsd-license seems to back up my suspicions here.

    sedited commented at 12:53 pm on March 27, 2025:
    I really hate copyright discussions, but I think it would good to at least apply it consistently in the project. Definitely not something that should be hashed out on this PR though.
    maflcko commented at 9:15 am on December 8, 2025:

    nit in 1d477c711599b7eeac40b18a30ee9331d951a726: Could drop the year, for entirely new files?

    This is commonly done, see git log --oneline -S 'Copyright (c) The Bitcoin Core developers'

    ryanofsky commented at 11:26 pm on December 9, 2025:

    re: #30214 (review)

    nit in 1d477c7: Could drop the year, for entirely new files?

    Yes, good catch, and done in the latest push

  51. in src/kernel/types.h:20 in 7e9ee9345a outdated 
    15+
16+namespace kernel {
17+//! Information about chainstate that notifications are sent from.
18+struct ChainstateRole {
19+    //! Whether this is a notification from chainstate that's been fully
20+    //! validated starting from the genesis block. False if is from an
    sedited commented at 4:53 pm on March 14, 2025:
    Nit: s/if is/if it is/. I think there are some missing articles around here too.
    ryanofsky commented at 7:16 pm on March 17, 2025:

    re: #30214 (review)

    Nit: s/if is/if it is/. I think there are some missing articles around here too.

    Thanks, fixed up various comments in this header.

  52. in src/kernel/types.h:24 in 7e9ee9345a outdated 
    19+    //! Whether this is a notification from chainstate that's been fully
20+    //! validated starting from the genesis block. False if is from an
21+    //! assumeutxo snapshot chainstate that has not been validated yet.
22+    bool validated{true};
23+
24+    //! Whether this a historical chainstate downloading old blocks to validate
    sedited commented at 4:58 pm on March 14, 2025:
    Nit: s/this a/this is a/
    ryanofsky commented at 7:16 pm on March 17, 2025:

    re: #30214 (review)

    Nit: s/this a/this is a/

    Thanks, fixed

  53. in src/validation.cpp:3919 in 5e22fdcdb2 outdated 
    3926@@ -3927,7 +3927,7 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd
3927     // m_chain_tx_count value is not zero, assert that value is actually correct.
3928     auto prev_tx_sum = [](CBlockIndex& block) { return block.nTx + (block.pprev ? block.pprev->m_chain_tx_count : 0); };
3929     if (!Assume(pindexNew->m_chain_tx_count == 0 || pindexNew->m_chain_tx_count == prev_tx_sum(*pindexNew) ||
3930-                pindexNew == CurrentChainstate().SnapshotBase())) {
3931+                std::any_of(m_chainstates.begin(), m_chainstates.end(), [&](auto& cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->SnapshotBase() == pindexNew; }))) {
    sedited commented at 8:07 pm on March 14, 2025:
    I don’t quite follow this change. What is wrong with using CurrentChainstate here?
    ryanofsky commented at 5:30 pm on March 18, 2025:

    In commit “refactor: Add ChainstateManager::m_chainstates member” (5e22fdcdb2665c0bb5e47c53719796096afe2dd1)

    re: #30214 (review)

    I don’t quite follow this change. What is wrong with using CurrentChainstate here?

    IMO chainstatemanager should generally just validate and update chainstates with incoming data and not rely on concept of a “current” chainstate. The concept of a current chainstate is a little vague and makes more sense at the appilcation level than this level. For example wallets will treat chainstates loaded from assumeutxo snapshots as the current chainstate while indexes ignore that chainstate and treat the fully validated background chainstate as the current one.

    More specifically in this case avoiding CurrentChainstate() makes this code more correct, and would avoid a failed Assume check and a warning if multiple snapshot chainstates were loaded, since loading a snapshot updates the snapshots block’s m_chain_tx_count value when when the previous block may have a 0 value. (We don’t currently support loading multiple snapshot chainstates now, but there is no reason we could not allow it in the future, for example to support loading a new snapshot after an older one has been loaded.)

  54. in src/validation.cpp:6510 in 58265b955a outdated 
    6505@@ -6514,3 +6506,24 @@ std::optional<std::pair<const CBlockIndex*, const CBlockIndex*>> ChainstateManag
6506     if (!chainstate) return {};
6507     return std::make_pair(chainstate->m_chain.Tip(), chainstate->TargetBlock());
6508 }
6509+
6510+util::Result<void> ChainstateManager::ActivateBestChains() const
    sedited commented at 8:36 pm on March 14, 2025:
    Why is this not re-used for ProcessNewBlock?
    ryanofsky commented at 5:30 pm on March 18, 2025:

    In commit “refactor: Add ChainstateManager::ActivateBestChains() method” (58265b955a3d8622f22958943f79409129d58b36)

    re: #30214 (review)

    Why is this not re-used for ProcessNewBlock?

    ProcessNewBlock() passes a non-null block pointer to ActivateBestChain so it can’t directly reuse the new method. It might make sense for generalize the method, but the main goal here to get rid of unnecessary uses of ChainstateManager::GetAll() by outside code, so it’s a bit outside the scope of what this particular commit is trying to do.

  55. in src/validation.cpp:6519 in 58265b955a outdated 
    6514+    // the relevant pointers before the ABC call.
6515+    std::vector<Chainstate*> chainstates;
6516+    {
6517+        LOCK(GetMutex());
6518+        for (auto& chainstate : m_chainstates) {
6519+            chainstates.emplace_back(chainstate.get());
    sedited commented at 8:38 pm on March 14, 2025:
    Shouldn’t this skip over the historical chainstate if m_target_utxohash is set?
    ryanofsky commented at 5:30 pm on March 18, 2025:

    In commit “refactor: Add ChainstateManager::ActivateBestChains() method” (58265b955a3d8622f22958943f79409129d58b36)

    re: #30214 (review)

    Shouldn’t this skip over the historical chainstate if m_target_utxohash is set?

    Yes good catch. Updated this to be equivalent.

  56. in src/node/blockstorage.cpp:327 in ca48478fbf outdated 
    309@@ -310,8 +310,9 @@ void BlockManager::FindFilesToPrune(
310 {
311     LOCK2(cs_main, cs_LastBlockFile);
312     // Distribute our -prune budget over all chainstates.
313+    const int num_chainstates{chainman.HistoricalChainstate() ? 2 : 1};
    sedited commented at 8:40 pm on March 14, 2025:
    I’m surprised this is not just taking the m_chainstates size. Why is that?
    ryanofsky commented at 5:30 pm on March 18, 2025:

    In commit “refactor: Delete ChainstateManager::GetAll() method” (ca48478fbfcc83cc6f8c928e57800707fe0c3b51)

    re: #30214 (review)

    I’m surprised this is not just taking the m_chainstates size. Why is that?

    Wrote a better comment since existing comment wasn’t really explaining what was happening here. It’s possible this code might need to be updated to customize pruning behavior if more chainstates are added in the future. But I think default behavior should just be to reserve block storage for the two chainstates we know about: the most-work chainstate and a historical chainstate, not to reserve storage for an arbitrary number of chainstates which may or may not need it.

  57. in src/test/validation_chainstatemanager_tests.cpp:372 in ca48478fbf outdated 
    368@@ -365,15 +369,17 @@ struct SnapshotTestSetup : TestChain100Setup {
369 
370         BOOST_TEST_MESSAGE("Simulating node restart");
371         {
372-            for (Chainstate* cs : chainman.GetAll()) {
373-                LOCK(::cs_main);
374-                cs->ForceFlushStateToDisk();
375+            LOCK(::cs_main);
    sedited commented at 8:52 pm on March 14, 2025:
    I’m a bit confused when it is appropriate to use ::cs_main and when chainman.GetMutex(). Is there a rule to that?
    ryanofsky commented at 5:30 pm on March 18, 2025:

    In commit “refactor: Delete ChainstateManager::GetAll() method” (ca48478fbfcc83cc6f8c928e57800707fe0c3b51)

    re: #30214 (review)

    I’m a bit confused when it is appropriate to use ::cs_main and when chainman.GetMutex(). Is there a rule to that?

    I don’t really make any distinction between the two but probably it’s better to use the latter so switched to that in these tests

  58. sedited commented at 9:24 pm on March 14, 2025: contributor

    Approach ACK

    I am not yet convinced of the last three commits changing the code (5e22fdcdb2665c0bb5e47c53719796096afe2dd1, 58265b955a3d8622f22958943f79409129d58b36, ca48478fbfcc83cc6f8c928e57800707fe0c3b51), maybe they could do with some more motivation in the commit message? I guess the last of the commits ca48478fbfcc83cc6f8c928e57800707fe0c3b51 is kind of the result of the preceding two and it is nice not to have to construct the vector on the fly for every GetAll() call anymore.

    There is an opportunity for also simplifying the pruning code a bit, I made a commit here https://github.com/TheCharlatan/bitcoin/commit/fe76cc3a04c94ca4fdbb9550d478909f3b13d8ec that removes the chainman arguments from the manual pruning function. I think it would fit in well with the patch set here.

  59. ryanofsky force-pushed on Mar 18, 2025
  60. ryanofsky commented at 9:26 pm on March 18, 2025: contributor

    Rebased d09b82156ced5d543d08226e8d7b8fda2e0ec532 -> 467528960689c2913c101ef75bc833e8f04bd0f3 (pr/cstate.9 -> pr/cstate.10, compare) due to conflict 31961 and implementing various suggestions.

    re: #30214#pullrequestreview-2685423528

    Thanks for the review! I added comments to the m_chainstates commits to motivate them better, also added your simplification fe76cc3a04c94ca4fdbb9550d478909f3b13d8ec and implemented other suggestions below.

  61. in src/test/validation_chainstatemanager_tests.cpp:607 in df99f19d81 outdated 
    599@@ -600,9 +600,14 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup)
600     // This call reinitializes the chainstates.
601     this->LoadVerifyActivateChainstate();
602 
603+    std::vector<Chainstate*> chainstates;
604     {
605         LOCK(chainman_restarted.GetMutex());
606-        BOOST_CHECK_EQUAL(chainman_restarted.GetAll().size(), 2);
607+        chainstates = chainman_restarted.GetAll();
608+        BOOST_CHECK_EQUAL(chainstates.size(), 2);
    mzumsande commented at 7:17 pm on March 19, 2025:

    df99f19d81537ecfbf66f6662bc9814b594ef8d2: I was trying to understand why this fix is correct / the height suddenly changes from 109 to 110, and I find the entire chainstatemanager_snapshot_init subtest very confusing: First, there is a comment saying “Test that simulating a shutdown (…) we end up with a single chainstate that is at tip”, followed by a check that there are still 2 chainstates - so the test contradicts itself.

    Then the test calls the test-only function LoadVerifyActivateChainstate which only calls ABC for the active chainstate, instead of doing it for all chainstates like its counterpart in ImportBlocks() during actual init does - why is this using an artificial init process that differs from the way it’s done in production?

    Because of that, only during mineBlocks() (as a result of ProcessNewBlock() calling ABC for both chainstates) the block that was previously disconnected in a hacky way (by simply calling DisconnectBlock() instead of e.g. invalidating it) is being connected again, and the background snapshot is verified, which is a surprising side effects of just trying to connect some blocks on the tip - and the actual reason why the height is suddenly 110. I think that this deserves an explanation, but more importantly, I don’t see the point / strategy of the subtest in general - what situation is it actually trying to test?

    I realize that this isn’t really what this PR is about, so maybe there could be a separate PR to clean this test up properly, especially if the other subtests (which I didn’t check) would have similar problems?

    ryanofsky commented at 12:53 pm on March 20, 2025:

    re: #30214 (review)

    I find the entire chainstatemanager_snapshot_init subtest very confusing

    Yes. And thanks for digging into the test. I added comments to try to make it less confusing. For reference the test was added in e4d7995 and seems basically unchanged since when it was written.

    First, there is a comment saying “Test that simulating a shutdown (…) we end up with a single chainstate that is at tip”, followed by a check that there are still 2 chainstates - so the test contradicts itself.

    Good catch, the comment is wrong. If you search for “single chainstate” you can see the same comment is copied from 2 other tests in this file and is ok in those places. Fixed in latest push.

    Then the test calls the test-only function LoadVerifyActivateChainstate which only calls ABC for the active chainstate, instead of doing it for all chainstates like its counterpart in ImportBlocks() during actual init does - why is this using an artificial init process that differs from the way it’s done in production?

    I don’t know the answer to this. The unit tests do a lot of artificial things and I’d agree it is usually better when we can remove artificial code so tests are more realistic and there is less code to maintain.

    Because of that, only during mineBlocks() (as a result of ProcessNewBlock() calling ABC for both chainstates) the block that was previously disconnected in a hacky way (by simply calling DisconnectBlock() instead of e.g. invalidating it) is being connected again, and the background snapshot is verified, which is a surprising side effects of just trying to connect some blocks on the tip - and the actual reason why the height is suddenly 110.

    Added a comment about that instead of changing it so this is still a focused bugfix. Agree if test were more realistic it might be clearer.

    I think that this deserves an explanation, but more importantly, I don’t see the point / strategy of the subtest in general - what situation is it actually trying to test?

    I think the main thing it is trying to test is BOOST_CHECK_EQUAL(chainstates.size(), 2); i.e. “that snapshot chainstates initialize properly when found on disk” and that the chainstates function as expected when more blocks are mined. Expanded the comment at the top of the test, too.

    I realize that this isn’t really what this PR is about, so maybe there could be a separate PR to clean this test up properly, especially if the other subtests (which I didn’t check) would have similar problems?

    I think it would be nice if tests were more realistic, and the main thing that annoys me about these tests that is instead of using literal numbers and expressions, the tests like to declare lots of helper variables, so there are many similarly named variables it it is difficult to keep track of what they all mean and how they are used. It probably would be nice to clean these tests up and make them simpler, though not a priority.

  62. in src/validation.h:641 in a1fd5a8862 outdated 
    605@@ -598,12 +606,16 @@ class Chainstate
606     //! target the most-work, valid block.
607     std::optional<uint256> m_target_blockhash GUARDED_BY(::cs_main);
608 
609+    //! Hash of the UTXO set at the target block, computed when the chainstate
610+    //! reaches the target block, and null before then.
611+    std::optional<AssumeutxoHash> m_target_utxohash GUARDED_BY(::cs_main);
    mzumsande commented at 8:55 pm on March 19, 2025:

    a1fd5a886252ebd813ee69029dcf17de6883e047: Why is m_target_utxohash a std::optional<AssumeutxoHash> if it’s only ever used as a bool? The hash, once set , is never read anywhere as far as I can see.

    nit: it should be m_target_utxohash instead of m_target_blockhash in the commit message.

    sedited commented at 9:44 am on March 20, 2025:
    I was thinking that this might be interesting to add to the getchainstates rpc call.
    ryanofsky commented at 2:12 pm on March 20, 2025:

    re: #30214 (review)

    Thanks, fixed commit message. Ultimately I think it would be best to drop the m_target_utxohash member and just check the ReachedTarget() condition everywhere instead of using m_target_utxohash.has_value(). But this needs to be done carefully, and it might require getting rid of some “belt and suspenders” checks in the current code which are technically unnecessary but give some confidence the code works correctly.

    This commit is a more of a pure refactoring and tries to keep code the working the way it does currently, but add transparency so it clearer why it works and what assumptions it is making. It’s trying to replace the opaque m_disabled bool which could indicate either (1) that the chainstate target is invalid or (2) that a chainstate is valid but no longer in use after its utxo hash was checked. Storing the utxo hash in the latter state seemed more natural representation than adding a bool utxohash_checked member, but I could switch to that if preferred.

  63. ryanofsky removed the label Needs rebase on Mar 19, 2025
  64. mzumsande commented at 9:42 pm on March 19, 2025: contributor
    Concept ACK only reviewed up to 199a2eb87c3ad9382f2fe4ae97524099224a7b04 so far.
  65. DrahtBot added the label Needs rebase on Mar 20, 2025
  66. ryanofsky force-pushed on Mar 20, 2025
  67. ryanofsky force-pushed on Mar 20, 2025
  68. ryanofsky commented at 2:18 pm on March 20, 2025: contributor

    Thanks for the reviews

    Rebased 467528960689c2913c101ef75bc833e8f04bd0f3 -> 00c3e4db49ebd2c3bf2f3c7f2a8c9ba5dfb3c8f5 (pr/cstate.10 -> pr/cstate.11, compare) due to conflict with #31519, also adding test comments. Update 00c3e4db49ebd2c3bf2f3c7f2a8c9ba5dfb3c8f5 -> 137d5980abd49a1d2bf783cb57a9386b1941550f (pr/cstate.11 -> pr/cstate.12, compare) just fixing a commit message

  69. DrahtBot removed the label Needs rebase on Mar 20, 2025
  70. ryanofsky force-pushed on Apr 7, 2025
  71. ryanofsky commented at 10:11 pm on April 7, 2025: contributor
    Updated 137d5980abd49a1d2bf783cb57a9386b1941550f -> abe6f0642493402d858dc3aa297ec941bfeaaf87 (pr/cstate.12 -> pr/cstate.13, compare) adding a new assert to deal with concern in #30214 (review) and make sure the impossible condition that would have previously triggered a BASE_BLOCKHASH_MISMATCH error will still reliably be caught, instead of potentially keeping the background chainstate around forever in a state where it is not syncing.
  72. in src/test/fuzz/utxo_snapshot.cpp:163 in 529890d836 outdated 
    159@@ -160,7 +160,7 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer)
160             const auto* index{chainman.m_blockman.LookupBlockIndex(block->GetHash())};
161             Assert(index);
162             Assert(index->nTx == 0);
163-            if (index->nHeight == chainman.GetSnapshotBaseHeight()) {
164+            if (index->nHeight == chainman.CurrentChainstate().SnapshotBase()->nHeight) {
    sedited commented at 9:35 am on April 8, 2025:

    In commit 529890d836cb75175c7943ee0a79c536412d002c

    I get a compilation error here. The change seems to be two commits too early, but the line also needs to be changed in this commit, so there would be some line churn.

  73. sedited commented at 9:45 am on April 8, 2025: contributor
    Last changes look good, but I think the interim compilation error in the fuzz binary should be fixed. There are some formatting and include ordering issues, can you run clang-format-diff over the commits?
  74. ryanofsky commented at 4:07 pm on April 8, 2025: contributor

    re: #30214#pullrequestreview-2749329967

    Thanks for the suggestion. Will try to make sure all the intermediate commits compile again and run clang-format-diff on them in the next few days.

  75. in src/kernel/types.h:18 in 24aaf0d3a1 outdated 
    13+#ifndef BITCOIN_KERNEL_TYPES_H
14+#define BITCOIN_KERNEL_TYPES_H
15+
16+namespace kernel {
17+//! Information about chainstate that notifications are sent from.
18+struct ChainstateRole {
    mzumsande commented at 9:23 pm on April 11, 2025:
    commit 24aaf0d3a149bf532dd5f87dc58466822f847b07: Going from a 3-state enum to a struct of two bools means that one of the 4 possible combinations (validated = false, historical = true) is now something users of ChainstateRole must consider as a theoretical possibility, although it shouldn’t be possible to actually occur.
    ryanofsky commented at 3:06 pm on April 14, 2025:

    re: #30214 (review)

    commit 24aaf0d: Going from a 3-state enum to a struct of two bools means that one of the 4 possible combinations (validated = false, historical = true) is now something users of ChainstateRole must consider as a theoretical possibility, although it shouldn’t be possible to actually occur.

    It would be more accurate to say that state doesn’t occur than that it shouldn’t occur. The point of this change is to only give clients (wallet & indexes) chainstate information they actually care about instead of making them deal with assumeutxo design and implementation details.

    Specifically indexes care (for now) whether block notifications pertain to blocks that have been fully validated from genesis since the indexes don’t currently use snapshot data, while wallets do work with snapshots and only care about whether notifications pertain to the most-work chainstate. So this is information the API now directly provides instead of assumeutxo states that wallets and indexes had to make assumptions and inferences about.

    A (validated = false, historical = true) chainstate could also be possible and even useful if you had a previous utxo snapshot that you wanted to use to generate a new snapshot targetting a later block. Theoretically you’d want the node API and wallet and index code not to require any changes if a feature like this were added, which is now the case after this commit.

    fjahr commented at 11:56 pm on May 16, 2025:
    I’m currently not sure why we need both ChainValidity and ChainstateRole, couldn’t we have one thing that serves both purposes?
    ryanofsky commented at 3:19 pm on June 17, 2025:

    re: #30214 (review)

    I’m currently not sure why we need both ChainValidity and ChainstateRole, couldn’t we have one thing that serves both purposes?

    Yes, but this would be less ideal and lead to problems.

    It would be bad to replace the Chainstate::m_validity enum value with ChainstateRole because the ChainstateRole::historical value is redundant with the Chainstate::m_target_blockhash value, so the representation would be denormalized and both fields need to be kept in sync, possibly leading to bugs.

    It would be bad to do the opposite and replace ChainstateRole with ChainValidity in wallet and indexing code because ChainstateRole has the information they need to process notifications while ChainValidity does not have enough information.

    Specifically, the wallet code does not care whether chainstates are partially validated starting from a snapshot block or fully validated from the genesis block. So wallets should not be looking at ChainValidity or ChainstateRole::validated fields. The wallet only cares whether notifications come from the current chainstate or a historical one so they should only looking at the ChainstateRole::historical field to drop historical notifications.

    Indexing code could get away with only looking at ChainValidity since all the indexes currently need information starting from the genesis block, and ChainValidity::VALIDATED implies this. But indexing behavior is not ideal right now. More ideally indexes could use notifications from the current chainstate as well. It might even be useful for indexes to able to add new chainstates, for example to support partial indexing with the ability to index old states and not disrupt the node. Supporting these things would leave index code in the same position as wallet code where ChainValidity does not provide enough information. ChainstateRole by contrast provides all the information needed to distinguish chainstates currently and it could easily be extended to provide more information in the future.

    One change that might make sense to connect these two types is changing the ChainstateRole bool validated member to a ChainValidity validity member. But I didn’t implement that because that would make it possible to send notifications containing a ChainValidity::INVALID value which is not something validation code should do, or something wallet and indexes should need to check for and handle. Also the ChainValidity enum seems like something that could be replaced or extended in the future, so it would be nice if that could happen without needing to change index and wallet code.

  76. in src/validation.cpp:3889 in 46a44dcae0 outdated 
    3960@@ -3961,7 +3961,7 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd
3961             }
3962             pindex->m_chain_tx_count = prev_tx_sum(*pindex);
3963             pindex->nSequenceId = nBlockSequenceId++;
3964-            for (Chainstate *c : GetAll()) {
3965+            for (const auto& c : m_chainstates) {
    mzumsande commented at 9:33 pm on April 11, 2025:

    commit 46a44dcae073758d6808444ea33eb37acd0b1248 Replacing GetAll() by a loop over m_chainstates looks a bit dubious as a refactor to me. The latter also includes successfully validated chainstates and invalid chainstates, whereas the former doesn’t. I see that this is explicitly handled in some spots such as ActivateBestChains(), but not everywhere, so I wonder what the consequences of this are.

    In this spot, for example, it looks that we might add blocks to setBlockIndexCandidates for an invalid or validate chainstate. This should at least be discussed in the commit message in my opinion, even if the consequences wouldn’t be noticable.

    ryanofsky commented at 3:09 pm on April 14, 2025:

    re: #30214 (review)

    commit 46a44dc Replacing GetAll() by a loop over m_chainstates looks a bit dubious as a refactor to me. The latter also includes successfully validated chainstates and invalid chainstates, whereas the former doesn’t.

    The point getting rid of GetAll is to make code more explicit. If you want to skip invalid chainstates, you should explicitly skip them, not rely on a GetAll() method that includes and excludes chainstates in a complicated and arbitrary way. (And a misleading one, if you are a naive person who thinks “All” means all.)

    I see that this is explicitly handled in some spots such as ActivateBestChains(), but not everywhere, so I wonder what the consequences of this are.

    This is a refactoring commit, so if there are consequences this is a bug.

    In this spot, for example, it looks that we might add blocks to setBlockIndexCandidates for an invalid or validate chainstate. This should at least be discussed in the commit message in my opinion, even if the consequences wouldn’t be noticable.

    Thanks, I think it’d be good to add something like if (Validity() = ChainValidity::INVALID) return; to Chainstate::TryAddBlockIndexCandidate with a comment to make it more obvious no behavior is changing here. This could also be a future-proofing mechanism in case in the future we wanted to handle invalid snapshots in a different way that didn’t trigger an immediate shutdown.

    ryanofsky commented at 5:45 pm on April 16, 2025:

    re: #30214 (review)

    Thanks, I think it’d be good to add something like if (Validity() = ChainValidity::INVALID) return; to Chainstate::TryAddBlockIndexCandidate with a comment to make it more obvious no behavior is changing here.

    This is now done in the latest push (https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2810282674)

  77. ryanofsky commented at 3:11 pm on April 14, 2025: contributor
    Thanks for the reviews! I still planning to address the fuzz/clang-tidy comments, and will also update TryAddBlockIndexCandidate as mentioned below to clarify the GetAll() removal commit.
  78. DrahtBot added the label Needs rebase on Apr 14, 2025
  79. ryanofsky force-pushed on Apr 16, 2025
  80. ryanofsky commented at 5:28 pm on April 16, 2025: contributor

    Rebased abe6f0642493402d858dc3aa297ec941bfeaaf87 -> feaf9fc6d15a9f06470a94423bb9e167b8657652 (pr/cstate.13 -> pr/cstate.14, compare) due to conflict with #31282, also running clang-tidy and fixing the early compile error in the fuzz test

    re: #30214#pullrequestreview-2749329967

    Last changes look good, but I think the interim compilation error in the fuzz binary should be fixed. There are some formatting and include ordering issues, can you run clang-format-diff over the commits?

    Thanks for pointing this out! These should all be fixed now

  81. ryanofsky force-pushed on Apr 16, 2025
  82. ryanofsky commented at 5:46 pm on April 16, 2025: contributor
    Updated feaf9fc6d15a9f06470a94423bb9e167b8657652 -> 12f276ba2bfc106083c1615539857cdefff9794b (pr/cstate.14 -> pr/cstate.15, compare) adding an extra validity check to address a comment #30214 (review)
  83. DrahtBot removed the label Needs rebase on Apr 16, 2025
  84. in src/validation.cpp:3902 in 12f276ba2b outdated 
    3894@@ -3867,23 +3895,30 @@ void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) {
3895 void Chainstate::TryAddBlockIndexCandidate(CBlockIndex* pindex)
3896 {
3897     AssertLockHeld(cs_main);
3898+
3899+    // Do not continue building a chainstate that is based on an invalid
3900+    // snapshot. This is a belt-and-suspenders type of check because if an
3901+    // invalid snapshot is loaded, the node will shut down to force a manual
3902+    // intervention. But it is good handle this case correctly regardless.
    sedited commented at 8:11 pm on April 16, 2025:
    Nit: s/But it is good handle/But it is good to handle.
    ryanofsky commented at 2:15 pm on June 17, 2025:

    re: #30214 (review)

    Thanks fixed typo now

  85. sedited approved
  86. sedited commented at 8:12 pm on April 16, 2025: contributor
    ACK 12f276ba2bfc106083c1615539857cdefff9794b
  87. DrahtBot requested review from fjahr on Apr 16, 2025
  88. DrahtBot requested review from mzumsande on Apr 16, 2025
  89. DrahtBot added the label Needs rebase on Apr 22, 2025
  90. in src/validation.h:1116 in 1735fb6169 outdated 
    1128@@ -1106,22 +1129,19 @@ class ChainstateManager
1129     //! Returns nullptr if no snapshot has been loaded.
1130     const CBlockIndex* GetSnapshotBaseBlock() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
1131 
1132+    //! Return historical chainstate targeting a specific block, if any.
1133+    Chainstate* HistoricalChainstate() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex())
    fjahr commented at 2:19 pm on May 16, 2025:
    Are we then commited to the ‘historical’ naming now with this and I can remove all the remaining ibd/background references in a follow-up? Because that naming inconsistency has always annoyed me and now we have three names with this. I think it’s very confusing for newcomers as well.
    ryanofsky commented at 7:07 pm on June 17, 2025:

    re: #30214 (review)

    Are we then commited to the ‘historical’ naming now with this and I can remove all the remaining ibd/background references in a follow-up? Because that naming inconsistency has always annoyed me and now we have three names with this. I think it’s very confusing for newcomers as well.

    I think so and the last commit “doc: Improve ChainstateManager documentation, use consistent terms” was started to do this and I should just complete it.

  91. in src/validation.cpp:6073 in 55b7bfba6d outdated 
    6125-        m_ibd_chainstate->Validity() != ChainValidity::VALIDATED ||
6126-        !m_ibd_chainstate->m_target_blockhash ||
6127-        *m_ibd_chainstate->m_target_blockhash != *m_snapshot_chainstate->m_from_snapshot_blockhash ||
6128-        !m_ibd_chainstate->m_chain.Tip() ||
6129-        !m_ibd_chainstate->ReachedTarget()) {
6130+    if (from_snapshot_chainstate.Validity() != ChainValidity::ASSUMED_VALID ||
    fjahr commented at 2:29 pm on May 16, 2025:
    nit: Would be really great if this conditional could be broken up to be more readable. Or maybe move comments below inline. But feel free to leave this for a follow-up.
    fjahr commented at 2:36 pm on May 16, 2025:
    (not directly related to this line but I have to put it somewhere) Looks like commit 83d19fd09818e11a03b9ebdbfbf76196f3808ae2 just contains some formatting fixes and has the same commit message as the commit before 55b7bfba6d12a95a6cccb3652c75dc9cee2f8751 , so they were probably meant to be squashed.
    ryanofsky commented at 2:17 pm on June 17, 2025:

    re: #30214 (review)

    Thanks, fixed unsquashed commits now

    ryanofsky commented at 3:45 pm on June 17, 2025:

    re: #30214 (review)

    nit: Would be really great if this conditional could be broken up to be more readable. Or maybe move comments below inline. But feel free to leave this for a follow-up.

    Thanks, reworked this to minimize differences from previous code and add inline comments.

  92. fjahr commented at 0:00 am on May 17, 2025: contributor
    Leaving some comments in case you want to address some of them with the rebase. I am mostly happy but there are a few things that I need to spend some more time on.
  93. DrahtBot requested review from fjahr on May 17, 2025
  94. ryanofsky force-pushed on Jun 17, 2025
  95. ryanofsky commented at 7:14 pm on June 17, 2025: contributor

    Thanks for the reviews! I think this PR is in a pretty good state but I need to complete the last commit to make the terminology more consistent.

    Rebased 12f276ba2bfc106083c1615539857cdefff9794b -> a0672af4c75b9edcf656c5dc43aca6cb930bb165 (pr/cstate.15 -> pr/cstate.16, compare) due to various conflicts, implementing various suggestions. Updated a0672af4c75b9edcf656c5dc43aca6cb930bb165 -> f6bb0803fb6d6de5bbde84343ac14df211ed17e3 (pr/cstate.16 -> pr/cstate.17, compare) fixing clang-tidy error and renaming some variables for more consistent naming

  96. DrahtBot removed the label Needs rebase on Jun 17, 2025
  97. DrahtBot added the label CI failed on Jun 17, 2025
  98. DrahtBot commented at 9:43 pm on June 17, 2025: contributor

    🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/44286138120 LLM reason (✨ experimental): The CI failure is caused by clang-tidy reporting an unused using declaration, which is treated as an error due to -warnings-as-errors.

    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.

  99. ryanofsky force-pushed on Jun 18, 2025
  100. DrahtBot removed the label CI failed on Jun 18, 2025
  101. in src/node/chainstate.cpp:173 in 9c0ecea4c4 outdated 
    167@@ -168,15 +168,16 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
168     Chainstate& validated_cs{chainman.InitializeChainstate(options.mempool)};
169 
170     // Load a chain created from a UTXO snapshot, if any exist.
171-    bool has_snapshot = chainman.DetectSnapshotChainstate();
172+    Chainstate* assumed_valid_cs{chainman.DetectAssumedValidChainstate()};
173 
174-    if (has_snapshot && options.wipe_chainstate_db) {
175+    if (assumed_valid_cs && options.wipe_chainstate_db) {
    sedited commented at 9:45 am on June 18, 2025:

    In commit 9c0ecea4c4df01f8ed3492e9a4f538fcef1e51df

    This new assumed_valid terminology seems confusing to me, because assume valid and assumeutxo have different semantics and different activation heights. Snapshot seemed clear to me tbh.

    ryanofsky commented at 12:30 pm on June 18, 2025:

    re: #30214 (review)

    This new assumed_valid terminology seems confusing to me, because assume valid and assumeutxo have different semantics and different activation heights. Snapshot seemed clear to me tbh.

    Thanks, I’m experimenting with the names a little and not committed to this. The name is mostly used for local variables in a few places and the idea was to make the local variable names match the ChainValidity case names:

    0enum class ChainValidity {
1    //! Every block in this chain has been validated.
2    VALIDATED,
3    //! Only blocks after an assumetxo snapshot have been validated, and the snapshot has not yet been validated.
4    ASSUMED_VALID,
5    //! Only blocks after an assumetxo snapshot have been validated, and the snapshot failed validation.
6    INVALID,
7};

    where validated_cs has VALIDATED state and assumed_valid_cs has ASSUMED_VALID state. I’m thinking the consistency is good but maybe the enum case names are bad. Maybe something like VALIDATED/ASSUMEUTXO/INVALID would be better?

    The name “snapshot” always seemed confusing and misleading to me because I’d expect a “snapshot” object to hold the state at the point of a snapshot. And more generally I’d expect validation state to reflect what level of validation has been done, not what mechanism was used to load the data.

    sedited commented at 12:51 pm on June 18, 2025:
    It sounds a bit clunky to my ears, but how about ASSUMED_UTXO instead? That seems to at least accurately convey what it is and that it is unrelated to assumed valid.
  102. ryanofsky force-pushed on Jun 18, 2025
  103. ryanofsky commented at 2:48 pm on June 18, 2025: contributor

    Updated f6bb0803fb6d6de5bbde84343ac14df211ed17e3 -> e6abe3ae66fc940ab826ac4fd6908c0724870f65 (pr/cstate.17 -> pr/cstate.18, compare) to clean up a few things and avoid ASSUMED_VALID / assumed_valid everywhere since that sounds too much like assumevalid (https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2154165371).

    Updated e6abe3ae66fc940ab826ac4fd6908c0724870f65 -> 32c89eeb113ad3bff4037f661a8430837c942ed2 (pr/cstate.18 -> pr/cstate.19, compare) with more small cleanups

    Rebased 32c89eeb113ad3bff4037f661a8430837c942ed2 -> 48e7e82808da43a52e31fb3ff95cdf763c077c20 (pr/cstate.19 -> pr/cstate.20, compare)

    Rebased 48e7e82808da43a52e31fb3ff95cdf763c077c20 -> 5be5a45847f710451a74aed10f6e4efcbace4b45 (pr/cstate.20 -> pr/cstate.21, compare) due to conflicts with #33336

    Rebased 5be5a45847f710451a74aed10f6e4efcbace4b45 -> be1af13426e0d4c7aa1da0eb78f15cf1fb166ba2 (pr/cstate.21 -> pr/cstate.22, compare) due to conflicts with #30595

  104. ryanofsky force-pushed on Jun 18, 2025
  105. in src/validation.cpp:6412 in 32c89eeb11 outdated 
    6457-        // the snapshot chain.
6458-        prune_start = *Assert(GetSnapshotBaseHeight()) + 1;
6459+    if (m_from_snapshot_blockhash && m_assumeutxo != Assumeutxo::VALIDATED) {
6460+        // Only prune blocks _after_ the snapshot if this is a snapshot chain
6461+        // that has not been fully validated yet. The earlier blocks need to be
6462+        // kept to validate the snapshot
    sedited commented at 7:46 pm on June 22, 2025:
    Been thinking about this again, but getting less sure about the mechanics at play here. What is the scenario where the historical chainstate needs the blocks to validate the snapshot chainstate?
    ryanofsky commented at 12:50 pm on June 23, 2025:

    re: #30214 (review)

    In commit “refactor: Add Chainstate m_assumeutxo and m_target_utxohash members” (a416c6d1f3747dd586650432988bdcb32b81224e)

    Been thinking about this again, but getting less sure about the mechanics at play here. What is the scenario where the historical chainstate needs the blocks to validate the snapshot chainstate?

    This is confusing and it makes me realize the surrounding documentation here is not good. For example the GetPruneRange documentation doesn’t even mention the chainstate argument.

    But the idea is that GetPruneRange is called via FindFilesToPrune via FlushStateToDisk for just one chainstate at a time. And the function is supposed to return a range of blocks to prune for just that chainstate. Each chainstate basically owns a region of the chain and should only prune its own blocks, not other blocks.

    The historical chainstate is responsible for pruning blocks between genesis and the snapshot block (i.e. its target block) and the current chainstate is responsible for pruning blocks after the snapshot block it was created from. Neither chainstate should be pruning blocks it did not validate. In practice, if this check was not here it would probably cause historical blocks to be pruned before they could be validated.

    This whole pruning model doesn’t generalize and is probably unnecessarily confusing. It would probably be better if each chainstate returned the range of blocks it didn’t want pruned and then the blockmanager was given all of those ranges and figured out which block to prune based on the ranges. A change like that could make sense as a followup.

  106. DrahtBot added the label Needs rebase on Jul 25, 2025
  107. ryanofsky force-pushed on Oct 15, 2025
  108. DrahtBot removed the label Needs rebase on Oct 15, 2025
  109. in src/validation.h:526 in 48e7e82808 outdated 
    521+enum class Assumeutxo {
522+    //! Every block in the chain has been validated.
523+    VALIDATED,
524+    //! Blocks after an assumetxo snapshot have been validated but the snapshot itself has not been validated.
525+    UNVALIDATED,
526+    //! The assumetxo snapshot failed validation.
    maflcko commented at 1:29 pm on October 16, 2025:

    llm nits:

    assumetxo -> assumeutxo [spelling mistake in comments; inconsistent with the rest of the code/documentation]
assumetxo -> assumeutxo [same misspelling repeated elsewhere in the added enum comments]
contains database -> contains a database [missing article makes the sentence ungrammatical / harder to parse]
    maflcko commented at 2:58 pm on December 7, 2025: 
    0assumetxo -> assumeutxo [spelling mistake in comments; inconsistent with the rest of the code/documentation]
    ryanofsky commented at 10:33 pm on December 9, 2025:

    re: #30214 (review)

    llm nits:

    Thanks, fixed

  110. sedited approved
  111. sedited commented at 10:30 am on October 23, 2025: contributor
    Re-ACK 48e7e82808da43a52e31fb3ff95cdf763c077c20
  112. DrahtBot added the label Needs rebase on Oct 24, 2025
  113. ryanofsky force-pushed on Oct 28, 2025
  114. sedited commented at 3:01 pm on October 28, 2025: contributor
    Re-ACK 5be5a45847f710451a74aed10f6e4efcbace4b45
  115. DrahtBot removed the label Needs rebase on Oct 28, 2025
  116. DrahtBot added the label Needs rebase on Nov 4, 2025
  117. ryanofsky force-pushed on Nov 12, 2025
  118. DrahtBot removed the label Needs rebase on Nov 12, 2025
  119. sedited approved
  120. sedited commented at 10:26 am on November 27, 2025: contributor
    Re-ACK be1af13426e0d4c7aa1da0eb78f15cf1fb166ba2
  121. in src/validation.h:637 in 0cb19b78cc outdated 
    622@@ -620,13 +623,39 @@ class Chainstate
623      */
624     const std::optional<uint256> m_from_snapshot_blockhash;
625 
626+    //! Target block for this chainstate. If this is not set, chainstate will
627+    //! target the most-work, valid block. If this is set, ChainstateManager
628+    //! considers this a "historical" chainstate since it will only contain old
629+    //! blocks up to the target block, not newer blocks.
630+    std::optional<uint256> m_target_blockhash GUARDED_BY(::cs_main);
    fjahr commented at 1:00 pm on November 27, 2025:

    in 0cb19b78cc297ceca591ee8be6f63b4ea7b494cc

    nit: Might have been possible to get to a naming that makes these two values even more aligned. Something like m_snapshot_base_blockhash and m_historical_target_blockhash. I guess in general I would prefer if the “Target” functions would be named “HistoricalTarget” so it’s clear these are assumeutxo functions. But it’s also a bit too verbose for my taste. So feel free to ignore.

    ryanofsky commented at 10:18 pm on December 9, 2025:

    re: #30214 (review)

    nit: Might have been possible to get to a naming that makes these two values even more aligned. Something like m_snapshot_base_blockhash and m_historical_target_blockhash. I guess in general I would prefer if the “Target” functions would be named “HistoricalTarget” so it’s clear these are assumeutxo functions. But it’s also a bit too verbose for my taste. So feel free to ignore.

    Not sure about this, but as you say these renaming could be done as a scripted diff later if it would be helpful. I think the suggested naming might conflate concepts from ChainstateManager and Chainstate levels. The idea behind adding the m_target_blockhash member is that validation code updating the chainstates should not need to care how higher level code uses the chainstates. I think “current” and “historical” concepts only make sense at the ChainstateManager level and above, not the Chainstate level. It makes more sense to refer to “base” and “target” blocks at the Chainstate level, and generally avoid referring to snapshots and historical chainstates there.

  122. in src/test/validation_chainstatemanager_tests.cpp:492 in 1107e04328 outdated 
    488@@ -489,8 +489,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
489     }
490 
491     // Note: cs2's tip is not set when ActivateExistingSnapshot is called.
492-    Chainstate& cs2 = WITH_LOCK(::cs_main,
493-        return chainman.ActivateExistingSnapshot(*assumed_base->phashBlock));
494+    Chainstate& cs2 = WITH_LOCK(::cs_main, return chainman.AddChainstate(std::make_unique<Chainstate>(nullptr, chainman.m_blockman, chainman, *assumed_base->phashBlock)));
    fjahr commented at 1:13 pm on November 27, 2025:

    in 1107e04328735abe636692b1a0179c6e46613d4e

    micro-nit: one of these you moved over to curly brace notation but not the others.

    ryanofsky commented at 10:34 pm on December 9, 2025:

    re: #30214 (review)

    micro-nit: one of these you moved over to curly brace notation but not the others.

    Thanks, switched to brace notation in the other places

  123. in src/validation.cpp:6183 in 21d638c92f outdated 
    6198         return SnapshotCompletionResult::MISSING_CHAINPARAMS;
6199     }
6200 
6201     const AssumeutxoData& au_data = *maybe_au_data;
6202-    std::optional<CCoinsStats> maybe_ibd_stats;
6203+    std::optional<CCoinsStats> maybe_validated_stats;
    fjahr commented at 1:20 pm on November 27, 2025:

    in 21d638c92f56f5ad4d32c47240700b41e8868789

    nit: How about maybe_historical_target_stats 🙈 It’s not validated until later and the maybe refers to std::optional afaict.

    maflcko commented at 11:46 am on December 8, 2025:

    It’s not validated until later and the maybe refers to std::optional afaict.

    I think the name is fine, because the stats (if they exist), are validated, because they are derived from the validated chainstate. But not strong opinion.

    ryanofsky commented at 11:10 pm on December 9, 2025:

    re: #30214 (review)

    nit: How about maybe_historical_target_stats 🙈 It’s not validated until later and the maybe refers to std::optional afaict.

    I think the name is technically correct for the reason Marco gave, but I do think it’s confusing, so renamed to validated_cs_stats

  124. in src/validation.h:1107 in a162863f27 outdated 
    1139@@ -1140,8 +1140,11 @@ class ChainstateManager
1140     //! if it does happen, it is a fatal error.
1141     SnapshotCompletionResult MaybeValidateSnapshot(Chainstate& validated_cs, Chainstate& unvalidated_cs) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
1142 
1143-    //! Returns nullptr if no snapshot has been loaded.
1144-    const CBlockIndex* GetSnapshotBaseBlock() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
1145+    //! Return current chainstate targeting the most-work, network tip.
1146+    Chainstate& CurrentChainstate() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex())
    fjahr commented at 1:28 pm on November 27, 2025:

    in a162863f2796cb4a5f15f2e80b8e51e7de9cfb50

    nit: For me “current” isn’t clearer than “active” and I’m very used to active, so I am kind of -0 on this particular name change. Maybe MostWorkChainstate or TipChainstate would make explicit what is currently written in the comment.

    maflcko commented at 11:45 am on December 8, 2025:

    TipChainstate

    Not sure this is clearer. All chainstates have a tip, each. So it would be unclear, which tip-chainstate this refers to :sweat_smile:

    ryanofsky commented at 11:24 pm on December 9, 2025:

    re: #30214 (review)

    nit: For me “current” isn’t clearer than “active” and I’m very used to active, so I am kind of -0 on this particular name change. Maybe MostWorkChainstate or TipChainstate would make explicit what is currently written in the comment.

    Interesting. I feel pretty strongly that “active” is a vague and misleading name because it implies other chainstates are not active, even though both chainstates are actively being validated and having blocks attached, and both are used for applications. The current chainstate is used as the source of data for wallets and RPCs, and the historical chainstate is used as the source of data for indexes. The name “current” reflects that this chainstate is targeting the latest block as opposed to a historical one.

    MostWork could also be ok but I don’t think it’s as good as Current because MostWork is not the opposite of Historical. MostWork also sounds like it refers blocks already in the chainstate, when the relevant distinction is not which blocks are in the chainstate, but which block the chainstate is targeting.

  125. in doc/design/assumeutxo.md:66 in 21d638c92f outdated 
    62@@ -63,7 +63,7 @@ chainstate and a sync to tip begins. A new chainstate directory is created in th
63 datadir for the snapshot chainstate called `chainstate_snapshot`.
64 
65 When this directory is present in the datadir, the snapshot chainstate will be detected
66-and loaded as active on node startup (via `DetectSnapshotChainstate()`).
67+and loaded as active on node startup (via `DetectAssumeutxoChainstate()`).
    fjahr commented at 3:18 pm on November 27, 2025:

    in 21d638c92f56f5ad4d32c47240700b41e8868789

    nit: DetectUnvalidatedChainstate would have been more consistent with the rest of the naming choices in this commit

    maflcko commented at 11:44 am on December 8, 2025:
    I think the current name is fine, but if we are allowed to bike-shed, then I’d replace Detect With Add, or Load, because the function does more than just read-only detection: LoadAssumeutxoChainstate().
    ryanofsky commented at 10:46 pm on December 9, 2025:

    re: #30214 (review)

    nit: DetectUnvalidatedChainstate would have been more consistent with the rest of the naming choices in this commit

    Good catch on the inconsistency, and also agree with Marco’s comment that “Load” would be better than “Detect”. I think validated_cs and unvalidated_cs names make sense internally within the MaybeValidateSnapshot function but “assumed-valid” or “assumeutxo” names are better ways to describe the chainstate in code responsible for loading the assumeutxo data. So went with LoadAssumeutxoChainstate and FindAssumeutxoChainstateDir and assumeutxo_cs outside of MaybeValidateSnapshot

  126. in src/validation.cpp:6313 in f85ff75dad outdated 
    6391-    m_active_chainstate = m_ibd_chainstate.get();
6392-    m_active_chainstate->m_mempool = m_snapshot_chainstate->m_mempool;
6393-    m_snapshot_chainstate.reset();
6394+    std::unique_ptr<Chainstate> prev_chainstate{RemoveChainstate(chainstate)};
6395+    Chainstate& curr_chainstate{CurrentChainstate()};
6396+    assert(prev_chainstate->m_mempool->size() == 0);
    fjahr commented at 4:33 pm on November 27, 2025:

    in f85ff75dad4ce8712fe65a636cf36d57b4066785

    If RemoveChainstate returned a nullptr this would crash. Maybe not a scenario that can be realistically hit currently but might still be good to handle it explicitly here or by changing RemoveChainstate.

    maflcko commented at 11:43 am on December 8, 2025:
    I think this is just a refactor, and if a nullptr existed here, it would already exist on current master?
    ryanofsky commented at 11:30 pm on December 9, 2025:

    re: #30214 (review)

    If RemoveChainstate returned a nullptr this would crash. Maybe not a scenario that can be realistically hit currently but might still be good to handle it explicitly here or by changing RemoveChainstate.

    Thanks, added assert here since RemoveChainstate isn’t asserting. It is true that this condition should never happen because this function is called with an existing chainstate reference, but maybe some future code or a test could call it incorrectly.

  127. in src/validation.h:1330 in d4cbeaff4a outdated 
    1332@@ -1333,6 +1333,8 @@ class ChainstateManager
1333     //! Get range of historical blocks to download.
1334     std::optional<std::pair<const CBlockIndex*, const CBlockIndex*>> GetHistoricalBlockRange() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
1335 
1336+    util::Result<void> ActivateBestChains() LOCKS_EXCLUDED(::cs_main);
    fjahr commented at 4:35 pm on November 27, 2025:

    in d4cbeaff4ae82203b588c3d5b9da011e4bb644d2

    nit: Could have added a comment

    ryanofsky commented at 11:35 pm on December 9, 2025:

    re: #30214 (review)

    nit: Could have added a comment

    Good catch, added documentation comment

  128. fjahr commented at 5:38 pm on November 27, 2025: contributor

    tACK be1af13426e0d4c7aa1da0eb78f15cf1fb166ba2

    Tested this with my own Signet snapshot and with and without pruning.

    Some of the renamings could be even more descriptive for my taste but this is a big step forward, so no need to address my nits unless you really want to. If others like them they could still be applied in a simple scripted-diff follow-up.

  129. fjahr commented at 11:33 pm on November 27, 2025: contributor
    Had another thought: since there are quite a bit of small-ish helper functions refactored or replaced with different ones, it might be interesting to check a coverage report if there are any of these new functions that aren’t covered and that coverage didn’t go down somewhere in general. I didn’t have time to look into this myself yet though.
  130. in src/validation.cpp:1966 in 0cb19b78cc outdated 
    1961+    if (!m_target_blockhash) return nullptr;
1962+    if (!m_cached_target_block) m_cached_target_block = Assert(m_chainman.m_blockman.LookupBlockIndex(*m_target_blockhash));
1963+    return m_cached_target_block;
1964+}
1965+
1966+void Chainstate::SetTargetBlock(CBlockIndex* block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
    maflcko commented at 7:54 am on November 28, 2025:
    0cb19b78cc297ceca591ee8be6f63b4ea7b494cc: Please don’t add lock annotations to the cpp file. They won’t be visible anyway. You’ll have to add them to the .h file.
    ryanofsky commented at 10:12 pm on December 9, 2025:

    re: #30214 (review)

    Please don’t add lock annotations to the cpp file

    Thanks, removed. (These were in the header file too and were probably just copied and pasted.)

  131. in src/validation.cpp:6353 in 1107e04328 outdated 
    6354     // Set target block for historical chainstate to snapshot block.
6355     assert(m_ibd_chainstate.get());
6356     assert(!m_ibd_chainstate->m_target_blockhash);
6357-    m_ibd_chainstate->SetTargetBlockHash(base_blockhash);
6358+    assert(m_snapshot_chainstate->m_from_snapshot_blockhash);
6359+    m_ibd_chainstate->SetTargetBlockHash(*Assert(m_snapshot_chainstate->m_from_snapshot_blockhash));
    maflcko commented at 1:21 pm on November 28, 2025:
    nit in 1107e04328735abe636692b1a0179c6e46613d4e: Seems a bit odd to call the exact same assert twice
    ryanofsky commented at 10:35 pm on December 9, 2025:

    re: #30214 (review)

    Seems a bit odd to call the exact same assert twice

    Good catch, removed the first one.

  132. DrahtBot added the label Needs rebase on Dec 2, 2025
  133. in src/validation.cpp:3910 in f85ff75dad outdated 
    3906@@ -3907,7 +3907,7 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd
3907     // m_chain_tx_count value is not zero, assert that value is actually correct.
3908     auto prev_tx_sum = [](CBlockIndex& block) { return block.nTx + (block.pprev ? block.pprev->m_chain_tx_count : 0); };
3909     if (!Assume(pindexNew->m_chain_tx_count == 0 || pindexNew->m_chain_tx_count == prev_tx_sum(*pindexNew) ||
3910-                pindexNew == CurrentChainstate().SnapshotBase())) {
3911+                std::any_of(m_chainstates.begin(), m_chainstates.end(), [&](auto& cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->SnapshotBase() == pindexNew; }))) {
    maflcko commented at 2:00 pm on December 7, 2025:

    nit f85ff75dad4ce8712fe65a636cf36d57b4066785: use ranges::any_of, and const auto&?

    Also, in the commit message: is not just ambiguous because actively confusing -> is not just ambiguous but actively confusing

    ryanofsky commented at 11:28 pm on December 9, 2025:

    re: #30214 (review)

    use ranges::any_of, and const auto&?

    Make sense, applied change.

  134. in src/validation.cpp:5476 in f85ff75dad outdated 
    5478+                        // targeting a specific block, pindex must be in
5479+                        // setBlockIndexCandidates. Otherwise, pindex only
5480+                        // needs to be added if it is an ancestor of the target
5481+                        // block.
5482+                        if (!c->TargetBlock() || c->TargetBlock()->GetAncestor(pindex->nHeight) == pindex) {
5483+                            assert(c->setBlockIndexCandidates.count(const_cast<CBlockIndex*>(pindex)));
    maflcko commented at 2:01 pm on December 7, 2025:
    nit https://github.com/bitcoin/bitcoin/commit/f85ff75dad4ce8712fe65a636cf36d57b4066785: Any reason to revert contains to count?
    ryanofsky commented at 11:28 pm on December 9, 2025:

    re: #30214 (review)

    nit f85ff75: Any reason to revert contains to count?

    Nope, switched to contains.

  135. in src/validation.cpp:5516 in f85ff75dad outdated 
    5511@@ -5512,12 +5512,10 @@ void ChainstateManager::CheckBlockIndex() const
5512             //    tip.
5513             // So if this block is itself better than any m_chain.Tip() and it wasn't in
5514             // setBlockIndexCandidates, then it must be in m_blocks_unlinked.
5515-            for (const Chainstate* c : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) {
5516-                if (!c) continue;
5517-                const bool is_active = c == &ActiveChainstate();
5518-                if (!CBlockIndexWorkComparator()(pindex, c->m_chain.Tip()) && !c->setBlockIndexCandidates.contains(const_cast<CBlockIndex*>(pindex))) {
5519+            for (const auto& c : m_chainstates) {
5520+                if (!CBlockIndexWorkComparator()(pindex, c->m_chain.Tip()) && c->setBlockIndexCandidates.count(const_cast<CBlockIndex*>(pindex)) == 0) {
    maflcko commented at 2:01 pm on December 7, 2025:
    nit https://github.com/bitcoin/bitcoin/commit/f85ff75dad4ce8712fe65a636cf36d57b4066785: Any reason to revert contains to count?
    ryanofsky commented at 11:28 pm on December 9, 2025:

    re: #30214 (review)

    nit f85ff75: Any reason to revert contains to count?

    Nope, switched to contains.

  136. in src/validation.h:1345 in f85ff75dad outdated 
    1340@@ -1359,6 +1341,15 @@ class ChainstateManager
1341     CCheckQueue<CScriptCheck>& GetCheckQueue() { return m_script_check_queue; }
1342 
1343     ~ChainstateManager();
1344+
1345+    //! List of chainstates. Note: in general, it is not safe delete Chainstate
    maflcko commented at 2:57 pm on December 7, 2025:
    f85ff75dad4ce8712fe65a636cf36d57b4066785: to delete (typo)
    ryanofsky commented at 11:34 pm on December 9, 2025:

    re: #30214 (review)

    to delete (typo)

    Thanks, fixed

  137. in src/validation.h:912 in be1af13426 
    928- *
929- * *IBD chainstate*: a chainstate whose current state has been "fully"
930- *   validated by the initial block download process.
931+ * Interface for managing multiple \ref Chainstate objects, where each
932+ * chainstate is associated with chainstate* subdirectory in the data directory
933+ * and contains database of UTXOs existing at a different point in history. (See
    maflcko commented at 3:08 pm on December 7, 2025:
    contains database -> contains a database [missing article makes the sentence ungrammatical / harder to parse]
    ryanofsky commented at 11:36 pm on December 9, 2025:

    re: #30214 (review)

    contains database -> contains a database [missing article makes the sentence ungrammatical / harder to parse]

    Thanks, fixed

  138. in src/validation.cpp:1987 in be1af13426 
    1982+        m_target_blockhash.reset();
1983+    }
1984+    m_cached_target_block = block;
1985+}
1986+
1987+void Chainstate::SetTargetBlockHash(uint256 block_hash) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
    maflcko commented at 10:09 am on December 8, 2025:
    0cb19b7: Please don’t add lock annotations to the cpp file. They won’t be visible anyway. You’ll have to add them to the .h file.
    ryanofsky commented at 10:13 pm on December 9, 2025:

    re: #30214 (review)

    Please don’t add lock annotations to the cpp file.

    Thanks, removed. (These were in the header file too and were probably just copied and pasted.)

  139. in src/validation.cpp:3468 in 811bb3db84 outdated 
    3467-    if (WITH_LOCK(::cs_main, return m_disabled)) {
3468-        LogPrintf("m_disabled is set - this chainstate should not be in operation. "
3469+    // Belt-and-suspenders check that we aren't attempting to advance the
3470+    // chainstate past the target block.
3471+    if (WITH_LOCK(::cs_main, return m_target_utxohash)) {
3472+        LogPrintf("m_target_utxohash is set - this chainstate should not be in operation. "
    maflcko commented at 10:31 am on December 8, 2025:

    nit in 811bb3db84a42799ef7d263ba761ba44e8156fbb: Could use error logging for internal bugs, and possibly add an Assume(false) on top?

    0LogError("%s", STR_INTERNAL_BUG("m_target_utxohash is set - this chainstate should not be in operation."));
1Assume(false);
    ryanofsky commented at 10:32 pm on December 9, 2025:

    re: #30214 (review)

    Could use error logging for internal bugs, and possibly add an Assume(false) on top?

    Nice, added this.

  140. in src/validation.cpp:6042 in be1af13426 outdated 
    6125-        // Background IBD not complete yet.
6126-        return SnapshotCompletionResult::SKIPPED;
6127+    // If the snapshot does not need to be validated...
6128+    if (unvalidated_cs.m_assumeutxo != Assumeutxo::UNVALIDATED ||
6129+            // Or if either chainstate is unusable...
6130+            !unvalidated_cs.m_from_snapshot_blockhash ||
    maflcko commented at 11:05 am on December 8, 2025:
    q: Is there a code-path where the unvalidated chainstate has no snapshot block set? Could make sense to use !Assume(unvalidated_cs.m_from_snapshot_blockhash) here?
    ryanofsky commented at 10:53 pm on December 9, 2025:

    #30214 (review)

    q: Is there a code-path where the unvalidated chainstate has no snapshot block set?

    It think it can probably happen on startup even if it shouldn’t happen normally. MaybeValidateSnapshot is called on startup and whenever a new block is connected, and I don’t think we want callers to have to check condition of the chainstates before calling it, when it can check them itself and perform the validation if appropriate, and return SKIPPED otherwise.

  141. in src/validation.cpp:6044 in be1af13426 outdated 
    6127+    // If the snapshot does not need to be validated...
6128+    if (unvalidated_cs.m_assumeutxo != Assumeutxo::UNVALIDATED ||
6129+            // Or if either chainstate is unusable...
6130+            !unvalidated_cs.m_from_snapshot_blockhash ||
6131+            validated_cs.m_assumeutxo != Assumeutxo::VALIDATED ||
6132+            !validated_cs.m_chain.Tip() ||
    maflcko commented at 11:06 am on December 8, 2025:
    q: Is there a code-path where the validated chainstate has no tip set? Could make sense to use !Assume(validated_cs.m_chain.Tip()) here?
    ryanofsky commented at 10:56 pm on December 9, 2025:

    re: #30214 (review)

    q: Is there a code-path where the validated chainstate has no tip set?

    See #30214 (review), but I think it’s best if this just returns SKIPPED if it can’t validate, instead of making too many assumptions about how it will be called. Since this is called on startup, it’s hard to know what state the chainstates will be in when it is called.

  142. in src/validation.cpp:6046 in be1af13426 outdated 
    6129+            // Or if either chainstate is unusable...
6130+            !unvalidated_cs.m_from_snapshot_blockhash ||
6131+            validated_cs.m_assumeutxo != Assumeutxo::VALIDATED ||
6132+            !validated_cs.m_chain.Tip() ||
6133+            // Or the validated chainstate is not targeting the snapshot block...
6134+            !validated_cs.m_target_blockhash ||
    maflcko commented at 11:09 am on December 8, 2025:
    q: Is there a code-path where the validated chainstate has no target blockash set when an unvalidated chainstate exists? Could make sense to use !Assume(validated_cs.m_target_blockhash) here?
    ryanofsky commented at 10:58 pm on December 9, 2025:

    re: #30214 (review)

    q: Is there a code-path where the validated chainstate has no target blockash set when an unvalidated chainstate exists? Could make sense to use !Assume(validated_cs.m_target_blockhash) here?

    Possibly, but see #30214 (review) and #30214 (review) for more rationale

    maflcko commented at 8:36 am on December 10, 2025:

    re #30214 (review)

    i’d say assumes are fine to detect logic bugs or storage corruption, but no strong opinion, if the goal is to somehow make this more flexible for future changes

    ryanofsky commented at 12:46 pm on December 12, 2025:

    re: #30214 (review)

    i’d say assumes are fine to detect logic bugs or storage corruption, but no strong opinion, if the goal is to somehow make this more flexible for future changes

    Yeah I think it needs to be up to callers to check their own expected states or to check the return status of this function. Having this function make unnecessary assumptions about particular states it thinks it will be called in seems like it could be easily break if any outside code changes

  143. maflcko commented at 11:41 am on December 8, 2025: member

    needs rebase, otherwise this looks good.

    left some nits/questions, but those can be ignored.

    review ACK be1af13426e0d4c7aa1da0eb78f15cf1fb166ba2 💐

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK be1af13426e0d4c7aa1da0eb78f15cf1fb166ba2 💐
3tEVAlSI74587DOpIAcL5S2nTSMPqVt5M5BH+ebh2w27hPD9PctnGNMoQfKAnL7usnU/pNoYdlI9WAiSZpquaCA==
  144. in src/interfaces/chain.h:396 in cb3e7af4ed outdated 
    390@@ -391,7 +391,9 @@ class Chain
391     //! removed transactions and already added new transactions.
392     virtual void requestMempoolTransactions(Notifications& notifications) = 0;
393 
394-    //! Return true if an assumed-valid chain is in use.
395+    //! Return true if an assumed-valid snapshot is in use. Note that this
396+    //! returns true even after the snapshot is validated, until the next node
397+    //! restart.
    maflcko commented at 3:21 pm on December 9, 2025:
    q in cb3e7af4ed04295e9928c2b60d8ab4ba64c4efd5: I wonder if this should be fixed in a follow-up, because the comment in AttachChain says that the loop called when hasAssumedValidChain is true may be slow.
    ryanofsky commented at 12:38 pm on December 12, 2025:

    re: #30214 (review)

    q in cb3e7af: I wonder if this should be fixed in a follow-up, because the comment in AttachChain says that the loop called when hasAssumedValidChain is true may be slow.

    Yes it could be nice to change this and make loading old wallets on non-pruned nodes faster, even though it’s a little bit of an edge case

  145. ryanofsky force-pushed on Dec 9, 2025
  146. ryanofsky commented at 11:43 pm on December 9, 2025: contributor

    Thanks for the thorough reviews! Rebased to avoid conflicts and made (minor) updates to address all the review comments.

    Rebased be1af13426e0d4c7aa1da0eb78f15cf1fb166ba2 -> 5dd7cbf675e7ce058ccb0666dabcb39a175ae83c (pr/cstate.22 -> pr/cstate.23, compare)

  147. DrahtBot removed the label Needs rebase on Dec 10, 2025
  148. in src/test/util/chainstate.h:84 in 6c98ddc87a 
    80@@ -81,7 +81,7 @@ CreateAndActivateUTXOSnapshot(
81             Chainstate& chain = node.chainman->ActiveChainstate();
82             Assert(chain.LoadGenesisBlock());
83             // These cache values will be corrected shortly in `MaybeRebalanceCaches`.
84-            chain.InitCoinsDB(1 << 20, true, false, "");
85+            chain.InitCoinsDB(1 << 20, true, false);
    maflcko commented at 8:33 am on December 10, 2025:

    llm nit in 6c98ddc87a895d9bbc392db8fedc0e8268ff1cdd: could use named args while touching?

    0            chain.InitCoinsDB(1 << 20, /*in_memory=*/true, /*should_wipe=*/false);
    ryanofsky commented at 12:38 pm on December 12, 2025:

    re: #30214 (review)

    llm nit in 6c98ddc: could use named args while touching?

    Makes sense, added.

  149. in src/validation.cpp:5759 in 6c98ddc87a 
    5755@@ -5752,7 +5756,7 @@ util::Result<CBlockIndex*> ChainstateManager::ActivateSnapshot(
5756         LOCK(::cs_main);
5757         snapshot_chainstate->InitCoinsDB(
5758             static_cast<size_t>(current_coinsdb_cache_size * SNAPSHOT_CACHE_PERC),
5759-            in_memory, false, "chainstate");
5760+            in_memory, false);
    maflcko commented at 8:35 am on December 10, 2025:

    llm nit in the same commit: name args?

    0/*should_wipe=*/false);
    ryanofsky commented at 12:39 pm on December 12, 2025:

    re: #30214 (review)

    llm nit in the same commit: name args?

    Thanks also added this

  150. maflcko commented at 8:35 am on December 10, 2025: member

    left some more nits and questions, but they can be ignored.

    re-review ACK 5dd7cbf675e7ce058ccb0666dabcb39a175ae83c 🍓

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-review ACK 5dd7cbf675e7ce058ccb0666dabcb39a175ae83c 🍓
3IaZbopyDWq1aMP/wH02dao+7gmjK58ndI88eFQbAAnTEhSjYE+BlIdQ6Wam8MWiXzHApxTBD8MdQga7spNLAAg==
  151. DrahtBot requested review from sedited on Dec 10, 2025
  152. DrahtBot requested review from fjahr on Dec 10, 2025
  153. sedited approved
  154. sedited commented at 8:59 am on December 10, 2025: contributor

    Re-ACK 5dd7cbf675e7ce058ccb0666dabcb39a175ae83c

    Rebased and addressed a bunch of nits since my last review.

  155. DrahtBot added the label Needs rebase on Dec 11, 2025
  156. test: Fix broken chainstatemanager_snapshot_init check 
    The following test code never checked anything because the if statement was
always false:

    if (cs != &chainman_restarted.ActiveChainstate()) {
        BOOST_CHECK_EQUAL(cs->m_chain.Height(), 109);
    }

Also, the height of the background chainstate it was intending to check is 110,
not 109. Fix both problems by rewriting the check.
    de00e87548
  157. refactor: Add Chainstate::m_target_blockhash member 
    Make Chainstate objects aware of what block they are targeting. This makes
Chainstate objects more self contained, so it's possible for validation code to
look at one Chainstate object and know what blocks to connect to it without
needing to consider global validation state or look at other Chainstate
objects.

The motivation for this change is to make validation and networking code more
readable, so understanding it just requires knowing about chains and blocks,
not reasoning about assumeutxo download states. This change also enables
simplifications to the ChainstateManager interface in subsequent commits, and
could make it easier to implement new features like creating new Chainstate
objects to generate UTXO snapshots or index UTXO data.

Note that behavior of the MaybeCompleteSnapshotValidation function is not
changing here but some checks that were previously impossible to trigger like
the BASE_BLOCKHASH_MISMATCH case have been turned into asserts.
    6082c84713
  158. refactor: Add Chainstate m_assumeutxo and m_target_utxohash members 
    Get rid of m_disabled/IsUsable members. Instead of marking chains disabled for
different reasons, store chainstate assumeutxo status explicitly and use that
information to determine how chains should be treated.
    9fe927b6d6
  159. refactor: Deduplicate Chainstate activation code 
    Move duplicate code from ChainstateManager::ActivateSnapshot and
ChainstateManager::ActivateExistingSnapshot methods to a new
ChainstateManager::AddChainstate method.

The "AddChainstate" method name doesn't mention snapshots even though it is
only used to add snapshot chainstates now, because it becomes more generalized
in a later commit in this PR ("refactor: Add ChainstateManager::m_chainstates
member")
    1598a15aed
  160. refactor: Pass chainstate parameters to MaybeCompleteSnapshotValidation 
    Remove hardcoded references to m_ibd_chainstate and m_snapshot_chainstate so
MaybeCompleteSnapshotValidation function can be simpler and focus on validating
the snapshot without dealing with internal ChainstateManager states.

This is a step towards being able to validate the snapshot outside of
ActivateBestChain loop so cs_main is not locked for minutes when the snapshot
block is connected.
    840bd2ef23
  161. refactor: Add Chainstate::StoragePath() method 
    Use to simplify code determining the chainstate leveldb paths. New method is
the now the only code that needs to figure out the storage path, so the path
doesn't need to be constructed multiple places and backed out of leveldb.
    a9b7f5614c
  162. refactor: Add ChainstateManager::CurrentChainstate() method 
    CurrentChainstate() is basically the same as ActiveChainstate() except it
requires cs_main to be locked when it is called, instead of locking cs_main
internally.

The name "current" should also be less confusing than "active" because multiple
chainstates can be active, and CurrentChainstate() returns the chainstate
targeting the current network tip, regardless of what chainstates are being
downloaded or how they are used.
    a229cb9477
  163. refactor: Add ChainstateManager::ValidatedChainstate() method 
    ValidatedChainstate() accessor replaces GetChainstateForIndexing() with no
change in behavior.
    352ad27fc1
  164. refactor: Convert ChainstateRole enum to struct 
    Change ChainstateRole parameter passed to wallets and indexes. Wallets and
indexes need to know whether chainstate is historical and whether it is fully
validated. They should not be aware of the assumeutxo snapshot validation
process.
    4dfe383912
  165. refactor: Delete ChainstateManager::IsSnapshotActive() method 
    IsSnapshotActive() method is only called one place outside of tests and
asserts, and is confusing because it returns true even after the snapshot is
fully validated.

The documentation which said this "implies that a background validation
chainstate is also in use" is also incorrect, because after the snapshot is
validated, the background chainstate gets disabled and IsUsable() would return
false.
    d9e82299fc
  166. refactor: Delete ChainstateManager::IsSnapshotValidated() method 
    IsSnapshotValidated() is only called one place outside of tests, and is use
redundantly in some tests, asserting that a snapshot is not validated when a
snapshot chainstate does not even exist. Simplify by dropping the method and
checking Chainstate m_assumeutxo field directly.
    ee35250683
  167. refactor: Delete ChainstateManager::SnapshotBlockhash() method 
    SnapshotBlockhash() is only called two places outside of tests, and is used
redundantly in some tests, checking the same field as other checks. Simplify by
dropping the method and using the m_from_snapshot_blockhash field directly.
    e514fe6116
  168. refactor: Add ChainstateManager::m_chainstates member 
    Use to replace m_active_chainstate, m_ibd_chainstate, and m_snapshot_chainstate
members. This has several benefits:

- Ensures ChainstateManager treats chainstates instances equally, making
  distinctions based on their attributes, not having special cases and making
  assumptions based on their identities.

- Normalizes ChainstateManager representation so states that should be
  impossible to reach and validation code has no handling for (like
  m_snapshot_chainstate being set and m_ibd_chainstate being unset, or both
  being set but m_active_chainstate pointing to the m_ibd_chainstate) can no
  longer be represented.

- Makes ChainstateManager more extensible so new chainstates can be added for
  different purposes, like indexing or generating and validating assumeutxo
  snapshots without interrupting regular node operations. With the
  m_chainstates member, new chainstates can be added and handled without needing
  to make changes all over validation code or to copy/paste/modify the existing
  code that's been already been written to handle m_ibd_chainstate and
  m_snapshot_chainstate.

- Avoids terms that are confusing and misleading:

  - The term "active chainstate" term is confusing because multiple chainstates
    will be active and in use at the same time. Before a snapshot is validated,
    wallet code will use the snapshot chainstate, while indexes will use the IBD
    chainstate, and netorking code will use both chainstates, downloading
    snapshot blocks at higher priority, but also IBD blocks simultaneously.

  - The term "snapshot chainstate" is ambiguous because it could refer either
    to the chainstate originally loaded from a snapshot, or to the chainstate
    being used to validate a snapshot that was loaded, or to a chainstate being
    used to produce a snapshot, but it is arbitrary used to refer the first
    thing. The terms "most-work chainstate" or "assumed-valid chainstate" should
    be less ambiguous ways to refer to chainstates loaded from snapshots.

  - The term "IBD chainstate" is not just ambiguous but actively confusing
    because technically IBD ends and the node is considered synced when the
    snapshot chainstate finishes syncing, so in practice the IBD chainstate
    will mostly by synced after IBD is complete. The term "fully-validated" is
    a better way of describing the characteristics and purpose of this
    chainstate.
    491d827d52
  169. refactor: Add ChainstateManager::ActivateBestChains() method 
    Deduplicate code looping over chainstate objects and calling
ActivateBestChain() and avoid need for code outside ChainstateManager to use
the GetAll() method.
    6a572dbda9
  170. refactor: Delete ChainstateManager::GetAll() method 
    Just use m_chainstates array instead.
    ae85c495f1
  171. refactor: Simplify pruning functions 
    Move GetPruneRange from ChainstateManager to Chainstate.
    af455dcb39
  172. doc: Improve ChainstateManager documentation, use consistent terms 82be652e40
  173. ryanofsky force-pushed on Dec 12, 2025
  174. maflcko commented at 12:41 pm on December 12, 2025: member

    Only change is rebase and fix of two LLM nits.

    re-review ACK 82be652e40ec7e1bea4b260ee804a92a3e05f496 🕍

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-review ACK 82be652e40ec7e1bea4b260ee804a92a3e05f496 🕍
3DCfUEEgxlpRFVrZNQ1J8RKhoRVJ+eV5+NOwbq0XgVloeD4T3M3orc/5G1cUGNM0b0D5Qc4CexDOp8iiv9lFvCg==
  175. DrahtBot requested review from sedited on Dec 12, 2025
  176. ryanofsky commented at 12:47 pm on December 12, 2025: contributor

    Thanks for the reviews!

    Rebased 5dd7cbf675e7ce058ccb0666dabcb39a175ae83c -> 82be652e40ec7e1bea4b260ee804a92a3e05f496 (pr/cstate.23 -> pr/cstate.24, compare) fixing conflict #32414 and also adding suggested arg name comments

  177. fjahr commented at 1:16 pm on December 12, 2025: contributor

    re-ACK 82be652e40ec7e1bea4b260ee804a92a3e05f496

    Confirmed that changes since last review were addressing nits and rebasing.

  178. DrahtBot removed the label Needs rebase on Dec 12, 2025
  179. DrahtBot added the label CI failed on Dec 12, 2025
  180. DrahtBot removed the label CI failed on Dec 12, 2025


ryanofsky DrahtBot fjahr sedited maflcko mzumsande


fjahr sedited mzumsande

Labels
Refactoring

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: 2025-12-13 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me