-
Notifications
You must be signed in to change notification settings - Fork 739
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 - Add improved congestion control mechanism #6231
base: master
Are you sure you want to change the base?
Conversation
bot fmt |
659be89
to
b48b8a5
Compare
a663bc2
to
cbc6ae7
Compare
bot fmt |
c78f9bc
to
501a5c0
Compare
bot fmt |
c78e707
to
152389a
Compare
bot fmt |
edd9c5c
to
38f1bb3
Compare
/cmd bench --runtime asset-hub-westend asset-hub-rococo --pallet pallet_xcm_bridge_hub_router |
bot bench cumulus-assets --runtime=asset-hub-westend --pallet=pallet_xcm_bridge_hub_router |
f06433a
to
d329dec
Compare
bot bench -v PIPELINE_SCRIPTS_REF=bko-fix cumulus-assets --runtime=asset-hub-westend --pallet=pallet_xcm_bridge_hub_router bot bench -v PIPELINE_SCRIPTS_REF=bko-fix cumulus-bridge-hubs --runtime=bridge-hub-rococo --pallet=pallet_bridge_messages |
bot bench -v PIPELINE_SCRIPTS_REF=bko-fix cumulus-bridge-hubs --runtime=bridge-hub-rococo --pallet=pallet_bridge_messages |
21baa7f
to
c00ff6b
Compare
bot bench -v PIPELINE_SCRIPTS_REF=bko-fix cumulus-bridge-hubs --runtime=bridge-hub-rococo --pallet=pallet_bridge_messages bot bench -v PIPELINE_SCRIPTS_REF=bko-fix cumulus-bridge-hubs --subcommand=xcm --runtime=bridge-hub-rococo --pallet=pallet_xcm_benchmarks::generic bot bench -v PIPELINE_SCRIPTS_REF=bko-fix cumulus-assets --runtime=asset-hub-westend --pallet=pallet_xcm_bridge_hub_router |
Command "fmt" has finished ✅ See logs here |
…(check Xcmp, check UMP, ..)
/cmd fmt |
Command "fmt" has started 🚀 See logs here |
Command "fmt" has finished ✅ See logs here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the XCM transport (pallet-messages layer) will be deployed to Asset Hub instead of Bridge Hub, so would it be easier to not go through all of the effort of upgrading Bridge Hub with all this only to deprecate it on the next iteration?
What if instead of changing and renaming xcm-bridge-hub
to support permissionless lanes, we just create a new pallet xcm-bridge
which is what you basically have in this PR, and we revert xcm-bridge-hub
to the same code actually deployed today on Bridge Hub?
That way we keep legacy code with legacy lane on BH without any migrations or risk, and all of this new code goes straight to AH, then at some point we switch the AH exporter from legacy to new?
I know this is late and maybe we should've had this idea earlier, so I'm leaving it up to you to decide which way is easiest to do.
@@ -1127,7 +1126,6 @@ mod tests { | |||
Option<PreDispatchData<ThisChainAccountId, BridgedChainBlockNumber, TestLaneIdType>>, | |||
TransactionValidityError, | |||
> { | |||
sp_tracing::try_init_simple(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why remove this? it's useful in debugging, no?
// Here, we have the actual result fees covering bridge fees, so now we need to check/apply | ||
// the congestion and dynamic_fees features (if possible). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Here, we have the actual result fees covering bridge fees, so now we need to check/apply | |
// the congestion and dynamic_fees features (if possible). | |
// `fees` is populated with base bridge fees, now let's apply congestion/dynamic fees if required. |
/// Adapter implementation for [`SendXcm`] that allows adding a message size fee and/or dynamic fees | ||
/// based on the `BridgeId` resolved by the `T::BridgeIdResolver` resolver, if and only if `E` | ||
/// supports routing. This adapter can be used, for example, as a wrapper over | ||
/// [`xcm_builder::UnpaidLocalExporter`], enabling it to compute message and/or dynamic fees using a | ||
/// fee factor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming of these is confusing. When/how is UnpaidLocalExporter
used when there are bridge fees to be paid?
Maybe rename UnpaidLocalExporter
to just LocalExporter
or smth?.
Or maybe this one can completely integrate UnpaidLocalExporter
instead of being a wrapper over it?
bridged_dest.clone() | ||
} | ||
} else { | ||
// if `bridged_dest` does not contain `GlobalConsensus`, let's prepend one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a case when ensure_is_remote(UniversalLocation::get(), dest.clone())
returns Ok, but dest
does not contain GlobalConsensus
?
should we even support such a case?
//! | ||
//! ### On the Same Chain as the Message Exporter | ||
//! In this setup, the router directly calls an `ExportXcm` implementation. In this case, | ||
//! `ViaLocalBridgeHubExporter` can be used as a wrapper with `T::ToBridgeHubSender`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to rename T::ToBridgeHubSender
since it's not fit for the local case, where Bridge Hub is not involved.
/// `bool` - `true` means that we keep accepting messages to the bridge. | ||
Suspended(bool), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add another enum variant instead of overcharging Suspended
:
/// `bool` - `true` means that we keep accepting messages to the bridge. | |
Suspended(bool), | |
/// We keep accepting messages to the bridge to allow any inflight messages to be processed. | |
SoftSuspended, | |
/// Bridge is suspended and new messages are now being actively dropped. | |
HardSuspended, |
@@ -151,6 +144,20 @@ where | |||
message, | |||
)?; | |||
|
|||
// Here, we know that the message is relevant to this pallet instance, so let's check for | |||
// congestion defense. | |||
if bridge.state == BridgeState::Suspended(false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be much more readable
if bridge.state == BridgeState::Suspended(false) { | |
if bridge.state == BridgeState::HardSuspended { |
BridgeState::Suspended(true) => { | ||
if enqueued_messages > T::CongestionLimits::get().outbound_lane_stop_threshold { | ||
// If its suspended and reached `outbound_lane_stop_threshold`, we stop | ||
// accepting new messages (a.k.a. start dropping). | ||
Bridges::<T, I>::mutate_extant(bridge_id, |bridge| { | ||
bridge.state = BridgeState::Suspended(false); | ||
}); | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BridgeState::Suspended(true) => { | |
if enqueued_messages > T::CongestionLimits::get().outbound_lane_stop_threshold { | |
// If its suspended and reached `outbound_lane_stop_threshold`, we stop | |
// accepting new messages (a.k.a. start dropping). | |
Bridges::<T, I>::mutate_extant(bridge_id, |bridge| { | |
bridge.state = BridgeState::Suspended(false); | |
}); | |
return | |
BridgeState::SoftSuspended => { | |
if enqueued_messages > T::CongestionLimits::get().outbound_lane_stop_threshold { | |
// If its suspended and reached `outbound_lane_stop_threshold`, we stop | |
// accepting new messages (a.k.a. start dropping). | |
Bridges::<T, I>::mutate_extant(bridge_id, |bridge| { | |
bridge.state = BridgeState::HardSuspended; | |
}); | |
return |
// We cannot accept new messages to the suspended bridge, hoping that it'll be | ||
// actually resumed soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// We cannot accept new messages to the suspended bridge, hoping that it'll be | |
// actually resumed soon | |
// We cannot accept new messages and start dropping messages, until the outbound lane goes below the drop limit. |
@@ -0,0 +1,45 @@ | |||
title: Bridges - revert-back and improve congestion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
title: Bridges - revert-back and improve congestion | |
title: Bridges - Add improved congestion control mechanism |
Closes: #5551 ## Description With [permissionless lanes PR#4949](#4949), the congestion mechanism based on sending `Transact(report_bridge_status(is_congested))` from `pallet-xcm-bridge-hub` to `pallet-xcm-bridge-hub-router` was replaced with a congestion mechanism that relied on monitoring XCMP queues. However, this approach could cause issues, such as suspending the entire XCMP queue instead of isolating the affected bridge. This PR reverts back to using `report_bridge_status` as before. ## TODO - [x] benchmarks - [x] prdoc ## Follow-up #6231 --------- Co-authored-by: GitHub Action <[email protected]> Co-authored-by: command-bot <> Co-authored-by: Adrian Catangiu <[email protected]>
Closes: #5551 ## Description With [permissionless lanes PR#4949](#4949), the congestion mechanism based on sending `Transact(report_bridge_status(is_congested))` from `pallet-xcm-bridge-hub` to `pallet-xcm-bridge-hub-router` was replaced with a congestion mechanism that relied on monitoring XCMP queues. However, this approach could cause issues, such as suspending the entire XCMP queue instead of isolating the affected bridge. This PR reverts back to using `report_bridge_status` as before. ## TODO - [x] benchmarks - [x] prdoc ## Follow-up #6231 --------- Co-authored-by: GitHub Action <[email protected]> Co-authored-by: command-bot <> Co-authored-by: Adrian Catangiu <[email protected]> (cherry picked from commit 8f4b99c) # Conflicts: # Cargo.lock # cumulus/parachains/runtimes/assets/asset-hub-rococo/tests/tests.rs # cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs
Closes: #5551 ## Description With [permissionless lanes PR#4949](#4949), the congestion mechanism based on sending `Transact(report_bridge_status(is_congested))` from `pallet-xcm-bridge-hub` to `pallet-xcm-bridge-hub-router` was replaced with a congestion mechanism that relied on monitoring XCMP queues. However, this approach could cause issues, such as suspending the entire XCMP queue instead of isolating the affected bridge. This PR reverts back to using `report_bridge_status` as before. ## TODO - [x] benchmarks - [x] prdoc ## Follow-up #6231 --------- Co-authored-by: GitHub Action <[email protected]> Co-authored-by: command-bot <> Co-authored-by: Adrian Catangiu <[email protected]>
for (bridge_id, previous_value, bridge_state) in bridges_to_update.into_iter() { | ||
let new_value = bridge_state.delivery_fee_factor; | ||
log::info!( | ||
target: LOG_TARGET, | ||
"Bridge channel with id {:?} is uncongested. Decreasing fee factor from {} to {}!", | ||
bridge_id, | ||
previous_value, | ||
new_value, | ||
); | ||
Bridges::<T, I>::insert(&bridge_id, bridge_state); | ||
Self::deposit_event(Event::DeliveryFeeFactorDecreased { | ||
previous_value, | ||
new_value, | ||
bridge_id, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious why not guard the weight usage with meter
in second loop, what if there is a lot of bridges to update/remove here while the weight is used up in first loop?
// For congestion - allow only calls from BH. | ||
type BridgeHubOrigin = | ||
AsEnsureOriginWithArg<EnsureXcm<Equals<xcm_config::bridging::SiblingBridgeHub>>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems param bridge_id
is not used when check the origin here, would be simpler to just use EnsureXcm<Equals<xcm_config::bridging::SiblingBridgeHub>>>
?
/cmd fmt |
Command "fmt" has started 🚀 See logs here |
Command "fmt" has finished ✅ See logs here |
All GitHub workflows were cancelled due to failure one of the required jobs. |
Closes: #5551
Closes: #5550
Context
Before permissionless lanes, bridges only supported hard-coded, static lanes. The congestion mechanism was based on sending
Transact(report_bridge_status(is_congested))
frompallet-xcm-bridge-hub
topallet-xcm-bridge-hub-router
. Depending onis_congested
, we adjusted the fee factor to increase or decrease fees. This congestion mechanism relied on monitoring XCMP queues, which could cause issues like suspending the entire XCMP queue rather than just the affected bridge.Additionally, we are progressing with deploying bridge message pallets/routing directly on AssetHub, where we don’t interact with XCMP to perform
ExportXcm
locally.Description
This PR re-introduces and improves congestion for bridges:
Enhanced Bridge Congestion Mechanism: The bridge queue mechanism has been restructured to operate independently of XCMP, with a refined protocol for congestion detection and suspension management.
Bridge-Specific Channel Suspension:
pallet-xcm-bridge-hub
andpallet-xcm-bridge-hub-router
now useBridgeId
to identify specific bridges, enabling selective suspension and resumption of individual bridge channels.Dynamic Congestion Detection:
pallet-xcm-bridge-hub
now includes callbacks forfn suspend_bridge
andfn resume_bridge
based on congestion status:xcm::Transact(report_bridge_status(bridge_id, is_congested))
using the stored callback information.New Stop Threshold: A
stop_threshold
limit inpallet-xcm-bridge-hub
enables or disablesExportXcm::validate
, providing a fallback mechanism when the router does not adhere to thesuspend
signal.Flexible Message Routing:
pallet-xcm-bridge-hub-router
has been refactored to support message routing for both sibling chains (ExportMessage
) and local deployment (ExportXcm
).These updates improve modularity, allow more granular bridge congestion handling, and support diverse deployment scenarios.