Coming from the lack of m_chain usage in this function, I'm wondering if we should add a sanity-check here?

Assume(m_last_flushed_block->GetBlockHash() == CoinsTip().GetBestBlock());

or.. maybe directly use

m_last_flushed_block =  m_blockman.LookupBlockIndex(CoinsTip().GetBestBlock());

489bb4a validation: track last flushed block:

Could this new ChainStateFlushed locator come from the recorded flushed block instead of rereading m_chain.Tip()?

diff --git a/src/validation.cpp b/src/validation.cpp
--- a/src/validation.cpp	(revision 30c92e346762ec5d046d81d46d08811ccd9e26b3)
+++ b/src/validation.cpp	(revision 2296c4e0101ee7de230478d0dd7323b04a849fe4)
@@ -2831,7 +2831,7 @@
     }
     if (full_flush_completed && m_chainman.m_options.signals) {
         // Update best block in wallet (so we can detect restored wallets).
-        m_chainman.m_options.signals->ChainStateFlushed(this->GetRole(), GetLocator(m_chain.Tip()));
+        m_chainman.m_options.signals->ChainStateFlushed(this->GetRole(), GetLocator(Assert(m_last_flushed_block)));
     }
     } catch (const std::runtime_error& e) {
         return FatalError(m_chainman.GetNotifications(), state, strprintf(_("System error while flushing: %s"), e.what()));

I added the suggested sanity check, and reused m_last_flushed. Note that we did use m_chain for the ChainStateFlushed() notification there.
However, I just noticed that some unit tests (validation_chainstate_resize_caches) don't populate m_chain and fail with the Assume. For now I have fixed this by making it conditional to a tip existing - I'll look later into changing the test instead.

Still doesn't work. The Assume is also hit by the validation_chainstatemanager_tests which manually changes the tip to be lagging behind the CoinsTip()- this is a dirty hack to make AssumeUtxo testable in unit tests (Snapshots will only activate when they have more work than the tip...).
I have removed the Assume() for now. Changing it to CoinsTip() would work, but then we should probably also change the notification. Would you prefer that?

@furszy is probably correct, the cached boundary should reflect the coins view that was actually flushed. But the ChainStateFlushed() locator change I asked about should probably stay as-is.

I understand that during IBD we can't have reorgs, but during steady state DisconnectTip(), the temporary coins view is flushed back to pindexDelete->pprev before the active tip is updated, so m_chain.Tip() can still name the disconnected block while CoinsTip().GetBestBlock() already names its parent:

struct TestChain100SetupNoMempoolHeadroom : TestChain100Setup {
    TestChain100SetupNoMempoolHeadroom() : TestChain100Setup{ChainType::REGTEST, TestOpts{.extra_args = {"-maxmempool=0"}}} {}
};

BOOST_FIXTURE_TEST_CASE(flushed_boundary_uses_disconnected_coins_tip, TestChain100SetupNoMempoolHeadroom)
{
    Chainstate& chainstate{Assert(m_node.chainman)->ActiveChainstate()};

    const CBlockIndex* tip{Assert(WITH_LOCK(::cs_main, return chainstate.m_chain.Tip()))};
    const CBlockIndex* parent{tip->pprev};

    {
        LOCK2(m_node.chainman->GetMutex(), chainstate.MempoolMutex());
        chainstate.m_coinstip_cache_size_bytes = 0;
        BlockValidationState state{};
        BOOST_REQUIRE(chainstate.DisconnectTip(state, nullptr));
    }

    BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return chainstate.m_chain.Tip()), parent);
    BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return chainstate.GetLastFlushedBlock()), parent);
    m_node.validation_signals->SyncWithValidationInterfaceQueue();
}

The test fails currently because GetLastFlushedBlock() records the disconnected tip instead of its parent:

m_last_flushed_block = m_blockman.LookupBlockIndex(CoinsTip().GetBestBlock());

That is an interesting situation, which I think is a bug in master. So the issue is that we call FlushStateToDisk(state, FlushStateMode::IF_NEEDED)) in DisconnectTip() after disconnecting the coins, but before setting the new tip (the IF_NEEDED makes this relatively unlikely, but it is still possible).

But the ChainStateFlushed() locator change I asked about should probably stay as-is.

why? It gives the wrong locator for the notification in this situation.
I changed it to use CoinsTip().GetBestBlock() for the last flushed block and also added a commit at the end to no longer use m_chain.Tip() for the existing ChainStateFlushed notification.

nit: Could log where the index is at vs the chain height, so the diff gets logged.

nit: Also LogInfo on every skip seems a bit excessive. Maybe make this a debug log?

I added the heights. It should be quite rare though (only happens at the end of Sync), so I'm not sure if it's worth adding a new debug log category for indexes just for this.

nit: Also LogInfo on every skip seems a bit excessive. Maybe make this a debug log?

5af4d6e test: add test to ensure indexes dont commit too early:

This seems like a good example for #35260 - it would help me with the review to know what the state was before the change and which commit changes the assumption and how and where it changes it.
Maybe something like this documenting the state before the fix:

BOOST_FIXTURE_TEST_CASE(coinstatsindex_commits_ahead_of_chainstate_flush, TestChain100Setup)
{
    Chainstate& chainstate = Assert(m_node.chainman)->ActiveChainstate();
    BlockValidationState state;

    auto current_tip{[&] { return Assert(WITH_LOCK(::cs_main, return chainstate.m_chain.Tip())); }};
    auto coinsdb_best_is{[&](const CBlockIndex& block) {
        LOCK(::cs_main);
        BOOST_CHECK(chainstate.CoinsDB().GetBestBlock() == block.GetBlockHash());
    }};
    auto sync_fresh_index{[&](int expected_height) {
        CoinStatsIndex index{interfaces::MakeChain(m_node), /*n_cache_size=*/1_MiB, /*f_memory=*/false, /*f_wipe=*/true};
        BOOST_REQUIRE(index.Init());
        index.Sync();
        BOOST_CHECK_EQUAL(index.GetSummary().best_block_height, expected_height);
        index.Stop();
    }};
    auto read_persisted_index_height{[&] {
        CoinStatsIndex index{interfaces::MakeChain(m_node), /*n_cache_size=*/1_MiB};
        BOOST_REQUIRE(index.Init());
        const int height{index.GetSummary().best_block_height};
        index.Stop();
        return height;
    }};
    auto flush_and_commit_index{[&] {
        CoinStatsIndex index{interfaces::MakeChain(m_node), /*n_cache_size=*/1_MiB};
        BOOST_REQUIRE(index.Init());
        index.Sync();
        BOOST_REQUIRE(chainstate.FlushStateToDisk(state, FlushStateMode::FORCE_FLUSH));
        m_node.validation_signals->SyncWithValidationInterfaceQueue();
        index.Stop();
    }};
    // For one unflushed index tip, check the durable height before and after the chainstate catches up.
    auto check_unflushed_index_commit{[&](const CBlockIndex& index_tip, int expected_persisted_height) {
        sync_fresh_index(index_tip.nHeight);
        BOOST_CHECK_EQUAL(read_persisted_index_height(), expected_persisted_height);
        flush_and_commit_index();
        BOOST_CHECK_EQUAL(read_persisted_index_height(), index_tip.nHeight);
    }};

    // Start from a known durable chainstate boundary.
    BOOST_REQUIRE(chainstate.FlushStateToDisk(state, FlushStateMode::FORCE_FLUSH));
    auto* flushed_tip{current_tip()};
    coinsdb_best_is(*flushed_tip);

    // Straight-line case: connect one more block without flushing the chainstate.
    CreateAndProcessBlock({}, CScript() << OP_TRUE);
    auto* unflushed_tip{current_tip()};
    BOOST_REQUIRE_GT(unflushed_tip->nHeight, flushed_tip->nHeight);
    coinsdb_best_is(*flushed_tip);

    check_unflushed_index_commit(*unflushed_tip, /*expected_persisted_height=*/unflushed_tip->nHeight); // TODO: Persists ahead of the flushed chainstate.

    auto* reorg_flushed_tip{current_tip()};
    coinsdb_best_is(*reorg_flushed_tip);

    // Fork case: leave the flushed chainstate behind, then index a competing unflushed tip.
    BOOST_REQUIRE(chainstate.InvalidateBlock(state, reorg_flushed_tip));
    NodeClockContext clock_ctx{};
    clock_ctx += 1s;
    CreateAndProcessBlock({}, CScript() << OP_TRUE);
    clock_ctx += 1s;
    CreateAndProcessBlock({}, CScript() << OP_TRUE);

    auto* fork_tip{current_tip()};
    BOOST_REQUIRE_EQUAL(fork_tip->nHeight, reorg_flushed_tip->nHeight + 1);
    BOOST_CHECK(fork_tip->GetAncestor(reorg_flushed_tip->nHeight) != reorg_flushed_tip);
    coinsdb_best_is(*reorg_flushed_tip);

    check_unflushed_index_commit(*fork_tip, /*expected_persisted_height=*/fork_tip->nHeight); // TODO: Persists a fork from the flushed chainstate.
}

so that the commit fixing it would have a minimal diff of what it affects exactly:

diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp
--- a/src/test/coinstatsindex_tests.cpp	(revision 7177975c1d50fd4e0164fa6093a06b68302f0115)
+++ b/src/test/coinstatsindex_tests.cpp	(revision e8197142a66c98e72d2f8c6f138f4e29a9d83b94)
@@ -124,7 +124,7 @@
     }
 }
 
-BOOST_FIXTURE_TEST_CASE(coinstatsindex_commits_ahead_of_chainstate_flush, TestChain100Setup)
+BOOST_FIXTURE_TEST_CASE(coinstatsindex_commit_is_limited_by_chainstate_flush, TestChain100Setup)
 {
     Chainstate& chainstate = Assert(m_node.chainman)->ActiveChainstate();
     BlockValidationState state;
@@ -175,7 +175,7 @@
     BOOST_REQUIRE_GT(unflushed_tip->nHeight, flushed_tip->nHeight);
     coinsdb_best_is(*flushed_tip);
 
-    check_unflushed_index_commit(*unflushed_tip, /*expected_persisted_height=*/unflushed_tip->nHeight); // TODO: Persists ahead of the flushed chainstate.
+    check_unflushed_index_commit(*unflushed_tip, /*expected_persisted_height=*/0);
 
     auto* reorg_flushed_tip{current_tip()};
     coinsdb_best_is(*reorg_flushed_tip);
@@ -193,7 +193,7 @@
     BOOST_CHECK(fork_tip->GetAncestor(reorg_flushed_tip->nHeight) != reorg_flushed_tip);
     coinsdb_best_is(*reorg_flushed_tip);
 
-    check_unflushed_index_commit(*fork_tip, /*expected_persisted_height=*/fork_tip->nHeight); // TODO: Persists a fork from the flushed chainstate.
+    check_unflushed_index_commit(*fork_tip, /*expected_persisted_height=*/0);
 }
 
 BOOST_AUTO_TEST_SUITE_END()

I have tried this approach in the past, I prefer to add the full test (which can be cherry-picked on master to see the failure). Otherwise, it adds a lot of churn to check every comment etc. if it already made sense before, without the context of the fix. But I did rewrite the unit test, it should be very obvious how it fails on master.

re: #34897 (comment)

Agree with l0rinc, there is no significant churn here and adding the test first would make the bugfix commit easier to understand, and avoid the need for reviewers to do manual work to verify the test meaningfully checks the changed behavior, when this could be checked by the "test ancestor commits" CI job.

My diff to confirm the test was:

--- b/src/test/coinstatsindex_tests.cpp
+++ a/src/test/coinstatsindex_tests.cpp
@@ -145,7 +145,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_no_commit_ahead_of_flush, TestChain100Set
     {
         CoinStatsIndex index{interfaces::MakeChain(m_node), /*n_cache_size=*/1_MiB};
         BOOST_REQUIRE(index.Init());
-        BOOST_CHECK_EQUAL(index.GetSummary().best_block_height, 100);
+        BOOST_CHECK_EQUAL(index.GetSummary().best_block_height, 0);
         index.Stop();
     }
 
@@ -182,7 +182,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_no_commit_ahead_of_flush, TestChain100Set
     {
         CoinStatsIndex index{interfaces::MakeChain(m_node), /*n_cache_size=*/1_MiB};
         BOOST_REQUIRE(index.Init());
-        BOOST_CHECK_EQUAL(index.GetSummary().best_block_height, 101);
+        BOOST_CHECK_EQUAL(index.GetSummary().best_block_height, 100);
         index.Stop();
     }
 }

I have tried this approach in the past, I prefer to add the full test (which can be cherry-picked on master to see the failure). Otherwise, it adds a lot of churn to check every comment etc.

