Reduce base proof size weight component to zero

[KiChjang]

It was found out that Polkadot still uses FixedWeightBounds for weighing XCM instructions, and as such is using a BaseXcmWeight that has a proof size component that's too large for average use cases such as asset transfers. This PR reduces it to 1KiB instead of the previous 64KiB.

While it's true that the relay chain doesn't require any PoV, the Weigher is being used in more places than just weighing incoming XCM instructions, so it does not seem wise to reduce it to 0.

[girazoki]

I think even lowering the proof size might break some scenarios. A V2 XCM composed of WithdrawAssset || BuyExecution(Limited) || Transact. Here, with FixedWeightBounds, the proof size of the message will be, if I am not mistaken:

• WithdrawAsset: 1KB
• BuyExecution: 1KB
• Transact: 1KB + whatever is written in required_weight_at_most. in a V2->V3 Transact translation this will be, if I am not mistaken, 64KB.

Total: 67Kb

Since the default v2->v3 conversion makes BuyExecution buy 64KB, then the message will fail.

I see xcm weights in Kusama and Westend runtimes do not account for any proof size. Is there a reason for accounting it here?
https://github.com/paritytech/polkadot/blob/master/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs
https://github.com/paritytech/polkadot/blob/master/runtime/kusama/src/weights/xcm/pallet_xcm_benchmarks_fungible.rs

[KiChjang]

The benchmarks really should be accounting for proof sizes. I'm not sure if the 0 you see there means it has already been accounted for and it's benchmarked as 0, or that the benchmarking infrastructure doesn't actually measure PoV sizes. Maybe @ggwpez can help in answering this?

[xlc]

the Weigher is being used in more places than just weighing incoming XCM instructions

what are the other places? can we only modify the value for xcm executor?

[KiChjang]

the Weigher is being used in more places than just weighing incoming XCM instructions

what are the other places? can we only modify the value for xcm executor?

Indeed we can, I've pushed a commit introducing TempFixedXcmWeight which doesn't have a PoV weight component. This should work with the XCM executor weigher only and not interfere with other operations.

[librelois]

@KiChjang and what solution do you envisage for XCM v2 message with Transact and Limited weight ?

The TempFixedXcmWeight taht you introduced not fix thje problme with Transact. The problem come from the conversion V2->V3 that apply a weight limit of 64Kb for the proof part of require_weight_at_most.

• One solution can be to apply zero on the conversion -> edit: it doesn't work, it will fail with error XcmError::MaxWeightInvalid
• Another solution is to buy more proof size by default in BuyExecution conversion

[KiChjang]

@librelois The Transact is using up all 64KiB of the PoV size because it is intended to, since we were not properly accounting for any proof size weights before, but now we do. The proper fix is on the sender side to upgrade to V3, as moving forwards, we should always be factoring in proof size weights, and is not optional.

[librelois]

The proper fix is on the sender side to upgrade to V3, as moving forwards, we should always be factoring in proof size weights, and is not optional.

In this case it means that there is actually no full downward compatibility, which is a critical issue for moonbeam, as we have some smart contracts that regularly send XCM messages with Transact on relay, like Lido for DOT liquid staking for example.

Is it possible to modify the V2->V3 conversion of BuyExecution to buy several times the default proof size (at least 2 or 3 times) ?

[ggwpez]

I see xcm weights in Kusama and Westend runtimes do not account for any proof size. Is there a reason for accounting it here?
https://github.com/paritytech/polkadot/blob/master/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs
https://github.com/paritytech/polkadot/blob/master/runtime/kusama/src/weights/xcm/pallet_xcm_benchmarks_fungible.rs

Looks like something with the re-generation did not work. I just did it locally and they do report PoV.
Will try to get the bot to update it.

PS: we recently added a CI job to spot this earlier in the release process #6996

[KiChjang]

@librelois What you're asking for is forward compatibility, not backward or downward, since you're asking specifically for a V2 Transact to have a lower PoV weight when converting to a V3 Transact.

In short, there is no forward compatibility for V2 weights in V3, as there's no way to specify the PoV weight component in V2. We must then treat the incoming XCM Transact in the worst case possible as we are not certain how much PoV it is going to consume. Unless you have some way of letting the destination know ahead of time just how much PoV weight that your V2 XCM is going to consume, I don't see how we can get around this limitation.

[librelois]

What you're asking for is forward compatibility, not backward or downward,

@KiChjang We can nitpick on the terms, but the fact is that some currently functional V2 messages will not work anymore, and for us this is a blocking problem.

What about converting BuyExecution v2 with Limited weights to Unlimited weights in v3?

I think the design error comes from not allowing to limit weights by dimension, we are obliged to limit all dimensions or nothing.

[KiChjang]

@librelois The real issue is that you should upgrade your chain to v3, and we should be talking about that. Could you please upgrade to v3 so that your applications will work as intended?

[gavofyork]

In this case it means that there is actually no full downward compatibility

There is no full compatibility between v3 and v2, and this is simply not possible since v2 does not contain information critical for weight specification.

@KiChjang what about introducing a feature in the XCM crate zero_legacy_pov_weight which makes the v2 -> v3 conversion (both for any instructions and Transact/QueryResponse) have a zero (or very low) PoV component.

Parachains which don't mind being unsafe (or don't care about the PoV components like Relays) can enable this feature.

[girazoki]

Just for the sake of clarifying, we have released a new runtime with support for XCM v3. Considering all the changes and the security issues that were introduced with this new version we are expecting to deploy to Moonbeam (on Polkadot) around May 18th. This is our standard delay (5 weeks) for deployment on Polkadot, for security reasons.

Since we expect XCM V3 to be deployed in Polkadot sooner than that, there will be a period of time in which Moonbeam will be in XCM V2, while Polkadot will be in XCM V3. We have a usecase in Moonbeam in which we use the set of instructions:

WithdrawAsset || BuyExecution || Transact

If Polkadot upgrades to XCM V3 with a Weigher that accounts for proof sizes while Moonbeam is still using XCM V2, this usecase will break.

That being said, the PR is currently using TempFixedXcmWeight with a 0 proof component. This would work for us, but I am not sure if this is the expected change that will be deployed in the next Polkadot runtime.

Is the intention to go in Polkadot with TempFixedXcmWeight for the most immediate runtime upgrade, and introduce a proof-size-accounting Weigher in the following one? Or am I missinterpreting this?

[gavofyork]

Yes, the intention is to keep the inserted PoV weight component minimal to keep existing use-cases working during the transition period to XCMv3 with 2D weights and the new MQ system.

Until the new MQ is merged (and the 2D weights are accurately calculated), we really don't need to enforce any PoV-weight component limits.

Once the new MQ is merged and chains have accurate weights associated with their instructions, then we can begin placing and enforcing sensible PoV-weight limits. As we do this, we can lift the very low message-execution limits and restrictions we have as a workaround at present.

[KiChjang]

Since we already have restrictive whitelists and message limits to guard against PoV weight exhaustion, I've set the DEFAULT_PROOF_SIZE to be 0, so that V2 Transact instructions would have a PoV of 0 KB during the conversion to V3. Per what Gav has mentioned, we will revert it back to 64KB once we start enforcing PoV weight limits.

[gavofyork]

Since we already have restrictive whitelists and message limits to guard against PoV weight exhaustion, I've set the DEFAULT_PROOF_SIZE to be 0, so that V2 Transact instructions would have a PoV of 0 KB during the conversion to V3. Per what Gav has mentioned, we will revert it back to 64KB once we start enforcing PoV weight limits.

Actually, once we have a full stack deployed for dealing with PoV weights (proper XCM weights, PoV-aware MQ, XCMv3) then we should simply disallow any XCMv2 1D-weight values into XCMv3 2D-weight values. This will make certain parts of XCMv2 untranslatable into XCMv3 but it's the only way to safely remove the limitations we have in place, so it's important for chains to move to XCMv3 promptly.

[KiChjang]

bot merge

[girazoki]

Since we already have restrictive whitelists and message limits to guard against PoV weight exhaustion, I've set the DEFAULT_PROOF_SIZE to be 0, so that V2 Transact instructions would have a PoV of 0 KB during the conversion to V3. Per what Gav has mentioned, we will revert it back to 64KB once we start enforcing PoV weight limits.

If we set the Transact v2->v3 conversion to have 0 default proof-size, Transact will not be able to do anything meaningful since most of the extrinsics in the relay have non-zero proof size component. An example: hrmp_init_open_channel:

polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs

Line 66 in dd4e054

fn hrmp_init_open_channel() -> Weight {

I believe a V2 Transact for opening HRMP channels would fail, since the v2-> v3 Transact conversion sets 0 require_weight_at_most proof size. I believe this line would fail:

polkadot/xcm/xcm-executor/src/lib.rs

Line 554 in dd4e054

ensure!(weight.all_lte(require_weight_at_most), XcmError::MaxWeightInvalid);

I think we should have set the default conversion to 64Kb proof size, which would cover most of the cases involving a message of the form:

WithdrawAsset || BuyExecution || Transact

BuyExecution buys 64Kb by default, and require_weight_at_most in Transact is set to 64Kb. As long as the Weigher does not charge any proof size for the instruction, I think this setup would work.

[girazoki]

Or we can also make the xcm-executor Transact ignore the proof size until these are properly accounted

[albertov19]

Note that as @girazoki mentioned, this won't allow for any parachain that is on XCM V2 to Accept or Propose HRMP channels which seems like a major problem