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. A summary of reviews will appear here.

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.

Contributors
furszy DrahtBot maflcko mzumsande

Labels
Validation