getchaintips doesn’t mark headers-only chain as invalid #8050

1. 
   instagibbs commented at 8:29 pm on May 12, 2016: member

   During headers-first downloading, if an invalid block is found, the client stops syncing that branch, but forever lists it as “headers-only”. It seems to me it should be marked as “invalid”. This is due to the subsequent headers not being marked “BLOCK_FAILED_CHILD”.
2. jonasschnelli added the label P2P on May 14, 2016
3. 
   TheBlueMatt commented at 10:01 pm on December 6, 2017: member

   This got much better in 0.15 (and is fully fixed upon node restart), but to (fully) fix would require something like https://github.com/TheBlueMatt/bitcoin/tree/2017-10-best-header-tracking
4. MarcoFalke added the label Block storage on Mar 23, 2018
5. 
   glozow commented at 2:24 pm on October 24, 2022: member

   I wonder if this is fixed now?
6. 
   MarcoFalke commented at 2:47 pm on October 24, 2022: member

   Probably still an issue, I guess? You can check with something like:

    0diff --git a/src/validation.cpp b/src/validation.cpp
    1index 37e68cfe4a..811ff2f9eb 100644
    2--- a/src/validation.cpp
    3+++ b/src/validation.cpp
    4@@ -2201,7 +2201,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
    5         {
    6             CAmount txfee = 0;
    7             TxValidationState tx_state;
    8-            if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) {
    9+            if (Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) {
   10                 // Any transaction validation failure in ConnectBlock is a block consensus failure
   11                 state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
   12                             tx_state.GetRejectReason(), tx_state.GetDebugMessage());
   

   then call ./src/qt/bitcoin-qt -datadir=/tmp -signet -printtoconsole=1 and observe that getchaintips only corrects on a restart.

7. MarcoFalke removed the label P2P on Oct 24, 2022
8. MarcoFalke removed the label Block storage on Oct 24, 2022
9. MarcoFalke added the label Validation on Oct 24, 2022
10. 
    pinheadmz commented at 3:43 pm on March 22, 2023: member

    I’m looking into this, and I’ve written a functional test that reproduces the issue. One thing we could do is add some extra logic into getchaintips itself:

    • Populate setTips
    • Iterate through setTips and set status strings
    • Then we could add this here:
      • If the status is "headers-only", walk that branch from tip to main chain
      • If an ancestor with BLOCK_FAILED_MASK is found, reverse walk back to tip adding BLOCK_FAILED_CHILD flag and inserting into m_dirty_blockindex (like we do on startup in LoadBlockIndex)

    I think the only factors that would make this take too long are the number of headers-only chaintips and the branch length of those tips. My VPS full node only has one such tip, with a length of 1. I can check my longer-running node at home later but I expect it to be a trivial amount of extra work for this RPC call.

    Is it hacky to trigger status-updating logic like this inside an RPC call vs a more comprehensive solution like @TheBlueMatt branch?

11. 
    pinheadmz commented at 8:26 pm on April 6, 2023: member

    I’m looking into this

    Draft PR: #27434

12. pinheadmz assigned pinheadmz on Jun 2, 2023

Contributors
instagibbs TheBlueMatt glozow MarcoFalke pinheadmz

Labels
Validation