-
Notifications
You must be signed in to change notification settings - Fork 170
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
feat: add raw checkpoint creation #52
Conversation
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.
Looks good, except for the boundary check which really needs encapsulation :)
x/checkpointing/abci.go
Outdated
// get the height of the last block in this epoch | ||
epochBoundary := k.GetEpochBoundary(ctx) | ||
// if this block is the second block of an epoch | ||
if uint64(ctx.BlockHeight())-2 == epochBoundary.Uint64() { |
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.
This underlines the point of this conversation #32 (comment) with @SebastianElvis
It doesn't look right: GetEpochBoundary
, as per the comment, returns the last block. We are looking for the 2nd block of the current epoch, so maybe this should be block.height - 2 == previous_epoch_boundary
.
This is why that Epoch
interface would be cool, you could add a method to ask IsSecondBlock(ctx)
and it would figure out what you mean. Currently if we have epochs 1000 blocks long and we are in block 1002, this would compare that against 2000 I think.
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.
Yeah, I agree that the Epoch
interface is cool.
It doesn't look right: GetEpochBoundary, as per the comment, returns the last block. We are looking for the 2nd block of the current epoch, so maybe this should be block.height - 2 == previous_epoch_boundary.
This is why that Epoch interface would be cool, you could add a method to ask IsSecondBlock(ctx) and it would figure out what you mean. Currently if we have epochs 1000 blocks long and we are in block 1002, this would compare that against 2000 I think.
You are right, the current GetEpochBoundary
does not give what we want. If we don't want change GetEpochBoundary
, the checkpointing module can implement the AfterEpochEnds
hood to store the epoch boundary of the previous epoch so that we don't need to call GetEpochBoundary
during BeginBlock
but obtain the epoch boundary from the storage.
Or, we can insert epochNum
into GetEpochBoundary(ctx, epochNum)
so that we can get the boundaries we want. What do you think?
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 would implement the Epoch
interface and return something that can tell us everything about the current (or historical) module, and keep using the epoching
module as the source of truth, rather than all modules subscribing and storing their own pieces.
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.
Doing so would allow us to encapsulate the logic of how the boundaries are calculated. Currently it's at equal intervals, so what we actually return could be something like type epochBoundary struct { epoch_number, epoch_length }
but nobody should interact with that directly. If things change and the length becomes uneven, this can be accommodated, but the interface will not change.
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 see. It makes sense. I'll add a TODO here and wait for @SebastianElvis to finish the epoch interface. Thanks!
Thanks for your review @aakoshh. I also fixed the duplicate prefix of the storage. Please see the updates. Thanks! |
…pointing-ckpt-creation
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.
Looks good! 👍
This PR fixes BM-47.
This PR implements handlers upon entering an epoch boundary. In particular,
LastCommitHash
extracted from the block header.ACCUMULATING
.The following PR will be implementing the BLS signer which creates a
BLS-sig
transaction and distributes it to the network.