index: coinstats reorg, fail when block cannot be reversed #28427
Conversation
During a reorg, continuing execution when a block cannot be reversed leaves the coinstats index in an inconsistent state, which was surely overlooked when 'CustomRewind' was implemented.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
There was a problem hiding this comment.
Code review ACK eef5955
This seems like it should avoid corrupting the index if ReverseBlock fails, since it avoid writing the best block. It also could prevent assert failures because it avoids subsequent ReverseBlock calls.
It would be good to add [[nodiscard]] to Reverseblock and other functions in indexing code to prevent this problem from happening other places. Grepping a little for functions in src/index/ that write to the database and return bool I see TxIndex::DB::WriteTxs and CopyHeightIndexToHashIndex and there may be others.
@furszy do you want to push an additional commit? |
There was a problem hiding this comment.
It would be good to add [[nodiscard]] to Reverseblock and other functions in indexing code to prevent this problem from happening other places. Grepping a little for functions in src/index/ that write to the database and return bool I see TxIndex::DB::WriteTxs and CopyHeightIndexToHashIndex and there may be others.
As said, adding nodiscard would be nice.
@furszy do you want to push an additional commit?
yep, pushed. A bit late due time zone diff.
Thanks both!
During a reorg, continuing execution when a block cannot be reversed leaves the coinstats index in an inconsistent state, which was surely overlooked when 'CustomRewind' was implemented. Github-Pull: bitcoin#28427 Rebased-From: eef5955
…e reversed c0bf667 index: add [nodiscard] attribute to functions writing to the db (furszy) eef5955 index: coinstats reorg, fail when block cannot be reversed (furszy) Pull request description: Found it while reviewing bitcoin#24230 (comment). During a reorg, continuing execution when a block cannot be reversed leaves the coinstats index in an inconsistent state. This was surely overlooked when 'CustomRewind' was implemented. ACKs for top commit: ryanofsky: Code review ACK c0bf667. Only change since last review is new commit adding [[nodiscard]] Tree-SHA512: f4fc8522508d23e4fff09a29c935971819b1bd3b2a260e08e2e2b72f9340980d74fbec742a58fe216baf61d27de057c7c8300e8fa075f8507cd1227f128af909
Found it while reviewing #24230 (comment).
During a reorg, continuing execution when a block cannot be reversed leaves the
coinstats index in an inconsistent state.
This was surely overlooked when 'CustomRewind' was implemented.