-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
zoneconcierge: API for querying headers in a given epoch #261
Changes from all commits
fe58599
e254c62
4cc3f9d
2c3de9a
f6b9a11
3f3ea1b
8bc1cdf
494a04d
f2f923f
c743f6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package keeper | ||
|
||
import ( | ||
bbn "github.com/babylonchain/babylon/types" | ||
"github.com/babylonchain/babylon/x/zoneconcierge/types" | ||
"github.com/cosmos/cosmos-sdk/store/prefix" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
|
@@ -19,6 +20,45 @@ func (k Keeper) GetEpochChainInfo(ctx sdk.Context, chainID string, epochNumber u | |
return &chainInfo, nil | ||
} | ||
|
||
// GetEpochHeaders gets the headers timestamped in a given epoch, in the ascending order | ||
func (k Keeper) GetEpochHeaders(ctx sdk.Context, chainID string, epochNumber uint64) ([]*types.IndexedHeader, error) { | ||
headers := []*types.IndexedHeader{} | ||
|
||
// find the last timestamped header of this chain in the epoch | ||
epochChainInfo, err := k.GetEpochChainInfo(ctx, chainID, epochNumber) | ||
if err != nil { | ||
return nil, err | ||
} | ||
// it's possible that this epoch's snapshot is not updated for many epochs | ||
// this implies that this epoch does not timestamp any header for this chain at all | ||
if epochChainInfo.LatestHeader.BabylonEpoch < epochNumber { | ||
return nil, types.ErrEpochHeadersNotFound | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
// now we have the last header in this epoch | ||
headers = append(headers, epochChainInfo.LatestHeader) | ||
|
||
// append all previous headers until reaching the previous epoch | ||
canonicalChainStore := k.canonicalChainStore(ctx, chainID) | ||
lastHeaderKey := sdk.Uint64ToBigEndian(epochChainInfo.LatestHeader.Height) | ||
// NOTE: even in ReverseIterator, start and end should still be specified in ascending order | ||
canonicalChainIter := canonicalChainStore.ReverseIterator(nil, lastHeaderKey) | ||
defer canonicalChainIter.Close() | ||
for ; canonicalChainIter.Valid(); canonicalChainIter.Next() { | ||
var prevHeader types.IndexedHeader | ||
k.cdc.MustUnmarshal(canonicalChainIter.Value(), &prevHeader) | ||
if prevHeader.BabylonEpoch < epochNumber { | ||
// we have reached the previous epoch, break the loop | ||
break | ||
} | ||
headers = append(headers, &prevHeader) | ||
} | ||
|
||
// reverse the list so that it remains ascending order | ||
bbn.Reverse(headers) | ||
|
||
return headers, nil | ||
} | ||
|
||
// recordEpochChainInfo records the chain info for a given epoch number of given chain ID | ||
// where the latest chain info is retrieved from the chain info indexer | ||
func (k Keeper) recordEpochChainInfo(ctx sdk.Context, chainID string, epochNumber uint64) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import ( | |
"testing" | ||
|
||
"github.com/babylonchain/babylon/testutil/datagen" | ||
ibctmtypes "github.com/cosmos/ibc-go/v5/modules/light-clients/07-tendermint/types" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
|
@@ -14,7 +15,8 @@ func FuzzEpochChainInfoIndexer(f *testing.F) { | |
f.Fuzz(func(t *testing.T, seed int64) { | ||
rand.Seed(seed) | ||
|
||
_, babylonChain, czChain, zcKeeper := SetupTest(t) | ||
_, babylonChain, czChain, babylonApp := SetupTest(t) | ||
zcKeeper := babylonApp.ZoneConciergeKeeper | ||
|
||
ctx := babylonChain.GetContext() | ||
hooks := zcKeeper.Hooks() | ||
|
@@ -35,3 +37,68 @@ func FuzzEpochChainInfoIndexer(f *testing.F) { | |
require.Equal(t, numForkHeaders, uint64(len(chainInfo.LatestForks.Headers))) | ||
}) | ||
} | ||
|
||
func FuzzGetEpochHeaders(f *testing.F) { | ||
datagen.AddRandomSeedsToFuzzer(f, 10) | ||
|
||
f.Fuzz(func(t *testing.T, seed int64) { | ||
rand.Seed(seed) | ||
|
||
_, babylonChain, czChain, babylonApp := SetupTest(t) | ||
zcKeeper := babylonApp.ZoneConciergeKeeper | ||
epochingKeeper := babylonApp.EpochingKeeper | ||
|
||
ctx := babylonChain.GetContext() | ||
hooks := zcKeeper.Hooks() | ||
|
||
numReqs := datagen.RandomInt(5) + 1 | ||
|
||
epochNumList := []uint64{datagen.RandomInt(10) + 1} | ||
nextHeightList := []uint64{0} | ||
numHeadersList := []uint64{} | ||
expectedHeadersMap := map[uint64][]*ibctmtypes.Header{} | ||
numForkHeadersList := []uint64{} | ||
|
||
// we test the scenario of ending an epoch for multiple times, in order to ensure that | ||
// consecutive epoch infos do not affect each other. | ||
for i := uint64(0); i < numReqs; i++ { | ||
epochNum := epochNumList[i] | ||
// enter a random epoch | ||
if i == 0 { | ||
for j := uint64(0); j < epochNum; j++ { | ||
epochingKeeper.IncEpoch(ctx) | ||
} | ||
} else { | ||
for j := uint64(0); j < epochNum-epochNumList[i-1]; j++ { | ||
epochingKeeper.IncEpoch(ctx) | ||
} | ||
} | ||
|
||
// generate a random number of headers and fork headers | ||
numHeadersList = append(numHeadersList, datagen.RandomInt(100)+1) | ||
numForkHeadersList = append(numForkHeadersList, datagen.RandomInt(10)+1) | ||
// trigger hooks to append these headers and fork headers | ||
expectedHeaders, _ := SimulateHeadersAndForksViaHook(ctx, hooks, czChain.ChainID, nextHeightList[i], numHeadersList[i], numForkHeadersList[i]) | ||
expectedHeadersMap[epochNum] = expectedHeaders | ||
// prepare nextHeight for the next request | ||
nextHeightList = append(nextHeightList, nextHeightList[i]+numHeadersList[i]) | ||
|
||
// simulate the scenario that a random epoch has ended | ||
hooks.AfterEpochEnds(ctx, epochNum) | ||
// prepare epochNum for the next request | ||
epochNumList = append(epochNumList, epochNum+datagen.RandomInt(10)+1) | ||
} | ||
|
||
// attest the correctness of epoch info for each tested epoch | ||
for i := uint64(0); i < numReqs; i++ { | ||
epochNum := epochNumList[i] | ||
// check if the headers are same as expected | ||
headers, err := zcKeeper.GetEpochHeaders(ctx, czChain.ChainID, epochNum) | ||
require.NoError(t, err) | ||
require.Equal(t, len(expectedHeadersMap[epochNum]), len(headers)) | ||
for j := 0; j < len(expectedHeadersMap[epochNum]); j++ { | ||
require.Equal(t, expectedHeadersMap[epochNum][j].Header.LastCommitHash, headers[j].Hash) | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we have pagination here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I also encountered a similar issue with the
MainChain
query of thebtclightclient
module (ref). I think it is possible although it makes the code more complex so we should decide on the tradeoff.