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(hard_fork): Lightclient rollback #1363

Merged
merged 13 commits into from
Oct 31, 2024

Conversation

mtsitrin
Copy link
Contributor

@mtsitrin mtsitrin commented Oct 30, 2024

  • Implement the OnHardFork hook in the LightClient module
  • LightClient HardFork mode
    • no LC updates are accepted until the fork is fully resolved.
  • ResolveHardFork

Base automatically changed from delyayedAck_callback to mtsitrin/937-rollapp-hard-fork-hub-side October 31, 2024 08:51
@mtsitrin mtsitrin changed the title feat(hard_fork): part3 (Lightclient rollback) feat(hard_fork): Lightclient rollback Oct 31, 2024
@mtsitrin mtsitrin marked this pull request as ready for review October 31, 2024 13:53
@mtsitrin mtsitrin requested a review from a team as a code owner October 31, 2024 13:53
@mtsitrin mtsitrin merged commit 617fa63 into mtsitrin/937-rollapp-hard-fork-hub-side Oct 31, 2024
3 of 5 checks passed
@mtsitrin mtsitrin deleted the lightclient_callback branch October 31, 2024 13:53
return nil
}

func (k Keeper) RollbackCanonicalClient(ctx sdk.Context, rollappId string, height uint64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not clear if height is target height or fraud height. I suggest renaming it accordingly.

Comment on lines +30 to 32
if h.GetRevisionHeight() < height {
return false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

2 things here:

  1. if it's target height it should probably be <= (unless it's the fraud height?)
  2. the acennding iteration could be very pricy. desc iteration would be far more efficeint but I guess we don't have this api? thinking of 1 year of iteration.. not sure if the consensus states get pruned at some point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. it's fraud height
  2. consensus states got pruned after TrustingPeriod. anyway I'll change it to descending

}

// FIXME: assure there's no case with no proposer
func (k Keeper) getValidatorHashForHeight(ctx sdk.Context, rollappId string, height uint64) []byte {
Copy link
Contributor

Choose a reason for hiding this comment

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

height is not used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants