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.

Sign up for GitHub

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

Jump to bottom

XCM: fix for Transact v5->v4 decoding #6609

Closed
wants to merge 2 commits into from
Closed

XCM: fix for Transact v5->v4 decoding #6609

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c96a7ba
Fix for Transact v5->v4 decoding
bkontur Nov 22, 2024
7cc63a5
Update from bkontur running command 'fmt'
actions-user Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion polkadot/xcm/src/v4/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1315,7 +1315,9 @@ impl<Call: Decode + GetDispatchInfo> TryFrom<NewInstruction<Call>> for Instructi
HrmpChannelClosing { initiator, sender, recipient } =>
Self::HrmpChannelClosing { initiator, sender, recipient },
Transact { origin_kind, mut call } => {
let require_weight_at_most = call.take_decoded()?.get_dispatch_info().call_weight;
let require_weight_at_most = call.take_decoded()
.map(|c| c.get_dispatch_info().call_weight)
.unwrap_or(Weight::MAX);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@franciscoaguirre I am not sure if MAX is ok, we could also go with MAX/2, MAX/4 ..., previously we had here:

// We use a big weight here but not `Weight::MAX` as a best effort.
const DEFAULT_WEIGHT_FOR_TRANSACT_CONVERSION: Weight = Weight::from_parts(10_000_000_000, 1_000_000);

at least MAX is working for #6585
I tested this with chopstics for https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fwestend-rpc.polkadot.io#/explorer/query/0x01cd07a4ede58f34d76422febdf1adc28669c1a2ed5fa5ccaa7d15a7d1bfb0a5

Probably Weight::MAX shouldn't be used, I am no sure, because of chains which have just XCMv4 processing:
https://github.com/paritytech/polkadot-sdk/blob/stable2409/polkadot/xcm/xcm-executor/src/lib.rs#L862-L871
https://github.com/paritytech/polkadot-sdk/blob/stable2409/polkadot/xcm/xcm-builder/src/weight.rs#L65

Copy link
Contributor

Choose a reason for hiding this comment

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

if call.take_decoded()?.get_dispatch_info() only fails for calls of type (), then using MAX should be ok, in practice there should be no Transact instructions for any Xcm<()>. Any real Transact will have a call type different than (), no?

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is when we send it we'll end up telling the receiver to use MAX. I think MAX divided by some factor should be good. Or looking at the on-chain data and picking a value that is big enough for most transacts

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that the weight here in require_weight_at_most is what is actually initially charged for execution fees on older chains, so dryrunning an xcmv5 program says use these fees, then sending that program to an xcmv4 chain will have the call fail with not enough fees

(someone needs to double check this and confirm in code)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we should go for a more backwards compatible solution

Self::Transact { origin_kind, require_weight_at_most, call: call.into() }
},
ReportError(response_info) => Self::ReportError(QueryResponseInfo {
Expand Down
Loading