-
Notifications
You must be signed in to change notification settings - Fork 170
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: find the BTC-finalised chain info before specific CZ height #264
Changes from 4 commits
f58de1c
7443392
f03de7b
3d96534
b2a331f
65e8792
9d958d8
04e16ce
0518e1c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,42 @@ | ||
package keeper | ||
|
||
import ( | ||
"fmt" | ||
|
||
sdkerrors "cosmossdk.io/errors" | ||
"github.com/babylonchain/babylon/x/zoneconcierge/types" | ||
"github.com/cosmos/cosmos-sdk/store/prefix" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
) | ||
|
||
// FindClosestHeader finds the IndexedHeader that is closest to (but not after) the given height | ||
// TODO: test | ||
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 see tests for this in this PR. |
||
func (k Keeper) FindClosestHeader(ctx sdk.Context, chainID string, height uint64) (*types.IndexedHeader, error) { | ||
chainInfo := k.GetChainInfo(ctx, chainID) | ||
if chainInfo.LatestHeader == nil { | ||
return nil, fmt.Errorf("chain with ID %s does not have a timestamped header", chainID) | ||
} | ||
|
||
// if the given height is no lower than the latest header, return the latest header directly | ||
if chainInfo.LatestHeader.Height <= height { | ||
return chainInfo.LatestHeader, nil | ||
} | ||
|
||
// the requested height is lower than the latest header, trace back until finding a timestamped header | ||
store := k.canonicalChainStore(ctx, chainID) | ||
heightBytes := sdk.Uint64ToBigEndian(height) | ||
iter := store.ReverseIterator(nil, heightBytes) | ||
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. iterators need to be closed, 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. Nice catch! |
||
// if there is no key within range [0, height], return error | ||
if !iter.Valid() { | ||
return nil, fmt.Errorf("chain with ID %s does not have a timestamped header before height %d", chainID, height) | ||
} | ||
// find the header in bytes, decode and return | ||
headerBytes := iter.Value() | ||
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. Very nice way of finding the closest header 👏 |
||
var header types.IndexedHeader | ||
k.cdc.MustUnmarshal(headerBytes, &header) | ||
return &header, nil | ||
} | ||
|
||
func (k Keeper) GetHeader(ctx sdk.Context, chainID string, height uint64) (*types.IndexedHeader, error) { | ||
store := k.canonicalChainStore(ctx, chainID) | ||
heightBytes := sdk.Uint64ToBigEndian(height) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,9 +20,9 @@ func FuzzCanonicalChainIndexer(f *testing.F) { | |
ctx := babylonChain.GetContext() | ||
hooks := zcKeeper.Hooks() | ||
|
||
// invoke the hook a random number of times to simulate a random number of blocks | ||
// simulate a random number of blocks | ||
numHeaders := datagen.RandomInt(100) + 1 | ||
headers := SimulateHeadersViaHook(ctx, hooks, czChain.ChainID, numHeaders) | ||
headers := SimulateHeadersViaHook(ctx, hooks, czChain.ChainID, 0, numHeaders) | ||
|
||
// check if the canonical chain index is correct or not | ||
for i := uint64(0); i < numHeaders; i++ { | ||
|
@@ -42,3 +42,44 @@ func FuzzCanonicalChainIndexer(f *testing.F) { | |
require.Equal(t, headers[numHeaders-1].Header.LastCommitHash, chainInfo.LatestHeader.Hash) | ||
}) | ||
} | ||
|
||
func FuzzFindClosestHeader(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 | ||
|
||
ctx := babylonChain.GetContext() | ||
hooks := zcKeeper.Hooks() | ||
|
||
// no header at the moment, FindClosestHeader invocation should give error | ||
_, err := zcKeeper.FindClosestHeader(ctx, czChain.ChainID, 100) | ||
require.Error(t, err) | ||
|
||
// simulate a random number of blocks | ||
numHeaders := datagen.RandomInt(100) + 1 | ||
headers := SimulateHeadersViaHook(ctx, hooks, czChain.ChainID, 0, numHeaders) | ||
|
||
header, err := zcKeeper.FindClosestHeader(ctx, czChain.ChainID, numHeaders) | ||
require.NoError(t, err) | ||
require.Equal(t, headers[len(headers)-1].Header.LastCommitHash, header.Hash) | ||
|
||
// skip some a non-zero number of headers in between | ||
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 don't entirely understand this sentence 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. Sorry that's a typo 😢 I was meaning that here we skip a non-zero number of headers in between, in order to create a gap of non-timestamped headers. Fixed |
||
gap := datagen.RandomInt(10) + 1 | ||
|
||
// simulate a random number of blocks | ||
// where the new batch of headers has a gap with the previous batch | ||
SimulateHeadersViaHook(ctx, hooks, czChain.ChainID, numHeaders+gap+1, numHeaders) | ||
|
||
// get a random height that is in this gap | ||
randomHeightInGap := datagen.RandomInt(int(gap+1)) + numHeaders | ||
// find the closest header with the given randomHeightInGap | ||
header, err = zcKeeper.FindClosestHeader(ctx, czChain.ChainID, randomHeightInGap) | ||
require.NoError(t, err) | ||
// the header should be the same as the last header in the last batch | ||
require.Equal(t, headers[len(headers)-1].Header.LastCommitHash, header.Hash) | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,39 @@ | ||
package keeper | ||
|
||
import ( | ||
"fmt" | ||
|
||
btcctypes "github.com/babylonchain/babylon/x/btccheckpoint/types" | ||
checkpointingtypes "github.com/babylonchain/babylon/x/checkpointing/types" | ||
epochingtypes "github.com/babylonchain/babylon/x/epoching/types" | ||
"github.com/babylonchain/babylon/x/zoneconcierge/types" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
) | ||
|
||
// GetCkptInfoForFinalizedEpoch gets the raw checkpoint and the associated best submission of a finalised epoch | ||
// CONTRACT: the function can only take an epoch that has already been finalised as input | ||
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. this is public function, at some point someone will do not read this comment and break this invariant. In general I would avoid panicking in public function. I would either return error in case of providing not finalized epoch number or restructure code somehow. 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. Good point. This code snippet was not a function and panicking was expected in that scenario (specifically in RPC 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. Sounds good. I have nothing against panicking in private helpers, if some module invariants are broken due to programming error |
||
func (k Keeper) GetCkptInfoForFinalizedEpoch(ctx sdk.Context, epochNumber uint64) (*checkpointingtypes.RawCheckpoint, *btcctypes.SubmissionKey, error) { | ||
// find the btc checkpoint tx index of this epoch | ||
ed := k.btccKeeper.GetEpochData(ctx, epochNumber) | ||
if ed.Status != btcctypes.Finalized { | ||
err := fmt.Errorf("epoch %d should have been finalized, but is in status %s", epochNumber, ed.Status.String()) | ||
panic(err) // this can only be a programming error | ||
} | ||
if len(ed.Key) == 0 { | ||
err := fmt.Errorf("finalized epoch %d should have at least 1 checkpoint submission", epochNumber) | ||
panic(err) // this can only be a programming error | ||
} | ||
bestSubmissionKey := ed.Key[0] // index of checkpoint tx on BTC | ||
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. hmm the fact that finalized epoch must have one submission keys, which is best submission is a bit implementation detail and the fact that we are doing this check:
is a bit of a smell that something is not right. Maybe btccheckpoing module should have another query like
? 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. Yeah I feel it's better to make the btccheckpoint module to provide the functionality of choosing the best submission. I have implemented a function Nevertheless, I left |
||
|
||
// get raw checkpoint of this epoch | ||
rawCheckpointBytes := ed.RawCheckpoint | ||
rawCheckpoint, err := checkpointingtypes.FromBTCCkptBytesToRawCkpt(rawCheckpointBytes) | ||
if err != nil { | ||
return nil, bestSubmissionKey, err | ||
} | ||
return rawCheckpoint, bestSubmissionKey, nil | ||
} | ||
|
||
// GetFinalizedEpoch gets the last finalised epoch | ||
// used upon querying the last BTC-finalised chain info for CZs | ||
func (k Keeper) GetFinalizedEpoch(ctx sdk.Context) (uint64, error) { | ||
|
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.
Extra empty line.