validation: remove BLOCK_FAILED_CHILD #32950
Conversation
…InvalidateBlock - there is no functional difference between BLOCK_FAILED_VALID and BLOCK_FAILED_CHILD and it's unnecessary code complexity to correctly categorise them.
to_mark_failed was useful to track last disconnected block so that it's children can be marked as BLOCK_FAILED_CHILD. since the previous commit removes BLOCK_FAILED_CHILD, the existing variable invalid_walk_tip is sufficient. invalid_walk_tip can track the last disconnected block since we exit out of the loop before invalid_walk_tip is reset. this is needed in code later on to mark last disconnected block as invalid.
even though we have a distinction between BLOCK_FAILED_VALID and BLOCK_FAILED_CHILD in the codebase, we don't use it for anything. since there's no functional difference between them and it's unnecessary code complexity to categorise them correctly, just mark as BLOCK_FAILED_VALID instead.
since it's the same as BLOCK_FAILED_VALID now
…ng from disk - there maybe existing block indexes stored in disk with BLOCK_FAILED_CHILD - since they don't exist anymore, clean up block index entries with BLOCK_FAILED_CHILD and reset it to BLOCK_FAILED_VALID.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32950. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. LLM Linter (✨ experimental)Possible typos and grammar issues:
No other typos affecting comprehension were found. drahtbot_id_4_m |
|
Concept ACK |
There was a problem hiding this comment.
Concept ACK
I think removing is a better option. The only weak argument that we could have for not removing is that BLOCK_FAILED_VALID right now marks the start of the invalid block chain and is better for debugging manually. However , the burden of maintaining this distinction seems too high for debugging purposes with no actual benefit functionally.
| if (!(pindex->nStatus & BLOCK_FAILED_MASK) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_MASK)) { | ||
| pindex->nStatus |= BLOCK_FAILED_CHILD; | ||
| if (!(pindex->nStatus & BLOCK_FAILED_VALID) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_VALID)) { | ||
| // if BLOCK_FAILED_CHILD already exists in disk, clear it |
There was a problem hiding this comment.
exists in disk -> exists on disk [“in disk” is nonstandard; use “on disk”]
An alternative wording could be "if ... was persisted, clear it"
There was a problem hiding this comment.
Or:
// BLOCK_FAILED_CHILD is deprecated, but may still exist on disk. Replace it with BLOCK_FAILED_VALID.
| if (!(pindex->nStatus & BLOCK_FAILED_MASK) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_MASK)) { | ||
| pindex->nStatus |= BLOCK_FAILED_CHILD; | ||
| if (!(pindex->nStatus & BLOCK_FAILED_VALID) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_VALID)) { | ||
| // if BLOCK_FAILED_CHILD already exists in disk, clear it |
There was a problem hiding this comment.
Or:
// BLOCK_FAILED_CHILD is deprecated, but may still exist on disk. Replace it with BLOCK_FAILED_VALID.
There was a problem hiding this comment.
TestedACK 06db700
Block 2 (valid)
/ \
(valid)Block height 3 Block height 3 (BLOCK_FAILED_CHILD)
| |
(valid) Block height 4 Block height 4(BLOCK_FAILED_CHILD)
| |
Before removing BLOCK_FAILED_CHILD,this test(#32173 (comment)) would fail at: self.log.info(self.nodes[0].reconsiderblock(res["bestblock"]))
(Reconsidering block at height 3 — which has BLOCK_FAILED_CHILD)
This failure happened because:
This check fails, block 3 has BLOCK_FAILED_CHILD (not BLOCK_FAILED_VALID).
and so in this assertion -> https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L5363
pindex->nStatus & BLOCK_FAILED_MASK) != 0.
Now, after removing BLOCK_FAILED_CHILD:
block 3 , status is BLOCK_FAILED_VALID, and pindexFirstInvalid is set correctly.
Fixes #32173
even though we have a distinction between
BLOCK_FAILED_VALIDandBLOCK_FAILED_CHILDin the codebase,we don't use it for anything. Whenever we check for BlockStatus, we use
BLOCK_FAILED_MASKwhich encompasses both of them.Since there is no functional difference between
BLOCK_FAILED_VALIDandBLOCK_FAILED_CHILDand it's addedcode complexity to correctly categorise them (ex: #31405 (comment), #16856 (comment)), we could just remove it.
Looking for conceptual feedback on whether it's better to improve handling of
BLOCK_FAILED_CHILDin the codebase or removeBLOCK_FAILED_CHILD.Of less relevance, but it would also fix a
reconsiderblockcrash that could happen in the situation mentioned in #32173 (comment)Similar attempt in the past in #16856 (comment)