In commit “validation: Don’t repeat work in InvalidateBlock” (30b190eebab9df9273586e0a4b6462847883d9d1)
re: #32843 (review)
Apologies for mostly coming with stop energy here, but I think I’m approach NACK on 30b190e
I tend to disagree and think the code is clearer with this change than without it. Without it, intent is not possible to discern. The code is doing the same work twice without an indication of why it is doing that. With this change, behavior actually makes sense and intent is clear.
I do agree code could be reorganized better and made less fragile, but I don’t see the new code here as being more fragile than current code, just more explicit.
I do share some concern about the default value, though, and I think it would be better if InvalidChainFound was not commenting on invalidateblock behavior, which could change at some point. So might suggest:
0--- a/src/validation.cpp
1+++ b/src/validation.cpp
2@@ -2064,13 +2064,12 @@ void Chainstate::CheckForkWarningConditions()
3 }
4
5 // Called both upon regular invalid block discovery *and* InvalidateBlock
6-void Chainstate::InvalidChainFound(CBlockIndex* pindexNew, bool calc_flags_and_header = true)
7+void Chainstate::InvalidChainFound(CBlockIndex* pindexNew, bool calc_flags_and_header)
8 {
9 AssertLockHeld(cs_main);
10 if (!m_chainman.m_best_invalid || pindexNew->nChainWork > m_chainman.m_best_invalid->nChainWork) {
11 m_chainman.m_best_invalid = pindexNew;
12 }
13- // Unnecessary in the invalidateblock case, which has its own accounting for the failure flags and best header.
14 if (calc_flags_and_header) {
15 SetBlockFailureFlags(pindexNew);
16 if (m_chainman.m_best_header != nullptr && m_chainman.m_best_header->GetAncestor(pindexNew->nHeight) == pindexNew) {
17@@ -2098,7 +2097,7 @@ void Chainstate::InvalidBlockFound(CBlockIndex* pindex, const BlockValidationSta
18 pindex->nStatus |= BLOCK_FAILED_VALID;
19 m_blockman.m_dirty_blockindex.insert(pindex);
20 setBlockIndexCandidates.erase(pindex);
21- InvalidChainFound(pindex);
22+ InvalidChainFound(pindex, /*calc_flags_and_header=*/true);
23 }
24 }
25
26@@ -3376,7 +3375,7 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex*
27 if (state.IsInvalid()) {
28 // The block violates a consensus rule.
29 if (state.GetResult() != BlockValidationResult::BLOCK_MUTATED) {
30- InvalidChainFound(vpindexToConnect.front());
31+ InvalidChainFound(vpindexToConnect.front(), /*calc_flags_and_header=*/true);
32 }
33 state = BlockValidationState();
34 fInvalidFound = true;
35@@ -3809,6 +3808,8 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
36 }
37 }
38
39+ // Record invalid chain, avoiding recomputing m_best_header and applying
40+ // BLOCK_FAILED_CHILD flags since these things were done above.
41 InvalidChainFound(to_mark_failed, /*calc_flags_and_header=*/false);
42 }
43
Or if this commit will be dropped, I think there should at least be a comment added before the InvalidChainFound call in InvalidateBlock saying InvalidChainFound will repeat some work that was already before cs_main was released, to make it clear the duplication was at least not an accident.
