-
Notifications
You must be signed in to change notification settings - Fork 20
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
chore: add finality activation height #101
Conversation
RafilxTenfen
commented
Oct 19, 2024
•
edited
Loading
edited
- Add checks for finality activation height
- Pub rand commit start height at activation height or last commited height whatever is higher
- Finality vote, only vote in blocks after the finality activation height
- Update babylon to v014
…sumer client controller
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.
In general looks good 👍 though lets wait for @gitferry review
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.
Maybe I was missing something, but shouldn't we add a check to prevent the fp sending finality sig before activation height?
@@ -395,6 +389,18 @@ func (fp *FinalityProviderInstance) tryFastSync(targetBlock *types.BlockInfo) (* | |||
return fp.FastSync(startHeight, targetBlock.Height) | |||
} | |||
|
|||
func (fp *FinalityProviderInstance) fastSyncStartHeight(lastFinalizedHeight uint64) (uint64, error) { |
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.
fastSyncStartHeight
is called after we have known that there's finalized blocks, indicating that activation is already started, so maybe we don't need to check activation height here?
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.
fastSyncStartHeight returns the height that fast sync should start right?
I probably can remove the check inside the fastSync than
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.
Lgtm! Minor nits:
Yeap, you are right, there was another loop that sends finality, check added at 7175eba |
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.
Thanks for the changes! Lgtm!