-
Notifications
You must be signed in to change notification settings - Fork 224
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
Minimal implementation of backward verification for IBC relayer #709
Merged
Merged
Changes from all commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
8a5820b
Use custom error type instead of anomaly::BoxError
romac 6df08e6
prototype backward verification
romac 7935643
backward tests
romac d748766
re-enabled model based tests
romac 4e12072
fix backward algorithm
romac 66ed3c4
optimize backward verification
romac def731d
disable backward opti
romac e54abad
Fix wrong assertion
romac adae637
Compute last_block_id hash when generating a light chain
romac e580505
Add test for light chain correctness
romac 31ba7cc
Add property-based tests for backward verification
romac 28e21aa
Comment out `bad` test
romac 5e37c5d
Add more tests
romac a3cd242
Remove println statement
romac 0650fe5
Use prop_assert!
romac cf82e7d
Formatting
romac cb4083d
Remove hacky backward verification test
romac dcc2e4e
Add negative tests for backward verification
romac 6ba571c
Feature-guard backward verification behind "backward-verif" flag
romac 2e141c6
Rename LightClient::verify_bisection to LightClient::verify_forward
romac be1179a
Update changelog
romac 6439ccc
Update doc comments
romac 96edef3
Formatting
romac 14ed02d
Fixup after rebase
romac b022d36
Add integration test for backward verification
romac 3753a63
Remove `backward-verif` feature in favor of `unstable`
romac 81311d9
Merge branch 'master' into romac/backward-verif
romac fc98f81
Cleanup + couple comments
romac 13b0825
Check that root state for backward verif is within trusting period
romac b544f71
Add doc comment
romac 0b78243
Fix mock clock time in testgen-based tests
romac e892095
Remove unused import
romac 14afa8e
Revert "Use custom error type instead of anomaly::BoxError"
romac 50d2209
Formatting
romac File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,3 +14,6 @@ Cargo.lock | |
|
||
# RPC probe results | ||
/rpc-probe/probe-results/ | ||
|
||
# Proptest regressions dumps | ||
**/*.proptest-regressions | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Aren't we supposed to keep the regressions in version control?
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.
Good question, I would have thought not since those are evidence of a test failing and would typically be fixed and thus obsolete before the PR gets merged. But perhaps I am understanding this wrong?
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.
My understanding of proptest's failure persistence is that it re-runs any previous known failure cases before trying to generate any more novel ones, which provides additional assurance in future that the bug has been eliminated.
I haven't used proptest extensively though and may not be using it properly yet. In playing around with it today I found that it works well when you write failing tests for the first time (it persists the random seed it used to create that failure as claimed in the docs). When you then introduce a new failing test, it doesn't update the regressions file for some reason.
But if you delete the regressions folder entirely and both tests fail, it saves the seeds for both failing tests 🤷♂️
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.
Oh I had missed that, really neat feature actually, though it seems a bit brittle indeed in some cases.
Then it makes sense to not ignore those files and leave it it up to developers to check in these regressions in or not (one may not want to check-in a regression for a broken test).
What do you think?
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.
Yeah I'd say that, with property-based testing, any regressions anyone finds should be captured and committed.
Not checking it in is analogous to writing a test locally that fails on your machine (potentially showing that something's broken), but not adding it to version control, so the bug/broken code never gets found by others 😁