feat: support multiple chain ids in FinalizedChainsInfo #365

gusin13 · 2023-04-27T11:52:49Z

Description

This PR is part of the optimizations being done to improve babylon-api performance.

FinalizedChainInfo rpc query is called multiple times by the explorer apis. I have replaced this method with FinalizedChainsInfo which will accept a list of chain ids so we can fetch info of multiple cz's at once.

Testing

Have added relevant fuzz tests and cli query

SebastianElvis

Nice!

SebastianElvis · 2023-04-28T07:57:38Z

x/zoneconcierge/keeper/grpc_query.go

+ encountered := map[string]bool{}
+ for _, chainID := range req.ChainIds {
+ if len(chainID) == 0 {
+ return nil, status.Error(codes.InvalidArgument, "chain ID cannot be empty")
+ }
+
+ // check for duplicates and return error on first duplicate found
+ if encountered[chainID] {
+ return nil, status.Errorf(codes.InvalidArgument, "duplicate chain ID %s", chainID)
+ } else {
+ encountered[chainID] = true
+ }
 }


Maybe we can wrap L204-216 as a function for checking duplication/empty value in a list of strings. This might be useful in a number of range queries

sure, fixed.

vitsalis

Nice work! Some suggestions:

test/e2e/configurer/chain/queries.go

vitsalis · 2023-04-28T09:08:00Z

test/e2e/configurer/chain/queries.go

- q.Add(parameters[i], parameters[i+1])
- }
- req.URL.RawQuery = q.Encode()
+ if queryParams != nil {


I suggest that in the case that we do not want to pass any parameters, we do not pass nil, but instead pass an empty queryParams object.

vitsalis · 2023-04-28T09:10:23Z

x/zoneconcierge/keeper/grpc_query.go

+ if err != nil {
+ // skip if the finalised chain info is not found
+ if errors.Is(err, types.ErrEpochChainInfoNotFound) {
+ k.Logger(ctx).Error("finalised chain info not found", "chain ID", chainID)


No need to produce an error log for an invalid request. Could lead to someone hogging the logs by sending invalid requests.

vitsalis · 2023-04-28T09:11:46Z

x/zoneconcierge/keeper/grpc_query.go

+ finalizedEpoch, chainInfo, err := k.GetLastFinalizedChainInfo(ctx, chainID)
+ if err != nil {
+ // skip if the finalised chain info is not found
+ if errors.Is(err, types.ErrEpochChainInfoNotFound) {


I don't like checking for particular errors as this might lead to inconsistencies if something minor is changed. I suggest we add a method FinalizedChainInfoExists or something similar, to check whether we don't need to add anything for this chain.

Co-authored-by: Vitalis Salis <VitSalis@gmail.com>

vitsalis · 2023-05-02T06:57:04Z

x/zoneconcierge/keeper/grpc_query.go


- resp.RawCheckpoint = rawCheckpoint.Ckpt
+ exists, err := k.HasFinalizedChainInfo(ctx, chainID)


The HasFinalizedChainInfo method retrieves the latest finalized epoch each time. This is a storage read operation and is quite costly, especially given that this value won't change in-between iterations. I suggest we retrieve this value outside of this loop and instead of using the HasFinalizedChainInfo(ctx, chainID) function which is non-performant, we use a function EpochChainInfoExists(ctx, epochNum, chainID), which is similar to this method but only looks for existence.

I am not sure how we will only check for existence as we have to perform this check inside that function which will involve a storage read.

vitsalis · 2023-05-02T06:57:39Z

x/zoneconcierge/keeper/epoch_chain_info_indexer.go

@@ -20,6 +21,23 @@ func (k Keeper) GetEpochChainInfo(ctx sdk.Context, chainID string, epochNumber u
 return &chainInfo, nil
 }

+// HasFinalizedChainInfo checks if chain info exists for a given chain ID in the last finalised epoch
+func (k Keeper) HasFinalizedChainInfo(ctx sdk.Context, chainID string) (bool, error) {


This method is not that performant and does more than it should. I suggest we remove this and use something similar to this method to check for existence given a chain ID and epoch.

I am not sure how we will only check for existence as we have to perform this check inside that function which will involve a storage read.

vitsalis · 2023-05-02T06:58:33Z

x/zoneconcierge/keeper/grpc_query.go

- }
+ // return if chain IDs contain duplicates or empty strings
+ if err := bbntypes.CheckForDuplicatesAndEmptyStrings(req.ChainIds); err != nil {
+ return nil, status.Error(codes.InvalidArgument, err.Error())


The errors from the above function are not specific and helpful for the user. For example, "Duplicate string" might not lead the user to understand that they should not provide duplicate chain IDs.

sure, as CheckForDuplicatesAndEmptyStrings is a generic util and can be used in any module I have wrapped the error from the caller side instead. Have fixed it now..

vitsalis · 2023-05-02T12:14:15Z

x/zoneconcierge/keeper/epoch_chain_info_indexer.go

@@ -20,15 +21,16 @@ func (k Keeper) GetEpochChainInfo(ctx sdk.Context, chainID string, epochNumber u
 return &chainInfo, nil
 }

+// EpochChainInfoExists checks if the latest chain info exists of a given epoch for a given chain ID
+func (k Keeper) EpochChainInfoExists(ctx sdk.Context, chainID string, epochNumber uint64) bool {


This method can also be used on the first lines of the GetEpochChainInfo function above in order to not have duplicated logic.

vitsalis · 2023-05-02T12:26:22Z

x/zoneconcierge/keeper/epoch_chain_info_indexer.go

- return 0, nil, err
- }
-
+func (k Keeper) GetLastFinalizedChainInfo(ctx sdk.Context, chainID string, finalizedEpoch uint64) (uint64, *types.ChainInfo, error) {


Few things:

Why do we return a uint64 corresponding to the finalized epoch if this is available from chainInfo.LatestHeader.BabylonEpoch?

This method seems exactly the same as GetEpochChainInfo, with the main difference that if the epoch parameter is different than the chainInfo.LatestHeader.BabylonEpoch, then we call it again with the epoch parameter set to chainInfo.LatestHeader.BabylonEpoch. So two questions:
a. Why do we need to call GetEpochChainInfo again with the correct epoch? Does anything actually change on the chainInfo object?
b. Maybe we can have a single method. This particular function does not need to be aware that it is retrieving something finalized as it just retrieves the data given to it in the epoch parameter. I would argue we don't actually need this method, especially if (a) doesn't change anything. If we do indeed need it, let's remove any reference to finalized as this function does not do anything finalized specific.

sure, fixed in my last commits.

x/zoneconcierge/keeper/grpc_query.go

Co-authored-by: Vitalis Salis <VitSalis@gmail.com>

gusin13 requested review from SebastianElvis, vitsalis and KonradStaniec April 28, 2023 05:25

gusin13 marked this pull request as ready for review April 28, 2023 05:25

SebastianElvis approved these changes Apr 28, 2023

View reviewed changes

gusin13 added 14 commits April 28, 2023 14:06

finalized chains info

9ad14f8

update proto

3ae0569

comments

8a8cb01

update tests

b2723d9

update comments

d8caeaa

update query

823d3a5

fix e2e tests

5e729dc

fix query params in e2e test

841411a

add e2e test

a67ad65

add log

86b0302

clean up proto

a7feb68

fix swag

34f5a95

update comment

c2e1fda

fix e2e tests

69a3775

gusin13 force-pushed the BM-603-api-add-new-rpc-queries-finalized branch from 4c848d8 to 69a3775 Compare April 28, 2023 08:37

gusin13 changed the base branch from cz-rpc-chains-info to dev April 28, 2023 08:37

wrap up input checker fn

e956f83

vitsalis reviewed Apr 28, 2023

View reviewed changes

gusin13 and others added 4 commits April 28, 2023 15:39

update docstring

6d8ee0c

Co-authored-by: Vitalis Salis <VitSalis@gmail.com>

use empty url vals instead of nil

3c7002b

remove err log

160ed0a

optimize

bcaa672

KonradStaniec approved these changes May 2, 2023

View reviewed changes

gusin13 added 2 commits May 2, 2023 10:38

helper func

1f0868e

add docstring

6184aaa

vitsalis reviewed May 2, 2023

View reviewed changes

gusin13 added 5 commits May 2, 2023 13:16

desc error message by caller

4c76dc4

wrap error

b0936b6

optimize as per suggestions

ac08c02

reduce loc

d161505

extra line

5141fea

vitsalis reviewed May 2, 2023

View reviewed changes

gusin13 and others added 5 commits May 2, 2023 18:06

simplify exists

ba3e00b

Co-authored-by: Vitalis Salis <VitSalis@gmail.com>

simplify exists

425c3e7

simplify logic

dcbb49d

fix comments

4b14662

more comments

abccd46

vitsalis approved these changes May 2, 2023

View reviewed changes

gusin13 merged commit 73f57be into dev May 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support multiple chain ids in FinalizedChainsInfo #365

feat: support multiple chain ids in FinalizedChainsInfo #365

gusin13 commented Apr 27, 2023 •

edited

Loading

SebastianElvis left a comment

SebastianElvis Apr 28, 2023

gusin13 Apr 28, 2023

vitsalis left a comment

vitsalis Apr 28, 2023

gusin13 Apr 29, 2023

vitsalis Apr 28, 2023

gusin13 Apr 29, 2023

vitsalis Apr 28, 2023

gusin13 May 2, 2023

vitsalis May 2, 2023

gusin13 May 2, 2023

vitsalis May 2, 2023

gusin13 May 2, 2023

vitsalis May 2, 2023

gusin13 May 2, 2023

vitsalis May 2, 2023

gusin13 May 2, 2023

vitsalis May 2, 2023

gusin13 May 2, 2023


		resp.RawCheckpoint = rawCheckpoint.Ckpt
		exists, err := k.HasFinalizedChainInfo(ctx, chainID)

feat: support multiple chain ids in FinalizedChainsInfo #365

feat: support multiple chain ids in FinalizedChainsInfo #365

Conversation

gusin13 commented Apr 27, 2023 • edited Loading

Description

Testing

SebastianElvis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

gusin13 commented Apr 27, 2023 •

edited

Loading