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

[bridges-v2] Permissionless lanes #4949

Merged
merged 105 commits into from
Sep 2, 2024
Merged

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Jul 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 from the old parity-bridges-common repo.

Explanation

Please read bridges/modules/xcm-bridge-hub/src/lib.rs to understand how managing bridges works. The basic concepts around BridgeId and LaneId are also explained there.

TODO

  • 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
  • TODO: there's only one impl of EnsureOrigin<Success = Location>

TODO - not blocking review

benchmarking:

testing:

  • add xcm-emulator tests for Rococo/Penpal to Westend/Penpal with full opening channel and sending/receiving xcm::Transact

migrations:

investigation:

@bkontur bkontur added the T15-bridges This PR/Issue is related to bridges. label Jul 4, 2024
@bkontur bkontur self-assigned this Jul 4, 2024
Base automatically changed from bko-bridges-v2-backport-pruning to bko-bridges-v2-backport-refactoring July 10, 2024 10:13
Base automatically changed from bko-bridges-v2-backport-refactoring to master July 12, 2024 08:48
@bkontur bkontur force-pushed the bko-bridges-v2-permissionless-lanes branch from 2119b50 to c00298e Compare July 16, 2024 08:45
@bkontur bkontur force-pushed the bko-bridges-v2-permissionless-lanes branch from cc9e609 to 2ddf5f2 Compare July 17, 2024 15:36
@bkontur bkontur force-pushed the bko-bridges-v2-permissionless-lanes branch 4 times, most recently from c7a254f to 2431b1a Compare July 29, 2024 15:00
@bkontur
Copy link
Contributor Author

bkontur commented Jul 30, 2024

bot help

@bkontur bkontur force-pushed the bko-bridges-v2-permissionless-lanes branch from 021ddda to 648498b Compare July 30, 2024 22:41
bkontur and others added 16 commits July 31, 2024 13:23
Original PR with more context: paritytech/parity-bridges-common#2211


Signed-off-by: Branislav Kontur <bkontur@gmail.com>
Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
…=bridge-hub-westend --runtime_dir=bridge-hubs --target_dir=cumulus --pallet=pallet_bridge_messages
…=bridge-hub-rococo --runtime_dir=bridge-hubs --target_dir=cumulus --pallet=pallet_bridge_messages
* add LaneState enum and a field to lanes data

* use OptionQuery in InboundLanes and OutboundLanes

* ensure that lanes are opened when accessing them

* an option to specify opened lanes in genesis config + fixed tests

* added PR reference to TODO

* fix failing benchmarks

* spelling

* get_or_init_data -> data

bump Substrate, Polkadot and Cumulus (#2223)
* 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
* some useful stuff like LanesManager

* clippy

* more clippy

* Error::LanesManager

* {in, out}bound_lane -> active_{in, out}bound_lane

* merge two impl blocks in one

* fmt
* Updated version according to Cumulus

* Updated docs
* allow delivery confirmations on closed outbound lanes

* fix benchmarks compilation
* XCM over bridge pallet (won't ever be merged to this repo): initial commit

* try both bridge-id and lane-id

* flush

* flush

* flush

* flush

* first prototype

* started working on xcm-over-bridge tests

* proper open_bridge_works test

* more open_bridge tests

* request_bridge_closure tests

* tests for close_bridge

* report_misbehavior tests

* renaming xcm-over-bridge ad xcm-bridge-hub: we'll have two similar pallets for dynamic lanes/fees support. One will be deployed at the bridge hub (xcm-bridge-hub) and another one (xcm-bridge-hub-router) at sibling/parent chains to send messages to remote network over the bridge hub

* added couple of TODOs

* moved BridgeQueuesState here + TODO for implementing report_bridge_queues_state + fmt

* fixing TODOs

* fixing TODOs

* moved bridge locations to primitives

* added a couple of TODOs

* ref issue from TODO

* clippy and spelling

* fix tests

* another TODOs

* typo

* LaneState -> force_close_bridge

* start_closing_the_bridge -> start_closing_bridge

* removed Closing bridge state, so now we only have the Opened -> optional temporary Closed state -> None

* spelling

* started prototyping suspend+resume on misbehavior

* continue prototyping

* more tests for open_bridge

* more tests for close_bridge

* more tests for report_misbehavior

* started tests for resume_misbehaving_bridges

* implemented tests for resume_misbehaving_bridges

* remove UnsuspendedChannelWithMisbehavingOwner because now, when all bridges are resumed at once + we assume that the inbound XCM channel is suspended immediately it is no longer actual

* added TODO

* add comment re misbehavior reporter if he's associated with the bridge owner

* ignored clippy

* fmt

* use versioned XCM structs in public interfaces and storage + Box XCM locations where possible

* spelling

* removed TODO in favor of paritytech/parity-bridges-common#2257

* LocalChannelManager -> LocalXcmChannelManager

* removed code specific to #2233, because it isn't the only option now

* removemisbehavior mentions

* also temporary (?) remove BridgesByLocalOrigin because the storage format will likely change to be able to resume bridges from the on_iniitalize/on_idle

* AllowedOpenBridgeOrigin -> OpenBridgeOrigin

* - TooManyBridgesForLocalOrigin

* use bridge_destination_relative_location in args

* better impl of strip_low_level_junctions

* get rid of xcm_into_latest

* clippy

* added #![warn(missing_docs)] to new crates
)

* implement ExportXcm and MessageDispatch for pallet-xcm-bridge-hub

* spelling
… pallet-xcm-bridge-hub (#2261)

Xcm bridge hub router v2 (backport to master branch) (#2312)

* copy new pallet (palle-xcm-bridge-hub-router) from dynamic-fees-v1 branch

* added remaining traces of pallet-xcm-bridge-hub-router

* added comment about sharing delivery fee factor between all bridges, opened by this chain

* spelling

* clippy

Implement additional require primitives for dynamic fees directly for pallet-xcm-bridge-hub (#2261)

* added backoff mechanism to inbound bridge queue

* impl backpressure in the XcmBlobHaulerAdapter

* leave TODOs

* BridgeMessageProcessor prototype

* another TODO

* Revert "also temporary (?) remove BridgesByLocalOrigin because the storage format will likely change to be able to resume bridges from the on_iniitalize/on_idle"

This reverts commit bdd7ae11a8942b58c5db6ac6d4e7922aa28cece4.

* prototype for QueuePausedQuery

* implement ExportXcm and MessageDispatch for pallet-xcm-bridge-hub

* spelling

* flush

* small comments to myself

* more backports from dynamic-fees-v1

* use new pallet as exporter and dispatcher in Millau

* use new pallet as exporter and dispatcher in Rialto

* use new pallet as exporter and dispatcher in RialtoParachain

* flush

* fix remaining compilation issues

* warnings + fmt

* fix tests

* LocalXcmChannelManager

* change lane ids

* it works!

* remove bp-xcm-bridge-hub-router and use LocalXcmChannelManager everywhere

* removed commented code

* cleaning up

* cleaning up

* cleaning up

* - separated BridgeId and LaneId
- BridgeId now uses versioned universal locations
- added missing stuff to exporter.rs

* OnMessagesDelivered is back

* start using bp-xcm-bridge-hub as OnMessagesDelivered

* cleaning up

* spelling

* fix stupid issues

* Backport latest relevant dynamic fees changes from v1 to v2 (#2372)

* backport latest relevant dynamic fees changes from v1 to v2

* fix comment

Added remaining unit tests for pallet-xcm-bridge-hub (#2499)

* added backoff mechanism to inbound bridge queue

* impl backpressure in the XcmBlobHaulerAdapter

* leave TODOs

* BridgeMessageProcessor prototype

* another TODO

* Revert "also temporary (?) remove BridgesByLocalOrigin because the storage format will likely change to be able to resume bridges from the on_iniitalize/on_idle"

This reverts commit bdd7ae11a8942b58c5db6ac6d4e7922aa28cece4.

* prototype for QueuePausedQuery

* implement ExportXcm and MessageDispatch for pallet-xcm-bridge-hub

* spelling

* flush

* small comments to myself

* more backports from dynamic-fees-v1

* use new pallet as exporter and dispatcher in Millau

* use new pallet as exporter and dispatcher in Rialto

* use new pallet as exporter and dispatcher in RialtoParachain

* flush

* fix remaining compilation issues

* warnings + fmt

* fix tests

* LocalXcmChannelManager

* change lane ids

* it works!

* remove bp-xcm-bridge-hub-router and use LocalXcmChannelManager everywhere

* removed commented code

* cleaning up

* cleaning up

* cleaning up

* - separated BridgeId and LaneId
- BridgeId now uses versioned universal locations
- added missing stuff to exporter.rs

* OnMessagesDelivered is back

* start using bp-xcm-bridge-hub as OnMessagesDelivered

* cleaning up

* spelling

* fix stupid issues

* added remaining unit tests for pallet-xcm-bridge-hub

fixed benchmarks (#2504)

Remove pallet_xcm_bridge_hub::SuspendedBridges (#2505)

* remove pallet_xcm_bridge_hub::SuspendedBridges

* apply review suggestions
@@ -12,6 +12,7 @@ scale-info = { features = ["derive"], workspace = true }
frame-support = { workspace = true }
sp-core = { workspace = true }
sp-runtime = { workspace = true }
sp-std = { workspace = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't keep on using sp-std: #2101

Copy link
Contributor

@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 ! Just left a few nits.

}

/// Get existing inbound lane without any additional state checks.
pub fn any_state_inbound_lane(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I would name this simply inbound_lane(). any_state seems redundant. In my opinion we need to specify only if there is a filter, for example active_inbound_lane(). But if there is no filter, inbound_lane() is enough.

The same for any_state_outbound_lane()

Comment on lines +212 to +214
// TODO: https://github.com/paritytech/parity-bridges-common/issues/2006 we either need fishermens
// to watch this rule violation (suspended, but keep sending new messages), or we need a
// hard limit for that like other XCM queues have
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment still valid ? I see that the linked issue is closed and it's not clear to me from the description what we would need fishermen for.

pub fn close_bridge(
origin: OriginFor<T>,
bridge_destination_universal_location: Box<VersionedInteriorLocation>,
may_prune_messages: MessageNonce,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Should we rename this to something like max_pruned_messages ?

let two_writes_weight = write_weight + write_weight;
let mut spent_weight = Weight::zero();
/// Remove message from the storage. Doesn't perform any checks.
pub fn remove_oldest_unpruned_message(&mut self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I would just name this prune_message. I think it should be obvious that we remove the oldest unpruned one first.

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-kusama-bridge/2971/61

@hqwangningbo
Copy link

@bkontur @serban300 Hey guys, can you speed up that feature?🙏
Bifrost wants to implement cross-chaining of BNC between Polkadot-Kusama. Currently this feature is implemented through centralised exchanges, but it's not user friendly.
We need to implement BNC across the chain using TeleportAssets.

@serban300
Copy link
Contributor

@bkontur @serban300 Hey guys, can you speed up that feature?🙏 Bifrost wants to implement cross-chaining of BNC between Polkadot-Kusama. Currently this feature is implemented through centralised exchanges, but it's not user friendly. We need to implement BNC across the chain using TeleportAssets.

@hqwangningbo for me, apart from a couple of nits, this PR is good to go, but we still need a security audit for it and after that there are still a couple of things that are left to be done as I understand from the description. Also @bkontur is on vacation this week.

Also after the development is complete, I think we'll still need to wait until the next release window (which I think is once every 3 months ?) to deploy this on Polkadot. But I'm not 100% sure. Maybe @acatangiu or @bkontur have more details about this feature and the timeline.

@hqwangningbo when would you need this ? Do you have a specific date in mind ?

@hqwangningbo
Copy link

@bkontur @serban300 Hey guys, can you speed up that feature?🙏 Bifrost wants to implement cross-chaining of BNC between Polkadot-Kusama. Currently this feature is implemented through centralised exchanges, but it's not user friendly. We need to implement BNC across the chain using TeleportAssets.

@hqwangningbo for me, apart from a couple of nits, this PR is good to go, but we still need a security audit for it and after that there are still a couple of things that are left to be done as I understand from the description. Also @bkontur is on vacation this week.

Also after the development is complete, I think we'll still need to wait until the next release window (which I think is once every 3 months ?) to deploy this on Polkadot. But I'm not 100% sure. Maybe @acatangiu or @bkontur have more details about this feature and the timeline.

@hqwangningbo when would you need this ? Do you have a specific date in mind ?

For me of course the sooner the better, but I understand you have a lot of work to do. It would be helpful to have a clear timeline for us as well.

@lurpis
Copy link

lurpis commented Aug 29, 2024

It has been more than a year and a half since I first heard about the P <> K bridge. Is there any possibility of speeding it up? Since Bifrost assets are distributed on two parachains, Bifrost-Kusama and Bifrost-Polkadot, we need to rely on the opening of the bridge before launching the tokenomics 2.0. but Bifrost Tokenomics 2.0 is scheduled to be released in October.

@acatangiu
Copy link
Contributor

acatangiu commented Sep 2, 2024

@lurpis

It has been more than a year and a half since I first heard about the P <> K bridge. Is there any possibility of speeding it up? Since Bifrost assets are distributed on two parachains, Bifrost-Kusama and Bifrost-Polkadot, we need to rely on the opening of the bridge before launching the tokenomics 2.0. but Bifrost Tokenomics 2.0 is scheduled to be released in October.

The P <> K bridge is live since April this year. It supports bi-directional asset transfers since then.

IIUC correctly, what you want is custom bridged token handling: have dedicated bridge lane between Bifrost-Kusama and Bifrost-Polkadot so you can directly transfer PolkadotBNC and KusamaBNC between the two (without going through Asset Hubs), and have custom logic on your parachains to treat the two assets as one single/global BNC asset.

@hqwangningbo

For me of course the sooner the better, but I understand you have a lot of work to do. It would be helpful to have a clear timeline for us as well.

The permissionless lanes feature (this PR) is currently under security audit, but we are confident it will be released with the end-of-September Polkadot-SDK release. The new SDK components will then have to be picked up by https://github.com/polkadot-fellows/runtimes/ maintainers and released with a new Runtimes release, then those new BridgeHub runtimes need to be upgraded on-chain.

An optimistic estimate for all of the above is Nov 2024.

Bifrost Tokenomics 2.0 is scheduled to be released in October.

You could develop Bifrost Tokenomics 2.0 in parallel, test it out on XCM emulator or local networks (or Rococo Westend if you have Bifrost chains deployed there) using the upcoming SDK, and when BH is updated ~Nov, you can be ready and launch immediately.

Comment on lines +31 to +34
In our [Kusama<>Polkadot bridge](../../docs/polkadot-kusama-bridge-overview.md) we are using lane
as a channel of communication between two parachains of different relay chains. For example, lane
`[0, 0, 0, 0]` is used for Polkadot <> Kusama Asset Hub communications. Other lanes may be used to
bridge other parachains.
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add updated documentation here - how are lanes opened and used.

@bkontur bkontur added this pull request to the merge queue Sep 2, 2024
Merged via the queue into master with commit 2210099 Sep 2, 2024
182 of 189 checks passed
@bkontur bkontur deleted the bko-bridges-v2-permissionless-lanes branch September 2, 2024 16:27
x3c41a pushed a commit that referenced this pull request 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:
#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>
@acatangiu
Copy link
Contributor

FYI: @lurpis @hqwangningbo there's a public Polkadot-SDK Release Calendar you can now use. The individual releases boards (e.g. Stable2409 LTS Release) will also be populated with the PRs part of said releases.

@lurpis
Copy link

lurpis commented Sep 6, 2024

@acatangiu

IIUC correctly, what you want is custom bridged token handling: have dedicated bridge lane between Bifrost-Kusama and Bifrost-Polkadot so you can directly transfer PolkadotBNC and KusamaBNC between the two (without going through Asset Hubs), and have custom logic on your parachains to treat the two assets as one single/global BNC asset.

Yes, we completed the logic here in August and passed on the testnet. What we are waiting for now is the restriction to be lifted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T15-bridges This PR/Issue is related to bridges.
Projects
Status: Done
Status: In progress
Status: Done
Development

Successfully merging this pull request may close these issues.

Investigate how XCM version upgrade will affect the bridge
9 participants