Reject wrapped store/renew, keep management call validation#332
Merged
Conversation
Storage-mutating calls (store, store_with_cid_config, renew) must now be submitted as direct extrinsics only — wrapping them in batch, sudo, proxy, etc. is rejected at validation time with InvalidTransaction::Call. Authorization management calls (authorize_*, refresh_*, remove_expired_*) can still be wrapped and are validated via validate_signed as before. This simplifies the extension logic by removing: - preserves_origin tracking from TraverseResult and inspect_wrapper - Mixed-batch rejection logic (store + management in same batch) - Origin transformation for wrappers (no longer needed) - authorized_scope accumulation in the wrapper path The visitor now skips validate_signed for storage-mutating calls found in wrappers (they'll be rejected by the has_storage_mutating check), ensuring the correct InvalidTransaction::Call error is returned regardless of whether the caller has authorization.
Collaborator
|
I wanted to argue that its actually nice to keep possibility to batch multiple stores - but then realized that |
Collaborator
Author
I think the |
Collaborator
Author
@karolk91 both store calls won't be mapped to the batch extrinsic_index? How does it work? |
Collaborator
|
@bkontur, yes you're right, its not going to work with the same |
karolk91
added a commit
that referenced
this pull request
Mar 19, 2026
* Improvements: simply authorizations, missing xcm configs, test coverage * Adapt benchmarking * Remove call level auhtorization consumption * improvements and tests * Pass authorization via custom origin for signed transactions * add test for prepare * Apply suggestions from code review Co-authored-by: Branislav Kontur <bkontur@gmail.com> * Update pallets/transaction-storage/src/lib.rs Co-authored-by: Branislav Kontur <bkontur@gmail.com> * replacing ipfs-http-client usage (#272) * replacing ipfs-http-client usage * remove unused const * added ValidTransaction tag for check_signed (#269) * added ValidTransaction tag for check_signed * Apply suggestion from @bkontur * Fix call to renamed parameter in check_store_renew_unsigned The `hash` parameter was renamed to `content_hash` but the call site was not updated, causing a compilation error. * Unify preimage ValidTransaction tag between signed and unsigned paths Extract preimage_store_renew_valid_transaction helper so both check_store_renew_unsigned and check_signed use the same tag prefix and provides, ensuring the tx pool deduplicates across signed and unsigned submissions of the same pre-authorized content. * Test that signed preimage tags match unsigned preimage tags Extend validate_signed_account_authorization_has_provides_tag to verify that the preimage-authorized signed path produces the same ValidTransaction provides tag as the unsigned path, ensuring the transaction pool deduplicates across both submission types. * Test cross-signer dedup for preimage-authorized content Verify that different signers submitting the same preimage-authorized content produce identical provides tags, confirming dedup is content-based and not signer-based. --------- Co-authored-by: Branislav Kontur <bkontur@gmail.com> * Minor cleanups: pass scope by ref, remove unused deps, add Dependabot npm grouping (#276) * Pass scope by reference in check_authorization and check_authorization_expired * Removed unused * ci: add Dependabot npm monitoring for console-ui and examples Group version and security updates per directory so each produces a single PR instead of one per dependency. * fmt :D * Revert and fix CI * feat: add /format skill for pre-commit formatting checks (#275) * feat: add /format skill for pre-commit formatting checks Add a new Claude skill that runs all formatting and cleaning tasks: - Rust formatting (cargo +nightly fmt) - TOML formatting (taplo) - Zepter feature propagation checks - Clippy linting Update CLAUDE.md to instruct Claude to automatically run /format after generating code and before creating commits. * Update SKILL.md description to include linting * Update .claude/skills/format/SKILL.md Co-authored-by: Rohit Sarpotdar <rohit.sarpotdar@parity.io> --------- Co-authored-by: Andrii <ndk@parity.io> Co-authored-by: Rohit Sarpotdar <rohit.sarpotdar@parity.io> * Review fixes * Improvements * Nit * Add comment explaining ValidTransaction construction logic * Replace EnsureAuthorized with fn ensure_authorized Replace the EnsureAuthorized struct (EnsureOrigin impl) with a Pallet::ensure_authorized() method that validates store/renew origins and returns a rich AuthorizedCaller enum. The extrinsics now explicitly check their origin instead of ignoring it. - Add AuthorizedCaller enum with Signed/Root/Unsigned variants - Add ensure_authorized() in impl Pallet<T> using into_caller().try_into() to extract the pallet Origin::Authorized, with fallbacks to root/none - Add TryInto<Origin<Self>> bound to Config via frame_system supertrait - Update store, store_with_cid_config, renew to call ensure_authorized - Remove EnsureAuthorized struct and its EnsureOrigin impl - Update tests to use ensure_authorized and AuthorizedCaller * Rename * Rename ValidateSigned -> AllowedSignedCalls, AuthorizeStorageSigned -> ValidateStorageCalls Rename transaction extensions to better reflect their purpose: - ValidateSigned -> AllowedSignedCalls: acts as a signed-call allowlist - AuthorizeStorageSigned -> ValidateStorageCalls: validates TransactionStorage calls Add test verifying AllowedSignedCalls preserves priority set by ValidateStorageCalls for TransactionStorage calls. * Add benchmarked weights for ValidateStorageCalls extension Add validate_store() and validate_renew() to WeightInfo trait and benchmark them using test_run() (validate + prepare + post_dispatch without executing the extrinsic). Wire extension's weight() method to dispatch to the benchmarked weights instead of hardcoded DbWeight estimates. Add placeholder weights to both runtime weight files. * Fix fmt for CI nightly and update ValidateStorageCalls doc comment * Expand ValidateStorageCalls doc: authorization is consumed in prepare * Fresh weights * Apply Karol's comment * Simplify ValidateStorageCalls: pass only signer via Val, not scope The scope is only needed in validate() to transform the origin and is unused in prepare(). Simplify Val from Option<(AccountId, Scope)> to Option<AccountId> and unify the two prepare() branches into a single val.or(origin) lookup. * Parameterize validate_store weight by data length The validate_store weight was ~18s because the benchmark used MaxTransactionSize (8MB). The expensive operation is blake2_256(data) which scales linearly with data size. Parameterize the benchmark and weight function by data length so small transactions get proportionally smaller weights. * Fresh weights from 195.154.91.224 * Move CallInspector to extension file * Extract common code, remove dead code * Clean code improvements * Formatting/clippy * mempool deduplication * Transform origin only for store/renew * Code review, tests, deduplication * Add logs related to traversal * Add more tests and sync existing ones between solochain and parachain * Clippy fix * Apply suggestions from code review Co-authored-by: Branislav Kontur <bkontur@gmail.com> * Adapt to review changes * Move is_storage_mutating_call into CallInspector trait as a provided method (#327) Refactor the standalone `is_storage_mutating_call` function into a provided method on the `CallInspector` trait. This changes the trait parameter from `CallInspector<Call>` to `CallInspector<T: Config>`, enabling the cleaner `Self::is_storage_mutating_call(call, 0)` syntax in runtime `Contains` impls instead of the verbose turbofish `pallet_transaction_storage::is_storage_mutating_call::<Runtime, Self>(call, 0)`. * Move traverse_storage_calls to the CallInspector * Fix clippy * Missing variables in tests genesis set-up * Reject wrapped store/renew, keep management call validation (#332) * Reject wrapped store/renew, keep management call validation Storage-mutating calls (store, store_with_cid_config, renew) must now be submitted as direct extrinsics only — wrapping them in batch, sudo, proxy, etc. is rejected at validation time with InvalidTransaction::Call. Authorization management calls (authorize_*, refresh_*, remove_expired_*) can still be wrapped and are validated via validate_signed as before. This simplifies the extension logic by removing: - preserves_origin tracking from TraverseResult and inspect_wrapper - Mixed-batch rejection logic (store + management in same batch) - Origin transformation for wrappers (no longer needed) - authorized_scope accumulation in the wrapper path The visitor now skips validate_signed for storage-mutating calls found in wrappers (they'll be rejected by the has_storage_mutating check), ensuring the correct InvalidTransaction::Call error is returned regardless of whether the caller has authorization. * simplify * More tests * Avoid unnecessary clones in extract_signer and ValidateStorageCalls (#337) * Avoid cloning RuntimeOrigin in extract_signer Use origin.caller() to match on OriginCaller by reference instead of cloning the entire RuntimeOrigin via into_caller(). Only the AccountId is cloned, which is needed for the return value. * Avoid unnecessary clone of scope in ValidateStorageCalls Move scope into Authorized origin instead of cloning it, since maybe_scope is owned and not used after this point. * Allow sudo(store) on bulletin-westend Remove Sudo from StorageCallInspector on Westend so the sudo key holder can store data via sudo(store) without authorization. Root origin is already accepted by ensure_authorized. Update existing sudo_as tests to reflect that sudo calls now pass validation (failing at dispatch instead when no sudo key is configured). * nit * nit * Remove stale test note about Westend call allowlist * Code-review improvements * Simplify ValidateStorageCalls::prepare by removing redundant branching (#338) traverse_storage_calls already handles both direct pallet calls (via is_sub_type) and wrapper calls (via inspect_wrapper), so the separate code paths in prepare() were unnecessary. --------- Co-authored-by: RafalMirowski1 <rafal@parity.io> Co-authored-by: Branislav Kontur <bkontur@gmail.com> Co-authored-by: Rohit Sarpotdar <rohit.sarpotdar@parity.io> Co-authored-by: Naren Mudigal <mudigal@users.noreply.github.com> Co-authored-by: Andrii <ndk@parity.io>
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
2 participants
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
InvalidTransaction::Callpreserves_origintracking, mixed-batch rejection logic, origin transformation for wrappers, andauthorized_scopeaccumulation from the wrapper pathinspect_wrapperreturn type fromOption<(Vec<&Call>, bool)>toOption<Vec<&Call>>across theCallInspectortrait and all runtime implementationsWhat is allowed / rejected
Signed transactions — storage calls
store/renewwith authorizationAuthorizedstore/renewwithout authorizationPayment)batch([store])with authorizationCall)batch([store, store])with authorizationCall)batch([store_preimage, store_account])Call)batch([store, authorize_account])Call)batch([store, system::remark])Call)sudo_as(store)with or without authCall)proxy(store)with or without authCall)batch_all([authorize_account])by Authorizerbatch([authorize_account])by non-AuthorizerMAX_WRAPPER_DEPTH(8)ExhaustsResources)XCM Transact (unchanged)
store/store_with_cid_config/renewSafeCallFilterbatch([store])authorize_account/authorize_preimage