assumevalid is not always applied when reindexing #31494

1. 
   mzumsande commented at 6:07 pm on December 13, 2024: contributor

   assumevalid (which disables script verification and therefore speeds up IBD) is not always applied in the context of a -reindex:

   The assumevalid criteria are https://github.com/bitcoin/bitcoin/blob/d73f37dda221835b5109ede1b84db2dc7c4b74a1/src/validation.cpp#L2497-L2500

   meaning that our best known header needs to have at least as much work than the hard-coded minchainwork. During normal IBD, we don’t connect blocks before having minchainwork headers (headers sync phase), so this is not a problem. However,-reindex deletes the block tree db and attempts to rebuild it based on local block files, without attempting to re-request headers for which we don’t have the full block yet (because we didn’t get to that point in the previous IBD).

   Therefore, users that encounter an error somewhere midway through IBD and need to reindex, won’t use -assumevalid and will rebuild the chainstate slowly. This has also been encountered in the context of benchmarking.

   Possible solutions:

   • always enable assumvalid while connecting blocks in the context of reindexing
   • attempt to sync remaining headers via the p2p nework after rebuilding the block tree db, but before calling ActivateBestChain() to start connecting blocks
2. 
   Eunovo commented at 6:46 pm on January 6, 2025: none

   I added a test reproducing this bug https://github.com/eunovo/bitcoin/commit/5e1ff5fe2c7374f97eb56454099c1b235c75e23c
3. 
   Eunovo commented at 6:48 pm on January 6, 2025: none

   • always enable assumvalid while connecting blocks in the context of reindexing

   Isn’t it still valuable to check that the current block is an ancestor of the most work chain?

4. 
   mzumsande commented at 9:34 pm on January 6, 2025: contributor

   I added a test reproducing this bug https://github.com/Eunovo/bitcoin/commit/5e1ff5fe2c7374f97eb56454099c1b235c75e23c

   Thanks, nice test (which should fail on master if I understand it correctly)!

   Isn’t it still valuable to check that the current block is an ancestor of the most work chain?

   Keeping the check doesn’t hurt, but I think in the situation of a reindex this should automatically be the case:

   Outside of a reindex, we could have additional headers received via p2p for which we don’t have the block yet, so that the m_chainman.m_best_header isn’t necessarily the best block we are trying to activate (we can’t fully validate blocks for which we just have the header). But during a reindex, we have disabled p2p header download and our only source for blocks and headers is the disk, so we cannot be in a situation where we have headers for which we don’t have the full block. We might have some full blocks on disk that don’t lead to the best header, but we shouldn’t attempt to connect them in the first place in ActivateBestChain(), which attempts to connect towards the most-work chain for which we have all the block data (see FindMostWorkChain).

5. 
   Eunovo commented at 6:31 am on January 7, 2025: none

   Thanks, nice test (which should fail on master if I understand it correctly)!

   Yes, it does.

Contributors
mzumsande Eunovo