That's exactly what this technique is solving: you don't need to cherry-pick on master, because you're already proving that the fix makes you adjust the test for it to pass. The CI already proved that the previous version passed and that the modified version also passes with the fix - no need to cherry-pick back and forth.

Also, the current regression coverage primarily demonstrates that the index will not commit if it is ahead by height. Could we extend it with a same-height competing block case, so a height-only check would fail and the ancestry requirement is tested directly?
That would make it clear that the gate is not only "height <= flushed height", but "same block or ancestor of the flushed block".

ok, I changed it. There is still churn: Now I had to separate out comments that refer to the last flushed block, since they make no sense in a commit before it was introduced. Whenever I will add things to the test, I will likely have to resolve conflicts since it's now spread over two commits. Also, ideally, one should not just have a succeeding texts but also add motivating comments in the original test why the behavior is not ideal and should be fixed (and then delete these comments when fixed), which would create even more churn.

So for the future I definitely prefer to have less churn in the git history and instead have reviewers cherry-pick (and, as a frequent reviewer myself, have absolutely no problem doing that).

7933e27 index: Don't commit ahead of the flushed chainstate:

coinstatsindex doesn't seem to be only index with a similar race, see: #34489 (comment)

Can we cover it in this PR?

All indexes are affected by it because we don't have a special logic for indexes without state. I added it to the coinstatsindex file because that index has state and actually needs to rewind. I think it would make sense to add a base_index tests for generic functionality that affects all indexes (and run the test for all index types, or maybe pick a random one), but I think it would be better to do that in a follow-up because it should affect other existing tests too.

Shouldn't we also test the reorg scenario?

I hope the updated test covers it.

Not the same-height reorg, I still think those should be covered (and docs updated)

Can you explain more? Unless I’m missing something, this doesn’t seem like a practical scenario to me:

1. same-height reorgs don’t really happen, outside of artificial situations such a preciousblock. You reorg to a better chain, and that chain is usually longer.
2. Even for a reorg scenario to a longer chain, I think that the following would need to happen: The Sync() thread reaches the tip, then while the index thread pauses in between

   bitcoin/src/index/base.cpp

   Lines 228 to 231 in f0da26c

   	if (!pindex_next) {
   	SetBestBlockIndex(pindex);
   	// No need to handle errors in Commit. See rationale above.
   	Commit();

   both a reorg and a flush must happen in the main thread. This seems like a very artificial situation to me timing-wise, but it is also hard to replicate in a unit test, because we cannot just do the reorg before calling Sync() as we do in the additional block scenario:
   If the reorg happens while the index is syncing earlier block the index never notices it (it chooses the next block sync based on the current chain), if a reorg happens after Sync() finished and we are in notification mode we will never flush before also doing the reorg in the index because notifications are processed in order.

7933e27 index: Don't commit ahead of the flushed chainstate:

Since this is basically the only usage of this method, we should be able to tailor it more, to let the chain interface take just the candidate block hash instead of a hash and height pair from the caller:

calling:

bool isAncestorOfLastFlushedBlock(const uint256& block_hash) override
{
    LOCK(::cs_main);
    const CBlockIndex* block{chainman().m_blockman.LookupBlockIndex(block_hash)};
    const CBlockIndex* last_flushed{chainman().ValidatedChainstate().GetLastFlushedBlock()};
    return block && last_flushed && last_flushed->GetAncestor(block->nHeight) == block;
}

I took this change, seems like a simpler interface with just one parameter, and the added lookup should not be performance critical since commits are rare.

7933e27 index: Don't commit ahead of the flushed chainstate:

This isn't a "real" nullptr check, so we could probably simplify the structure a bit by something like:

const CBlockIndex* best_block_index{m_best_block_index.load()};
if (!best_block_index) return true;

if (!m_chain->isAncestorOfLastFlushedBlock(best_block_index->GetBlockHash())) {
    return true;
}
...

this would change the logic, we currently return false if best_block_index is not set. This relates to above discussion about the (unused) return value.

Yes, if we remove the return value we could apply this - which would simplify the logic, reuse, and indentation:

diff --git a/src/index/base.cpp b/src/index/base.cpp
index 0be591e7da..3ee70fdd8a 100644
--- a/src/index/base.cpp
+++ b/src/index/base.cpp
@@ -273,31 +273,29 @@ void BaseIndex::Sync()
     }
 }
 
