Do not allow descendants of BLOCK_FAILED_VALID blocks to be BLOCK_FAILED_VALID #16856
Conversation
Previously, we could release cs_main while leaving the block index in a state that would fail CheckBlockIndex, because setBlockIndexCandidates was not being fully populated before releasing cs_main.
This has no functional affect, as the any CBlockIndex*s which to_mark_failed is set to will already have been marked failed.
This adds a new test in CheckBlockIndex which tests that any descendants of a block marked BLOCK_FAILED_VALID will not be marked BLOCK_FAILED_VALID as they should never have been connected in the first place. Note that this check requires further changes in InvalidateBlock as if you iteratively call InvalidateBlock to walk the chain backwards (instead of calling InvalidateBlock on an old block and allowing it to do the walking), the later-in-the-chain blocks will violate this.
| @@ -2813,33 +2848,62 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c | |||
| setDirtyBlockIndex.insert(invalid_walk_tip); | |||
| setBlockIndexCandidates.erase(invalid_walk_tip); | |||
| setBlockIndexCandidates.insert(invalid_walk_tip->pprev); | |||
| if (invalid_walk_tip->pprev == to_mark_failed && (to_mark_failed->nStatus & BLOCK_FAILED_VALID)) { | |||
| if (invalid_walk_tip == to_mark_failed->pprev && (to_mark_failed->nStatus & BLOCK_FAILED_VALID)) { | |||
There was a problem hiding this comment.
I'm confused by this fix. Let's say we called invalidateblock on block 10. In that case to_mark_failed is block 10, to_mark_failed->pprev is block 9. Let's say we're almost done and we just disconnected the tip at height 11. Then invalid_walk_tip is block 11. So this comparison always returns false?
There was a problem hiding this comment.
If we're almost done, to_mark_failed got set to block 12 in the last iteration of the loop (yes, these variable names make no sense).
There was a problem hiding this comment.
Ah, it's actually pindex that stays at block 10 throughout this function. to_mark_failed is set to invalid_walk_tip. By the time we reach this line we chipped one block off of the tip.
Looking at the first iteration: let's say the original tip is block 20. to_mark_failed starts at block 10. We disconnect block 20 and mark it BLOCK_FAILED_VALID. Theif statement here returns false, so block 20 temporarily remains BLOCK_FAILED_VALID. The second iteration to_mark_failed points to block 20, so then we set it to BLOCK_FAILED_CHILD.
Do I understand correctly that the reason for this "we fix it the next iteration" is because we might get shut down? So we need to wait with BLOCK_FAILED_CHILD until we're sure the parent is BLOCK_FAILED_VALID .
There was a problem hiding this comment.
Right, I mean its not strictly because we want to be safe in case of shutdown (though that's nice), and more that this hunk previously never did anything at all (though its intent was "descendants get marked FAILED_CHILD").
|
Looking at the |
2a4e60b Fix block index inconsistency in InvalidateBlock() (Suhas Daftuar) Pull request description: Previously, we could release `cs_main` while leaving the block index in a state that would fail `CheckBlockIndex()`, because `setBlockIndexCandidates` was not being fully populated before releasing `cs_main`. ACKs for top commit: TheBlueMatt: utACK 2a4e60b. I also discovered another issue in InvalidateBlock while reviewing, see #16856. Sjors: ACK 2a4e60b. Tested on top of #16899. Also tested `invalidateblock` with `-checkblockindex=1`. fjahr: ACK 2a4e60b. Ran tests, reviewed code, inspected behavior while manually testing `invalidateblock`. Tree-SHA512: ced12f9dfff0d413258c709921543fb154789898165590b30d1ee0cdc72863382f189744f7669a7c924d3689a1cc623efdf4e5ae3efc60054572c1e6826de612
2a4e60b Fix block index inconsistency in InvalidateBlock() (Suhas Daftuar) Pull request description: Previously, we could release `cs_main` while leaving the block index in a state that would fail `CheckBlockIndex()`, because `setBlockIndexCandidates` was not being fully populated before releasing `cs_main`. ACKs for top commit: TheBlueMatt: utACK 2a4e60b. I also discovered another issue in InvalidateBlock while reviewing, see bitcoin#16856. Sjors: ACK 2a4e60b. Tested on top of bitcoin#16899. Also tested `invalidateblock` with `-checkblockindex=1`. fjahr: ACK 2a4e60b. Ran tests, reviewed code, inspected behavior while manually testing `invalidateblock`. Tree-SHA512: ced12f9dfff0d413258c709921543fb154789898165590b30d1ee0cdc72863382f189744f7669a7c924d3689a1cc623efdf4e5ae3efc60054572c1e6826de612
|
Can you give a bit of motivation for why we want to mark descendants of If that's the case, then I think we should just mark all descendants of the block to invalidate as |
|
Right, I don't think we really use it anywhere, I just figured as long as we make the distinction, make the distinction. Maybe @sipa recalls why it was there in the first place? |
|
I don't recall exactly why they're separate. One point is that BLOCK_FAILED_CHILD doesn't actually need to be stored on disk, as it is recomputed at db load time. So an advantage of keeping them separate is that if only FAILED_CHILD gets set on a blockindex object, it doesn't need to be made dirty and written to disk. I'm not sure that optimization is ever taken advantage of (and it probably can be achieved in other ways too). |
Looks to me like it isn't used. I'd vote to just use |
|
Alternative here https://github.com/jnewbery/bitcoin/tree/2019-12-block-failed-valid-descendants just removes tracking of |
|
@jnewbery , that branch doesn't remove all references to BLOCK_FAILED_CHILD. Was the intent to remove them all? |
|
@BrannonKing - The intent was to demonstrate how removing |
|
@BrannonKing - I've rebased my branch on master and removed all the remaining instances of BLOCK_FAILED_CHILD. @TheBlueMatt - This PR hasn't had any activity for about a year. Are you happy to close it? |
2a4e60b Fix block index inconsistency in InvalidateBlock() (Suhas Daftuar) Pull request description: Previously, we could release `cs_main` while leaving the block index in a state that would fail `CheckBlockIndex()`, because `setBlockIndexCandidates` was not being fully populated before releasing `cs_main`. ACKs for top commit: TheBlueMatt: utACK 2a4e60b. I also discovered another issue in InvalidateBlock while reviewing, see bitcoin#16856. Sjors: ACK 2a4e60b. Tested on top of bitcoin#16899. Also tested `invalidateblock` with `-checkblockindex=1`. fjahr: ACK 2a4e60b. Ran tests, reviewed code, inspected behavior while manually testing `invalidateblock`. Tree-SHA512: ced12f9dfff0d413258c709921543fb154789898165590b30d1ee0cdc72863382f189744f7669a7c924d3689a1cc623efdf4e5ae3efc60054572c1e6826de612
2a4e60b Fix block index inconsistency in InvalidateBlock() (Suhas Daftuar) Pull request description: Previously, we could release `cs_main` while leaving the block index in a state that would fail `CheckBlockIndex()`, because `setBlockIndexCandidates` was not being fully populated before releasing `cs_main`. ACKs for top commit: TheBlueMatt: utACK 2a4e60b. I also discovered another issue in InvalidateBlock while reviewing, see bitcoin#16856. Sjors: ACK 2a4e60b. Tested on top of bitcoin#16899. Also tested `invalidateblock` with `-checkblockindex=1`. fjahr: ACK 2a4e60b. Ran tests, reviewed code, inspected behavior while manually testing `invalidateblock`. Tree-SHA512: ced12f9dfff0d413258c709921543fb154789898165590b30d1ee0cdc72863382f189744f7669a7c924d3689a1cc623efdf4e5ae3efc60054572c1e6826de612
Based on #16849, this was intended primarily to fix n invalid-direction comparison (see the first commit) but discovered there were further issues preventing the addition of a new rule in CheckBlockIndex. We can either go this way, or just delete the code that attempts (incorrectly) to mark blocks BLOCK_FAILED_CHILD in InvalidateBlock and not require this as an invariant (I don't have a strong opinion here, just PR'ing this as I wrote it during testing and figured I'd demonstrate).