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

Change LaneId underlying type from [u8; 4] to H256 #2221

Merged
merged 10 commits into from
Jun 22, 2023

Conversation

svyatonik
Copy link
Contributor

@svyatonik svyatonik commented Jun 20, 2023

related to #2213, #2451 and #2457

This PR changes the lane identifier from LaneId([u8; 4]) to LaneId(H256). This gives us ability to generate unique lane identifiers on both sides of the bridge given that two bridge endpoints have their own unique identifiers. In bridge testnets it is the ChainId (which will also be deprecated soon in #2457) and in XCM world it is the location within GlobalConsensus. So now e.g. lane for serving bridge between two asset hubs (Kusama.AssetHub <> Polkadot.AssetHub) doesn't need to be hardcoded anywhere - it's just a blake2_256(order(Kusama.AssetHub.XcmLocation, Polkadot.AssetHub.Location).encode())

I've started implementing map of (location1, location2) => LaneId in #2213, but decided to do this refactor first - it'll make our life significantly easier. For example we won't get any accidental collisions due to lack of "bridge-open" synchronization protocol.

IMPORTANT: there are two breaking changes in this PR, which would require storage migrations:

  1. all storage values that contains LaneId either in values and/or keys. This include: pallet_bridge_messages::InboundLanes, pallet_bridge_messages::OutboundLanes, pallet_bridge_messages::OutboundLanes, pallet_bridge_relayers::RelayerRewards;
  2. the encoding of bp_relayers::RewardsAccountParams has changed, but it is already included in the migration from (1);
  3. the (1) and (2) implies the change in sovereign accounts that are refunding relayers and pay rewards. So their balance shall be transferred to new accounts.

This is a breaking change and would require storage migration on all chains with previous version of pallets.

Obviously, breaking changes also makes new relayer incompatible with old runtimes and vice versa.

Please say now if you have something against this approach or know a better approach. Meanwhile I'll work on remaining TODOs here:

  • separate PR to box proof argument in delivery call of messages pallet - after this PR it is too large;
  • cleanup;
  • fix testnets + manual tests.

@svyatonik svyatonik added P-Relay P-Runtime PR-audit-needed A PR has to be audited before going live. PR-breaksrelay A PR that is going to break existing relayers. I.e. some Runtime changes render old relayers unusabl PR-breaksruntime A PR that is going to break runtime bridge compatibility. We need to be careful with upgrade. labels Jun 20, 2023
@svyatonik svyatonik marked this pull request as ready for review June 21, 2023 05:43
Copy link
Collaborator

@serban300 serban300 left a comment

Choose a reason for hiding this comment

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

Looks good !

bin/millau/runtime/src/rialto_messages.rs Outdated Show resolved Hide resolved
@svyatonik svyatonik merged commit 594ebac into master Jun 22, 2023
@svyatonik svyatonik deleted the change-lane-id-from-u32-to-h256 branch June 22, 2023 10:13
svyatonik pushed a commit that referenced this pull request Jul 17, 2023
* Collectives integration tests xcm v3

* remove comment

* review fixs

---------

Co-authored-by: parity-processbot <>
bkontur pushed a commit that referenced this pull request May 7, 2024
* change LaneId underlying type from [u8; 4] to H256

* fixed typo

* added some tests

* spelling

* started fixing testnets

* uncommented call size test

* changed RewardsAccountParams encoding + added values separator when computing LaneId

* review suggestions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-Relay P-Runtime PR-audit-needed A PR has to be audited before going live. PR-breaksrelay A PR that is going to break existing relayers. I.e. some Runtime changes render old relayers unusabl PR-breaksruntime A PR that is going to break runtime bridge compatibility. We need to be careful with upgrade.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants