validation: fix m_best_header tracking and BLOCK_FAILED

mzumsande commented at 5:36 pm on August 16, 2024: contributor

m_best_header (the most-work header not known to be on an invalid chain) can be wrong in the context of invalidation / reconsideration of blocks. This can happen naturally (a valid header is received and stored in our block tree db; when the full block arrives, it is found to be invalid) or triggered by the user with the invalidateblock / reconsiderblock rpc.

We don’t currently use m_best_header for any critical things (see OP of #16974 for a list that still seems up-to-date), so it being wrong affects mostly rpcs.

This PR proposes to recalculate it if necessary by looping over the block index and finding the best header. It also suggest to mark headers between an invalidatetd block and the previous m_best_header as invalid, so they won’t be considered in the recalculation. It adds tests to rpc_invalidateblock.py and rpc_getchaintips.py that fail on master.

One alternative to this suggested in the past would be to introduce a continuous tracking of header tips (#12138). While this might be more performant, it is also more complicated, and situations where we need this data are only be remotely triggerable by paying the cost of creating a valid PoW header for an invalid block. Therefore I think it isn’t necessary to optimise for performance here, plus the solution in this PR doesn’t perform any extra steps in the normal node operation where no invalidated blocks are encountered.

Fixes #26245

DrahtBot commented at 5:36 pm on August 16, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30666.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fjahr, TheCharlatan, achow101
Approach ACK	jonatack

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Validation on Aug 16, 2024

in src/validation.cpp:6416 in 4b444ebdc3 outdated

6409@@ -6410,6 +6410,20 @@ std::optional<int> ChainstateManager::GetSnapshotBaseHeight() const
6410     return base ? std::make_optional(base->nHeight) : std::nullopt;
6411 }
6412 
6413+void ChainstateManager::RecalculateBestHeader()
6414+{
6415+    AssertLockHeld(cs_main);
6416+    // If, due to invalidation / reconsideration of blocks, the previous

fjahr commented at 11:41 am on August 18, 2024:

Could move this comment into validation.h and make it available for doxygen.

mzumsande commented at 5:25 pm on September 16, 2024:

done

in test/functional/rpc_getchaintips.py:84 in c9bf06a531 outdated

79+        self.log.info("Submit headers-only chain")
80+        n0.submitheader(invalid_block.serialize().hex())
81+        n0.submitheader(block2.serialize().hex())
82+        tips = n0.getchaintips()
83+        assert_equal(len(tips), 3)
84+        assert_equal(tips[0]['height'], start_height + 2)

fjahr commented at 11:58 am on August 18, 2024:

nit: not sure if this needs to be check over the life time of the test. master only fails on the last added line so I feel this is a bit verbose and distracting from what this test is actually added for.

mzumsande commented at 5:25 pm on September 16, 2024:

Removed the line here and below

fjahr commented at 12:25 pm on August 18, 2024: contributor

tACK c9bf06a531617fdd70b64e23572931af2b969828

I reviewed the code and confirmed manually that all the added tests do test the behavior changes in their commits.

I would also say this is a bugfix, so the title could be changed to “Fix m_best_header tracking” and get a bugfix label.

in test/functional/rpc_invalidateblock.py:53 in 7d42b0e4c2 outdated

48+        block = create_block(tip, create_coinbase(self.nodes[0].getblockcount()), block_time, version=4)
49+        block.solve()
50+        self.nodes[0].submitheader(block.serialize().hex())
51 
52         self.log.info("Invalidate block 2 on node 0 and verify we reorg to node 0's original chain")
53+        badhash = self.nodes[1].getblockhash(2)

TheCharlatan commented at 8:08 pm on August 18, 2024:

It might be a good idea to expand this a bit:

 0@@ -45 +45 @@ class InvalidateTest(BitcoinTestFramework):
 1-        tip = int(self.nodes[0].getbestblockhash(), 16)
 2+        tip = self.nodes[0].getbestblockhash()
 3@@ -47 +47 @@ class InvalidateTest(BitcoinTestFramework):
 4-        block = create_block(tip, create_coinbase(self.nodes[0].getblockcount()), block_time, version=4)
 5+        block = create_block(int(tip, 16), create_coinbase(self.nodes[0].getblockcount()), block_time, version=4)
 6@@ -49,0 +50 @@ class InvalidateTest(BitcoinTestFramework):
 7+        assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"] + 1)
 8@@ -56,0 +58,11 @@ class InvalidateTest(BitcoinTestFramework):
 9+        assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"])
10+
11+        self.log.info("Reconsider block 6 on node 0 again and verify that the best header is set correctly")
12+        self.nodes[0].reconsiderblock(tip)
13+        assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"] + 1)
14+
15+        self.log.info("Invalid block 2 on node 0 and verify we reorg to node 0's original chain again")
16+        self.nodes[0].invalidateblock(badhash)
17+        assert_equal(self.nodes[0].getblockcount(), 4)
18+        assert_equal(self.nodes[0].getbestblockhash(), besthash_n0)
19+        # Should report consistent blockchain info

This additionally checks that the new header can be reconsidered again correctly (BLOCK_FAILED_CHILD is indeed reset). It’s a bit verbose and repetitive though and following along with the various chainstates is already a bit annoying, so not sure if it delivers enough value.

mzumsande commented at 5:26 pm on September 16, 2024:

Done - even if it’s a little bit repetitive, I agree it’s good to have coverage for this.

TheCharlatan approved

TheCharlatan commented at 8:21 pm on August 18, 2024: contributor

ACK c9bf06a531617fdd70b64e23572931af2b969828

in src/validation.cpp:6419 in c9bf06a531 outdated

6424+    m_best_header = ActiveChain().Tip();
6425+    for (auto& entry : m_blockman.m_block_index) {
6426+        if (!(entry.second.nStatus & BLOCK_FAILED_MASK) && m_best_header->nChainWork < entry.second.nChainWork) {
6427+            m_best_header = &entry.second;
6428+        }
6429+    }

TheCharlatan commented at 8:22 am on August 23, 2024:

I’ve been looking the logic over again and think it might be a good idea to call NotifyHeaderTip here too. It basically tracks along m_best_header, but with the current code will only notify when processing the next block or header.

mzumsande commented at 5:26 pm on September 16, 2024:

Makes sense. I’ve added a call to NotifyHeaderTip.

mzumsande commented at 5:36 pm on September 16, 2024:

actually, that didn’t work because NotifyHeaderTip can’t be called when cs_main is held. Reverting this, I’ll think about this a bit more.

TheCharlatan commented at 1:56 pm on September 17, 2024:

Ah right, I don’t think this a blocker for this PR anyway, just something that came to my mind.

mzumsande commented at 3:36 pm on September 17, 2024:

I thought about it a bit and don’t see a great non-invasive solution to this: We could add a call to NotifyHeaderTip in the calling functions after cs_main is released, which would be in ActivateBestChain() (plus in the invalidateblock / reconsiderblock rpcs which seem less important), and I’m unsure if we want to refactor this function to always call NotifyBestHeader() just for the special case where a connected block turns out to be invalid. In any case, since m_best_header is already being reset in InvalidChainFound() in master (just not always correctly) this would not be a regression introduced by this PR, so I think it’s ok not to address it here for now.

Yygik commented at 12:07 pm on November 15, 2024:

各方面看过嗯结果OK

in src/validation.cpp:2065 in 7d42b0e4c2 outdated

2059@@ -2060,6 +2060,11 @@ void Chainstate::InvalidChainFound(CBlockIndex* pindexNew)
2060         m_chainman.m_best_invalid = pindexNew;
2061     }
2062     if (m_chainman.m_best_header != nullptr && m_chainman.m_best_header->GetAncestor(pindexNew->nHeight) == pindexNew) {
2063+        CBlockIndex* index_walk{m_chainman.m_best_header};
2064+        while (index_walk != pindexNew) {
2065+            index_walk->nStatus |= BLOCK_FAILED_CHILD;

mzumsande commented at 10:45 am on August 24, 2024:

I think that this is not a complete fix. This would only mark indexes between m_best_header and the failed block as BLOCK_FAILED_CHILD, but if this was not a linear chain and there were forked blocks, those wouldn’t be marked as BLOCK_FAILED_CHILD. I think I’ll change the logic to also set BLOCK_FAILED_CHILD while iterating over the entire block index.

mzumsande commented at 5:28 pm on September 16, 2024:

I’ve reworked this by introducing SetBlockFailureFlags (as a counterpart to ResetBlockFailureFlags that removes the flags after reconsiderblock)

mzumsande commented at 11:02 am on August 24, 2024: contributor

Thanks for the reviews so far! Ill address feedback soon, plus I want to rework 7d42b0e4c2722b398c73695d5547fbf8bd2ae01c, see comment above. I’m currently on holidays, so it might take a few days until I make the changes.

One more issue I’ve noticed while writing the tests this PR is that if we connect an invalid block received via p2p, InvalidChainFound() is called twice: The first time in ConnectTip(), via InvalidBlockFound() here
The second time in ActivateBestChainStep() after ConnectTip() fails due to an invalid block (here). Does anyone know if there is a reason for this? It seems to me that the call in ActivateBestChainStep() is unnecessary and could be dropped.

luke-jr commented at 11:48 pm on August 26, 2024: member

It also suggest to mark headers between an invalidatetd block and the previous m_best_header as invalid, so they won’t be considered in the recalculation.

You mean if you invalidateblock, then reconsiderblock, the rest of the child blocks also need to be reconsidered individually? That seems like a conceptual bug IMO, backward incompatible, and quite annoying.

mzumsande commented at 7:05 am on August 27, 2024: contributor

You mean if you invalidateblock, then reconsiderblock, the rest of the child blocks also need to be reconsidered individually?

That’s already happening, reconsiderblock already goes over the entire block index db on master, looking at each index individually, see Chainstate::ResetBlockFailureFlags (this also has a bug, #30479, but that is unrelated to the failure flags).

DrahtBot added the label Needs rebase on Sep 3, 2024

mzumsande force-pushed on Sep 16, 2024

mzumsande renamed this:
~~validation: improve m_best_header tracking~~
validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment
on Sep 16, 2024

mzumsande commented at 5:33 pm on September 16, 2024: contributor

c9bf06a to 321097a:

rebased
addressed feedback
reworked commit f44a403ad27297c10256df11c3876b27c58dc1e4 (fix m_best_header tracking and BLOCK_FAILED_CHILD ) to set BLOCK_FAILED_CHILD for all blocks building on the invalid block, instead of just the ancestors of m_best_header

mzumsande force-pushed on Sep 16, 2024

DrahtBot commented at 5:38 pm on September 16, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30214184316

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Sep 16, 2024

DrahtBot removed the label Needs rebase on Sep 16, 2024

DrahtBot removed the label CI failed on Sep 16, 2024

in src/validation.h:738 in 321097a685 outdated

734@@ -735,6 +735,9 @@ class Chainstate
735         EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex)
736         LOCKS_EXCLUDED(::cs_main);
737 
738+    /** Set invalidity status to a all descendants of a block */

jonatack commented at 7:29 pm on September 16, 2024:

“to a all” -> unclear what this means

in src/validation.h:1328 in 321097a685 outdated

1323@@ -1321,6 +1324,11 @@ class ChainstateManager
1324     //! nullopt.
1325     std::optional<int> GetSnapshotBaseHeight() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
1326 
1327+    //! If, due to invalidation / reconsideration of blocks, the previous
1328+    //! best header is no longer valid or guaranteed to be the most-work

jonatack commented at 7:32 pm on September 16, 2024:

perhaps more clear

0    //! best header is no longer valid nor guaranteed to be the most-work

mzumsande commented at 3:37 pm on September 17, 2024:

will change to “/” so it mirrors the “invalidation / reconsideration” in the line before.

in src/validation.h:739 in 321097a685 outdated

734@@ -735,6 +735,9 @@ class Chainstate
735         EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex)
736         LOCKS_EXCLUDED(::cs_main);
737 
738+    /** Set invalidity status to a all descendants of a block */
739+    void SetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

jonatack commented at 7:36 pm on September 16, 2024:

For references to cs_main, are we still generally preferring to use the scope resolution operator? (you do use it in line 1330)

0    void SetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

mzumsande commented at 3:41 pm on September 17, 2024:

changed to ::cs_main

jonatack commented at 8:03 pm on September 16, 2024: member

Approach ACK 321097a685b67f82c32f53652227b7177fe97a95 on first look

fjahr commented at 1:35 pm on September 17, 2024: contributor

actually, that didn’t work because NotifyHeaderTip can’t be called when cs_main is held. Reverting this, I’ll think about this a bit more.

Is this ready for review or should we wait until you have addressed this @mzumsande ?

validation: add RecalculateBestHeader() function

It recalculates m_best_header by looping over the entire
block index. Even though this is not very performant, it
will only be used in rare situations that cannot be
triggered by others without a cost:
As part of to invalidateblock / reconsiderblock rpcs, or when a
block with an accepted header with valid PoW turns out to be invalid
later during full validation.

a51e91783a

rpc: call RecalculateBestHeader as part of reconsiderblock

Co-authored-by: Fabian Jahr <fjahr@protonmail.com>

9275e9689a

validation: call RecalculateBestHeader in InvalidChainFound

This means that it is being called in two situations:
1.) As part of the invalidateblock rpc
2.) When we receive a block for which we have a valid
header in our block index, but the block turns out to be invalid

783cb7337f

validation: mark blocks building on an invalid block as BLOCK_FAILED_CHILD

Without doing so, header-only chains building on a chain that
will be marked as invalid would still be eligible for m_best_header.
This improves both getblockchaininfo and getchaintips behavior.

While this adds an iteration over the entire block index, it can only be
triggered by the user (invalidateblock) or by others at a cost (the
header needs to be accepted in the first place, so it needs valid PoW).

Co-authored-by: TheCharlatan <seb.kung@gmail.com>

f5149ddb9b

test: cleanup rpc_getchaintips.py

Remove whitespace that doesn't conform with pep8 and turn some
comments into log messages.

ccd98ea4c8

test: add test for getchaintips behavior with invalid chains

This test would fail to mark the chain tip as "invalid" instead
of "headers-only" without the previous commit marking the headers
as BLOCK_FAILED_CHILD.

0bd53d913c

mzumsande force-pushed on Sep 17, 2024

mzumsande commented at 3:44 pm on September 17, 2024: contributor

321097a to 0bd53d9: addressed feedback by @jonatack

Is this ready for review or should we wait until you have addressed this @mzumsande ?

It’s ready for review, see #30666 (review) - but of course I’m open to suggestions!

fjahr commented at 9:29 pm on September 17, 2024: contributor

reACK 0bd53d913c1c2ffd2d0779f01bc51c81537b6992

DrahtBot requested review from jonatack on Sep 17, 2024

DrahtBot requested review from TheCharlatan on Sep 17, 2024

TheCharlatan approved

TheCharlatan commented at 1:52 pm on October 10, 2024: contributor

Re-ACK 0bd53d913c1c2ffd2d0779f01bc51c81537b6992

DrahtBot added the label CI failed on Oct 18, 2024

DrahtBot removed the label CI failed on Oct 23, 2024

achow101 commented at 9:44 pm on November 14, 2024: member

ACK 0bd53d913c1c2ffd2d0779f01bc51c81537b6992

achow101 merged this on Nov 14, 2024

achow101 closed this on Nov 14, 2024

Yygik commented at 12:08 pm on November 15, 2024: none

i额迷你发光你放哪更好

mzumsande deleted the branch on Nov 21, 2024

validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment #30666

Code Coverage & Benchmarks

Reviews

Conflicts