Added fallback_max_weight to Transact for sending messages to V4 chains #6643
Conversation
|
/cmd fmt |
|
Command "fmt" has started 🚀 See logs here |
|
Command "fmt" has finished ✅ See logs here |
|
/cmd prdoc --audience runtime_dev --bump major |
| @@ -135,6 +135,7 @@ impl CoretimeInterface for CoretimeAllocator { | |||
| Instruction::Transact { | |||
| origin_kind: OriginKind::Native, | |||
| call: request_core_count_call.encode().into(), | |||
| fallback_max_weight: Some(Weight::from_parts(1_000_000_000, 200_000)), | |||
There was a problem hiding this comment.
@seadanda We had to keep the weight in the form of a fallback when dealing with chains that haven't yet upgraded to V5. This means if you're only sending this messages when you know the other chain has already upgraded then you can put None and have the benefits of never having too little weight.
Please check if any of these calls could have None or should have a fallback.
There was a problem hiding this comment.
This seems reasonable.
The calls should have a fallback in this case, as the order of upgrade is not guaranteed and it's a bidirectional interface. On Westend it would be possible to do relay -> coretime to enable us to have no fallback on the coretime chain and only keep fallbacks for the relay.
But I think it's better to do this the way we'll do it on Kusama/Polkadot so let's do fallbacks to previous values.
| @@ -333,6 +333,7 @@ impl<T: Config> OnNewSession<BlockNumberFor<T>> for Pallet<T> { | |||
| fn mk_coretime_call<T: Config>(call: crate::coretime::CoretimeCalls) -> Instruction<()> { | |||
| Instruction::Transact { | |||
| origin_kind: OriginKind::Superuser, | |||
| fallback_max_weight: None, | |||
There was a problem hiding this comment.
@seadanda Another place to check
bkontur
added
the
A4-needs-backport
| Transact { | ||
| origin_kind: OriginKind::Superuser, | ||
| call: poke.encode().into(), | ||
| fallback_max_weight: None, |
There was a problem hiding this comment.
isn't this ToParachainIdentityReaper obsolete now? used for identity migration which has since completed? maybe just remove it in this or separate PR.
if still used, then this should be Some(fallback_weight) - Relay will upgrade first to v5 and People chain will still be on v4 for a bit before upgrading.
| Transact { origin_kind, call, fallback_max_weight } => { | ||
| let require_weight_at_most = fallback_max_weight.unwrap_or(Weight::MAX); |
There was a problem hiding this comment.
This looks incorrect, shouldn't we use fallback_max_weight only when call.take_decoded()?.get_dispatch_info() is unavailable? Basically only when call is ()?
IIUC with the current code, any XCMv5 Transact with fallback_max_weight: None will be converted to a XCMv4 Transact with require_weight_at_most = Weight::MAX even for those messages where call weight is available.
There was a problem hiding this comment.
Messages only get converted on the sender, so call.take_decoded()?.get_dispatch_info() is never going to be called in the receiver and work.
We do convert "upwards" in the receiver, but we don't convert "downwards" so this would never be called.
There was a problem hiding this comment.
What about on the local chain converting a V5 transact (of a local call) to a V4 transact of same local call.
I don’t have an example in mind where this would happen, but we could easily support it, so I’m proposing we use the info if we have it. It’s defensive programming and doesn’t cost us anything.
There was a problem hiding this comment.
Sure, will add it back
|
/cmd fmt |
|
Command "fmt" has started 🚀 See logs here |
|
Command "fmt" has finished ✅ See logs here |
Co-authored-by: Adrian Catangiu <[email protected]>
Co-authored-by: Adrian Catangiu <[email protected]>
Co-authored-by: Adrian Catangiu <[email protected]>
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
Closes: #6585
Removing the
require_weight_at_mostparameter in V5 Transact had only one problem. Converting a message from V5 to V4 to send to chains that didn't upgrade yet. The conversion would not know what weight to give to the Transact, since V4 and below require it.To fix this, I added back the weight in the form of an
Option<Weight>calledfallback_max_weight. This can be set toNoneif you don't intend to deal with a chain that hasn't upgraded yet. If you set it toSome(_), the behaviour is the same. The plan is to totally remove this in V6 since there will be a good conversion path from V6 to V5.