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

Bridge: make some headers submissions free #4102

Merged
merged 60 commits into from
Apr 25, 2024

Conversation

svyatonik
Copy link
Contributor

@svyatonik svyatonik commented Apr 12, 2024

supersedes paritytech/parity-bridges-common#2873

Draft because of couple of TODOs:

…of_ex + check it from the transaction extension
…ge GRANDPA transactions are obsolete and, if not, it may apply priority boost to
…coming from CheckAndBoostBridgeGrandpaTransactions
…de of tx body, it is `None` and otherwise it is `Some`
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.

really good test coverage too! 💪

Comment on lines -107 to +111
pub const BridgeHubRococoBaseDeliveryFeeInRocs: u128 = 5_651_581_649;
pub const BridgeHubRococoBaseDeliveryFeeInRocs: u128 = 314_037_860;

/// Transaction fee that is paid at the Rococo BridgeHub for delivering single outbound message confirmation.
/// (initially was calculated by test `BridgeHubRococo::can_calculate_fee_for_complex_message_confirmation_transaction` + `33%`)
pub const BridgeHubRococoBaseConfirmationFeeInRocs: u128 = 5_380_901_781;
pub const BridgeHubRococoBaseConfirmationFeeInRocs: u128 = 57_414_813;
Copy link
Contributor

Choose a reason for hiding this comment

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

10x-15x reduction ❤️

prdoc/pr_4102.prdoc Show resolved Hide resolved
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 !

bridges/primitives/runtime/src/chain.rs Show resolved Hide resolved
@@ -22,6 +22,7 @@ scale-info = { version = "2.11.1", default-features = false, features = [
"derive",
] }
serde = { optional = true, features = ["derive"], workspace = true, default-features = true }
tuplex = { version = "0.1", default-features = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

where do we use tuplex here in BHR or BHW? Or is it because of some macro expansion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is used by generate_bridge_reject_obsolete_headers_and_messages

Copy link
Contributor

Choose a reason for hiding this comment

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

aha, ok, I am not sure if it is possible to hide/re-export tuplex inside that macro, I think we did something similar for other macros for paste?

// Re-export macro to avoid include paste dependency everywhere
pub use sp_runtime::paste;

but also ok to leave it as it is

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6026453

@svyatonik svyatonik added this pull request to the merge queue Apr 25, 2024
Merged via the queue into master with commit a633e95 Apr 25, 2024
138 of 140 checks passed
@svyatonik svyatonik deleted the sv-refund-non-mandatory-headers-fee-polkadot-sdk branch April 25, 2024 05:54
svyatonik added a commit that referenced this pull request May 8, 2024
supersedes paritytech/parity-bridges-common#2873

Draft because of couple of TODOs:
- [x] fix remaining TODOs;
- [x] double check that all changes from
paritytech/parity-bridges-common#2873 are
correctly ported;
- [x] create a separate PR (on top of that one or a follow up?) for
https://github.com/paritytech/polkadot-sdk/tree/sv-try-new-bridge-fees;
- [x] fix compilation issues (haven't checked, but there should be
many).

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
@svyatonik
Copy link
Contributor Author

The plan is to deploy it on Rococo <> Westend before the next release. So I've prepared a branch: https://github.com/paritytech/polkadot-sdk/tree/sv-new-bridges-burning. It is forked from last release and all required commits are cherry-picked:

    git checkout polkadot-v1.11.0
    git checkout -b sv-new-bridges-burning
    git cherry-pick a633e954f3b88697aa797d9792e8a5b5cf310b7e
    git cherry-pick 7e68b2b8da9caf634ff4f6c6d96d2d7914c44fb7
    git cherry-pick a99948939785eb62377a235b448a7f4ae65c01b1
    git cherry-pick 871281783c1be03157319d5143096fd3dd860d0a
    + bump Rococo and Westend BH versions to 1_011_001

I'll build Rococo and Westend BH runtimes on monday and we'll do a upgrade. Then we need to migrate all our relayers to our new scheme - guess it can take a couple of days. After that, we need to update our dashboards and alerts (it should be easy).

github-merge-queue bot pushed a commit that referenced this pull request May 8, 2024
…#4385)

silent, because it'll be deployed with the
#4102, where this code
has been introduced

I've planned originally to avoid doing that check in the runtime code,
because it **may be** checked offchain. But actually, the check is quite
cheap and we could do that onchain too.
paritytech-ci pushed a commit that referenced this pull request May 8, 2024
…#4385)

silent, because it'll be deployed with the
#4102, where this code
has been introduced

I've planned originally to avoid doing that check in the runtime code,
because it **may be** checked offchain. But actually, the check is quite
cheap and we could do that onchain too.
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this pull request Jun 5, 2024
…paritytech#4385)

silent, because it'll be deployed with the
paritytech#4102, where this code
has been introduced

I've planned originally to avoid doing that check in the runtime code,
because it **may be** checked offchain. But actually, the check is quite
cheap and we could do that onchain too.
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
supersedes paritytech/parity-bridges-common#2873

Draft because of couple of TODOs:
- [x] fix remaining TODOs;
- [x] double check that all changes from
paritytech/parity-bridges-common#2873 are
correctly ported;
- [x] create a separate PR (on top of that one or a follow up?) for
https://github.com/paritytech/polkadot-sdk/tree/sv-try-new-bridge-fees;
- [x] fix compilation issues (haven't checked, but there should be
many).

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
…paritytech#4385)

silent, because it'll be deployed with the
paritytech#4102, where this code
has been introduced

I've planned originally to avoid doing that check in the runtime code,
because it **may be** checked offchain. But actually, the check is quite
cheap and we could do that onchain too.
github-merge-queue bot referenced this pull request 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 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:
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
T15-bridges This PR/Issue is related to bridges.
Projects
Status: Done
Status: Audited
Development

Successfully merging this pull request may close these issues.

6 participants