feat: btclightclient: gRPC query fuzz tests #69
Conversation
|
Thank you for the detailed rationale of the change. Makes sense; I suppose the only thing you could do to make it query-based both ways is to actually store the main chain separately. |
| // The base header is located at the end of the mainchain | ||
| // which requires starting at the end | ||
| mainchain := k.headersState(sdkCtx).GetMainChain() | ||
| // Reverse the mainchain -- we want to retrieve results starting from the base header |
There was a problem hiding this comment.
The accepted answer at https://stackoverflow.com/questions/28058278/how-do-i-reverse-a-slice-in-go shows a generic variant; another one is in golang/go#47988. Since you bumped the Go version to 1.18, maybe it's worth sticking it somewhere. It's bonkers to have to do this every time.
There was a problem hiding this comment.
Implemented this under a types/utils.go global file.
| if _, ok := headersMap[hash.String()]; !ok { | ||
| t.Fatalf("Hashes returned a hash that was not created") | ||
| } | ||
| hashesFound[hash.String()] = true |
There was a problem hiding this comment.
Maybe check that you don't encounter duplicates. Although I see at the bottom you check that both were found, so not necessary 👍
There was a problem hiding this comment.
Yep, I delegate this check there.
| } | ||
|
|
||
| // Test with an existing header | ||
| tree := genRandomTree(blcKeeper, ctx, 1, 10) |
There was a problem hiding this comment.
Is the keeper empty up to this point? Maybe it would be better to load the data first, and then try whether a non-existing header is found or not. At least give the store a chance to return something, right?
There was a problem hiding this comment.
Yep, the keeper is empty up to this point. I will do the non-existent tests after this point.
- add handling of jury and validator signatures for unbonding transaction.
The main novelty of this PR is the fuzz testing of gRPC queries that involve pagination.
During testing, it became apparent that the
MainChainquery does not work as expected. More specifically, it did not support aReversequery and it's overallnextKeymanagement system was clumsy at best. This led to a major refactoring that involves the removal ofquery.FilteredPaginatemechanism in favor of a custom mechanism.The main drawback of the refactoring is that the new implementation is less performant -- i.e. we retrieve the entire mainchain in the case of the reverse query and all the mainchain elements up to a particular depth in the normal query. However, this performance degradation is required to have correctness. More specifically: