-
Notifications
You must be signed in to change notification settings - Fork 105
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
Skip mandatory commitment when beefy authorities not changed #1215
Open
yrong
wants to merge
41
commits into
main
Choose a base branch
from
ron/skip-mandatory-commitment
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 7 commits
Commits
Show all changes
41 commits
Select commit
Hold shift + click to select a range
a686dfa
Permit non-consecutive increases in validator set
Lederstrumpf a223f97
adjust submitInitial too
Lederstrumpf ea2f146
Improve filtering
vgeddes ebb5794
Skip mandatory commitment
yrong 0faba1f
Merge remote-tracking branch 'Lederstrumpf/permit-nonconsecutive-incr…
yrong 5eba01b
Fix updating with current beefy state
yrong 482e8a2
Ignore submit when authorities not change
yrong 40f9592
Cleanup
yrong 8853005
Improve BEEFY relayer
vgeddes 629a478
Sync beefy commitment on demand
yrong 67c52e8
Minor fix
yrong dba8cfc
More comments
yrong a886879
More refactoring
yrong fd7db5b
review feedback
vgeddes 58d83bc
review feedback #2
vgeddes 80b4dbd
unused code
vgeddes c11ed7d
Fix for boundary update
yrong 10f38d4
Fix for skip mandatory commitment
yrong 98e3e74
Some refactoring
yrong fc29329
Fix ci breaking
yrong 6efd6c1
Merge branch 'vincent/beefy-relay-improvements' into ron/beefy-relay-…
yrong 715ac5a
Find for next beefy block
yrong 7d06d62
Merge branch 'ron/beefy-relay-improvements' into ron/skip-mandatory-c…
yrong 16c5012
Merge branch 'ron/skip-mandatory-commitment' of https://github.com/Sn…
yrong 913960e
Improve log
yrong edd9f21
Remove check unrelated
yrong 47b0530
Merge branch 'ron/beefy-relay-improvements' into ron/skip-mandatory-c…
yrong 08c70f1
Check commitment in CurrentValidatorSetID
yrong 95845a3
Sync beefy commitment on demand (#1217)
yrong bb0c52f
Resolve conflicts
yrong 6d26d04
unused context parameter
vgeddes 39aac20
Merge branch 'main' into vincent/beefy-relay-improvements
vgeddes ba37da4
Merge branch 'vincent/beefy-relay-improvements' into ron/skip-mandato…
yrong 3d8daa6
Test happy path
yrong 47cceeb
More tests
yrong 0f221e5
More tests
yrong ae9c1ad
Merge branch 'main' into ron/skip-mandatory-commitment
yrong 585619e
For rococo compatibility
yrong 4b80965
Merge branch 'main' into ron/skip-mandatory-commitment
yrong 2b30202
Revert changes
yrong c99028d
Allow commitment in future
yrong 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
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.
We need some tests in the solidity that skips a mandatory commitment.
Example scenario:
We are in validator set id 6 (next validator set 7), we skip 7 and 8 and then import 9. After importing 9 the current validator set will be 7 and the next validator set will be 10.
See here: https://github.com/Snowfork/snowbridge/pull/1215/files#diff-473fead6cd500a4a05f06d0cea4d9ef166c9bdbe2a2e1a06a33d2e1aedcc428eR379-R382
We also need to make sure that a future commit does not commit to a block number in the past and a test that covers it.
Context:
#1137 (comment)
#1137 (comment)
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 catch Alistair, yeah, we need more unit tests for this work.
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, so in this case the current&next validatorId pair will be (7,10) and
latestBeefyBlock
is in session 9.I would expect a minor change here
snowbridge/contracts/src/BeefyClient.sol
Line 259 in 40f9592
from
if (commitment.validatorSetID == currentValidatorSet.id)
toif (commitment.validatorSetID == currentValidatorSet.id) || commitment.validatorSetID == nextValidatorSet.id - 1
will just work but I'll definetely do more 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.
c11ed7d