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

Identify channels using a Hash type instead of parachain IDs #1005

Merged
merged 13 commits into from
Nov 14, 2023
Merged

Conversation

vgeddes
Copy link
Collaborator

@vgeddes vgeddes commented Nov 13, 2023

This allows us to extend the channel system in flexible ways in future, while right now it allows us to simplify handling of message priorities.

There are now two channels dedicated for governance commands:

Primary governance channel:
ID: 0x0000000000000000000000000000000000000000000000000000000000000001
Purpose: For high-priority messages such as Upgrade and SetOperatingMode.

Secondard governance channel:
ID: 0x0000000000000000000000000000000000000000000000000000000000000002
Purpose: For lower-priority messages such as CreateAgent and CreateChannel.

Having two channels means that higher-priority governance commands won't get backed up behind lower-priority governance commands, which other parachains can issue via XCM

All other channels have an ID deterministically derived from the parachain id: keccak256(("para", para_id)) . For example, the channel ID for AssetHub is 0xc173fac324158e77fb5840738a1a541f633cbec8884c6a601c567d2b376a0539.

@yrong, I learned that the MessageQueue pallet processes messages from different queues in a fair manner, using a round-robin approach. So in Block K, messages from one queue will be processed, and in block K+1, messages from the next queue will be processed. This means we don't need to have any special handling for high-priority messages, as we can rely on the MessageQueue pallet to have them processed fairly.

Other Changes:

  • GenesisConfig builder for the Control pallet ensures that built-in agents and channels are initialized in storage.
  • CreateChannel command now also includes the initial operating mode and outboundFee.
  • Removed defaultFee storage field from Gateway, as CreateChannel command includes the outboundFee.

@vgeddes vgeddes requested review from yrong and musnit November 13, 2023 17:31
Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (8e921b2) 81.14% compared to head (11e3efa) 80.98%.
Report is 1 commits behind head on main.

Files Patch % Lines
parachain/pallets/control/src/lib.rs 92.98% 4 Missing ⚠️
parachain/primitives/core/src/lib.rs 77.77% 4 Missing ⚠️
parachain/primitives/core/src/outbound.rs 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1005      +/-   ##
==========================================
- Coverage   81.14%   80.98%   -0.17%     
==========================================
  Files          53       52       -1     
  Lines        2132     2161      +29     
  Branches       72       72              
==========================================
+ Hits         1730     1750      +20     
- Misses        387      396       +9     
  Partials       15       15              
Flag Coverage Δ
rust 81.00% <91.42%> (-0.20%) ⬇️
solidity 80.87% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -14,6 +14,22 @@ function ParaIDNe(ParaID a, ParaID b) pure returns (bool) {
return !ParaIDEq(a, b);
}

function into(ParaID paraID) pure returns (ChannelID) {
Copy link
Collaborator Author

@vgeddes vgeddes Nov 13, 2023

Choose a reason for hiding this comment

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

This converts a ParaID into a ChannelID. Equivalent to impl From<ParaId> for ChannelId on the substrate side.

@musnit
Copy link
Collaborator

musnit commented Nov 13, 2023

  1. why not just use bridgeHub's paraID for the channel for CreateAgent and CreateChannel, rather than a special SECONDARY_GOVERNANCE_CHANNEL?
  2. should any agent be able to send CreateAgent and CreateChannel through their own channel? why do they need to go through this special SECONDARY_GOVERNANCE_CHANNEL?
  3. should an agent be able to create its own channel, not just a parachain?
  4. should a parachain/agent be able to create multiple of their own channels? why just 1? what if they want some priority system or multiple different channels with different payment models?

@yrong
Copy link
Contributor

yrong commented Nov 14, 2023

This means we don't need to have any special handling for high-priority messages, as we can rely on the MessageQueue pallet to have them processed fairly.

Yes, it's scheduled in a round-robin manner. But there is one nuance if we remove the special handling(i.e. yield for high-priority message) in case governance message does not get the chance to execute in same block when congest of low priority sibling messages, I've added one test for that in 11e3efa

Please check if that's the expected behavior.

@vgeddes
Copy link
Collaborator Author

vgeddes commented Nov 14, 2023

  1. why not just use bridgeHub's paraID for the channel for CreateAgent and CreateChannel, rather than a special SECONDARY_GOVERNANCE_CHANNEL?

The CreateAgent and CreateChannel commands are triggered by other parachains, and so its a security risk for high-priority commands (such as Upgrade, SetOperatingMode) to be backed up behind lower priority commands. Assuming some attacker has enough cash to spam these commands.

  1. should any agent be able to send CreateAgent and CreateChannel through their own channel? why do they need to go through this special SECONDARY_GOVERNANCE_CHANNEL?

Its a chicken-and-egg problem. A consensus system initially needs to create an agent and channel over a bridgehub/governance channel. Only after that can children of that consensus system use the newly created agent/channel of their parent.

And in indeed, certain commands such UpdateChannel are sent over the parachain's channel, not a governance channel, since a channel must exist before it can be updated.

  1. should an agent be able to create its own channel, not just a parachain.

This PR does make that easier to implement, post launch. But its not the goal of this change.

  1. should a parachain/agent be able to create multiple of their own channels? why just 1? what if they want some priority system or multiple different channels with different payment models?

Yip, again, this PR aims to make easier to implement in future. For now though, channels and parachains have a 1-1 mapping in the background.

Our approach of deriving ChannelIds using hashing can be extended to support the schemes you suggest. Or alternatively, post-launch, we can swap out the hashing for a mapping in storage.

@vgeddes
Copy link
Collaborator Author

vgeddes commented Nov 14, 2023

This means we don't need to have any special handling for high-priority messages, as we can rely on the MessageQueue pallet to have them processed fairly.

Yes, it's scheduled in a round-robin manner. But there is one nuance if we remove the special handling(i.e. yield for high-priority message) in case governance message does not get the chance to execute in same block when congest of low priority sibling messages, I've added one test for that in 11e3efa

Please check if that's the expected behavior.

This is fine. Having governance commands delayed by a single block isn't a problem.

@vgeddes vgeddes merged commit df81476 into main Nov 14, 2023
8 checks passed
@vgeddes vgeddes deleted the channel-id branch November 14, 2023 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants