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

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".

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

I wonder if this is fixed now?

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

diff --git a/src/validation.cpp b/src/validation.cpp
index 37e68cfe4a..811ff2f9eb 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2201,7 +2201,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
         {
             CAmount txfee = 0;
             TxValidationState tx_state;
-            if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) {
+            if (Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) {
                 // Any transaction validation failure in ConnectBlock is a block consensus failure
                 state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
                             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.

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?

I'm looking into this

Draft PR: #27434

I believe this issue was closed by #30666 @mzumsande

Confirmed by running a test I wrote for a draft PR

before #30666:

[{'height': 20, 'hash': '2182ec6e445c3869f7d17bcf2bcae03c286256cf7359b3c7c834cee3ad029a3f', 'branchlen': 10, 'status': 'headers-only'}, {'height': 10, 'hash': '2e13e49c2f95b6b17c8eece52ce971ce5610a9c8cd824b7be139fcbadae6d0b2', 'branchlen': 0, 'status': 'active'}]
[{'height': 20, 'hash': '2182ec6e445c3869f7d17bcf2bcae03c286256cf7359b3c7c834cee3ad029a3f', 'branchlen': 10, 'status': 'invalid'}, {'height': 10, 'hash': '2e13e49c2f95b6b17c8eece52ce971ce5610a9c8cd824b7be139fcbadae6d0b2', 'branchlen': 0, 'status': 'active'}]

current master:

[{'height': 20, 'hash': '4e3034b8076779935fe18496eee19a53317f22b65d9d50aa0e1a0bb0e904ad09', 'branchlen': 10, 'status': 'invalid'}, {'height': 10, 'hash': '2fae72feac2dfa268ea00747309b21aafad6e0f702bd57144e21bdd865dee838', 'branchlen': 0, 'status': 'active'}]
[{'height': 20, 'hash': '4e3034b8076779935fe18496eee19a53317f22b65d9d50aa0e1a0bb0e904ad09', 'branchlen': 10, 'status': 'invalid'}, {'height': 10, 'hash': '2fae72feac2dfa268ea00747309b21aafad6e0f702bd57144e21bdd865dee838', 'branchlen': 0, 'status': 'active'}]