zoneconcierge: fuzz tests for various indexers #203
Conversation
| hooks := zcKeeper.Hooks() | ||
|
|
||
| // invoke the hook a random number of times to simulate a random number of blocks | ||
| numHeaders := SimulateHeadersViaHook(ctx, hooks, czChain.ChainID) |
There was a problem hiding this comment.
Would it be better to generate a number of headers outside SimulateHeadersViaHook and pass it as a parameter? For example,
| numHeaders := SimulateHeadersViaHook(ctx, hooks, czChain.ChainID) | |
| numHeaders := datagen.RandomInt(100) + 1 | |
| headers := datagen.GenSequenceIBCTMHeaders(chainID, numHeaders) | |
| SimulateHeadersViaHook(ctx, hooks, czChain.ChainID, headers) |
We need to implement datagen.GenSequenceIBCTMHeaders. Suggesting this because (1) it feels a bit confusing to have a random number as the return value, and (2) changing it also allows the caller to compare the expected headers and the actual headers retrieved from zcKeeper.
| require.NotNil(t, header) | ||
| require.Equal(t, czChain.ChainID, header.ChainId) | ||
| require.Equal(t, i, header.Height) | ||
| } |
There was a problem hiding this comment.
Also compare LastCommitHash?
| @@ -39,7 +39,7 @@ func (k Keeper) updateLatestHeader(ctx sdk.Context, chainID string, header *type | |||
| } | |||
| // NOTE: we can accept header without ancestor since IBC connection can be established at any height | |||
| chainInfo := k.GetChainInfo(ctx, chainID) | |||
| chainInfo.LatestHeader = header | |||
| chainInfo.TryToUpdateHeader(header) | |||
There was a problem hiding this comment.
It seems not very natual to have a updateLatestHeader calls TryToUpdateHeader. Maybe a more natural way is to rename the former to tryToUpdateLatestHeader, which does some checks, then calls UpdateLatestHeader, which does not need to do any checks.
There was a problem hiding this comment.
Agree. Since after moving checks to the keeper function the UpdateLatestHeader function is just an assignment, I merged UpdateLatestHeader into the keeper function and rename the keeper function to tryToUpdateLatestHeader. Also did this for trpToUpdateLatestForkHeader. 👍
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func FuzzChainInfoIndexer_Canonical(f *testing.F) { |
There was a problem hiding this comment.
I feel like this test can be done in FuzzCanonicalChainIndexer altogether
| hooks := zcKeeper.Hooks() | ||
|
|
||
| // invoke the hook a random number of times to simulate a random number of blocks | ||
| _, numForkHeaders := SimulateHeadersAndForksViaHook(ctx, hooks, czChain.ChainID) |
There was a problem hiding this comment.
Same comment as SimulateHeadersViaHook. Maybe we can generate numForkHeaders first and then pass it in as a parameter
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func FuzzForkIndexer(f *testing.F) { |
There was a problem hiding this comment.
I fee like this can be done in FuzzCanonicalChainIndexer altogether
| // simulate the scenario that a random epoch has ended and finalised | ||
| epochNum := datagen.RandomInt(10) | ||
| hooks.AfterEpochEnds(ctx, epochNum) | ||
| hooks.AfterRawCheckpointFinalized(ctx, epochNum) |
There was a problem hiding this comment.
check error? I noticed that the current implementation will not trigger any errors but maybe we can still check error here just in case this will return errors in the future
There was a problem hiding this comment.
IIUC hooks can only invoked by other modules but not users. If the code is correctly written then the hook will not return any error. Any error would be a programming error and we'd better panic there.
| require.Equal(t, numHeaders-1, chainInfo.LatestHeader.Height) | ||
| require.Equal(t, numForkHeaders, uint64(len(chainInfo.LatestForks.Headers))) |
There was a problem hiding this comment.
Here you can only compare the height and number of headers but not the contents because the headers are generated within SimulateHeadersAndForksViaHook
| // SimulateHeadersViaHook generates a random non-zero number of canonical headers via the hook | ||
| func SimulateHeadersViaHook(ctx sdk.Context, hooks zckeeper.Hooks, chainID string) uint64 { |
There was a problem hiding this comment.
See previous comments. It would be better to generate headers and pass them in as a parameter. Otherwise, it hides the headers that you created.
| } | ||
|
|
||
| // SimulateHeadersViaHook generates a random non-zero number of canonical headers and fork headers via the hook | ||
| func SimulateHeadersAndForksViaHook(ctx sdk.Context, hooks zckeeper.Hooks, chainID string) (uint64, uint64) { |
Fixes BM-267
This PR provides fuzz tests for various functionalities of ZoneConcierge, including
FinalizedChainInfoqueryMost of them follow the same paradigm: Generating a random number of canonical headers and fork headers, then check the consistency between the index and the underlying IBC light client.
Along the way we also fixed some bugs and added some datagen functionalities.