-bool BaseIndex::Commit()
+void BaseIndex::Commit()
 {
     // Don't commit anything if we haven't indexed any block yet
     // (this could happen if init is interrupted).
-    bool ok = m_best_block_index != nullptr;
+    const CBlockIndex* index_tip{m_best_block_index.load()};
+    if (!index_tip) return;
+
+    // Don't commit if the index is ahead of the chainstate's last flushed block
+    if (!m_chain->isAncestorOfLastFlushedBlock(index_tip->GetBlockHash())) {
+        LogInfo("Skipping commit, index is ahead of flushed chainstate (index height %d, chain height %d)",
+                index_tip->nHeight, m_chain->getHeight().value_or(-1));
+        return;
+    }
+
+    CDBBatch batch(GetDB());
+    bool ok = CustomCommit(batch);
     if (ok) {
-        // Don't commit if the index is ahead of the chainstate's last flushed block
-        const CBlockIndex* index_tip = m_best_block_index.load();
-        if (!m_chain->isAncestorOfLastFlushedBlock(index_tip->GetBlockHash())) {
-            LogInfo("Skipping commit, index is ahead of flushed chainstate (index height %d, chain height %d)",
-                    index_tip->nHeight, m_chain->getHeight().value_or(-1));
-            return false;
-        }
-        CDBBatch batch(GetDB());
-        ok = CustomCommit(batch);
-        if (ok) {
-            GetDB().WriteBestBlock(batch, GetLocator(*m_chain, m_best_block_index.load()->GetBlockHash()));
-            GetDB().WriteBatch(batch);
-        }
+        GetDB().WriteBestBlock(batch, GetLocator(*m_chain, index_tip->GetBlockHash()));
+        GetDB().WriteBatch(batch);
     }
     if (!ok) {
         LogError("Failed to commit latest %s state", GetName());
-        return false;
     }
-    return true;
 }
 
 bool BaseIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip)
diff --git a/src/index/base.h b/src/index/base.h
index 4e4cd603c0..5f67b368ae 100644
--- a/src/index/base.h
+++ b/src/index/base.h
@@ -93,12 +93,11 @@ private:
     std::thread m_thread_sync;
     CThreadInterrupt m_interrupt;
 
-    /// Write the current index state (eg. chain block locator and subclass-specific items) to disk.
-    /// Will skip the commit if no block has been indexed yet or if the index's best block is
-    /// ahead of the chainstate's last flushed block. This avoids persisting state an unclean shutdown
-    /// could not roll back from. A later call commits when the chainstate has flushed far enough.
-    /// Returns whether a commit was done.
-    bool Commit();
+    /// Try to write the current index state (eg. chain block locator and subclass-specific items) to disk.
+    /// Returns without writing if no block has been indexed yet or if the index's best block is ahead of
+    /// the chainstate's last flushed block. This avoids persisting state an unclean shutdown could not roll
+    /// back from. A later call commits when the chainstate has flushed far enough.
+    void Commit();
 
     /// Loop over disconnected blocks and call CustomRemove.
     bool Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip);

I did inline it, but didn't do any further refactors of the function. The suggestion above changes behavior such that we no longer log an error if m_best_block_index is nullptr. That change may be desirable, but I want to keep it out of the scope of this PR.

489bb4a validation: track last flushed block:

Could this new ChainStateFlushed locator come from the recorded flushed block instead of rereading m_chain.Tip()?

diff --git a/src/validation.cpp b/src/validation.cpp
--- a/src/validation.cpp	(revision 30c92e346762ec5d046d81d46d08811ccd9e26b3)
+++ b/src/validation.cpp	(revision 2296c4e0101ee7de230478d0dd7323b04a849fe4)
@@ -2831,7 +2831,7 @@
     }
     if (full_flush_completed && m_chainman.m_options.signals) {
         // Update best block in wallet (so we can detect restored wallets).
-        m_chainman.m_options.signals->ChainStateFlushed(this->GetRole(), GetLocator(m_chain.Tip()));
+        m_chainman.m_options.signals->ChainStateFlushed(this->GetRole(), GetLocator(Assert(m_last_flushed_block)));
     }
     } catch (const std::runtime_error& e) {
         return FatalError(m_chainman.GetNotifications(), state, strprintf(_("System error while flushing: %s"), e.what()));

nit