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

XCM message weight computation must give the same result at both sides of the bridge #1417

Closed
svyatonik opened this issue May 26, 2022 · 2 comments · Fixed by #1430
Closed

Comments

@svyatonik
Copy link
Contributor

That's the actual bug found in #1412. In RialtoParachain configuration the weight of single XCM instruction was set to the 1_000_000, while at the Millau it was set to the 1_000_000_000.

Messages relay is currently using source client (RialtoParachain in our case) to get message dispatch weight. So for simplest message it has been returning 1_000_000 for the message. During dispatch, messages pallet has been using at-Millau XCM weigher, which has been returning 1_000_000_000 instead (see FromBridgedChainMessageDispatch::dispatch_weight). So delivery transaction was failing without even delivering single message, effectively blocking lane.

This is definitely a misconfiguration, but it has raised some questions. E.g. - can we assume that XCM weighers will always be in-sync on both chains? I don't think so, given possible runtime upgrades. So imo to avoid blocking lanes, we need to transfer pre-computed dispatch weight over the wire (as it was with encoded calls).

@svyatonik
Copy link
Contributor Author

svyatonik commented May 26, 2022

This issue shall be handled very carefully, since there are many caveats. There's buy execution command, which takes and withdraws assets at the target chain. Does xcm weigher return weight of this dispatch? If so, then it'll be paid twice - once at the source chain and once at the target (during XCM execution).

UPD: an alternate (and probably better) solution would be to add something like estimate_dispatch_weight runtime API to the target chain => actual weigher will always be used by the relayer

@svyatonik
Copy link
Contributor Author

Thoughts in-progress:

  • when dispatch fee is paid at the target chain (XCM case), the only valid reason we need to know dispatch weight at the source chain is to reject messages with dispatch weight larger than allowed. However this may be handled in a bit different way (see below);
  • so we don't want to transfer dispatch weight over wire;
  • instead we need to use XCM weigher of the target chain to get actual dispatch weight of the XCM message (is there an existing functionality for that?);
  • there are several possible issues with this approach:
    • if we are unable to decode message/XCM, then we shall return dispatch weight = 0. During actual dispatch we need to refuse to dispatch this message (i.e. it'll be delivered, but not dispatched);
    • if dispatch weight of the message/XCM is larger than we expect at the source chain (now it is set to 50% of max extrinsic weight of the target chain), we shall do the same - i.e. return dispatch weight = 0 and refuse to dispatch message. Why? The reasoning is the same as for the limit-at-the-source-chain - delivery tx has its own overhead, so we need a reserve for that. In XCM case (pay-at-target-chain) we have more flexibility, but imo it is better to have a strict limit - you'll know in advance whether your message will be dispatched or not.

So I suggest a following solution:

  • add (or use existing, if there is something) a runtime API method to the target chain that will return the actual dispatch weight of the XCM;
  • change relayer code a bit - if it calls a To<TargetChain>OutboundLaneApi::message_details and the message dispatch is paid at the target chain (MessageDetails::dispatch_fee_payment == DispatchFeePayment::AtTargetChain), then it must call mentioned method and use its result instead of MessageDetails::dispatch_weight.

This should work for all possible use cases - for XCM, for encoded calls, enums, ...

svyatonik pushed a commit that referenced this issue Jul 17, 2023
* [ci] Send bench results to S3

* move publish stage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment