feat: Add new query for the last checkpoint with a given status #253
Conversation
gitferry
force-pushed
the
checkpointing/add-last-checkpoint-api
branch
from
bc1ee3a
to
c4ebd57
Compare
| @@ -72,26 +66,6 @@ message QueryRawCheckpointListResponse { | |||
| cosmos.base.query.v1beta1.PageResponse pagination = 2; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Why are we removing RecentRawCheckpointList API?
There was a problem hiding this comment.
Do we need this API as we already have RawCheckpointList?
| // GetLastCheckpointedEpoch returns the last epoch number that associates with a checkpoint | ||
| func (k Keeper) GetLastCheckpointedEpoch(ctx sdk.Context) (uint64, error) { | ||
| // minus 1 is because the current epoch is not finished | ||
| tipEpoch := k.GetEpoch(ctx).EpochNumber - 1 |
There was a problem hiding this comment.
IIUC this function returns the last epoch that has an associated raw checkpoint. Is it possible that the second latest epoch may not have a raw checkpoint yet? For example, when the latest epoch only has 1-2 blocks, the second latest epoch hasn't been sealed yet. It also takes some blocks to receive sufficient BLS sigs to get a valid BLS multisig.
Since this function is only used in LastCheckpointWithStatus, maybe we can consider a slightly different approach to avoid the above issue. In LastCheckpointWithStatus we still iterate backwards from the latest epoch to the zero epoch. In each iteration, check if the epoch has an associated raw checkpoint. If not, continue to the next iteration. If yes, then check if the raw checkpoint is in the given status. If yes, return, otherwise continue to the next iteration.
There was a problem hiding this comment.
IUC this function returns the last epoch that has an associated raw checkpoint. Is it possible that the second latest epoch may not have a raw checkpoint yet? For example, when the latest epoch only has 1-2 blocks, the second latest epoch hasn't been sealed yet. It also takes some blocks to receive sufficient BLS sigs to get a valid BLS multisig.
This is not possible because the second epoch would at least have a status of Accumulating. Minus 1 is to ensure a checkpoint is created. I don't think we care about whether the checkpoint is Sealed or not.
There was a problem hiding this comment.
I see. Thanks for the explanation! Is it possible for this API to return error when the requested status is Sealed or Accumulating?
There was a problem hiding this comment.
Of course! I thought this API would be more general, covering all statuses. Why do we want to reject Sealed or Accumulating requests?
| func (k Keeper) GetLastCheckpointedEpoch(ctx sdk.Context) (uint64, error) { | ||
| // minus 1 is because the current epoch is not finished | ||
| tipEpoch := k.GetEpoch(ctx).EpochNumber - 1 | ||
| if tipEpoch < 0 { //nolint:staticcheck // uint64 doesn't go below zero but we want to let people know that's an invalid request. |
There was a problem hiding this comment.
As suggested by the linter, maybe we only need to check if tipEpoch == 0 or not.
There was a problem hiding this comment.
We just need to ensure that the tipEpoch is not below 0. Wouldn't it be weird if we have the following?
if targetEpoch == 0 { //nolint:staticcheck // uint64 doesn't go below zero
targetEpoch = 0
}
There was a problem hiding this comment.
Or can we return error directly if k.GetEpoch(ctx).EpochNumber is 0?
There was a problem hiding this comment.
Yep. Good idea. Thanks!
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get the raw checkpoint at epoch %v: %w", e, err) | ||
| } | ||
| if ckpt.Status == req.Status { |
There was a problem hiding this comment.
There might be a possibility that no existing checkpoint is in the requested status. For example, assuming all epochs are either finalised or submitted, but not confirmed. If there is a query for the last confirmed epoch, then the function will iterate all the way down to epoch 0 and eventually return an error.
In order to avoid this scenario, maybe we need an extra if statement here. If ckpt.Status is more "mature" than req.Status, then this means none of the existing epoch is in req.Status, and should return an error or something.
Or, upon such scenario, we return ckpt directly here. This also makes sense since for example, a finalised epoch is also confirmed.
There was a problem hiding this comment.
This is a great point. Thanks! Will make the change
| if curEpoch <= 0 { | ||
| return 0, fmt.Errorf("current epoch should be more than 0") | ||
| } |
There was a problem hiding this comment.
With this check L145-147 can be safely removed, right?
| @@ -104,6 +104,10 @@ func (cm *RawCheckpointWithMeta) Accumulate( | |||
| return true, nil | |||
| } | |||
|
|
|||
| func (cm *RawCheckpointWithMeta) IsMoreMatureThanStatus(status CheckpointStatus) bool { | |||
This PR adds a new query for the last checkpoint with a given status. This API will be used by the verifier as well as the ZoneConcierge module. This PR also removed some unused APIs.