zoneconcierge: API for querying headers in a given epoch #261
Conversation
| @@ -38,6 +38,10 @@ service Query { | |||
| rpc ListHeaders(QueryListHeadersRequest) returns (QueryListHeadersResponse) { | |||
| option (google.api.http).get = "/babylon/zoneconcierge/v1/headers/{chain_id}"; | |||
| } | |||
| // ListEpochHeaders queries the headers of a chain timestamped in a given epoch of Babylon, with pagination support | |||
| rpc ListEpochHeaders(QueryListEpochHeadersRequest) returns (QueryListEpochHeadersResponse) { | |||
| option (google.api.http).get = "/babylon/zoneconcierge/v1/epochs/{epoch_num}/headers/{chain_id}"; | |||
There was a problem hiding this comment.
nit: I suggest the link be /headers/{chain_id}/epoch/{epoch_num} as the epoch is a more specific part of a headers query.
| // QueryListEpochHeadersResponse is response type for the Query/ListEpochHeaders RPC method. | ||
| message QueryListEpochHeadersResponse { | ||
| // headers is the list of headers | ||
| repeated babylon.zoneconcierge.v1.IndexedHeader headers = 1; |
There was a problem hiding this comment.
I suggest we have pagination here.
There was a problem hiding this comment.
I was also considering pagination for this API. The problem is that this neither is straightforward nor gives us performance improvements due to the KVStore structure.
We are querying a segment in the KVStore and the keys in this KVStore is not consecutive. The non-consecutiveness requires us to find the start header and the end header of this epoch, which is already O(epoch_interval) in the worst case.
For the sake of efficiency, maybe we can consider a slightly different API, which only returns a list of heights whose corresponding headers are timestamped in this given epoch. With the list of heights the explorer can query its interested indexed headers from ZoneConcierge individually.
I have marked a TODO here. Once we have a decision I will make corresponding changes in a subsequent API.
There was a problem hiding this comment.
Yep, I also encountered a similar issue with the MainChain query of the btclightclient module (ref). I think it is possible although it makes the code more complex so we should decide on the tradeoff.
| // this implies that this epoch does not timestamp any header for this chain at all | ||
| // and the query should return empty | ||
| if epochChainInfo.LatestHeader.BabylonEpoch < epochNumber { | ||
| return nil, types.ErrEpochHeadersNotFound |
There was a problem hiding this comment.
Above you mention that the query should return empty, yet here you return an error. I'm happy with returning an empty set.
There was a problem hiding this comment.
I feel an error gives a more clear message to the function invoker. The comment in L34 is indeed inconsistent though so I deleted it.
| require.Equal(t, len(expectedHeaders), len(headers)) | ||
| for i := 0; i < len(expectedHeaders); i++ { | ||
| require.Equal(t, expectedHeaders[i].Header.LastCommitHash, headers[i].Hash) | ||
| } |
SebastianElvis
deleted the
zoneconcierge-api-query-timestamped-headers-in-epoch
branch
* zoneconcierge: API for querying the chain info of a given epoch (#260) * zoneconcierge: API for querying headers in a given epoch (#261) * zoneconcierge API: adding total number of timestamped headers to chainInfo (#263) * zoneconcierge API: find the BTC-finalised chain info before specific CZ height (#264) * zoneconcierge API: find header and fork headers at a given height (#266) * checkpointing API: add checkpoint lifecycle in `RawCheckpointWithMeta` (#267) * chore: refactor `FinalizedChainInfo` API (#268) * Add integration test for zoneconcierge checkpointing (#269) * Add integration test checking info about opposing cz chain * Fix: Increase gas in e2e test (#270) Increase gas * fix: checkpointing: Do not make the `home` flag a required one and unmarshall PubKey (#271) * Fix build Co-authored-by: Runchao Han <me@runchao.rocks> Co-authored-by: Vitalis Salis <VitSalis@gmail.com>
Fixes BM-401
Context: #254
This PR implements the API for querying headers in a given epoch.