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.

Sign up for GitHub

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

Jump to bottom

Unstuck Snowbridge #313

Merged
merged 28 commits into from
May 19, 2024
Merged

Unstuck Snowbridge #313

merged 28 commits into from
May 19, 2024

Conversation

claravanstaden
Copy link
Contributor

@claravanstaden claravanstaden commented May 17, 2024 •
edited
Loading

Upgrades Snowbridge with an Ethereum client fixes:

Adds a migration to reset the Ethereum checkpoint. Will be reset again to the moment recent checkpoint before merged.

TODO:

  • Check migration works
  • Check converting Sync Committee to prepared keys is OK for a migration

@claravanstaden claravanstaden mentioned this pull request May 17, 2024
Closed
10 tasks
acatangiu
acatangiu reviewed May 17, 2024
View reviewed changes
system-parachains/bridge-hubs/bridge-hub-polkadot/src/bridge_to_ethereum_unstuck.rs

pub struct UnstuckSnowbridge;

impl OnRuntimeUpgrade for UnstuckSnowbridge {
Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to add it to the list of migrations (example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reminder! b13f7a9

@claravanstaden
Copy link
Contributor Author

@acatangiu the migration is roughly doing the same as the force_checkpoint extrinsic. It looks like an expensive operation compared to the P<>K bridge migration - the weight is Weight::from_parts(101_492_831_000, 0). Do you think this will be a problem? We'll test of course but just wanted to also ask your opinion.

acatangiu
acatangiu reviewed May 17, 2024
View reviewed changes
system-parachains/bridge-hubs/bridge-hub-polkadot/src/bridge_to_ethereum_unstuck.rs Outdated Show resolved Hide resolved
system-parachains/bridge-hubs/bridge-hub-polkadot/src/bridge_to_ethereum_unstuck.rs Outdated Show resolved Hide resolved
system-parachains/bridge-hubs/bridge-hub-polkadot/src/bridge_to_ethereum_unstuck.rs Outdated Show resolved Hide resolved
system-parachains/bridge-hubs/bridge-hub-polkadot/src/bridge_to_ethereum_unstuck.rs Outdated
}

fn is_bridge_stuck() -> bool {
LatestFinalizedBlockRoot::<Runtime>::get() == LAST_IMPORTED_BEACON_HEADER.into()
Copy link
Contributor

Choose a reason for hiding this comment

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

We expect that this LAST_IMPORTED_BEACON_HEADER will not be reimported, yes?

(I guess the checkpoint is from a block newer than that)

@acatangiu
Copy link
Contributor

@acatangiu the migration is roughly doing the same as the force_checkpoint extrinsic. It looks like an expensive operation compared to the P<>K bridge migration - the weight is Weight::from_parts(101_492_831_000, 0). Do you think this will be a problem? We'll test of course but just wanted to also ask your opinion.

Should be ok as long as it fits a block.

claravanstaden and others added 6 commits May 17, 2024 16:23
@claravanstaden @acatangiu
Update system-parachains/bridge-hubs/bridge-hub-polkadot/src/bridge_t…
68877e5 
…o_ethereum_unstuck.rs

Co-authored-by: Adrian Catangiu <adrian@parity.io>
@claravanstaden @acatangiu
Update system-parachains/bridge-hubs/bridge-hub-polkadot/src/bridge_t…
1b555da 
…o_ethereum_unstuck.rs

Co-authored-by: Adrian Catangiu <adrian@parity.io>
@claravanstaden @acatangiu
Update system-parachains/bridge-hubs/bridge-hub-polkadot/src/bridge_t…
dcee64e 
…o_ethereum_unstuck.rs

Co-authored-by: Adrian Catangiu <adrian@parity.io>
@claravanstaden
compiler fixes
bdde1f9
@claravanstaden
Merge remote-tracking branch 'origin/unstuck-snowbridge' into unstuck…
5dcb03c 
…-snowbridge
@vgeddes
compiler warnings
ec0c1a3
@vgeddes
Copy link
Contributor

vgeddes commented May 17, 2024

@acatangiu the migration is roughly doing the same as the force_checkpoint extrinsic. It looks like an expensive operation compared to the P<>K bridge migration - the weight is Weight::from_parts(101_492_831_000, 0). Do you think this will be a problem? We'll test of course but just wanted to also ask your opinion.

Should be ok as long as it fits a block.

Yeah this is fine, is well below the block limit. And we know it works because our initial governance proposal already executed a force_checkpoint successfully on BridgeHub.

@claravanstaden claravanstaden marked this pull request as ready for review May 17, 2024 17:23
vgeddes
vgeddes approved these changes May 17, 2024
View reviewed changes
Copy link
Contributor

@vgeddes vgeddes left a comment

Choose a reason for hiding this comment

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

+1

acatangiu
acatangiu approved these changes May 18, 2024
View reviewed changes
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

Migration code looks good, also validated that the new checkpoint leads to https://beaconscan.com/slot/9094528.

@joepetrowski
Copy link
Contributor

/merge

@fellowship-merge-bot fellowship-merge-bot bot merged commit 2f8a10e into polkadot-fellows:main May 19, 2024
38 checks passed
@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acatangiu acatangiu acatangiu approved these changes

@joepetrowski joepetrowski joepetrowski approved these changes

@alistair-singh alistair-singh alistair-singh approved these changes

@vgeddes vgeddes vgeddes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@claravanstaden @acatangiu @vgeddes @joepetrowski @alistair-singh @yrong