-
Notifications
You must be signed in to change notification settings - Fork 43
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
RFC: Make Transact
weight limit optional
#55
Conversation
Transact
weight limit optionalTransact
weight limit optional
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/xcm-user-and-developer-experience-improvements/4511/21 |
|
||
## Security considerations | ||
|
||
TODO |
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.
Currently, an XCVM implementation can weigh a message just by looking at the decoded instructions without decoding the Transact's call
. Is it safe to decode the call
during the message weighing (i.e., during a "free" execution) to get the dispatch info?
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.
Conceptually not that much changes. Time for decoding is not included in the weight right now or maybe part of the base weight or similar.
However, right now you could already say that execution takes 5Weight
and then it continues. When you come to the point of applying the call you decode and realize that it actually requires 6Weight
and you bail. So, you also decode it in the current model for "free". Discussable if it was for free, because you already paid fees, but they are paid based on the weight.
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.
Considering the decoding is limited by MAX_XCM_DECODE_DEPTH
, even if the decoding was "free" (Transact
benchmarking is using the smallest possible call), it does have a limit on how much computation it can take. If you want to be extremely cautious about it, you could benchmark the Transact
instruction using a call close to MAX_XCM_DECODE_DEPTH
(max weight usage), and this will reduce the "free" side of it. However, as the difference in weight are not significant currently, you can consider it is safe to decode the call using weights generated with the current beckmarking.
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've updated the "security considerations" section.
The MAX_XCM_DECODE_DEPTH
is IMO an implementation detail and should not be part of the formal spec, so I haven't included it.
Replace the `require_weight_at_most: Weight` parameter of the `Transact` instruction with a weight limit: `weight_limit: WeightLimit` that can be specified as `Unlimited`.
9c50900
to
7f97004
Compare
Closing in favor of polkadot-fellows/RFCs#101 |
This is a continuation of polkadot-fellows/xcm-format#55 following the migration of XCM RFCs to this repo. # Summary Remove the `require_weight_at_most: Weight` parameter of the Transact instruction. # Motivation The UX of using Transact is not great, and one part of the problem is guesstimating this require_weight_at_most. We've seen multiple Transacts on-chain failures caused by the "incorrect" use or the parameter. In practice, this parameter only adds UX overhead. Use cases fall in one of two categories: 1. Unpaid execution of Transacts - in these cases the require_weight_at_most is not really useful, caller doesn't have to pay for it, and on the call site it either fits the block or not; 2. Paid execution of single Transact - the weight to be spent by the Transact is already covered by the BuyExecution weight limit parameter. We've had multiple OpenGov root/whitelisted_caller proposals initiated by core-devs, completely or partially fail because of incorrect configuration of `require_weight_at_most` parameter. This is a strong indication that the instruction in its current form is hard to use. --------- Signed-off-by: Adrian Catangiu <adrian@parity.io>
Summary
Replace the
require_weight_at_most: Weight
parameter of theTransact
instruction with an optional weight limit:
maybe_weight_limit: Option<WeightLimit>
Motivation
The UX of using
Transact
is not great, and one part of the problem is guesstimating thisrequire_weight_at_most
.We've seen multiple
Transact
s on-chain failures caused by the "incorrect" use or the parameter. In practice,this parameter only adds UX overhead. The vast majority of use cases fall in one of two categories:
require_weight_at_most
is not really useful, caller doesn'thave to pay for it, and on the call site it either fits the block or not;
BuyExecution
weight limit parameter.
We've had multiple OpenGov
root/whitelisted_caller
proposals initiated by core-devs completely or partially failbecause of incorrect configuration of
require_weight_at_most
parameter. This is a strong indication that theinstruction is hard to use.