epoching API: add epoch msg range queries and add timestamp to validator lifecycle #119

SebastianElvis · 2022-09-07T04:56:28Z

Fixes BM-120

This PR

implements the API /babylon/epoching/v1/latest_epoch_msgs/{num_latest_epochs} that given the number num_latest_epochs of epochs, returns a map of epoch msgs keyed by the epoch number
fixes a bug on validator lifecycle API and adds timestamp to validator lifecycle

Also marked @toliujiayi as reviewer since the bug was reported by Jack (Thanks!)

Fuzz tests on the new APIs will be implemented by subsequent PRs in the next sprint.

Example execution

vitsalis

Few minor things:

vitsalis · 2022-09-07T08:56:07Z

proto/babylon/epoching/v1/query.proto

+}
+
+// QueryLatestEpochMsgsResponse is response type for the Query/LatestEpochMsgs RPC method
+message QueryLatestEpochMsgsResponse {


Should we add pagination to this? Even though the query is supposed to be used for the "latest" epochs, someone could provide a large number as an argument.

vitsalis · 2022-09-07T08:57:44Z

x/epoching/keeper/grpc_query.go

+func (k Keeper) LatestEpochMsgs(c context.Context, req *types.QueryLatestEpochMsgsRequest) (*types.QueryLatestEpochMsgsResponse, error) {
+ ctx := sdk.UnwrapSDKContext(c)
+
+ // TODO: enforce limit of the number of epochs in each query


Why do we want to have a limit here? Seems a little bit arbitrary.

Usually gRPC listing endpoints have the option to limit the pagination size to protect themselves against resource exhaustion: https://cloud.google.com/apis/design/design_patterns#list_pagination

Great, I can always learn something new from your code reviews 👍

Yes I believe this API has to do pagination. There are two approaches to do the pagination:

Paginating the epoch number. Each page contains epoch msgs of a limited number of epochs

Paginating epoch messages. Each page contains a limited number of epoch messages, ordered by their corresponding epoch numbers

I'm inclined to do the former since we always want the complete set of msgs in an epoch. WDYT?

In addition, Cosmos SDK implements its own pagination functionalities, i.e., cosmos.base.query.v1beta1.PageRequest and cosmos.base.query.v1beta1.PageResponse, and does not make use of the List feature of HTTP (as shown in the screenshot below). Perhaps for our implementation let's stick to Cosmos SDK's approach for consistency?

I totally agree with using the Cosmos pagination machinery! I only included the link to quote the gRPC authorities on why adding pagination is a good idea, but it's a pain in the ass to do it their way, Cosmos is much more convenient than doing it from scratch.

vitsalis · 2022-09-07T09:02:25Z

x/epoching/keeper/grpc_query.go

+ msgMap := make(map[uint64]*types.QueuedMessageList)
+
+ for i := beginEpoch; i <= endEpoch; i++ {
+ msgs := k.GetEpochMsgs(ctx, i)


This iteration means that we are doing req.NumLatestEpochs accesses to storage. Since we are accessing concurrent epochs, I think a more performant way to be to iterate the epoch storage in reverse and stop as soon as you get to an epoch that has a lower height than beginEpoch.

aakoshh

Just commenting on the URL formats. Maybe it would be more flexible to add a begin_epoch and end_epoch parameter, or end_epoch and epoch_count to the query, so you can add pagination to the whole thing if you want to.

aakoshh · 2022-09-07T22:15:26Z

proto/babylon/epoching/v1/query.proto

@@ -26,6 +26,11 @@ service Query {
 option (google.api.http).get = "/babylon/epoching/v1/epoch_msgs/{epoch_num}";


Suggested change

option (google.api.http).get = "/babylon/epoching/v1/epoch_msgs/{epoch_num}";

option (google.api.http).get = "/babylon/epoching/v1/epochs/{epoch_num=*}/messages";

These are supposed to be RESTful looking URLs.

See:

https://cloud.google.com/apis/design/design_patterns#list_sub-collections

https://cloud.google.com/apis/design/custom_methods

https://cloud.google.com/apis/design/standard_methods

aakoshh · 2022-09-07T22:20:18Z

proto/babylon/epoching/v1/query.proto

+ // LatestEpochMsgs queries the messages within a given number of most recent epochs
+ rpc LatestEpochMsgs(QueryLatestEpochMsgsRequest) returns (QueryLatestEpochMsgsResponse) {
+ option (google.api.http).get = "/babylon/epoching/v1/latest_epoch_msgs/{num_latest_epochs}";
+ }


Suggested change

// LatestEpochMsgs queries the messages within a given number of most recent epochs

rpc LatestEpochMsgs(QueryLatestEpochMsgsRequest) returns (QueryLatestEpochMsgsResponse) {

option (google.api.http).get = "/babylon/epoching/v1/latest_epoch_msgs/{num_latest_epochs}";

}

// LatestEpochMsgs queries the messages within a given number of most recent epochs

rpc LatestEpochMsgs(QueryLatestEpochMsgsRequest) returns (QueryLatestEpochMsgsResponse) {

option (google.api.http).get = "/babylon/epoching/v1/epochs:latest/messages";

}

And the num_latest_epochs (I would name it epoch_count would appear in the query string like ?epoch_count=10. Again, the RESTful URL design; the count is not part of the resource name, it does not identify it like epochs/10 would the 10th epoch.

This is a custom method; we can have epochs/10/messages for listing messages in a specific epoch, and we could also use epochs/-/messages?account_key=0x123 to list stuff from a single account across all epochs, but it feels like having an ID in the path for this method would make no sense with an epoch_count parameter, so it's custom.

aakoshh · 2022-09-07T22:23:24Z

proto/babylon/epoching/v1/query.proto

+ // epoch_msg_map is the map where
+ // - key is the epoch number
+ // - value is the list of queued messages in this epoch
+ map<uint64, babylon.epoching.v1.QueuedMessageList> epoch_msg_map = 1;


Maps are rather poor in protobuf. They keys can't be custom types, and even though no matter how hard I tried I could not convince @gitferry that losing the information of a CheckpointStatus in favor of string is a bad tradeoff, I still think we should stay away from them. A list of key-value pairs (a nested type under QueryLatestEpochMsgsResponse, or maybe just adding an epoch number to QueuedMessageList should do it).

aakoshh · 2022-09-07T22:28:15Z

x/epoching/keeper/grpc_query.go

+func (k Keeper) LatestEpochMsgs(c context.Context, req *types.QueryLatestEpochMsgsRequest) (*types.QueryLatestEpochMsgsResponse, error) {
+ ctx := sdk.UnwrapSDKContext(c)
+
+ // TODO: enforce limit of the number of epochs in each query


Usually gRPC listing endpoints have the option to limit the pagination size to protect themselves against resource exhaustion: https://cloud.google.com/apis/design/design_patterns#list_pagination

SebastianElvis · 2022-09-08T07:20:02Z

Thanks for the comments Vitalis and Akosh! I learned a lot about API design via this PR 😎

Now this PR has fixed the comments. Specifically, this PR

makes QueryLatestEpochMsgs to use epoching/v1/epochs:latest/messages URL, have two parameters end_epoch and epoch_count, return a list of msg queues, and support pagination (via Cosmos SDK's PageRequest and PageResponse).
changes QueryEpochMsgs API's URL to babylon/epoching/v1/epochs/{epoch_num=*}/messages

An example invocation of QueryLatestEpochMsgs is attached below. Feel free to review again.

aakoshh · 2022-09-08T21:55:44Z

x/epoching/keeper/grpc_query.go

+
+ // iterate over queueLenStore since we only need to iterate over the epoch number
+ queueLenStore := k.msgQueueLengthStore(ctx)
+ pageRes, err := query.FilteredPaginate(queueLenStore, req.Pagination, func(key []byte, _ []byte, accumulate bool) (bool, error) {


Note the closure/anonymous function passing here ☝️

aakoshh

Awesome, thank your for looking into pagination!

SebastianElvis added 4 commits September 6, 2022 16:06

init

71ea317

Merge branch 'main' into epoching-api-range-query

4c3a4be

fix lifecycle bug

8b94345

fix

907987f

SebastianElvis requested review from vitsalis, aakoshh and toliujiayi September 7, 2022 04:56

Merge branch 'main' into epoching-api-range-query

1bc9004

vitsalis reviewed Sep 7, 2022

View reviewed changes

aakoshh reviewed Sep 7, 2022

View reviewed changes

SebastianElvis added 4 commits September 8, 2022 16:36

fix API

5d5a951

debug

4d12f61

fix

8033e2c

tidy

3275547

SebastianElvis requested review from vitsalis and aakoshh September 8, 2022 07:20

aakoshh reviewed Sep 8, 2022

View reviewed changes

aakoshh approved these changes Sep 8, 2022

View reviewed changes

Merge branch 'main' into epoching-api-range-query

374f632

SebastianElvis merged commit ea000fa into main Sep 9, 2022

SebastianElvis deleted the epoching-api-range-query branch September 9, 2022 00:38

vitsalis added a commit that referenced this pull request Jan 21, 2024

fix: Include unbonding time in stored struct (#119)

72406d0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

epoching API: add epoch msg range queries and add timestamp to validator lifecycle #119

epoching API: add epoch msg range queries and add timestamp to validator lifecycle #119

SebastianElvis commented Sep 7, 2022 •

edited

Loading

vitsalis left a comment

vitsalis Sep 7, 2022

vitsalis Sep 7, 2022

aakoshh Sep 7, 2022

SebastianElvis Sep 8, 2022

SebastianElvis Sep 8, 2022

aakoshh Sep 8, 2022

vitsalis Sep 7, 2022

aakoshh left a comment

aakoshh Sep 7, 2022

aakoshh Sep 7, 2022

aakoshh Sep 7, 2022

aakoshh Sep 7, 2022

SebastianElvis commented Sep 8, 2022

aakoshh Sep 8, 2022

aakoshh left a comment

		@@ -26,6 +26,11 @@ service Query {
		option (google.api.http).get = "/babylon/epoching/v1/epoch_msgs/{epoch_num}";

epoching API: add epoch msg range queries and add timestamp to validator lifecycle #119

epoching API: add epoch msg range queries and add timestamp to validator lifecycle #119

Conversation

SebastianElvis commented Sep 7, 2022 • edited Loading

Example execution

vitsalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aakoshh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianElvis commented Sep 8, 2022

Choose a reason for hiding this comment

aakoshh left a comment

Choose a reason for hiding this comment

SebastianElvis commented Sep 7, 2022 •

edited

Loading