Update best_header inside Connect/DisconnectTip

aureleoules commented at 3:22 pm on October 5, 2022: member

Fixes #26245. From furszy:

Update best_header inside Connect/DisconnectTip This solves a problem where best_header isn’t updated in case of forced reorg like a block invalidation and reconsideration.

I added a test.

DrahtBot added the label RPC/REST/ZMQ on Oct 5, 2022

glozow requested review from dergoegge on Oct 6, 2022

in src/rpc/blockchain.cpp:1572 in 1882ca227a outdated

1568@@ -1569,6 +1569,7 @@ static RPCHelpMan reconsiderblock()
1569             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
1570         }
1571 
1572+        chainman.m_best_header = pblockindex;

dergoegge commented at 11:08 am on October 6, 2022:

What happens if the ActivateBestChain() below fails? You set m_best_header but the reconsiderblock call failed.

pblockindex is also not guaranteed to be the index for the tip of the reconsidered chain, so you end up with the same inconsistent output as described in #26245. You can test that invalidating and reconsidering an older block, e.g.:

0bitcoin-cli getblockchaininfo
1bitcoin-cli invalidateblock <blockhash at height=tip-5>
2bitcoin-cli getblockchaininfo
3bitcoin-cli reconsiderblock <blockhash at height=tip-5>
4bitcoin-cli getblockchaininfo

dergoegge changes_requested

dergoegge commented at 11:25 am on October 6, 2022: member

I think this is a validation bug not a rpc bug, so setting m_best_header (validation specific state) on an rpc call does not seem right to me.

Currently, we only ever set m_best_header in three places:

When we add a new header to our index:

https://github.com/bitcoin/bitcoin/blob/5e82b9ba96b6c5614a1187382a086e5694dff544/src/validation.cpp#L3676

When an invalid block is found:

https://github.com/bitcoin/bitcoin/blob/5e82b9ba96b6c5614a1187382a086e5694dff544/src/validation.cpp#L1606

Or upon startup while loading the block index into memory:

https://github.com/bitcoin/bitcoin/blob/5e82b9ba96b6c5614a1187382a086e5694dff544/src/validation.cpp#L4268

afaict anytime we are reorging to an already known chain (i.e. we already have the headers in our index) we will not set m_best_headers until a new header is added to our index on that chain.

aureleoules marked this as a draft on Oct 6, 2022

Sjors commented at 12:09 pm on October 6, 2022: member

I’ll try this patch on ForkMonitor, at least for the v0.21.2 node (using g_best_block). We run two nodes that constantly roll back the chain in order to call gettxoutsetinfo on them (no longer needed for new nodes thanks to #24539) as well as to fetch stale blocks that we only have headers for. (and then it would call reconsiderblock).

I’ve occasionally gotten error emails where the resulting tip was not what was expected (and I think these happen just after a rollback).

That said, I agree with @dergoegge that we probably shouldn’t touch m_best_header directly from RPC code.

aureleoules force-pushed on Oct 6, 2022

aureleoules commented at 12:25 pm on October 6, 2022: member

Thanks @dergoegge and @Sjors. I think you are correct, I’ve moved the statement inside ActivateBestChain in validation.cpp. Is it better? I also added your test vector @dergoegge.

aureleoules marked this as ready for review on Oct 6, 2022

Sjors commented at 12:40 pm on October 6, 2022: member

It seems like a better place. I’ll have to think more about the change itself of course. Paging @ryanofsky and @jamesob who hang out in this part of the consensus code more often…

in src/validation.cpp:3021 in 092ca7f667 outdated

3017@@ -3018,6 +3018,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
3018                     pindexMostWork = nullptr;
3019                 }
3020                 pindexNewTip = m_chain.Tip();
3021+                m_chainman.m_best_header = pindexNewTip;

furszy commented at 12:57 pm on October 6, 2022:

Pretty sure that this isn’t correct, the headers-only chain is synchronized first. So, during IBD, it will be ahead of the “tip” block that we are connecting, and you are going to overwrite it.

dergoegge commented at 12:58 pm on October 6, 2022:

0                if(pindexNewTip->nChainWork > m_chainman.m_best_header->nChainWork) {
1                    m_chainman.m_best_header = pindexNewTip;
2                }

aureleoules marked this as a draft on Oct 6, 2022

furszy commented at 2:09 pm on October 6, 2022: member

Quick thing: in case of regular headers-sync, we only reorg if the chain has a higher diff, so best_header field is fine there.

In case of forced reorgs, like a block invalidation + reconsideration, we aren’t reorganizing the chain due PoW, thus why the best_header isn’t updated (we currently only update the best_header field after adding it to the block index, inside AddToBlockIndex).

So, for those cases, could either update best_header inside ConnectTip and DisconnectTip like https://github.com/furszy/bitcoin-core/commit/52f5d62957ad0d9e2b2c8d00333a369fb4a369c7 or.. update it inside the reconsider block process only (the invalidate block process is fine because we are updating the best_header inside InvalidChainFound).

aureleoules force-pushed on Oct 12, 2022

aureleoules marked this as ready for review on Oct 12, 2022

aureleoules commented at 3:52 pm on October 12, 2022: member

Thanks @furszy, I’ve cherry-picked your commit.

