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

Investigate how XCM version upgrade will affect the bridge #2500

Closed
svyatonik opened this issue Aug 25, 2023 · 6 comments · Fixed by paritytech/polkadot-sdk#4949
Closed

Investigate how XCM version upgrade will affect the bridge #2500

svyatonik opened this issue Aug 25, 2023 · 6 comments · Fixed by paritytech/polkadot-sdk#4949
Assignees
Labels
A-chores Something that has to be done, as part of regular maintenance P-Message Delivery

Comments

@svyatonik
Copy link
Contributor

svyatonik commented Aug 25, 2023

Recenty LaneId underlying type has been migrated from [u8; 4] to H256. The idea was that we can do blake2_256(ord(<universal-location-of-bridge-side-1, universal-location-of-bridge-side-2>) and it is guaranteed to be the same at both sides of the bridge. So we won't need any coordination between connected parachains - they could just say we want to connect to to <other-parachain-universal-location> and the lane id will be derived automatically.

I was assuming that we'll pass two MultiLocations to LaneId::new(). But the problem (that I've missed initially) is that MultiLocation is not the xcm::MultiLocation. It is the xcm::v3::MultiLocation. So if we'll ever migrate to XCM v4, the LaneId will change. If we'll dig up here a bit, we'll find another problems. What if source parachain is using the XCMv3 and bridge hub uses XCMv4. Which MultiLocation shall we use? How (and when/if) do we need to to coordinate XCM version upgrades?

So far I've introduced the separate BridgeId in #2261. The idea behind this type is that it'll be used in communications between bridge hub and its sibling/parent source chain. The LaneId will be used (as before) in cross-consensus communications. Apart from simple type separation, it'd allow us for example to have some storage map of BridgeId => LaneId and keep the previous LaneId if XCM version will be upgraded. However, I'm not sure if that's the right way to handle the upgrade - maybe we simply need to continue with new LaneId, which matches the new BridgeId.

We need to investigate how XCM upgrade will affect our identifiers and the whole bridge infrastructure as well.

UPD: by infrastructure I mean - can the chains that are using our BH upgrade XCM version and keep using the unupgraded bridge? Can BH be upgraded without waiting for other chains to upgrade? Can XCM version be upgraded on both bridge hubs independently, or we need to sync it?

@EmmanuellNorbertTulbure
Copy link

Needs a brainstorm session after V1 launch

@svyatonik
Copy link
Contributor Author

Design behind that: https://www.polkadot.network/blog/xcm-part-two-versioning-and-compatibility . It doesn't have much technical details, which are important in our case, though.

Two other important things that we know now (thanks to @bkontur @acatangiu ):

  1. old XCM versions will likely be dropped from the codebase with a time. I.e. when there'll be v10, there's a high chance that v3 primitives will go away. So we won't be able to use v3::MultiLocation always;
  2. SendXcm implementation of HRMP currently uses the VersionWrapper to prepare proper XCM version messages. The pallet-xcm trackes the XCM version of other chains (who's initiator of those subscriptions? are they sent to all chains that have open HRMP channels? or something like that?).

So it looks like we:

  1. need to have a migration procedure, which would migrate BridgeId that e.g. was using v3 to BridgeId that is using v4;
  2. also need to have this version wrapper. Who will manage such subscriptions? They need to be cross bridge. Also looks like it is better to have this version wrapper at the sending chain, not at the BH. Need to think about it a bit

@bkontur
Copy link
Contributor

bkontur commented Nov 21, 2023

HRMP has VersionWrapper feature, where pallet_xcm holds safeXcmVersion per destination and casts latest version to V3 or V2...,
so something like that we should maybe add between BridgeHubs - minimal safe xcm version between BridgeHubs, so even if BHK is on V4, it should downcast message to the V3 for BHP and so on

@bkontur
Copy link
Contributor

bkontur commented Nov 22, 2023

I am going to write some tests (with xcm emulator) above branch with XCMv4. Also try to add VersionWrapper to the pallet-xcm-bridge-hub-router something like XcmpQueue does, so on sending chain will be used the same mechanism for xcm version check.
But also we should probably make sure that Bridge struct (used for blob hauling) uses corrrect/compatible Versioned* stuff between BridgeHubs, somewhere here.
I will investigate.

When xcm versioned flow works, then lets explore subscriptions.

@bkontur bkontur self-assigned this Nov 22, 2023
@bkontur
Copy link
Contributor

bkontur commented Jul 31, 2024

Fixed, so I am closing this. I summarized all the details around LaneId, BridgeId, migration, and versioning here: polkadot-sdk/xcm-bridge-hub/src/lib.rs.

TL;DR: I disconnected BridgeId and LaneId. LaneId is generated only when open_bridge is called, using the same parameters as BridgeId but based on the XCM version. This allows the other side, even if using a different XCM version, to control open_bridge to generate the same LaneId. I also added numerous try_state checks to ensure consistency between upgrades. If we update to a new XCM version on one side of the bridge and encoding/checks are okay, we don't need to migrate BridgeId, and LaneId will never change. The only scenario requiring migration to the latest Versioned* is when we remove older XCM versions, but try_state checks for that. (Maybe I will add permanent migration to the runtimes, which migrates everytime we add new XCM version).

Additionally, I had to make LaneId backward compatible because we upgrade the fellows runtime asynchronously and the proofs use LaneId as a key. Thus, LaneId can be encoded/decoded from the old format [u8; 4] and the new format H256.

@acatangiu
Copy link
Collaborator

This is great Brani, nice work!

Can you also look at Snowbridge pallets and if they also support XCM version upgrades?

github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this issue Sep 2, 2024
Relates to:
paritytech/parity-bridges-common#2451
Closes: paritytech/parity-bridges-common#2500

## Summary

Now, the bridging pallet supports only static lanes, which means lanes
that are hard-coded in the runtime files. This PR fixes that and adds
support for dynamic, also known as permissionless, lanes. This means
that allowed origins (relay chain, sibling parachains) can open and
close bridges (through BridgeHubs) with another bridged (substrate-like)
consensus using just `xcm::Transact` and `OriginKind::Xcm`.

_This PR is based on the migrated code from the Bridges V2
[branch](#4427) from the
old `parity-bridges-common`
[repo](https://github.com/paritytech/parity-bridges-common/tree/bridges-v2)._

## Explanation

Please read
[bridges/modules/xcm-bridge-hub/src/lib.rs](https://github.com/paritytech/polkadot-sdk/blob/149b0ac2ce43fba197988f2642032fa24dd8289a/bridges/modules/xcm-bridge-hub/src/lib.rs#L17-L136)
to understand how managing bridges works. The basic concepts around
`BridgeId` and `LaneId` are also explained there.


## TODO

- [x] search and fix for comment: `// TODO:(bridges-v2) - most of that
stuff was introduced with free header execution:
https://github.com/paritytech/polkadot-sdk/pull/4102` - more info in the
comment
[bellow](#4427 (comment))
- [x] TODO: there's only one impl of `EnsureOrigin<Success = Location>`

## TODO - not blocking review

**benchmarking:**
- [x] regenerate all relevant weights for BH/AH runtimes
- [ ] regenerate default weights for bridging pallets e.g.
`modules/messages/src/weights.rs`
- [ ] add benchmarks for `xcm-bridge-hub` pallet
#5550

**testing:**
- [ ] add xcm-emulator tests for Rococo/Penpal to Westend/Penpal with
full opening channel and sending/receiving `xcm::Transact`

**migrations:**
- [x] add migrations for BridgeHubRococo/Westend
paritytech/parity-bridges-common#2794 (to be
reusable for P/K bridge)
  - [x] check also storage migration, if needed for pallets
  - [ ] migration for XCM type (optional)
  - [x] migration for static lanes to the dynamic (reuse for fellows)

**investigation:**
- [ ] revisit
paritytech/parity-bridges-common#2380
- [ ] check congestion around `LocalXcmChannelManager` and
`OutboundLanesCongestedSignals` impls -
#5551
  - to be reusable for polkadot-fellows
- return `report_bridge_status` was remove, so we need to `XcmpQueue`
alternative?

---------

Signed-off-by: Branislav Kontur <bkontur@gmail.com>
Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
x3c41a pushed a commit to paritytech/polkadot-sdk that referenced this issue Sep 4, 2024
Relates to:
paritytech/parity-bridges-common#2451
Closes: paritytech/parity-bridges-common#2500

## Summary

Now, the bridging pallet supports only static lanes, which means lanes
that are hard-coded in the runtime files. This PR fixes that and adds
support for dynamic, also known as permissionless, lanes. This means
that allowed origins (relay chain, sibling parachains) can open and
close bridges (through BridgeHubs) with another bridged (substrate-like)
consensus using just `xcm::Transact` and `OriginKind::Xcm`.

_This PR is based on the migrated code from the Bridges V2
[branch](#4427) from the
old `parity-bridges-common`
[repo](https://github.com/paritytech/parity-bridges-common/tree/bridges-v2)._

## Explanation

Please read
[bridges/modules/xcm-bridge-hub/src/lib.rs](https://github.com/paritytech/polkadot-sdk/blob/149b0ac2ce43fba197988f2642032fa24dd8289a/bridges/modules/xcm-bridge-hub/src/lib.rs#L17-L136)
to understand how managing bridges works. The basic concepts around
`BridgeId` and `LaneId` are also explained there.

## TODO

- [x] search and fix for comment: `// TODO:(bridges-v2) - most of that
stuff was introduced with free header execution:
https://github.com/paritytech/polkadot-sdk/pull/4102` - more info in the
comment
[bellow](#4427 (comment))
- [x] TODO: there's only one impl of `EnsureOrigin<Success = Location>`

## TODO - not blocking review

**benchmarking:**
- [x] regenerate all relevant weights for BH/AH runtimes
- [ ] regenerate default weights for bridging pallets e.g.
`modules/messages/src/weights.rs`
- [ ] add benchmarks for `xcm-bridge-hub` pallet
#5550

**testing:**
- [ ] add xcm-emulator tests for Rococo/Penpal to Westend/Penpal with
full opening channel and sending/receiving `xcm::Transact`

**migrations:**
- [x] add migrations for BridgeHubRococo/Westend
paritytech/parity-bridges-common#2794 (to be
reusable for P/K bridge)
  - [x] check also storage migration, if needed for pallets
  - [ ] migration for XCM type (optional)
  - [x] migration for static lanes to the dynamic (reuse for fellows)

**investigation:**
- [ ] revisit
paritytech/parity-bridges-common#2380
- [ ] check congestion around `LocalXcmChannelManager` and
`OutboundLanesCongestedSignals` impls -
#5551
  - to be reusable for polkadot-fellows
- return `report_bridge_status` was remove, so we need to `XcmpQueue`
alternative?

---------

Signed-off-by: Branislav Kontur <bkontur@gmail.com>
Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-chores Something that has to be done, as part of regular maintenance P-Message Delivery
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants