Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Enable over-bridge-messaging in Rococo/Wococo runtime #3377

Merged
9 commits merged into from
Jul 6, 2021

Conversation

svyatonik
Copy link
Contributor

This PR adds two pallets (every pallet is 'deployed' twice - on Rococo and on Wococo) to Rococo/Wocoo runtime. The messages pallet configuration isn't trivial, hence the separate bridge_messages.rs file. Normally there'll be to files like that - one that describes bridge with Chain2 and which is a part of Chain1 runtime and another one that describes bridge with Chain1 and which is a part of Chain2 runtime. Here we have a slightly different scenario when there's one runtime => the single file.

Short pallets description (since it is first reference from the polkadot repo):

  • pallet-bridge-messages: is the 'core' pallet of this PR. It has the only interesting (for end-user/apps) entrypoint - send_message, which enqueues your message (encoded call of the bridged chain) in the internal queue. Eventually, your message will be picked up by the offchain (not to confuse with offchain workers) relayer, which will submit delivery transaction to the bridged chain. Once messages are delivered, relayer will submit confirmation transaction back to the original chain. The side-effect of this tx is the MessagesDelivered event, where you could find info on whether message dispatch has succeeded or not;
  • pallet-bridge-dispatch: a simple pallet that will try to decode the message from the bridged chain as a call of this chain (in Rococo<>Wococo case it is the same call). If call is decoded and passes all other checks (like spec_version check, weight check, ...), it is dispatched. In any case - for every incoming message, it'll emit event (MessageWeightMismatch, MessageVersionSpecMismatch, MessageDispatched, ...).

Deployment details (mostly for ones, who has some internal knowledge of pallets):

  • there are no any parameters (like conversion rate) in messages pallet configuration;
  • the owner of pallets will be empty initially (so only root can halt pallet). Could implement runtime upgrade, but it would be easier to transfer ownership with sudo call (or not to do that at all);
  • all constants introduced in this PR (which are not a part of bp_rococo/bp_wococo) are a copy of Millau constants.

@svyatonik svyatonik added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jun 28, 2021
@svyatonik svyatonik added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jun 28, 2021
}

