bugfix: update chainman best_header after block reconsideration #29913

1. 
   furszy commented at 12:19 pm on April 19, 2024: member

   Fixes #26245. I’m doing it because it came to my mind after reviewing #28339.

   Inside the invalidateblock process, we update the best_header by manually calling InvalidChainFound after disconnecting blocks.

   We need to do the same for reconsiderblock and update the chain best_header field after finishing the process.

   Note: the only difference between this two commands is that reconsiderblock does not have its own separate function, it’s a plain RPC command that resets the block index flag and calls ActivateBestChain.

   The only place we forwardly update the best_header field is within AddToBlockIndex which is not called by reconsiderblock since the block is already in the block index. Thus why we need to manually update it after reconsidering a block.

   Testing Notes:

   • Comment the fix line and run the rpc_invalidateblock.py test.
   • Or, call invalidateblock and compare the getblockchaininfo() ‘headers’ num value against getblockcount(), then do the same after calling reconsiderblock().
2. bugfix: update chainman best_header after block reconsideration

   Inside the 'invalidateblock' process, we update the best_header by
   manually calling `InvalidChainFound` after disconnecting blocks.
   
   We need to do the same for 'reconsiderblock' and update the
   'best_header' field after finishing the process.
   
   Note: the only difference between this two commands is that
   ‘reconsiderblock’ does not have its own separate function,
   it's a plain RPC command that resets the block index flag
   and calls 'ActivateBestChain'.
   
   The only place we forwardly update the best_header field is
   within 'AddToBlockIndex' which is not called by 'reconsiderblock'
   since the block is already in the block index.

   5fb0b385a3
3. 
   DrahtBot commented at 12:19 pm on April 19, 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.

4. in src/rpc/blockchain.cpp:1620 in 5fb0b385a3

   1615@@ -1616,6 +1616,9 @@ static RPCHelpMan reconsiderblock()
   1616         throw JSONRPCError(RPC_DATABASE_ERROR, state.ToString());
   1617     }
   1618 
   1619+    // Update best header
   1620+    WITH_LOCK(::cs_main, chainman.m_best_header = chainman.ActiveChain().Tip());
   

   ---

   ---

   

   maflcko commented at 12:39 pm on April 19, 2024:

   Conceptually, this seems the wrong place for the fix. It would be nice if this was handled internally in chainman correctly.

   Also, given that cs_main is held only for this line, a race exits, where the getchainstates RPC is called before this line, and still returns the wrong result.

   ---

   

   furszy commented at 1:02 pm on April 19, 2024:

   Conceptually, this seems the wrong place for the fix. It would be nice if this was handled internally in chainman correctly.

   I did that initially (see #26260 - #26260 (comment)) but.. conceptually, all what is inside reconsiderblock seems to be at the wrong place for me too. We are resetting a block index flag at the RPC level. Ideally, we should encapsulate all this code within chainman too.

   Also, see #26260#pullrequestreview-1132786161 for the places where we set this field. We either set it when a new block is received, during startup or manually inside invalidateblock(). Thus why I think that setting it inside reconsiderblock is also “fine” (it is what we are already doing for its counterpart anyway).

   That being said, I’m also ok going with 52f5d62 if that gets more traction too. Both options fixes the issue.

   Also, given that cs_main is held only for this line, a race exits, where the getchainstates RPC is called before this line, and still returns the wrong result.

   True. But, as RPC calls are blocking and this RPC command is usually a one-time only not critical call, I think that it is a harmful race.

5. laanwj added the label Validation on Apr 22, 2024
6. 
   mzumsande commented at 6:31 pm on April 22, 2024: contributor

   In my opinion setting it to ActiveTip() is not correct because m_best_header was not necessarily equal to ActiveTip() in the first place (before invalidateblock) :

   During IBD they differ a lot (and invalidateblock / resonsiderblock is allowed then, too!). So after resonsiderblock it would still be set to the wrong value. I could verify that on signet via getblockchaininfo shortly after downloading the headers.

   I think that the correct approach would be to iterate through the header tree again and find the best-work chain of BLOCK_VALID_TREE headers - even if that sounds more complicated.

   [Edit:] Forgetting about reconsiderblock for a moment, I think the same should also be done during InvalidChainFound(): Currently, if we invalidate a block we roll the chain back from the original tip T1 to the last valid block X and set chainman.m_best_header to X. But there may be a chain fork from X to another tip T2 (with less work than T1, but more work than X) for which we only have headers. Seems to me that the correct thing to do would be to set m_best_header to T2 instead of X.

7. 
   fjahr commented at 8:39 pm on August 13, 2024: contributor

   Concept ACK @furszy Are you still working on this? I need a fix for this to do better testing in #29553.
8. 
   mzumsande commented at 8:44 pm on August 13, 2024: contributor

   I had a branch with a different solution that involves iterating over the entire block index (which is not super-efficient, but if it only is necessary in certain cases such as invalidateblock it may be ok?!): https://github.com/mzumsande/bitcoin/tree/202404_invalidblock I think I’ll clean that branch up in the next days and PR it as an alternative.
9. 
   fjahr commented at 8:47 pm on August 13, 2024: contributor

   I think I’ll clean that branch up in the next days and PR it as an alternative.

   That would be great, thanks!

10. 
    furszy commented at 9:03 pm on August 13, 2024: member

    IIRC, we have #30207 containing @mzumsande’s fix branch and my intention was to leave this open while was reviewing the other one but.. it seems I never pushed my review there.
11. 
    fjahr commented at 9:10 pm on August 13, 2024: contributor

    #30207 isn’t marked as fixing #26245 and also misses test coverage in rpc_invalidateblock so I think it’s something different than the branch @mzumsande mentions?
12. 
    mzumsande commented at 9:12 pm on August 13, 2024: contributor

    #30207 doesn’t contain a fix for the m_best_header issue, it just cautions against using m_best_header for anything important (see first commit) and fixes some other related bugs.
13. 
    furszy commented at 9:31 pm on August 13, 2024: member

    Ok, just remembered it. #30207 shares part of the fix branch, and thats why I moved there first. @mzumsande, guess that your upcoming PR will continue containing the same shared commit “validation: mark blocks between m_best_header and invalid block as BLOCK_FAILED_CHILD” ?

    So in any case, we could move forward with both PRs #30207 and the new one now that there is some review power :).

14. 
    mzumsande commented at 8:50 pm on August 16, 2024: contributor

    I opened #30666

    @mzumsande, guess that your upcoming PR will continue containing the same shared commit “validation: mark blocks between m_best_header and invalid block as BLOCK_FAILED_CHILD” ?

    Yes, I included that, without these block marked as BLOCK_FAILED_CHILD it m_best_header could still be incorrect.

15. 
    furszy commented at 1:34 pm on August 19, 2024: member

    closing in favor of #30666.
16. furszy closed this on Aug 19, 2024

    ---

Contributors
furszy DrahtBot maflcko mzumsande fjahr

Labels
Validation