Addresses #34503. See this issue for more details as well.
Fixes a bug where, under certain conditions, setBlockIndexCandidates had blocks in it that were worse than the tip. The block index candidate set uses nSequenceId as a sort key, so modifying this field while blocks are in the set results in undefined behavior. This PR populates setBlockIndexCandidates after the nSequenceId modifications, avoiding the UB.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | sedited, sipa, achow101 |
| Concept ACK | Jhackman2019, stickies-v, mzumsande, stringintech |
| Stale ACK | dergoegge, fjahr |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
note: Technically we still have UB with this fix because we alter the sequence ids of the whole chain of blocks back from the tip, and all of those blocks are in setBlockIndexCandidate at the time. I guess we could just fully clear the candidate set, change the ids, and then only add the tip and anything with more work? That might be a technically more complete fix. Although its weird to me that we populate the set with the whole block tree just to clear/prune it later.
I was trying to move the initial population of the candidate set from LoadBlockIndex to LoadChainTip in this branch but a few functional tests were failing.
I have a branch that I believe addresses both the UB summarized above and the deeper one mentioned in #34521#pullrequestreview-3761920357.
I'm open to whatever reviewers think is better.
I think that there are two types of UB remaining:
For the one mentionend in #34521 (comment) I think we could simply move the PruneBlockIndexCandidates(); up too, before erasing/readding the tip. That way all ancestors of the tip would have been removed from setBlockIndexCandidates before they are adjusted.
However, the second type of UB goes deeper: With assumeutxo, we can have two Chainstates with a setBlockIndexCandidates each. That means, even if we remove/readd the tip of CS1 while calling LoadChainTip for it, this would still be UB because the same index would not have been removed from CS2's setBlockIndexCandidates . We would have to argue that the setBlockIndexCandidates blocks from each chainstate don't overlap in a way that leads to UB, which seems very brittle.
(extra explanation that may be helpful for others: During normal operation setBlockIndexCandidates has all connectable blocks with at least as much work as the current tip. However, during startup, we first add all connectable indexes to the set because we don't know the tip yet, then load the tip, and then call PruneBlockIndexCandidates(); to remove almost all of them again.)
FYI @sipa @TheCharlatan @furszy @sr-gi who authored/acked the original PR - any idea how to best fix this?
Tested on ARM64 and it builds clean. Ran the startup and validation tests and all passed.
The alternative branch that clears and repopulates the candidate set seems like it would be a more complete fix.
Concept ACK
Since LoadChainTip() also mutates nSequenceId for ancestors, should we remove/reinsert all mutated entries (not only tip) before changing sequence IDs?
But more generally, can we avoid mutating CBlockIndex::nSequenceId while any setBlockIndexCandidates (across all chainstates) contains those indices?
Also, could we add a targeted test that stresses the startup tip selection under tie conditions, and asserts the candidate set contains no entries worse than tip?
Also, could we add a targeted test that stresses
The simplest test I found adds two equal-work siblings, resets block-sequence state, seeds CoinsTip with the worst-sorting candidate, calls LoadBlockIndex && LoadChainTip, then verifies every setBlockIndexCandidates entry is not worse than the tip and that set iteration order is comparator-consistent:
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
--- a/src/test/validation_chainstatemanager_tests.cpp (revision b2805eec35ce9fdb69892e864556be4ae057ce6a)
+++ b/src/test/validation_chainstatemanager_tests.cpp (date 1770547026792)
@@ -24,6 +24,9 @@
#include <tinyformat.h>
+#include <algorithm>
+#include <array>
+#include <ranges>
#include <vector>
#include <boost/test/unit_test.hpp>
@@ -591,6 +594,47 @@
BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.size(), num_indexes - last_assumed_valid_idx + 1);
}
+//! Regression test for candidate pruning when LoadChainTip rewrites nSequenceId.
+BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadchaintip_prunes_worse_candidates, TestChain100Setup)
+{
+ constexpr auto comparator{node::CBlockIndexWorkComparator{}};
+ auto& chainman{*Assert(m_node.chainman)};
+ auto& chainstate{chainman.ActiveChainstate()};
+ auto* active_tip{WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip())};
+ BOOST_REQUIRE(active_tip && active_tip->pprev);
+
+ // Add two equal-work siblings and use the worse one as CoinsTip best block.
+ LOCK(cs_main);
+ auto header{active_tip->GetBlockHeader()};
+ auto make_sibling{[&]() EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
+ ++header.nNonce;
+ auto* sibling{chainman.m_blockman.AddToBlockIndex(header, chainman.m_best_header)};
+ sibling->RaiseValidity(BLOCK_VALID_TRANSACTIONS);
+ sibling->m_chain_tx_count = active_tip->m_chain_tx_count;
+ return sibling;
+ }};
+ std::array candidates{active_tip, make_sibling(), make_sibling()};
+ chainman.ResetBlockSequenceCounters();
+ for (auto& index : chainman.m_blockman.m_block_index | std::views::values) {
+ index.nSequenceId = SEQ_ID_INIT_FROM_DISK;
+ }
+ std::ranges::sort(candidates, comparator);
+
+ chainstate.m_chain.SetTip(*active_tip);
+ chainstate.CoinsTip().SetBestBlock(candidates.front()->GetBlockHash());
+ chainstate.ClearBlockIndexCandidates();
+
+ BOOST_REQUIRE(chainman.LoadBlockIndex() && chainstate.LoadChainTip());
+ const CBlockIndex* prev{nullptr};
+ for (auto* pindex : chainstate.setBlockIndexCandidates) {
+ BOOST_CHECK_MESSAGE(!comparator(pindex, chainstate.m_chain.Tip()), "Candidate sorts worse than the tip");
+ if (prev) {
+ BOOST_CHECK_MESSAGE(!comparator(pindex, prev), "Wrong candidate set iteration order");
+ }
+ prev = pindex;
+ }
+}
+
//! Ensure that snapshot chainstate can be loaded when found on disk after a
//! restart, and that new blocks can be connected to both chainstates.
BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup)
Before the fix this fails with:
test/validation_chainstatemanager_tests.cpp:630: error: in "validation_chainstatemanager_tests/chainstatemanager_loadchaintip_prunes_worse_candidates": Candidate sorts worse than the tip
test/validation_chainstatemanager_tests.cpp:632: error: in "validation_chainstatemanager_tests/chainstatemanager_loadchaintip_prunes_worse_candidates": Wrong candidate set iteration order
And passes after with the erase in the loop:
diff --git a/src/validation.cpp b/src/validation.cpp
--- a/src/validation.cpp (revision b2805eec35ce9fdb69892e864556be4ae057ce6a)
+++ b/src/validation.cpp (date 1770547212278)
@@ -4614,15 +4614,15 @@
// Make sure our chain tip before shutting down scores better than any other candidate
// to maintain a consistent best tip over reboots in case of a tie.
+ // Erase each block from candidates before changing its nSequenceId. The set
+ // uses nSequenceId for ordering, so modifying it in-place is undefined behavior.
auto target = tip;
while (target) {
+ setBlockIndexCandidates.erase(target);
target->nSequenceId = SEQ_ID_BEST_CHAIN_FROM_DISK;
target = target->pprev;
}
- // Block index candidates are loaded before the chain tip, so we need to replace this entry
- // Otherwise the scoring will be based on the memory address location instead of the nSequenceId
- setBlockIndexCandidates.erase(tip);
TryAddBlockIndexCandidate(tip);
PruneBlockIndexCandidates();
(though I couldn't write a test that fails for your solution only (but passes for the looped erase), so the two fixes are likely equivalent - though conceptually mutating any key still in a std::set is UB so we should likely delete (or not insert) all of them).
I'd prefer this removal to be a separate commit at the beginning - it's independent from the later changes, and you could explain in the commit message the not-so-ovbious reason why it can be safely removed (it'd always return early).
Done
I think I understand the race, but deleting/reinserting here feels brittle. Would it be possible to avoid pre-populating candidate inserts until after sequence IDs are finalized?
Agreed, this lines up with what mzumsande pointed out regarding the dual chainstate case too. I believe marcofleon's alt branch already takes that approach.
I also think that the fix in https://github.com/marcofleon/bitcoin/tree/2026/02/fix-setBlockIndexCandidate-ub looks better and would suggest to switch to that approach. Irrespective of the bug being fixed here I think that the current way of adding the entire chain and then removing all of it again from the set is not very elegant, so it seems good to change that anyway.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task macOS native: https://github.com/bitcoin/bitcoin/actions/runs/21874154480/job/63137979156</sub>
<sub>LLM reason (✨ experimental): validation_chainstatemanager_tests failed (subprocess aborted) causing the CTest run to fail.</sub>
<details><summary>Hints</summary>
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.
</details>
I've switched the approach to only populating setBlockIndexCandidates after we call LoadChainTip and included an assert that all candidate sets (if we have two chainstates) are empty right before we change all the sequence ids. This should fix both types of undefined behavior outlined in #34521#pullrequestreview-3761920357.
@marcofleon, could you please add a test as a first commit that documents the current behavior and the fix in the next commit that that adjust the test to help reviewers assess the behavior change (i.e. documented and debuggable before & after behavior)?
Or if that's too involved, just a test that we can run without the fix to assess its correctness (we can't do that currently since it contains PopulateBlockIndexCandidates)?
Please see my suggestion in https://github.com/bitcoin/bitcoin/pull/34521/changes#r2779130922
@l0rinc I extended a functional test that restarts the node after a block race. If you remove the first commit and run the test, it should crash, although it might take a few runs. The undefined behavior doesn't always have an effect on the candidate set. It depends on the initial ordering of the blocks in the set, which on master, comes down to the pointer addresses of the competing blocks.
You can see that the conditions that cause the assert to fail are pretty specific. If you think the test is useful, I can keep the it there. Or I can take the commit out once you're confident that the UB is fixed. I don't mind either way.
tACK 72bd4c915ecaca7141d3c25e8d990e92b0551f67
Tested with the same test that found the issue originally. High confidence that this does not break anything.
4611 | @@ -4612,6 +4612,11 @@ bool Chainstate::LoadChainTip() 4612 | m_chainman.UpdateIBDStatus(); 4613 | tip = m_chain.Tip(); 4614 | 4615 | + // nSequenceId is about to be modified, so all candidate sets should be empty to avoid UB.
The referenced UB is not really obvious from just this sentence, future readers will thank you for elaborating. For example:
// nSequenceId is one of the keys used to sort setBlockIndexCandidates. Ensure all
// candidate sets are empty to avoid UB as nSequenceId is about to be modified.
Better comment, thanks. Taken.
816 | @@ -817,6 +817,8 @@ class Chainstate 817 | 818 | void ClearBlockIndexCandidates() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); 819 | 820 | + void PopulateBlockIndexCandidates() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
Would be nice to add a docstring for this public method. To avoid documenting the "wrong" function, this might mean also adding one to TryAddBlockIndexCandidate and referencing that here.
Done, added a docstring to both methods.
5756 | @@ -5753,6 +5757,11 @@ util::Result<CBlockIndex*> ChainstateManager::ActivateSnapshot( 5757 | assert(chaintip_loaded); 5758 | m_blockman.m_snapshot_height = Assert(chainstate.SnapshotBase())->nHeight; 5759 | 5760 | + // Due to the early return in LoadChainTip(), no nSequenceIds were 5761 | + // modified, so the historical chainstate's candidate set remains correctly 5762 | + // ordered. Only populate the snapshot chainstate's candidate set. 5763 | + chainstate.PopulateBlockIndexCandidates();
This comment is a bit scary. It references (and assumes?) implementation details in LoadChainTip(), and is not clear about which early return (and/or why that is hit). This stuff confuses me more than it gives me clarity.
I don't have any suggestions on how to improve it yet, but wanted to flag it already.
Agreed it's not the best of comments. I was having trouble trying to decide how in detail I should go here or whether to put a comment at all. I'll leave it for now until I have a better idea of how to improve it, or if you or other reviewers have suggestions.
I got confused by this too. First I thought that during Snapshot activation the added ( assert(cs->setBlockIndexCandidates.empty()) should fail because the historical chainstate should always have candidates in its set at the point in time where we activate a snapshot. But as the comment says, we always return before we get to that spot, so nothing fails.
But then this leads to the question why we call LoadChainTip() from ActivateSnapshot() in the first place here? In PopulateAndValidateSnapshot(), we set both SetBestBlock() and SetTip() so we should always return here in the ActivateSnapshot branch. Wouldn't it be simpler to remove both the call to LoadChainTip() and the comment?
Wouldn't it be simpler to remove both the call to
LoadChainTip()and the comment?
Yeah this makes more sense. I appreciate the suggestion (and explanation).
Concept ACK
Should SEQ_ID_BEST_CHAIN_FROM_DISK be used for the best chain in all chainstates, or only the tip chainstate? (I can't think of a scenario where they would contradict, but felt it might be good to ask in case anyone else thinks of such a case)
ACK 9b07bd85064d52d3f194eb421bf0a5abfce4f9bd
Concept ACK
4931 | + // may not have BLOCK_VALID_TRANSACTIONS (e.g. if we haven't yet downloaded 4932 | + // the block), so we special-case it here. 4933 | + const CBlockIndex* snap_base{m_chainman.CurrentChainstate().SnapshotBase()}; 4934 | + 4935 | + for (CBlockIndex* pindex : m_blockman.GetAllBlockIndices()) { 4936 | + if (pindex == snap_base ||
Now that this is moved to the Chainstate class, might it better to do the following check?
diff --git a/src/validation.cpp b/src/validation.cpp
index 8c02e94f94..b2b9a1e548 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4933 +4932,0 @@ void Chainstate::PopulateBlockIndexCandidates()
- const CBlockIndex* snap_base{m_chainman.CurrentChainstate().SnapshotBase()};
@@ -4936 +4935 @@ void Chainstate::PopulateBlockIndexCandidates()
- if (pindex == snap_base ||
+ if (pindex == TargetBlock() || pindex == SnapshotBase() ||
Taken, thanks. It's cleaner to use these directly now that it's a method on Chainstate.
4921 | @@ -4923,6 +4922,25 @@ void Chainstate::ClearBlockIndexCandidates() 4922 | setBlockIndexCandidates.clear(); 4923 | } 4924 | 4925 | +void Chainstate::PopulateBlockIndexCandidates() 4926 | +{ 4927 | + AssertLockHeld(::cs_main); 4928 | + setBlockIndexCandidates.clear();
Could we have an assertion here instead?
assert(setBlockIndexCandidates.empty());
At this point in time, can the candidate set contain anything besides nothing, or in some cases the genesis block? If so, could it be useful to assert that? I'm also not sure why the candidate set needs to be cleared here.
I think I was just clearing it to make sure it's empty beforehand. But agreed, it's better to assert the state the set should be in by the time this is called.
I added the assertion for empty or only the genesis block in the set. I found out that when starting a shiny new node for the first time, the genesis block will be in this set at this point.
Let me know if there's something else you had in mind for the assertion.
Thanks, I agree an assertion here is better. Added this, but allowing for the genesis block to be in the set as well, which happens when we start up a node for the first time.
4936 | + if (pindex == snap_base || 4937 | + (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && 4938 | + (pindex->HaveNumChainTxs() || pindex->pprev == nullptr))) { 4939 | + TryAddBlockIndexCandidate(pindex); 4940 | + } 4941 | + }
It is not clear to me why the snapshot base has to be added as candidate for the historical chainstate as well 🤔 (I understand this behavior already existed before).
Seems like adding it only to the snapshot chainstate candidates would break the chainstatemanager_loadblockindex unit test but then again the comment here also suggests that adding it to the historical chainstate candidates is not necessary. So maybe we can adapt both PopulateBlockIndexCandidates() and failing unit tests?
Good point, I thought about this as well. I couldn't find a good reason the snapshot base is added as a candidate for both, but maybe we're missing something. Either way, this feels tangential to this PR and afaict adding to both is harmless, so I'll keep as is.
Concept ACK
Concept ACK.
ACK cd2d6529f49dfef98ecc89c0d07b2ba0ac55a550
816 |
817 | void PruneBlockIndexCandidates();
818 |
819 | void ClearBlockIndexCandidates() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
820 |
821 | + /** Populate the candidate set by calling TryAddBlockIndexCandidate on all valid block indices. */
nit: usage could be documented more completely:
/** Populate the candidate set by calling TryAddBlockIndexCandidate on all valid block indices.
* This function may only be called once.
*
* [@pre](/bitcoin-bitcoin/contributor/pre/) setBlockIndexCandidates must be empty (except for genesis block).
(continued from #34521 (review))
Actually, I don't think we need any assertions or clearing here. This feels like leaking testing of the current behaviour into the function implementation. It makes sense to add assertions for preconditions that can e.g. lead to UB, but that doesn't seem to be the case here. Inserting twice may be useless, and callsites may want to prevent it from happening, but it's not an inherent requirement for populating the candidate sets.
The tests pass fine with the below diff, which removes the assert and duplicates the PopulateBlockIndexCandidates() calls:
<details> <summary>git diff on cd2d6529f4</summary>
diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index ff3de29822..69c3848881 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -131,6 +131,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
// set's comparator, changing it while blocks are in the set would be UB.
for (const auto& chainstate : chainman.m_chainstates) {
chainstate->PopulateBlockIndexCandidates();
+ chainstate->PopulateBlockIndexCandidates();
}
const auto& chainstates{chainman.m_chainstates};
diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h
index 3ceed5697e..b8076f6b49 100644
--- a/src/test/util/chainstate.h
+++ b/src/test/util/chainstate.h
@@ -106,6 +106,7 @@ CreateAndActivateUTXOSnapshot(
pindex = pindex->pprev;
}
chain.PopulateBlockIndexCandidates();
+ chain.PopulateBlockIndexCandidates();
}
BlockValidationState state;
if (!node.chainman->ActiveChainstate().ActivateBestChain(state)) {
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 31740f8d98..738e8caed7 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -82,6 +82,8 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager, TestChain100Setup)
c2.LoadChainTip();
for (const auto& cs : manager.m_chainstates) {
cs->PopulateBlockIndexCandidates();
+ cs->PopulateBlockIndexCandidates();
+
}
}
BlockValidationState _;
@@ -499,6 +501,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
chainman.LoadBlockIndex();
for (const auto& cs : chainman.m_chainstates) {
cs->PopulateBlockIndexCandidates();
+ cs->PopulateBlockIndexCandidates();
}
};
diff --git a/src/validation.cpp b/src/validation.cpp
index 1a55a789af..e2d1365694 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4925,10 +4925,6 @@ void Chainstate::ClearBlockIndexCandidates()
void Chainstate::PopulateBlockIndexCandidates()
{
AssertLockHeld(::cs_main);
- // It's possible that the genesis block is already in the candidate set.
- assert(setBlockIndexCandidates.empty() ||
- (setBlockIndexCandidates.size() == 1 &&
- (*setBlockIndexCandidates.begin())->GetBlockHash() == m_chainman.GetConsensus().hashGenesisBlock));
for (CBlockIndex* pindex : m_blockman.GetAllBlockIndices()) {
// With assumeutxo, the snapshot block is a candidate for the tip, but it
@@ -5749,6 +5745,7 @@ util::Result<CBlockIndex*> ChainstateManager::ActivateSnapshot(
Chainstate& chainstate{AddChainstate(std::move(snapshot_chainstate))};
m_blockman.m_snapshot_height = Assert(chainstate.SnapshotBase())->nHeight;
+ chainstate.PopulateBlockIndexCandidates();
chainstate.PopulateBlockIndexCandidates();
LogInfo("[snapshot] successfully activated snapshot %s", base_blockhash.ToString());
</details>
I think calling clear() at the beginning (as it was in a previous version) would be acceptable too if better/required, but then it should be documented and Repopulate is probably a better name than Populate?
True, it is a set so adding duplicates will be fine. I viewed the assertion as more of a check for unexpected state. In that case, an Assume could make more sense.
What do you think of an Assume vs clearing it? Or the other option is just not having anything there. I don't have a strong preference. I'll expand the documentation to match whichever one we end up doing.
I viewed the assertion as more of a check for unexpected state. In that case, an Assume could make more sense.
I don't think it does. Similar to assert, Assume just makes using and maintaining PopulateBlockIndexCandidates() unnecessarily confusing. PopulateBlockIndexCandidates's responsibility is to populate block index candidates, not to verify higher level program behaviour. Higher levels can add asserts or assumes where applicable.
I think I agree with stickies-v after all here. After posting my initial comment, I was less sure if the asserts would be useful: They don't actually guard against abuse or the bug in question here.
I'll remove everything then and keep the documentation as is, because there's nothing to expand on.
ACK cd2d6529f49dfef98ecc89c0d07b2ba0ac55a550
Reviewed the code and confirmed that the edited functional test fails about 20% of the time on current master and the code changes here address this.
ACK 1f7109af24570e786a236c9271794d32af2f16aa
I think the current approach is a reasonable way to address the stated problem. However, after spending a lot of time on this PR I keep finding it difficult to build sufficient confidence that the reshuffled initialization order doesn't introduce any (subtle) new logic bugs.
The current approach patches, but doesn't really fix the underlying issue. The fact that setBlockIndexCandidates is sorted by a public, mutable field is inherently brittle, and might easily cause similar bugs in the future.
Do we really need edit: turns out we do setBlockIndexCandidates to be pow-sorted at all times?It seems to me like we don't - we only need the ordering in FindMostWorkChain. So, my suggested alternative is to keep the set default-sorted, and do the pow scanning in FindMostWorkChain. It seems like this can be implemented in a much smaller diff than the current approach, too: https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2026-02/candidates-without-sequenceid
edit: I suspect we might be able to make further performance improvements by e.g. making setBlockIndexCandidates a multimap with nChainwork as key, and further encapsulation improvements by having a separate class BlockIndexCandidates, but it doesn't seem necessary for this minimal fix.
Do we really need setBlockIndexCandidates to be pow-sorted at all times? It seems to me like we don't - we only need the ordering in FindMostWorkChain. So, my suggested alternative is to keep the set default-sorted, and do the pow scanning in FindMostWorkChain. It seems like this can be implemented in a much smaller diff than the current approach, too: https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2026-02/candidates-without-sequenceid
I tried doing a quick signet reindex-chainstate and your branch was an order of magnitude slower than current master. Did you check how this impacts performance?
I tried doing a quick signet
reindex-chainstateand your branch was an order of magnitude slower than current master. Did you check how this impacts performance?
Thanks for the quick sanity check, you're right, reindex-chainstate performance is horrible. Fixed in this slightly more involved branch that still sorts on nChainwork, but not nSequenceId: https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2026-02/candidates-chainwork. Seems just as quick as master for me. Idea remains the same.
Since the current approach seems feasible and has had a good amount of review already, I'll open my approach as a separate PR tomorrow so they can be evaluated side-by-side rather than having to flip back and forth.
4928 | + 4929 | + for (CBlockIndex* pindex : m_blockman.GetAllBlockIndices()) { 4930 | + // With assumeutxo, the snapshot block is a candidate for the tip, but it 4931 | + // may not have BLOCK_VALID_TRANSACTIONS (e.g. if we haven't yet downloaded 4932 | + // the block), so we special-case it here. 4933 | + if (pindex == SnapshotBase() || pindex == TargetBlock() ||
Adding the pindex == TargetBlock() condition (without which a unit test would fail) makes a quirk of the existing AssumeUtxo logic more visible - from a high-level point of view it seems unnecessary:
Why do we need an exception for the historical chainstate? This chainstate starts at genesis, and when catching up, it will download, and connect the snapshot block normally, so it doesn't need any special logic as far as I can see.
Currently, we have an unconnectable block lingering in setBlockIndexCandidates, which makes no sense.
It is even documented in the unit tests https://github.com/bitcoin/bitcoin/blob/2eaf701bc0d5cfbb6bc458f467141451f2175cdf/src/test/validation_chainstatemanager_tests.cpp#L557-L561
I am planning to open a follow-up PR (not targeted for v31) that will 1) Remove this and 2) tighten the logic in CheckBlockIndex to check if there are no unconnectable block in setBlockIndexCandidates.
oh, I just saw that the same point was raised above by @stringintech - I do think this should be fixed, but not in this PR.
115 | - # G 116 | - # \ 117 | - # A2 118 | + # /- A1 119 | + # / 120 | + # G -- B1 --- A2
Was this subtest supposed to use the second node (which currently seems completely unused)? The diagram suggests this, currently the blocks are not build on Genesis but on the messy endstate of the first subtest. Maybe use the second node for that, and never connect the two (or keep it as is and remove the second node).
Good catch, thanks. Switched to using the second node.
130 | - prev_time = genesis_block["time"] 131 | + tip_block = node.getblock(node.getbestblockhash()) 132 | + prev_time = tip_block["time"] 133 | 134 | - for i in range(0, 2): 135 | + for i in range(0, 3):
Could you explain the test change a bit more in the commit message and/or the code? Why do we send a third block A3 now? Why do we no longer restart multiple times in a loop, but send a new block after the restart?
Done. I found out more about why two blocks doesn't cause the erase failure on my mac vs three blocks.
https://godbolt.org/z/covTcYT1c
Using libstdc++ (the standard library GCC uses I believe) would cause the two block race to fail. But as you can see, using clang's libc++ there are only failures with a three block race.
Maybe different tree structure shapes? Would be interesting to pinpoint exactly why.
I think the current approach is a reasonable way to address the stated problem. However, after spending a lot of time on this PR I keep finding it difficult to build sufficient confidence that the reshuffled initialization order doesn't introduce any (subtle) new logic bugs.
Fixed in this slightly more involved branch
I feel that this would be too risky as a last-minute change. I would prefer to either go with this PR or alternatively revert #29640 for v31 (the problem that PR solved was of a pretty theoretical nature at all - forks at the tip are rare, and changing the tip after a restart is certainly a bit weird but not really a practical problem for most nodes).
I feel that this would be too risky as a last-minute change
I did a first pass of stickies branch and I think it probably is a more robust long-term solution, although I would need to review more to be sure. We also ran the original antithesis test on the branch for about a day and it passed.
If we want to do a temporary (less complex) fix for now and then revisit approaches later on, that sounds fine to me. I'll address review comments today in case we end up deciding that this PR is the way to move forward for v31.
Is it possible the same issue still exists inside Chainstate::PreciousBlock? It modifies pindex->nSequenceId after erasing it from its own setBlockIndexCandidates, but doesn't erase it from the potentially existing other Chainstate.
Is it possible the same issue still exists inside Chainstate::PreciousBlock? It modifies pindex->nSequenceId after erasing it from its own setBlockIndexCandidates, but doesn't erase it from the potentially existing other Chainstate.
I thought about this question last week but came to the conclusion that it's probably not a problem:
PreciousBlock operates on the Active Chainstate, and will abort if the block is not on its tip. These blocks should never be in the setBlockIndexCandidates of the historical chainstate because they are not ancestors of the target block (see this criterion)
This call is a no-op. PopulateAndValidateSnapshot already sets both
the chain tip and the coins cache best block to the snapshot block,
so LoadChainTip always hits the early return when it finds that the
two match (tip->GetBlockHash() == coins_cache.GetBestBlock()).
The removal of the chain tip from setBlockIndexCandidates was
happening after nSequenceId was modified. Since the set uses
nSequenceId as a sort key, modifying it while the element is in the
set is undefined behavior, which can cause the erase to fail.
With assumeutxo, a second form of UB exists: two chainstates each
have their own candidate set, but share the same CBlockIndex
objects. Calling LoadChainTip on one chainstate mutates nSequenceIds
that are also in the other chainstate's set.
Fix by populating setBlockIndexCandidates after all changes to
nSequenceId.
Fix the from-disk subtest to use a separate node so it builds on a
clean genesis block, rather than the leftover chain from the
in-memory subtest.
Change from a two-way to a three-way block race. The UB in the old
LoadChainTip (mutating nSequenceId, a sort key, while the block is
in setBlockIndexCandidates) corrupts the internal tree structure,
resulting in a failed erase that leaves stale blocks in the set
alongside the tip. With only two competing blocks, this is caught
by libstdc++ but not by libc++. A three-way split triggers the bug
on both implementations.
To trigger CheckBlockIndex (where the crashing assertion is), replace
the restart loop with sending a new block after a single restart.
Re-ACK 20ae9b98eab20117344cf31f7cde39cadd70ca22
Code review ACK 20ae9b98eab20117344cf31f7cde39cadd70ca22
ACK 20ae9b98eab20117344cf31f7cde39cadd70ca22
Milestone
31.0