fn transaction_payment(transaction: MessageTransaction<Weight>) -> crate::Balance {
// current fee multiplier is used here
Copy link
Contributor Author

@svyatonik svyatonik Jun 28, 2021

Choose a reason for hiding this comment

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

This will also likely be changed (e.g. x2-ed, not sure yet) in the P<>K deployment, because it describes future transactions, where multiplier may increase. Actually in P<>K we'll need messages parameter for that, because this value must be taken from bridged runtime storage

Copy link
Contributor

@HCastano HCastano 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!

runtime/rococo/src/bridge_messages.rs Outdated Show resolved Hide resolved
runtime/rococo/src/bridge_messages.rs Outdated Show resolved Hide resolved
impl MessageBridge for AtRococoWithWococoMessageBridge {
const THIS_CHAIN_ID: ChainId = ROCOCO_CHAIN_ID;
const BRIDGED_CHAIN_ID: ChainId = WOCOCO_CHAIN_ID;
const RELAYER_FEE_PERCENT: u32 = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in the future it would be cool to have relayers dictate this rate themselves in order to create more realistic market dynamics

runtime/rococo/src/bridge_messages.rs Outdated Show resolved Hide resolved
// Since we deal with testnets here, in case of failure + urgency:
//
// 1) ping bridges team about this failure;
// 2) comment/remove the test.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// 2) comment/remove the test.
// 2) comment out or ignore the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't simply ignore failing test, if it blocks CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(unless you mean #[ignore], of course)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I meant #[ignore]. Should we make that more explicit in the comment?

runtime/rococo/src/bridge_messages.rs Outdated Show resolved Hide resolved
@@ -245,6 +248,13 @@ construct_runtime! {
// Validator Manager pallet.
ValidatorManager: validator_manager::{Pallet, Call, Storage, Event<T>},

// Bridge messages support. The same story as with the bridge grandpa pallet above ^^^ - when we're
// running as Rococo we only use `BridgeWococoMessages`/`BridgeWococoMessagesDispatch`, and vice versa.
BridgeRococoMessages: pallet_bridge_messages::{Pallet, Call, Storage, Event<T>, Config<T>} = 43,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a little unfortunate that we're being split by ValidatorManager, do you think anything (major) would break if we moved it after the message lane pallets 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It requires a transaction spec version update. I personally don't care about this split

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would nice to have all the bridge stuff side by side, but I won't push too hard on this

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree Hernando here. @shawntabrizi do you mind if we change the pallet index of ValidatorManager in the next restart/runtime upgrade of Rococo? I'd move it above the bridge stuff.

Comment on lines +251 to +252
// Bridge messages support. The same story as with the bridge grandpa pallet above ^^^ - when we're
// running as Rococo we only use `BridgeWococoMessages`/`BridgeWococoMessagesDispatch`, and vice versa.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Bridge messages support. The same story as with the bridge grandpa pallet above ^^^ - when we're
// running as Rococo we only use `BridgeWococoMessages`/`BridgeWococoMessagesDispatch`, and vice versa.
// Bridge messages support.
// The same story as with the bridge grandpa pallets: when we're
// running as Rococo we only use `BridgeWococoMessages`/`BridgeWococoMessagesDispatch`, and vice versa.

svyatonik and others added 5 commits June 30, 2021 12:43
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
@HCastano HCastano requested a review from tomusdrw June 30, 2021 16:31
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Awesome! Looking forward to see that deployed 👨‍🚀

runtime/rococo/src/bridge_messages.rs Show resolved Hide resolved
@@ -245,6 +248,13 @@ construct_runtime! {
// Validator Manager pallet.
ValidatorManager: validator_manager::{Pallet, Call, Storage, Event<T>},

// Bridge messages support. The same story as with the bridge grandpa pallet above ^^^ - when we're
// running as Rococo we only use `BridgeWococoMessages`/`BridgeWococoMessagesDispatch`, and vice versa.
BridgeRococoMessages: pallet_bridge_messages::{Pallet, Call, Storage, Event<T>, Config<T>} = 43,
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree Hernando here. @shawntabrizi do you mind if we change the pallet index of ValidatorManager in the next restart/runtime upgrade of Rococo? I'd move it above the bridge stuff.

runtime/rococo/src/lib.rs Outdated Show resolved Hide resolved
runtime/rococo/src/lib.rs Outdated Show resolved Hide resolved
runtime/rococo/src/lib.rs Show resolved Hide resolved
runtime/rococo/src/bridge_messages.rs Show resolved Hide resolved
@tomusdrw
Copy link
Contributor

tomusdrw commented Jul 6, 2021

bot merge

@ghost
Copy link

ghost commented Jul 6, 2021

Trying merge.

@ghost ghost merged commit ad6e755 into master Jul 6, 2021
@ghost ghost deleted the bridges-integration-3 branch July 6, 2021 10:03
ordian added a commit that referenced this pull request Jul 6, 2021
* master: (33 commits)
  Update all weights, add run_all_benches.sh script (#3400)
  Enable over-bridge-messaging in Rococo/Wococo runtime (#3377)
  paras.rs to FRAME V2 (#3403)
  Add XCM Tracing (#3353)
  Use MaxEncodedLen trait from new parity-scale-codec v2.2 (#3412)
  bump a bunch of deps in parity-common (#3402)
  Warn on low connectivity. (#3408)
  origin to frame v2 (#3405)
  Enable logging in the puppet worker (#3411)
  make it easier to dbg stalls (#3351)
  XCM `canonicalize` + `prepend_with` fix (#3269)
  cleanup stream polls (#3397)
  Staking Miner (#3141)
  Companion for Substrate#8953 (#3140)
  Bump version, specs & substrate in prep for v0.9.8 (#3387)
  Fix busy loops. (#3392)
  Minor refactor (#3386)
  add simnet tests (#3381)
  BEEFY: adjust gossip (#3372)
  Companion for #9193 (#3376)
  ...
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants