assumeutxo: Don’t load a snapshot if it’s not in the best header chain #30320

1. 
   mzumsande commented at 3:34 pm on June 21, 2024: contributor

   This was suggested by me in the discussion of #30288, which has more context.

   If the snapshot is not an ancestor of the most-work header (m_best_header), syncing from that alternative chain leading to m_best_header should be prioritised. Therefore it’s not useful loading the snapshot in this situation. If the other chain turns out to be invalid or the chain with the snapshot retrieves additional headers so that it’s the most-work one again (see functional test), m_best_header will change and loading the snapshot will be possible again.

   Because of the work required to generate a conflicting headers chain, a situation with two conflicting chains should only be possible under extreme circumstances, such as major forks.

   Includes the first commit from #30267 (which cleans up the loadtxoutset RPC error handling), so I’ll keep this in draft until that is merged.

2. refactor: Move early loadtxoutset checks into ActiveSnapshot

   Also changes the return type of ActiveSnapshot to allow returning the
   error message to the user of the loadtxoutset RPC.

   81e44391b8
3. validation: Don't load a snapshot if it's not in the best header chain.

   If the snapshot is not an ancestor of the most-work header (m_best_header),
   syncing from that alternative chain should be prioritised.
   Therefore don't accept loading a snapshot in this situation.
   
   If that other chain turns out to be invalid, m_best_header
   would be reset and loading the snapshot should be possible again.
   
   Because of the work required to generate a conflicting headers chain,
   this should only be possible under extreme circumstances, such as major forks.

   055af4d137
4. 
   DrahtBot commented at 3:34 pm on June 21, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	fjahr

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
   • #30329 (fuzz: improve utxo_snapshot target by mzumsande)
   • #30267 (assumeutxo: Check snapshot base block is not in invalid chain by fjahr)
   • #29996 (Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests by alfonsoromanz)
   • #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

   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.

5. in src/validation.cpp:5680 in 055af4d137

   5675@@ -5676,6 +5676,9 @@ util::Result<void> ChainstateManager::ActivateSnapshot(
   5676             return util::Error{strprintf(_("The base block header (%s) must appear in the headers chain. Make sure all headers are syncing, and call loadtxoutset again."),
   5677                           base_blockhash.ToString())};
   5678         }
   5679+        if (!m_best_header || m_best_header->GetAncestor(base_blockheight) != snapshot_start_block) {
   5680+            return util::Error{_("A forked headers-chain with more work than the chain with the base block header exists. Please proceed to sync without AssumeUtxo.")};
   

   ---

   ---

   

   fjahr commented at 3:39 pm on June 22, 2024:

   nit: Not sure if users understand what “base block” is. Maybe make it “snapshot base block”.

   ---

   

   mzumsande commented at 5:44 pm on June 24, 2024:

   will change to that when taking it out of draft.
6. 
   fjahr commented at 3:42 pm on June 22, 2024: contributor

   Concept ACK
7. DrahtBot added the label Needs rebase on Jul 2, 2024
8. 
   DrahtBot commented at 9:50 pm on July 2, 2024: contributor

   🐙 This pull request conflicts with the target branch and needs rebase.

Contributors
mzumsande DrahtBot fjahr

Labels
Needs rebase