-
Notifications
You must be signed in to change notification settings - Fork 161
Fix rollback logic #130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix rollback logic #130
Conversation
273f95d
to
5c81bd9
Compare
5c81bd9
to
5cb3be4
Compare
If @dcoutts is happy with this it can be merged. |
slotNo <- lift DB.queryLatestSlotNo | ||
if slotNo <= unSlotNo slot | ||
then | ||
liftIO . logInfo trce $ | ||
mconcat | ||
[ "Byron: No rollback required: db tip slot is ", textShow slotNo | ||
, ", ledger tip slot is ", textShow (unSlotNo slot) | ||
] | ||
else do | ||
liftIO . logInfo trce $ | ||
mconcat | ||
[ "Byron: Rollbacking to slot ", textShow (unSlotNo slot) | ||
, ", hash ", Byron.renderAbstractHash hash | ||
] | ||
void . lift $ DB.deleteCascadeSlotNo slotNo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this isn't right yet and can still be a lot simpler.
The slot
and hash
we are given is the point to roll back to. So we want to delete blocks after that point, keeping that point at the most recent point.
This means we never need to check slotNo <= unSlotNo slot
because it's an unnecessary special case. If we're told to roll back to our current tip point then by definition delete blocks after that point is a no-op.
But looking at DB.deleteCascadeSlotNo slotNo
, it does not delete blocks after the given slot. It deletes blocks in that slot, and only that slot. That cannot be right because it will only delete the current tip block, but we have to be able to cope with rollbacks of arbitrary depth.
So this should be unconditional: just delete all blocks that have a strictly higher slot number than the slot of the rollback point. As an assertion / paranoia check, we could check that the (slot,hash) pair do indeed exist in the DB, so that the rollback makes sense.
There's no special case needed for the "No rollback required" case. That comes out naturally.
Without the query and the
Which means the correct chain tip was dropped and then the roll forward fails because the tip of the chain has just been dropped. With the query and the
and it works correctly. |
4c67f7a
to
957f5ee
Compare
New version of fix.
|
Rollback messages from chainsync were not being handled correctly which meant that orphaned blocks were not removed. When orphaned blocks were not removed from the block table that table could end up with two or more rows with the same block number. Closes: #114
BlockId is not guaranteed to be monotonic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
957f5ee
to
65b6b70
Compare
Orphan blocks occur when a new block does not extend the chain with the
most recent block in the database, but extends it with a block before
the most recent one.