BM-2: feat(checkpointing): init checkpointing module and define proto types #5
Conversation
There was a problem hiding this comment.
Nice work!
It might have been better to follow what the other's have done and check in just the boilerplate scaffolding first, and then the actual checkpointing specific content as a separate PR, so it doesn't get lost in the sea of similarity.
One thing I'm not quite sure about is that this module contains a query to return raw checkpoints, yet there's a separate #4 for an actual rawcheckpoint module. Wouldn't a query like that belong there?
What modules do we plan to have?
- epoching
- BTC header oracle
- OP return oracle, i.e. BTC specific checkpoint module
- raw checkpoint module
- checkpoint module
That ☝️ looks redundant, there must be a duplicate among the last 3. Which one is this module?
|
Thanks, @aakoshh for the comments. I addressed them and replied if I have concerns. Please review the PR again. Many thanks! |
|
I had an issue when I tried to build proto code using Makefile. I used but I can see the |
|
It seems that your commit history is jumbled up a little bit. More specifically, all commits from init checkpointing module to wraps up the scaffolding of checkpointing are duplicatd. |
|
In order to build proto code I use |
gitferry
force-pushed
the
gai-checkpointing-module-setup
branch
2 times, most recently
from
3b505a8
to
f0ad00f
Compare
To @vitsalis, thanks for pointing it out. Sorry, the duplicate commits came from a bad rebase. |
gitferry
force-pushed
the
gai-checkpointing-module-setup
branch
from
f0ad00f
to
59756c3
Compare
@vitsalis I tried |
| // can't find cosmos_proto.scalar when compiling | ||
| // string signer_address = 5 [(cosmos_proto.scalar) = "cosmos.AddressString"]; |
There was a problem hiding this comment.
Try rebasing on #3
You will need that cosmos specific protoc plugin installed, which I think the script does before it runs.
There was a problem hiding this comment.
I tried make proto-gen uncommenting cosmos_proto.scalar but I still got errors. @vitsalis could you please take a look?
make proto-gen
Generating Protobuf files
proto/babylon/checkpointing/checkpoint.proto:44:30:field babylon.checkpointing.v1.BlsSig.signer_address: unknown extension cosmos_proto.scalar
proto/babylon/checkpointing/checkpoint.proto:44:30:field babylon.checkpointing.v1.BlsSig.signer_address: unknown extension cosmos_proto.scalar
proto/babylon/checkpointing/checkpoint.proto:44:30:field babylon.checkpointing.v1.BlsSig.signer_address: unknown extension cosmos_proto.scalar
make: *** [proto-gen] Error 100
There was a problem hiding this comment.
Seems like this feature does not exist in version 0.45.4 of cosmos. It was introduced by this commit:
cosmos/cosmos-sdk@94377f1
However, this commit only appears on versions 0.46.*. Searching for the word scalar under the proto directory on tag v0.45.4 does not produce any results
There was a problem hiding this comment.
Ah, I see. Thanks, @vitsalis for checking it. I guess I'll just use string for this PR and update it to scalar when we decide to use a newer version of cosmos.
|
@aakoshh Thanks for your second round of reviews. I fixed minor protobuf issues and replied to unaddressed ones. Thanks! |
|
Hi @KonradStaniec, could you please review the proto files of this module? Especially the |
|
I realized this PR is getting bigger. I'll not add more code except revision to this PR. And I'll replace this PR with smaller and more specific PRs after the review is over. |
gitferry
force-pushed
the
gai-checkpointing-module-setup
branch
from
4325ace
to
0ba412c
Compare
| option go_package = "github.com/babylonchain/babylon/x/checkpointing/types"; | ||
|
|
||
| // RawCheckpoint wraps the multi sig with meta data. | ||
| message RawCheckpoint { |
There was a problem hiding this comment.
As I understand RawCheckpoint is a checkpoint structure which will be serialized into bytes and put into two op return transactions on btc chain ?
If yes then two questions:
- Why do we need status here ? Isn't status a internal state a checkpoint manager about current checkpoint status, so it does not need to appear in RawCheckpoint serialized form ?
- I wonder if it would be more optimal to define rawcheckpint as standard go struct with custom serialization, as from what I remember protobuf does not have concept of fixed length arrays, so each
bytesfield always have its length pr-pended so it add few bytes of over head and each byte cost. Then serialization of such struct would be just concatendation of last_commit_hash, bitmap, bls_multi_sig and epoch_num serialized as varInt
There was a problem hiding this comment.
To be fair there's no actual field for Status, it's just a nested type. Maybe it will appear in its own collection in the Keeper.
Great point about the protobuf vs Go defined structure. FWIW I think what we need to avoid is rely on protobuf serialisation for hashing, as it has proven to be different across platforms. But defining stuff in protobuf that appears in API calls as well as on chain, like transaction messages themselves, seems to be the Tendermint/Cosmos way.
There was a problem hiding this comment.
@KonradStaniec @aakoshh Thanks for the discussion. The RawCheckpoint struct will be returned to BTC relayer via queries. Having Status in the struct will help the relayer make decisions on which raw checkpoint to submit based on the status. The Status will not be included in OP_RETURN to BTC.
There was a problem hiding this comment.
Just to clarify: there is currently no status in the struct, only a type that is namespaced, so you have a RawCheckpoint_Status type, but no field, see here.
But I think that's not a bad thing actually, you can always have a type like RawCheckpointWithStatus to couple to two for queries.
You would most likely have to store the status and the aggregated power somewhere else anyway. I think it would be nice if the RawCheckpoint corresponded to the information OP_RETURN as-is.
There was a problem hiding this comment.
Just to clarify: there is currently no status in the struct, only a type that is namespaced, so you have a
RawCheckpoint_Statustype, but no field, see here.But I think that's not a bad thing actually, you can always have a type like
RawCheckpointWithStatusto couple to two for queries.You would most likely have to store the status and the aggregated power somewhere else anyway. I think it would be nice if the
RawCheckpointcorresponded to the informationOP_RETURNas-is.
Sorry, I'm not sure I'm following this. The link you are referring seems the link of this page. I added an enum type called RawCheckpointStatus. Please correct me if I'm doing it wrong.
There was a problem hiding this comment.
At the time the status = 5 field was missing, that's all. I agree with Konrad that it should not be part of the RawCheckpoint data structure, to have something that is really just the checkpoint, as it was created, and a separate wrapper type that attaches a status which can change. To decouple the static fields from the dynamic ones.
In the Kubernetes API they have a convention of everything having a spec and a status, where spec is what the user can set and status is a data structure that changes according to what is happening in the system. This is a similar case to that.
gitferry
force-pushed
the
gai-checkpointing-module-setup
branch
from
0ba412c
to
a65243e
Compare
|
@aakoshh I changed some protos according to our discussion and rebased the PR with the main branch. Please review it again. Many thanks! |
|
|
||
| // QueryRawCheckpointRequest is the request type for the Query/RawCheckpoint | ||
| // RPC method. | ||
| message QueryRawCheckpointRequest { |
There was a problem hiding this comment.
I looked for a long time until I spotted the difference between the name of this and QueryRawCheckpointsRequest. I wonder if the Cosmos SDK has a convention of lists. In the default gRPC naming guide there was Get vs List to make it obvious.
There was a problem hiding this comment.
Thanks for the link to the staking module. If they do it like that as well then fine, we'll just have to strain our eyes a bit 8-)
| option (google.api.http).get = "/babylon/checkpointing/v1/raw_checkpoint/{epoch_num}"; | ||
| } | ||
|
|
||
| // LatestCheckpoint queries the latest checkpoint. |
There was a problem hiding this comment.
What does "latest" mean exactly? For example if for any reason there are multiple ones that haven't collected all signatures?
There was a problem hiding this comment.
I added docstring to describe that it queries the checkpoint with the highest epoch number.
For example if for any reason there are multiple ones that haven't collected all signatures?
I don't think this situation would happen because a checkpoint is only generated when sufficient bls-sigs are collected. Did I miss anything? Also, if there are multiple checkpoints generated at the same epoch, the system would panic.
| // RPC method. | ||
| message QueryRawCheckpointsRequest { | ||
| // status defines the status of the raw checkpoints of the query | ||
| string status = 1; |
There was a problem hiding this comment.
| string status = 1; | |
| RawCheckpoint.Status status = 1; |
| // QueryBlsPublicKeysResponse is the response type for the Query/BlsPublicKeys | ||
| // RPC method. | ||
| message QueryBlsPublicKeysResponse { | ||
| repeated bytes bls_pub_keys = 1; |
There was a problem hiding this comment.
Shouldn't this contain the validator address as well, so that I can look up the voting power they had at the time?
There was a problem hiding this comment.
Hm, did you add signer_address in response to this comment?
If so, I'm sorry, I should have been more specific. What I meant was that the response should contain (validator-address, bls-pub-key) pairs, so that we have a way to figure out what power they have. Something like message ValidatorWithBlsKey, it could even contain the power, to make it easier to use the API. So instead of repeated bytes bls_pub_keys it would be repeated ValidatorWithBlsKey validator_bls_keys = 1; or something similar.
There was a problem hiding this comment.
Oh, yes you are right. I added ValidatorWithBlsKey
|
@aakoshh I changed the query proto according to the comments, but I'm not sure I'm doing it right regarding on the |
gitferry
changed the title
|
Hi @aakoshh, I added |
added checkpointing tx proto add query proto for checkpointing generated proto code wraps up the scaffolding of checkpointing
gitferry
force-pushed
the
gai-checkpointing-module-setup
branch
from
8f8ccb0
to
1d751a2
Compare
This PR is to:
scaffold the module
x/checkpointingdefine msg and query protobuff
Future PRs will solve:
add a description of the data processing flows (interactions between modules, interaction between programs), the skeleton to x/checkpointing/spec.
develop and document the serializer/deserializer of other data structures (such as BTC related ones) that are not in protobuff
develop the keeper of the checkpointing module