My understanding is that this will only ever actually wait in three cases:
1.) first startup with an empty datadir
2.) reindex
3.) reindex-chainstate

where the genesis block is being connected by the initload thread, whereas otherwise the m_tip_block_cv wait condition should be satisfied because m_tip_block should have been set earlier by the same thread. So removing the if guard is not necessary but just a matter of taste.

After this PR, it will be harder to understand how the folllowing code block refers to the genesis block, so maybe the comment could be expanded to mention some of the above.

I drafted a comment, but I think it's wrong so I haven't pushed it yet:

    /* 
     * Wait for genesis block to be processed. Typically kernel_notifications.m_tip_block
     * has already been set, except in three cases where the genesis block is
     * being connected by the initload thread:
     *
     * 1. first startup with an empty datadir
     * 2. reindex
     * 3. reindex-chainstate
     */

I don't think the initload thread ever sets kernel_notifications.m_tip_block. The thread only deals with indexes derived from BaseIndex, which doesn't include CBlockIndex.

I'm still confused under what circumstance it would actually be unset.

It's clearly doing something though, because the following breaks several functional tests:

--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1792,13 +1792,14 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     });
 
     // Wait for genesis block to be processed
     {
         WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock);
-        kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
-            return !kernel_notifications.m_tip_block.IsNull() || ShutdownRequested(node);
-        });
+        Assert(!kernel_notifications.m_tip_block.IsNull());
+        // kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
+        //     return !kernel_notifications.m_tip_block.IsNull() || ShutdownRequested(node);
+        // });
     }

With some manual testing I indeed get all three conditions from your comment to hit that assert. I'm just confused why.

Outside the context of a snapshot, we only call LoadChainTip() if:

That explains why these three scenarios are different. With an empty datadir or when we -reindex(-chainstate) the UTXO set is empty[0].

Still looking into where it is set.

[0] more accurately, doesn't point to a tip or we asked to wipe it. The name is misleading, it doesn't matter if by some magic every UTXO ends up spent.

Thanks, so is the scenario specifically the case when a node started with -reindex is stopped before the final ActivateBestChain call? If it is stopped during the final ActivateBestChain call it seems like it would be picking up work where it left off when ActivateBestChain was called again, due to LoadChainTip setting the previous tip.

we will call already ActivateBestChain early, when the best header is somewhere mid-chain, connect a first batch of blocks to the chainstate, then reindex the remaining block files, and then connect a second batch.

This is referring to the ActivateBestChain call in LoadExternalBlockFile call you linked to earlier? And removing this call would fix the weirdness and assumevalid issue? It does seem like it would be nice to remove this call. I'm not sure what the point of connecting the genesis block early but not connecting any other blocks is.

Thanks, so is the scenario specifically the case when a node started with -reindex is stopped before the final ActivateBestChain call?

Yes.

This is referring to the ActivateBestChain call in LoadExternalBlockFile call you linked to earlier?

Yes!

And removing this call would fix the weirdness and assumevalid issue? It does seem like it would be nice to remove this call. I'm not sure what the point of connecting the genesis block early but not connecting any other blocks is.

I don't think we'd want to remove it completely, because in case of a -reindex it allows init to complete normal startup without having to wait for the import thread to finish reindexing block files (which can take a long time for mainnet). I think that @l0rinc is looking into possible fixes, but without having tested anything my first idea was to only call ActivateBestChain there it if the chain is empty, so it only doesn't get called in the restart scenario where we already loaded Genesis during LoadChainTip.

This is in my backlog, but I would be very happy if someone who already understands this fixes it instead :)
I just noticed it during Core Dev that my perf logs are full of script validations when trying out different scenarios and cancelling a reindex.

This is in my backlog, but I would be very happy if someone who already understands this fixes it instead :)

Ok thanks, I'll try out my suggestion from above and will open a PR if it's as straightforward as I think.

#31439

This comment change is not correct and should just be dropped: m_blockfiles_indexed is true (its default value) in case of an empty datadir, false in case of a reindex.

Ok, then I'm confused about what causes case (1): #31346 (comment)

With an empty datadir, we don't have the genesis connected at this point. But we don't enter the reindex if clause here (the comment only refers to that), we just skip this and the loadblock block and go to the ActivateBestChain call below which connects Genesis and allows the init thread to continue.