in src/validation.cpp:2637 in 160a8fe66c outdated

2631@@ -2632,6 +2632,12 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra
2632 
2633     m_chain.SetTip(*pindexDelete->pprev);
2634 
2635+    // if we are roll-backing the chain, rollback the best header as well
2636+    if (m_chainman.m_best_header && m_chainman.m_best_header->GetBlockHash() == pindexDelete->GetBlockHash()
2637+        && pindexDelete->pprev) {

luke-jr commented at 5:05 pm on October 22, 2022:

Why the pprev check? If we really need that, maybe an assert would be better?

in src/validation.cpp:2756 in 160a8fe66c outdated

2750@@ -2745,6 +2751,12 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew,
2751     }
2752     // Update m_chain & related variables.
2753     m_chain.SetTip(*pindexNew);
2754+
2755+    // If the best header is the prev block, update the best header
2756+    if (pindexNew->pprev && m_chainman.m_best_header && m_chainman.m_best_header->GetBlockHash() == pindexNew->pprev->GetBlockHash()) {

luke-jr commented at 5:07 pm on October 22, 2022:

0    if (pindexNew->pprev == m_chainman.m_best_header) {

?

luke-jr changes_requested

luke-jr commented at 5:08 pm on October 22, 2022: member

This feels like a special-cased hack. Is there a reason we can’t adjust the current code that changes m_best_header when an invalid block is encountered?

(Also, PR title needs updating.)

validation: bugfix, update best_header inside Connect/DisconnectTip

This solves a problem where best_header isn't updated in case of forced reorg
like a block invalidation and reconsideration.

4a9ce1053d

test: Check reconsiderblock sets best header 0b975f51a4

aureleoules force-pushed on Nov 15, 2022

aureleoules renamed this:
~~rpc: Set best header after reconsiderblock~~
Update best_header inside Connect/DisconnectTip
on Nov 15, 2022

aureleoules commented at 3:35 pm on November 15, 2022: member

Thanks @luke-jr for the review.

This feels like a special-cased hack. Is there a reason we can’t adjust the current code that changes m_best_header when an invalid block is encountered?

I believe it is more robust to place the fix in ConnectTip and DisconnectTip. cc @furszy

furszy commented at 1:55 pm on November 24, 2022: member

This feels like a special-cased hack. Is there a reason we can’t adjust the current code that changes m_best_header when an invalid block is encountered?

I actually wrote two ways to fix this above #26260 (comment).

The problem is on the block reconsideration flow, not inside the invalidation one.

Inside the invalidateblock process, we update the best_header by manually calling InvalidChainFound after invalidating blocks. Which is fine.

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.

So, @aureleoules, could drop the other commit and cherry-pick this change to fix the problem too: https://github.com/furszy/bitcoin-core/commit/4f3318c684af631d7f79b872cd7b8d4feae4b828

Both works. This last one is specific to the reconsiderblock flow (which is where we have the issue).

DrahtBot commented at 1:56 pm on November 24, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	pinheadmz

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

achow101 requested review from Sjors on Apr 25, 2023

Sjors commented at 3:55 pm on May 8, 2023: member

I deployed this patch on the two ForkMonitor (mirror) nodes that frequently use invalidateblock and reconsiderblock. It’s cherry-picked on top of the v25.0rc1 and v24.1rc2 tags. Typically we get about one error message per week where the block it rewinds to is not the one we expected. We’ll see if that stops.

in src/validation.cpp:2639 in 0b975f51a4

2631@@ -2632,6 +2632,12 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra
2632 
2633     m_chain.SetTip(*pindexDelete->pprev);
2634 
2635+    // if we are roll-backing the chain, rollback the best header as well
2636+    if (m_chainman.m_best_header && m_chainman.m_best_header->GetBlockHash() == pindexDelete->GetBlockHash()) {
2637+        assert(pindexDelete->pprev);
2638+        m_chainman.m_best_header = pindexDelete->pprev;
2639+    }

pinheadmz commented at 6:52 pm on May 11, 2023:

Tests still pass with this chunk removed. I wonder if checking that getblockchaininfo headers == blocks isn’t enough? Might need to use getchaintips to cover.

pinheadmz changes_requested

pinheadmz commented at 6:54 pm on May 11, 2023: member

concept ACK

one question below.

in test/functional/rpc_invalidateblock.py:24 in 0b975f51a4

19@@ -20,6 +20,10 @@ def set_test_params(self):
20     def setup_network(self):
21         self.setup_nodes()
22 
23+    def check_blocks_headers(self, node):
24+        blockchaininfos = node.getblockchaininfo()

pinheadmz commented at 6:55 pm on May 11, 2023:

micro-nit, this is a silly variable name. i think it’s fine, but it is silly.

DrahtBot commented at 9:19 am on August 8, 2023: contributor

There hasn’t been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot added the label CI failed on Aug 30, 2023

DrahtBot removed the label CI failed on Sep 4, 2023

achow101 commented at 4:38 pm on September 20, 2023: member

Closing as up for grabs due to lack of activity.

achow101 closed this on Sep 20, 2023

achow101 requested review from furszy on Sep 20, 2023

Update best_header inside Connect/DisconnectTip #26260

Reviews