Skip to content
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

feat: btclightclient: gRPC query fuzz tests #69

Merged
merged 4 commits into from
Jul 21, 2022

Conversation

vitsalis
Copy link
Member

The main novelty of this PR is the fuzz testing of gRPC queries that involve pagination.

During testing, it became apparent that the MainChain query does not work as expected. More specifically, it did not support a Reverse query and it's overall nextKey management system was clumsy at best. This led to a major refactoring that involves the removal of query.FilteredPaginate mechanism 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:

  • We cannot get the answers to the reverse query without retrieving the mainchain since the mainchain is built starting from the tip and getting it's ancestry. We cannot deduce about whether something is part of the mainchain by starting from lower height elements.
  • In the case of the normal query, someone can provide a starting key (in the case of pagination) that corresponds to a header that is not a part of the mainchain, leading to a result that is not the mainchain. This does not affect the security of the system, but a user's mistake might lead to an invalid result without any indication that it is wrong. The only way that we can deduce whether a header is part of the mainchain is by checking whether this header is an ancestor of the tip. To do that, we have to traverse the ancestry of the tip up to the header and then to get the rest of the mainchain we have to get the ancestry of the input -- i.e. the entire mainchain. On the other hand, if the header provided is not on the mainchain, we cannot identify whether that is the case if we don't traverse up to the base header -- i.e. the entire mainchain.

@vitsalis vitsalis requested a review from aakoshh July 20, 2022 07:36
@aakoshh
Copy link
Contributor

aakoshh commented Jul 20, 2022

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.

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very comprehensive! 👏

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe check that you don't encounter duplicates. Although I see at the bottom you check that both were found, so not necessary 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I delegate this check there.

}

// Test with an existing header
tree := genRandomTree(blcKeeper, ctx, 1, 10)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the keeper is empty up to this point. I will do the non-existent tests after this point.

@vitsalis vitsalis merged commit 13fd000 into main Jul 21, 2022
@vitsalis vitsalis deleted the btclightclient-keeper-fuzz branch July 21, 2022 07:29
KonradStaniec added a commit that referenced this pull request Oct 25, 2023
- add handling of jury and validator signatures for unbonding transaction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants