fix(multisig): drop per-level encoded_size from execute weight annotation - #698
Open
Bortlesboat wants to merge 1 commit into
Open
Conversation
`Multisig::execute`'s weight annotation took `MaxCallSize.max(call.encoded_size())`. `encoded_size` walks the whole remaining call subtree, and `get_dispatch_info()` recurses into a nested `execute`, re-evaluating the annotation over that subtree in turn, so a chain of `k` nested wrappers around a `B`-byte payload cost O(k * B) of encode-walking. `frame_executive::validate_transaction` computes dispatch info right after the signature check, before `CheckWeight` and `ChargeTransactionPayment` run, so the walk happened even for transactions ultimately rejected for exhausted resources or insufficient balance -- charging no fee. Each wrapper is 38 encoded bytes and one codec depth level, so ~255 fit under `MAX_EXTRINSIC_DEPTH` in under 10 KB, leaving the rest of the normal block-length budget for the innermost payload. Reserve bookkeeping at a flat `WeightInfo::execute(MaxCallSize)` and clamp the body's `call_size` to the same constant. The clamp keeps the post-dispatch weight inside the declaration: `proposal.call` is a `BoundedVec<_, MaxCallSize>`, so a submitted call encoding to more than `MaxCallSize` can never be byte-equal to it and can only end in `CallMismatch`, which `execute(MaxCallSize)` covers. The `get_dispatch_info()` recursion is inherent and is unchanged; only the per-level byte count is removed. `Utility::batch_all` is already O(children) per level rather than O(bytes), so this brings `execute` in line with it. Adds a regression test for the over-sized-submission ordering, asserting against `execute(MaxCallSize)` -- the existing declared-weight test covers small-submission/large-stored, and a large inner call's own declared weight is enough to absorb the bookkeeping overshoot and hide the regression if only the full declaration is compared. Fixes Quantus-Network#673
Collaborator
|
Ok this is a weights issue - sometimes AI oscillates on these we've had a lot of them - they're not necessarily all accurate |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stop sizing
Multisig::execute's weight annotation bycall.encoded_size(), so nestingexecuteno longer costs O(depth × bytes) of encode-walking during transaction validation.This change:
WeightInfo::execute(T::MaxCallSize::get())instead ofMaxCallSize.max(call.encoded_size())call_sizetoMaxCallSizeso no error path can report more weight than was declaredRoot cause
The annotation at
pallets/multisig/src/lib.rswas:Two things compound:
Encode::encoded_sizeis O(bytes of the whole remaining subtree) — it runs a fullencode_tointo a size tracker.get_dispatch_info()recurses into the inner call, and when that inner call is anotherMultisig::execute, this same expression is evaluated again over its subtree.So
knestedexecutewrappers around aB-byte payload perform roughlyk × Bbytes of walking.frame_executive::validate_transactioncomputes the dispatch info immediately after the signature check:CheckWeightandChargeTransactionPaymentare transaction extensions and run after that point, so the walk happens even when the transaction is ultimately rejected for exhausting resources or for insufficient balance — in which case no fee is charged at all.Each
executewrapper costs 38 encoded bytes and exactly one codec depth level (WrapperTypeDecode for Boxcallsdescend_ref; derived enums do not), so ~255 wrappers fit underMAX_EXTRINSIC_DEPTH = 256in under 10 KB, leaving essentially the whole normalRuntimeBlockLengthbudget for the innermost payload.This is not equivalent to
Utility::batch_all, whose annotation callsweight_and_dispatch_class(&calls)— O(number of children) per level, never O(bytes). Nestingbatch_allis O(depth) in total.The
.max(...)was not gratuitous: it existed so the post-dispatchbookkeeping_weight, sized bymax(proposal.call.len(), call.encoded_size()), could never exceed the declaration. That constraint has to be preserved by any fix.Proposed solution
Reserve bookkeeping at a flat
WeightInfo::execute(MaxCallSize)and clampcall_sizein the body to the same constant.The clamp is what keeps the post-dispatch weight inside the declaration.
proposal.callis aBoundedVec<u8, MaxCallSize>, so a submitted call encoding to more thanMaxCallSizebytes can never be byte-equal to the stored payload; it can only ever fall through toCallMismatch, and charging that pathexecute(MaxCallSize)is correct. Every other path either reports a fixed small read count or the clampedbookkeeping_weight.The
get_dispatch_info()recursion is inherent to the call-carryingexecuteinterface and is left alone — it is the per-level byte count that made the annotation quadratic. This also bringsexecutein line withbatch_all's O(children)-per-level shape.The existing doc comment on
executealready described the intended behaviour ("Only the bookkeeping term is reserved atMaxCallSize, since the stored bytes' length is unknown pre-dispatch"); the code had drifted from it.Impact
executechain no longer scales with the payload size at every level; the annotation is now two constants plus the inherent inner-call recursion.executecalls:MaxCallSizewas already the floor of the old expression, so the declaration is unchanged for any call at or belowMaxCallSize.MaxCallSizesee a lower declaration — and those cannot do anything but failCallMismatch.Verification
SKIP_WASM_BUILD=1 cargo test --locked -p pallet-multisig --lib: 63 passedSKIP_WASM_BUILD=1 cargo test --locked -p quantus-runtime --lib: 77 passedSKIP_WASM_BUILD=1 cargo clippy --locked -p pallet-multisig --all-targets -- -D warnings: cleanscripts/fmt.sh --all -- --check: cleanThe new test
execute_oversized_submitted_call_stays_within_bookkeeping_reservationwas confirmed to fail without thecall_sizeclamp:It asserts against
execute(MaxCallSize)rather than only against the full declaration, because the declaration also carries the inner call's own weight — for a largeremarkthat term alone is enough to absorb the bookkeeping overshoot and hide the regression. The pre-existingexecute_mismatch_never_reports_more_weight_than_declaredcovers the opposite ordering (small submission, near-max stored call) and still passes.Fixes #673