From 32229a9c2f6d5c1f3defe8ccc90a8acb5ef51af2 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 14:01:54 +0800 Subject: [PATCH 01/47] check zk root on import --- frame/executive/Cargo.toml | 3 ++- frame/executive/src/lib.rs | 20 ++++++++++++++++++++ frame/executive/src/tests.rs | 22 ++++++++++++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/frame/executive/Cargo.toml b/frame/executive/Cargo.toml index 36657ab8..459252ec 100644 --- a/frame/executive/Cargo.toml +++ b/frame/executive/Cargo.toml @@ -21,6 +21,7 @@ frame-support.workspace = true frame-system.workspace = true frame-try-runtime = { optional = true, workspace = true } log.workspace = true +qp-header.workspace = true scale-info = { features = ["derive"], workspace = true } sp-core.workspace = true sp-io.workspace = true @@ -31,7 +32,6 @@ sp-tracing.workspace = true array-bytes = { workspace = true, default-features = true } pallet-balances = { workspace = true, default-features = true } pallet-transaction-payment = { workspace = true, default-features = true } -qp-header = { workspace = true, default-features = true } sp-inherents = { workspace = true, default-features = true } sp-version = { workspace = true, default-features = true } @@ -44,6 +44,7 @@ std = [ "frame-system/std", "frame-try-runtime/std", "log/std", + "qp-header/std", "scale-info/std", "sp-core/std", "sp-io/std", diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index 5fdc9ea8..6447cf52 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -131,6 +131,7 @@ use frame_support::{ MAX_EXTRINSIC_DEPTH, }; use frame_system::pallet_prelude::BlockNumberFor; +use qp_header::ZkTreeRootProvider; use sp_runtime::{ generic::Digest, traits::{ @@ -396,6 +397,15 @@ where header.state_root() == storage_root, "Storage root must match that calculated." ); + + // The ZK tree root is derived from storage, so it is only expected to match + // when the state itself is expected to match. + let zk_tree_root = new_header.zk_tree_root(); + header.zk_tree_root().check_equal(zk_tree_root); + assert!( + header.zk_tree_root() == zk_tree_root, + "ZK tree root must match that calculated." + ); } assert!( @@ -936,6 +946,16 @@ where header.extrinsics_root() == new_header.extrinsics_root(), "Transaction trie root must be valid.", ); + + // check ZK tree root. This field is part of the block-hash preimage, so it must be + // re-derived from state on import; otherwise a block author could commit an arbitrary + // root into the header. + let zk_tree_root = new_header.zk_tree_root(); + header.zk_tree_root().check_equal(zk_tree_root); + assert!( + header.zk_tree_root() == zk_tree_root, + "ZK tree root must match that calculated.", + ); } /// Check a given signed transaction for validity. This doesn't execute any diff --git a/frame/executive/src/tests.rs b/frame/executive/src/tests.rs index 24de5bb5..10b9b4d7 100644 --- a/frame/executive/src/tests.rs +++ b/frame/executive/src/tests.rs @@ -708,6 +708,28 @@ fn block_import_of_bad_state_root_fails() { }); } +#[test] +#[should_panic] +fn block_import_of_bad_zk_tree_root_fails() { + new_test_ext(1).execute_with(|| { + let mut header = Header::new( + 1, + array_bytes::hex_n_into_unchecked( + "03170a2e7597b7b7e3d84c05391d139a62b157e78786d8c082f29dcf4c111314", + ), + array_bytes::hex_n_into_unchecked( + "d6b465f5a50c9f8d5a6edc0f01d285a6b19030f097d3aaf1649b7be81649f118", + ), + [69u8; 32].into(), + Digest { logs: vec![] }, + ); + // Forge a ZK tree root that does not match on-chain state (which is the default zero + // root). Before the `final_checks` fix this block would import successfully. + header.set_zk_tree_root([42u8; 32].into()); + Executive::execute_block(TestBlock { header, extrinsics: vec![] }.into()); + }); +} + #[test] #[should_panic] fn block_import_of_bad_extrinsic_root_fails() { From 497352dcf16f47e49fbbb2de4511324cd8a4c803 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 14:52:17 +0800 Subject: [PATCH 02/47] Only inject the outer arguments if the inner attribute does not specify its own --- .../support-procedural/src/dynamic_params.rs | 126 ++++++++++++++++-- 1 file changed, 113 insertions(+), 13 deletions(-) diff --git a/frame/support-procedural/src/dynamic_params.rs b/frame/support-procedural/src/dynamic_params.rs index 9cc04e8a..466ff876 100644 --- a/frame/support-procedural/src/dynamic_params.rs +++ b/frame/support-procedural/src/dynamic_params.rs @@ -149,6 +149,9 @@ fn ensure_codec_index(attrs: &Vec, span: Span) -> Result<()> { /// /// This allows the outer `#[dynamic_params(..)]` attribute to specify some arguments that don't /// need to be repeated every time. +/// +/// Arguments already present on the inner attribute take precedence and are left untouched; +/// the outer arguments are only injected when the inner attribute has none. struct MacroInjectArgs { runtime_params: syn::Ident, params_pallet: syn::Type, @@ -159,20 +162,24 @@ impl VisitMut for MacroInjectArgs { let attr = item.attrs.iter_mut().find(|attr| attr.path().is_ident("dynamic_pallet_params")); if let Some(attr) = attr { - match &attr.meta { - syn::Meta::Path(path) => - assert_eq!(path.to_token_stream().to_string(), "dynamic_pallet_params"), - _ => (), + // Only inject the outer arguments if the inner attribute does not specify its own: + // either a bare `#[dynamic_pallet_params]` or an empty `#[dynamic_pallet_params()]`. + let has_own_args = match &attr.meta { + syn::Meta::Path(_) => false, + syn::Meta::List(list) => !list.tokens.is_empty(), + syn::Meta::NameValue(_) => true, + }; + + if !has_own_args { + let runtime_params = &self.runtime_params; + let params_pallet = &self.params_pallet; + + attr.meta = syn::parse2::(quote! { + dynamic_pallet_params(#runtime_params, #params_pallet) + }) + .unwrap() + .into(); } - - let runtime_params = &self.runtime_params; - let params_pallet = &self.params_pallet; - - attr.meta = syn::parse2::(quote! { - dynamic_pallet_params(#runtime_params, #params_pallet) - }) - .unwrap() - .into(); } visit_mut::visit_item_mod_mut(self, item); @@ -570,3 +577,96 @@ impl ToTokens for DynamicParamAggregatedEnum { fn crate_access() -> core::result::Result { generate_access_from_frame_or_crate("frame-support").map_err(|e| e.to_compile_error()) } + +#[cfg(test)] +mod tests { + use super::*; + + fn inject(module: TokenStream) -> syn::ItemMod { + let mut params_mod: syn::ItemMod = parse2(module).unwrap(); + MacroInjectArgs { + runtime_params: syn::Ident::new("RuntimeParameters", Span::call_site()), + params_pallet: parse2(quote!(dynamic_params::pallet_parameters::Parameters::)) + .unwrap(), + } + .visit_item_mod_mut(&mut params_mod); + params_mod + } + + fn inner_attr(params_mod: &syn::ItemMod) -> &syn::Attribute { + let (_, items) = params_mod.content.as_ref().unwrap(); + items + .iter() + .find_map(|i| match i { + syn::Item::Mod(m) => m + .attrs + .iter() + .find(|attr| attr.path().is_ident("dynamic_pallet_params")), + _ => None, + }) + .unwrap() + } + + #[test] + fn outer_args_are_injected_into_bare_inner_attribute() { + let params_mod = inject(quote! { + pub mod dynamic_params { + #[dynamic_pallet_params] + #[codec(index = 0)] + pub mod inner {} + } + }); + + let attr = inner_attr(¶ms_mod); + assert_eq!( + attr.meta.to_token_stream().to_string(), + quote!(dynamic_pallet_params( + RuntimeParameters, + dynamic_params::pallet_parameters::Parameters:: + )) + .to_string(), + ); + } + + #[test] + fn outer_args_are_injected_into_empty_inner_attribute() { + let params_mod = inject(quote! { + pub mod dynamic_params { + #[dynamic_pallet_params()] + #[codec(index = 0)] + pub mod inner {} + } + }); + + let attr = inner_attr(¶ms_mod); + assert_eq!( + attr.meta.to_token_stream().to_string(), + quote!(dynamic_pallet_params( + RuntimeParameters, + dynamic_params::pallet_parameters::Parameters:: + )) + .to_string(), + ); + } + + #[test] + fn inner_attribute_args_take_precedence_over_outer() { + let params_mod = inject(quote! { + pub mod dynamic_params { + #[dynamic_pallet_params(InnerRuntimeParameters, pallet_other::Parameters::)] + #[codec(index = 0)] + pub mod inner {} + } + }); + + let attr = inner_attr(¶ms_mod); + assert_eq!( + attr.meta.to_token_stream().to_string(), + quote!(dynamic_pallet_params( + InnerRuntimeParameters, + pallet_other::Parameters:: + )) + .to_string(), + ); + } +} From 2bc24d8b8afd76a8a276b06a43a6688c6ace80e3 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 15:07:05 +0800 Subject: [PATCH 03/47] max dev accounts --- pallets/balances/src/lib.rs | 16 ++++++++++++++++ pallets/balances/src/tests/mod.rs | 19 ++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/pallets/balances/src/lib.rs b/pallets/balances/src/lib.rs index 37cbf962..e3fa372e 100644 --- a/pallets/balances/src/lib.rs +++ b/pallets/balances/src/lib.rs @@ -197,6 +197,16 @@ const LOG_TARGET: &str = "runtime::balances"; // Default derivation(hard) for development accounts. const DEFAULT_ADDRESS_URI: &str = "//Sender//{}"; +/// Upper bound on the number of development accounts that may be derived from a single genesis +/// config. +/// +/// The `dev_accounts` genesis field is attacker-controllable when the genesis config is built +/// through the externally reachable `GenesisBuilder::build_state` runtime API. Each derived +/// account performs an expensive sr25519 key derivation and a storage write, so an unbounded +/// count would let a tiny request force effectively unbounded runtime work. This cap bounds that +/// work while staying far above any legitimate development setup. +pub const MAX_DEV_ACCOUNTS: u32 = 10_000; + type AccountIdLookupOf = <::Lookup as StaticLookup>::Source; #[frame_support::pallet] @@ -1340,6 +1350,12 @@ pub mod pallet { // Ensure that the number of accounts is not zero. assert!(num_accounts > 0, "num_accounts must be greater than zero"); + // Bound the attacker-controllable work: reject before deriving anything so an + // oversized `dev_accounts` count cannot force unbounded key derivations. + if num_accounts > MAX_DEV_ACCOUNTS { + return Err("num_accounts exceeds the maximum allowed dev accounts"); + } + assert!( balance >= >::ExistentialDeposit::get(), "the balance of any account should always be at least the existential deposit.", diff --git a/pallets/balances/src/tests/mod.rs b/pallets/balances/src/tests/mod.rs index 155f7888..7ece36a6 100644 --- a/pallets/balances/src/tests/mod.rs +++ b/pallets/balances/src/tests/mod.rs @@ -21,7 +21,7 @@ use crate::{ self as pallet_balances, AccountData, Config, CreditOf, Error, Pallet, TotalIssuance, - DEFAULT_ADDRESS_URI, + DEFAULT_ADDRESS_URI, MAX_DEV_ACCOUNTS, }; use codec::{Decode, DecodeWithMemTracking, Encode, MaxEncodedLen}; use frame_support::{ @@ -359,6 +359,23 @@ fn weights_sane() { assert_eq!(<() as crate::WeightInfo>::force_unreserve(), info.call_weight); } +#[test] +fn derive_dev_account_rejects_counts_above_cap() { + ExtBuilder::default().build().execute_with(|| { + let ed = ExistentialDeposit::get(); + + // A count within the cap is accepted (does real, but bounded, derivation work). + assert_ok!(Balances::derive_dev_account(1, ed, DEFAULT_ADDRESS_URI)); + + // A count above the cap is rejected before any derivation work is performed, so a + // caller cannot force unbounded runtime work through the `dev_accounts` genesis field. + assert_err!( + Balances::derive_dev_account(MAX_DEV_ACCOUNTS + 1, ed, DEFAULT_ADDRESS_URI), + "num_accounts exceeds the maximum allowed dev accounts" + ); + }); +} + #[test] fn check_whitelist() { let whitelist: BTreeSet = AllPalletsWithSystem::whitelisted_storage_keys() From b2ecef9d1c1e03c0999cc622b75fc7576dff462a Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 15:26:33 +0800 Subject: [PATCH 04/47] Bound RemovePallet/RemoveStorage deletion to prevent upgrade-time chain halt These OnRuntimeUpgrade helpers cleared an entire prefix with clear_prefix(.., None) in a mandatory upgrade hook. For a prefix whose key count users can inflate, the first post-upgrade block performs unbounded deletion work, never fits the block weight limit, and re-runs every attempt, halting the chain. Add a `Limit: Get` parameter and clear at most `Limit` keys per upgrade so the work is bounded. Excess keys remain (harmless orphaned storage) and are logged; raise the limit or use a multi-block migration to remove the rest. Co-authored-by: Cursor --- frame/support/src/migrations.rs | 158 +++++++++++++++++++++++++------- 1 file changed, 125 insertions(+), 33 deletions(-) diff --git a/frame/support/src/migrations.rs b/frame/support/src/migrations.rs index f0af1263..a06c117a 100644 --- a/frame/support/src/migrations.rs +++ b/frame/support/src/migrations.rs @@ -294,8 +294,8 @@ pub fn migrate_from_pallet_version_to_storage_version< /// } /// /// pub type Migrations = ( -/// RemovePallet, -/// RemovePallet, +/// RemovePallet>, +/// RemovePallet>, /// AnyOtherMigrations... /// ); /// @@ -304,27 +304,33 @@ pub fn migrate_from_pallet_version_to_storage_version< /// } /// ``` /// -/// WARNING: `RemovePallet` has no guard rails preventing it from bricking the chain if the -/// operation of removing storage for the given pallet would exceed the block weight limit. +/// `Limit` bounds how many keys are removed in a single upgrade so the migration cannot exceed +/// the block weight limit, even for a prefix whose key count can be grown by users before the +/// upgrade. `Limit` should be chosen so that removing that many keys comfortably fits within a +/// block. /// -/// If your pallet has too many keys to be removed in a single block, it is advised to wait for -/// a multi-block scheduler currently under development which will allow for removal of storage -/// items (and performing other heavy migrations) over multiple blocks -/// (see ). -pub struct RemovePallet, DbWeight: Get>( - PhantomData<(P, DbWeight)>, +/// If your pallet may hold more than `Limit` keys, the excess keys are left in place (they are +/// harmless orphaned storage under the removed pallet's prefix). To remove all of them, either +/// raise `Limit` to a value that is still safely below the block weight limit, or use a +/// multi-block migration which can spread removal of storage items (and other heavy migrations) +/// over multiple blocks (see ). +pub struct RemovePallet, DbWeight: Get, Limit: Get>( + PhantomData<(P, DbWeight, Limit)>, ); -impl, DbWeight: Get> frame_support::traits::OnRuntimeUpgrade - for RemovePallet +impl, DbWeight: Get, Limit: Get> + frame_support::traits::OnRuntimeUpgrade for RemovePallet { fn on_runtime_upgrade() -> frame_support::weights::Weight { let hashed_prefix = twox_128(P::get().as_bytes()); - let keys_removed = match clear_prefix(&hashed_prefix, None) { + let keys_removed = match clear_prefix(&hashed_prefix, Some(Limit::get())) { KillStorageResult::AllRemoved(value) => value, KillStorageResult::SomeRemaining(value) => { - log::error!( - "`clear_prefix` failed to remove all keys for {}. THIS SHOULD NEVER HAPPEN! 🚨", - P::get() + log::warn!( + "`RemovePallet` hit its per-upgrade limit of {} keys for {}; {} keys removed, more remain. \ + Raise `Limit` (staying below the block weight limit) or use a multi-block migration to remove the rest.", + Limit::get(), + P::get(), + value, ); value }, @@ -401,8 +407,8 @@ impl, DbWeight: Get> frame_support::traits /// } /// /// pub type Migrations = ( -/// RemoveStorage, -/// RemoveStorage, +/// RemoveStorage>, +/// RemoveStorage>, /// AnyOtherMigrations... /// ); /// @@ -411,27 +417,37 @@ impl, DbWeight: Get> frame_support::traits /// } /// ``` /// -/// WARNING: `RemoveStorage` has no guard rails preventing it from bricking the chain if the -/// operation of removing storage for the given pallet would exceed the block weight limit. +/// `Limit` bounds how many keys are removed in a single upgrade so the migration cannot exceed +/// the block weight limit, even for a storage item whose key count can be grown by users before +/// the upgrade. `Limit` should be chosen so that removing that many keys comfortably fits within +/// a block. /// -/// If your storage has too many keys to be removed in a single block, it is advised to wait for -/// a multi-block scheduler currently under development which will allow for removal of storage -/// items (and performing other heavy migrations) over multiple blocks -/// (see ). -pub struct RemoveStorage, S: Get<&'static str>, DbWeight: Get>( - PhantomData<(P, S, DbWeight)>, -); -impl, S: Get<&'static str>, DbWeight: Get> - frame_support::traits::OnRuntimeUpgrade for RemoveStorage +/// If your storage may hold more than `Limit` keys, the excess keys are left in place. To remove +/// all of them, either raise `Limit` to a value that is still safely below the block weight +/// limit, or use a multi-block migration which can spread removal of storage items (and other +/// heavy migrations) over multiple blocks (see ). +pub struct RemoveStorage< + P: Get<&'static str>, + S: Get<&'static str>, + DbWeight: Get, + Limit: Get, +>(PhantomData<(P, S, DbWeight, Limit)>); +impl, S: Get<&'static str>, DbWeight: Get, Limit: Get> + frame_support::traits::OnRuntimeUpgrade for RemoveStorage { fn on_runtime_upgrade() -> frame_support::weights::Weight { let hashed_prefix = storage_prefix(P::get().as_bytes(), S::get().as_bytes()); - let keys_removed = match clear_prefix(&hashed_prefix, None) { + let keys_removed = match clear_prefix(&hashed_prefix, Some(Limit::get())) { KillStorageResult::AllRemoved(value) => value, KillStorageResult::SomeRemaining(value) => { - log::error!( - "`clear_prefix` failed to remove all keys for storage `{}` from pallet `{}`. THIS SHOULD NEVER HAPPEN! 🚨", - S::get(), P::get() + log::warn!( + "`RemoveStorage` hit its per-upgrade limit of {} keys for storage `{}` from pallet `{}`; \ + {} keys removed, more remain. Raise `Limit` (staying below the block weight limit) or use a \ + multi-block migration to remove the rest.", + Limit::get(), + S::get(), + P::get(), + value, ); value }, @@ -1197,4 +1213,80 @@ mod tests { .is_err()); }); } + + crate::parameter_types! { + pub const RemovePalletTestName: &'static str = "SomePallet"; + pub const RemoveStorageTestStorage: &'static str = "SomeStorage"; + } + + type TestDbWeight = crate::weights::constants::RocksDbWeight; + + /// Insert `count` distinct keys under `prefix` so the deletion helpers have something to clear. + fn fill_prefix(prefix: &[u8], count: u32) { + for i in 0..count { + let mut key = prefix.to_vec(); + key.extend_from_slice(&i.to_le_bytes()); + unhashed::put(&key, &i); + } + } + + #[test] + fn remove_pallet_is_bounded_by_limit() { + use crate::traits::OnRuntimeUpgrade; + use sp_core::ConstU32; + + let prefix = twox_128(RemovePalletTestName::get().as_bytes()); + let mut ext = sp_io::TestExternalities::default(); + // Pre-fill more keys than the per-upgrade limit allows to be removed, and commit them to + // the backend (the `clear_prefix` limit only bounds committed keys). + ext.execute_with(|| fill_prefix(&prefix, 10)); + ext.commit_all().unwrap(); + + ext.execute_with(|| { + // The migration removes at most `Limit` keys and leaves the rest, so its work is + // bounded regardless of how many keys the prefix holds. + RemovePallet::>::on_runtime_upgrade(); + + assert_eq!(count_prefixed_keys(&prefix), 6, "only `Limit` keys should be removed"); + }); + } + + #[test] + fn remove_storage_is_bounded_by_limit() { + use crate::traits::OnRuntimeUpgrade; + use sp_core::ConstU32; + + let prefix = storage_prefix( + RemovePalletTestName::get().as_bytes(), + RemoveStorageTestStorage::get().as_bytes(), + ); + let mut ext = sp_io::TestExternalities::default(); + ext.execute_with(|| fill_prefix(&prefix, 10)); + ext.commit_all().unwrap(); + + ext.execute_with(|| { + RemoveStorage::< + RemovePalletTestName, + RemoveStorageTestStorage, + TestDbWeight, + ConstU32<4>, + >::on_runtime_upgrade(); + + assert_eq!(count_prefixed_keys(&prefix), 6, "only `Limit` keys should be removed"); + }); + } + + /// Count how many keys currently exist under `prefix`. + fn count_prefixed_keys(prefix: &[u8]) -> u32 { + let mut count = 0u32; + let mut previous = prefix.to_vec(); + while let Some(next) = sp_io::storage::next_key(&previous) { + if !next.starts_with(prefix) { + break; + } + count += 1; + previous = next; + } + count + } } From b80a5f9a2cc2bee7d0649ee23b800280c81c02a1 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 15:54:26 +0800 Subject: [PATCH 05/47] Add bounded move_prefix_bounded to stage attacker-growable prefix moves move_prefix drains and re-writes every key below a prefix in one call with no limit, cursor, or progress feedback. Used in a mandatory runtime-upgrade hook on a user-growable prefix, an attacker can inflate the prefix beforehand and force the first post-upgrade block to perform unbounded reads/deletes/writes, stalling the upgrade. Add `move_prefix_bounded(from, to, limit, maybe_cursor) -> MovePrefixResult` mirroring `clear_storage_prefix`'s bounded/cursored pattern, so a multi-block migration can move keys in bounded chunks and charge weight per moved key. `move_prefix` now delegates to it unbounded, preserving existing behaviour, and its docs (plus `move_storage_from_pallet`) warn about the unbounded call. Co-authored-by: Cursor --- frame/support/src/storage/migration.rs | 130 ++++++++++++++++++++++--- 1 file changed, 117 insertions(+), 13 deletions(-) diff --git a/frame/support/src/storage/migration.rs b/frame/support/src/storage/migration.rs index 1dd690c3..c9522ddc 100644 --- a/frame/support/src/storage/migration.rs +++ b/frame/support/src/storage/migration.rs @@ -309,6 +309,9 @@ pub fn take_storage_item /// `concat(twox_128(old_pallet_name), twox_128(storage_name))` and insert them at the key with /// the start replaced by `concat(twox_128(new_pallet_name), twox_128(storage_name))`. /// +/// WARNING: This is an unbounded operation (see [`move_prefix`]). If the moved storage item can be +/// grown by users, use [`move_prefix_bounded`] from within a multi-block migration instead. +/// /// # Example /// /// If a pallet named "my_example" has 2 storages named "Foo" and "Bar" and the pallet is renamed @@ -360,35 +363,94 @@ pub fn move_pallet(old_pallet_name: &[u8], new_pallet_name: &[u8]) { move_prefix(&Twox128::hash(old_pallet_name), &Twox128::hash(new_pallet_name)) } +/// Result of a bounded [`move_prefix_bounded`] call. +pub struct MovePrefixResult { + /// The number of keys moved during this call. + pub moved: u32, + /// If `Some`, the move is incomplete and another call is required, passing this value back as + /// the next call's `maybe_cursor`. If `None`, all matching keys have been moved. + pub maybe_cursor: Option>, +} + /// Move all `(key, value)` after some prefix to the another prefix /// /// This function will remove all value for which the key start with `from_prefix` /// and insert them at the key with the start replaced by `to_prefix`. /// /// NOTE: The value at the key `from_prefix` is not moved. +/// +/// WARNING: This is an unbounded operation: it reads, deletes, and writes every key below +/// `from_prefix` in a single call. If the prefix can be grown by users, calling this in a +/// (mandatory) runtime-upgrade hook lets an attacker inflate the prefix beforehand and force the +/// first post-upgrade block to perform unbounded work. For such prefixes, use +/// [`move_prefix_bounded`] to move keys in bounded chunks from within a multi-block migration. pub fn move_prefix(from_prefix: &[u8], to_prefix: &[u8]) { + // Drive the bounded mover to completion in one shot. `u32::MAX` keys is effectively no bound, + // preserving the historical all-at-once behaviour for callers that know the prefix is small. + move_prefix_bounded(from_prefix, to_prefix, u32::MAX, None); +} + +/// Move at most `limit` `(key, value)` pairs from `from_prefix` to `to_prefix`, resuming from +/// `maybe_cursor`. +/// +/// This is the bounded counterpart to [`move_prefix`]: it moves up to `limit` keys per call and +/// returns a [`MovePrefixResult`] carrying how many keys were moved and, when the move is not yet +/// complete, a cursor to pass to the next call. This lets a multi-block migration spread the work +/// (and charge weight proportional to `moved`) instead of doing an unbounded move in a single +/// mandatory upgrade hook. +/// +/// ## Cursors +/// +/// Pass `None` as `maybe_cursor` for the first call. If the returned `maybe_cursor` is `Some`, +/// another call is required to continue; pass that value back in. A returned `maybe_cursor` of +/// `None` means the move is complete. +/// +/// NOTE: The value at the key `from_prefix` is not moved. +pub fn move_prefix_bounded( + from_prefix: &[u8], + to_prefix: &[u8], + limit: u32, + maybe_cursor: Option<&[u8]>, +) -> MovePrefixResult { if from_prefix == to_prefix { - return + return MovePrefixResult { moved: 0, maybe_cursor: None } } - let iter = PrefixIterator::<_> { - prefix: from_prefix.to_vec(), - previous_key: from_prefix.to_vec(), - drain: true, - closure: |key, value| Ok((key.to_vec(), value.to_vec())), - phantom: Default::default(), - }; - - for (key, value) in iter { - let full_key = [to_prefix, &key].concat(); - unhashed::put_raw(&full_key, &value); + let previous_key = maybe_cursor.map(|c| c.to_vec()).unwrap_or_else(|| from_prefix.to_vec()); + let mut iter = PrefixIterator::<(Vec, Vec)>::new( + from_prefix.to_vec(), + previous_key, + |key, value| Ok((key.to_vec(), value.to_vec())), + ) + .drain(); + + let mut moved = 0u32; + while moved < limit { + match iter.next() { + Some((key, value)) => { + let full_key = [to_prefix, &key].concat(); + unhashed::put_raw(&full_key, &value); + moved += 1; + }, + // Ran out of matching keys before hitting the limit: the move is complete. + None => return MovePrefixResult { moved, maybe_cursor: None }, + } } + + // Hit the limit. Peek past the last moved key to see whether any matching keys remain, and if + // so hand back a cursor so the caller can resume. The last moved key was drained, but + // `next_key` still returns the lexicographically next key regardless. + let last = iter.last_raw_key().to_vec(); + let more_remain = + sp_io::storage::next_key(&last).map(|n| n.starts_with(from_prefix)).unwrap_or(false); + MovePrefixResult { moved, maybe_cursor: more_remain.then_some(last) } } #[cfg(test)] mod tests { use super::{ - move_pallet, move_prefix, move_storage_from_pallet, storage_iter, storage_key_iter, + move_pallet, move_prefix, move_prefix_bounded, move_storage_from_pallet, storage_iter, + storage_key_iter, }; use crate::{ hash::StorageHasher, @@ -448,6 +510,48 @@ mod tests { }) } + #[test] + fn test_move_prefix_bounded_moves_in_chunks() { + TestExternalities::new_empty().execute_with(|| { + // Fill the old map with more entries than a single chunk can hold. + for i in 0..10u32 { + OldStorageMap::insert(i, i * 10); + } + + let from = Twox128::hash(b"my_old_pallet"); + let to = Twox128::hash(b"my_new_pallet"); + + // Move in bounded chunks of 4, following the returned cursor to completion. + let mut cursor: Option> = None; + let mut total_moved = 0u32; + let mut calls = 0u32; + loop { + let res = move_prefix_bounded(&from, &to, 4, cursor.as_deref()); + assert!(res.moved <= 4, "a chunk must never exceed the limit"); + total_moved += res.moved; + calls += 1; + assert!(calls < 100, "cursor must make progress and terminate"); + match res.maybe_cursor { + Some(c) => cursor = Some(c), + None => break, + } + } + + // Chunking actually happened (10 keys / 4 per chunk => multiple calls). + assert!(calls > 1, "the move should have been split across multiple bounded calls"); + assert_eq!(total_moved, 10, "every key is moved exactly once across chunks"); + + // Old prefix fully drained. + assert_eq!(OldStorageMap::iter().count(), 0); + + // New prefix has all entries (Twox64Concat is not key-ordered, so sort before compare). + let mut got = NewStorageMap::iter().collect::>(); + got.sort(); + let expected = (0..10u32).map(|i| (i, i * 10)).collect::>(); + assert_eq!(got, expected); + }) + } + #[test] fn test_move_storage() { TestExternalities::new_empty().execute_with(|| { From 74ab733cd130055fc106a6f9f69e20097df13715 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 16:43:35 +0800 Subject: [PATCH 06/47] Check issuance headroom in Balanced::deposit before crediting account Co-authored-by: Cursor --- .../src/traits/tokens/fungible/regular.rs | 20 ++++++++++++- pallets/balances/src/tests/fungible_tests.rs | 30 +++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/frame/support/src/traits/tokens/fungible/regular.rs b/frame/support/src/traits/tokens/fungible/regular.rs index 54a04444..2997d6b7 100644 --- a/frame/support/src/traits/tokens/fungible/regular.rs +++ b/frame/support/src/traits/tokens/fungible/regular.rs @@ -37,7 +37,7 @@ use crate::{ }, }; use core::marker::PhantomData; -use sp_arithmetic::traits::{CheckedAdd, CheckedSub, One}; +use sp_arithmetic::traits::{Bounded, CheckedAdd, CheckedSub, One}; use sp_runtime::{traits::Saturating, ArithmeticError, DispatchError, TokenError}; use super::{Credit, Debt, HandleImbalanceDrop, Imbalance}; @@ -460,6 +460,24 @@ pub trait Balanced: Inspect + Unbalanced { value: Self::Balance, precision: Precision, ) -> Result, DispatchError> { + // Ensure the credited amount can be represented by total issuance *before* mutating the + // account. The returned `Debt` grows total issuance (via its `OnDropDebt` handler) by the + // credited amount using *saturating* arithmetic, so without this check a deposit made when + // issuance is near the maximum could credit the account by more than issuance can grow, + // breaking the `sum(balances) == total_issuance` invariant. Mirrors `Mutate::mint_into`. + let value = match precision { + // Must credit exactly `value`, so issuance must be able to grow by exactly `value`. + Exact => { + Self::total_issuance().checked_add(&value).ok_or(ArithmeticError::Overflow)?; + value + }, + // Best-effort: cap the deposit to the remaining issuance headroom. + BestEffort => { + let headroom = + Self::Balance::max_value().saturating_sub(Self::total_issuance()); + value.min(headroom) + }, + }; let increase = Self::increase_balance(who, value, precision)?; Self::done_deposit(who, increase); Ok(Imbalance::::new(increase)) diff --git a/pallets/balances/src/tests/fungible_tests.rs b/pallets/balances/src/tests/fungible_tests.rs index 5027aad0..2c27f81e 100644 --- a/pallets/balances/src/tests/fungible_tests.rs +++ b/pallets/balances/src/tests/fungible_tests.rs @@ -423,6 +423,36 @@ fn positive_imbalance_drop_handled_correctly() { }); } +#[test] +fn deposit_checks_total_issuance_headroom() { + ExtBuilder::default().build_and_execute_with(|| { + let account = 1; + // Leave only a small amount of headroom below the balance-type maximum. + let headroom = 10u64; + Balances::set_total_issuance(u64::MAX - headroom); + + // An `Exact` deposit that issuance cannot fully represent must fail and change nothing, + // rather than crediting the account and later saturating issuance on debt drop. + match >::deposit(&account, headroom + 1, Exact) { + Err(e) => assert_eq!(e, ArithmeticError::Overflow.into()), + Ok(_) => panic!("exact deposit exceeding issuance headroom must fail"), + } + assert_eq!(Balances::free_balance(&account), 0, "failed deposit must not credit"); + assert_eq!(Balances::total_issuance(), u64::MAX - headroom); + + // A `BestEffort` deposit is capped to the remaining issuance headroom, so the credited + // amount always matches the growth in issuance. + let debt = + >::deposit(&account, headroom + 100, BestEffort) + .expect("best-effort deposit should succeed"); + assert_eq!(Balances::free_balance(&account), headroom, "credit is capped to headroom"); + + // Dropping the debt grows issuance by exactly the credited amount (no saturation loss). + drop(debt); + assert_eq!(Balances::total_issuance(), u64::MAX); + }); +} + #[test] fn frozen_hold_balance_best_effort_transfer_works() { ExtBuilder::default() From 8ef8b60b92d17a5c8b3e2d359057546fee709f54 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 16:57:38 +0800 Subject: [PATCH 07/47] Reject nonce increments that overflow instead of wrapping to zero Co-authored-by: Cursor --- .../src/extensions/check_nonce.rs | 69 ++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-) diff --git a/pallets/frame-system/src/extensions/check_nonce.rs b/pallets/frame-system/src/extensions/check_nonce.rs index ef1b6a7e..688b3f1f 100644 --- a/pallets/frame-system/src/extensions/check_nonce.rs +++ b/pallets/frame-system/src/extensions/check_nonce.rs @@ -81,6 +81,13 @@ impl CheckNonce { if nonce < account.nonce { return Err(InvalidTransaction::Stale.into()); } + // The account nonce must remain representable after execution, otherwise the stored + // nonce could never move past this transaction and it would be replayable. This makes + // `T::Nonce::MAX` itself unusable: an account's nonce space is permanently exhausted + // rather than ever wrapping around. + if nonce.checked_add(&T::Nonce::one()).is_none() { + return Err(InvalidTransaction::Stale.into()); + } let provides = vec![Encode::encode(&(who.clone(), nonce))]; let requires = if account.nonce < nonce { @@ -101,7 +108,12 @@ impl CheckNonce { if nonce > account.nonce { return Err(InvalidTransaction::Future.into()); } - nonce = nonce.checked_add(&T::Nonce::one()).unwrap_or(T::Nonce::zero()); + // Never wrap the stored nonce back to zero: that would make previously-executed + // transactions with low nonces valid (and thus replayable) again. Instead the maximum + // nonce is rejected outright, permanently exhausting the account's nonce space. + nonce = nonce + .checked_add(&T::Nonce::one()) + .ok_or(TransactionValidityError::from(InvalidTransaction::Stale))?; crate::Account::::mutate(who, |account| account.nonce = nonce); Ok(()) } @@ -283,6 +295,61 @@ mod tests { }) } + #[test] + fn signed_ext_check_nonce_rejects_max_nonce_instead_of_wrapping() { + new_test_ext().execute_with(|| { + crate::Account::::insert( + 1, + crate::AccountInfo { + nonce: u64::MAX.into(), + consumers: 0, + providers: 1, + sufficients: 0, + data: 0, + }, + ); + let info = DispatchInfo::default(); + let len = 0_usize; + + // The maximum nonce is unusable: accepting it would require wrapping the stored + // nonce back to zero, making previously executed low-nonce transactions replayable. + assert_storage_noop!({ + assert_eq!( + CheckNonce::(u64::MAX.into()) + .validate_only(Some(1).into(), CALL, &info, len, External, 0) + .unwrap_err(), + TransactionValidityError::Invalid(InvalidTransaction::Stale) + ); + assert_eq!( + CheckNonce::(u64::MAX.into()) + .validate_and_prepare(Some(1).into(), CALL, &info, len, 0) + .unwrap_err(), + TransactionValidityError::Invalid(InvalidTransaction::Stale) + ); + }); + + // The stored nonce must not have wrapped, so an old low nonce is still stale. + assert_eq!(crate::Account::::get(1).nonce, u64::MAX.into()); + assert_eq!( + CheckNonce::(0u64.into()) + .validate_only(Some(1).into(), CALL, &info, len, External, 0) + .unwrap_err(), + TransactionValidityError::Invalid(InvalidTransaction::Stale) + ); + + // `MAX - 1` is the last usable nonce; afterwards the account nonce sits at `MAX`. + crate::Account::::mutate(1, |account| account.nonce = (u64::MAX - 1).into()); + assert_ok!(CheckNonce::((u64::MAX - 1).into()).validate_and_prepare( + Some(1).into(), + CALL, + &info, + len, + 0, + )); + assert_eq!(crate::Account::::get(1).nonce, u64::MAX.into()); + }) + } + #[test] fn signed_ext_check_nonce_requires_provider() { new_test_ext().execute_with(|| { From 3db1953b0a762686c597bbd2264cdb1e135ebdeb Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 17:16:51 +0800 Subject: [PATCH 08/47] Allow ensure_frozen/decrease_frozen on permitted over-freezes without balance check Co-authored-by: Cursor --- .../src/traits/tokens/fungible/freeze.rs | 14 +- .../src/traits/tokens/fungibles/freeze.rs | 135 +++++++++++++++++- 2 files changed, 142 insertions(+), 7 deletions(-) diff --git a/frame/support/src/traits/tokens/fungible/freeze.rs b/frame/support/src/traits/tokens/fungible/freeze.rs index 96efbc6a..62577241 100644 --- a/frame/support/src/traits/tokens/fungible/freeze.rs +++ b/frame/support/src/traits/tokens/fungible/freeze.rs @@ -93,8 +93,8 @@ pub trait Mutate: Inspect { /// Attempt to set the amount frozen under the given `id` to `amount`, iff this would increase /// the amount frozen under `id`. Do nothing otherwise. /// - /// Fail if the account of `who` has fewer freezable funds than `amount`, unless `fortitude` is - /// [`Fortitude::Force`]. + /// Fail if this would increase the amount frozen under `id` and the account of `who` has + /// fewer freezable funds than `amount`, unless `fortitude` is [`Fortitude::Force`]. fn ensure_frozen( id: &Self::Id, who: &AccountId, @@ -102,7 +102,15 @@ pub trait Mutate: Inspect { fortitude: Fortitude, ) -> DispatchResult { let force = fortitude == Fortitude::Force; - ensure!(force || Self::balance_freezable(who) >= amount, TokenError::FundsUnavailable); + // Only require freezable funds if this call would actually increase the amount frozen + // under `id`; otherwise `extend_freeze` is a no-op and must remain idempotent even when + // an existing (permitted) over-freeze exceeds the current freezable balance. + ensure!( + force || + amount <= Self::balance_frozen(id, who) || + Self::balance_freezable(who) >= amount, + TokenError::FundsUnavailable + ); Self::extend_freeze(id, who, amount) } diff --git a/frame/support/src/traits/tokens/fungibles/freeze.rs b/frame/support/src/traits/tokens/fungibles/freeze.rs index 244f7005..b3a6bdfe 100644 --- a/frame/support/src/traits/tokens/fungibles/freeze.rs +++ b/frame/support/src/traits/tokens/fungibles/freeze.rs @@ -105,8 +105,8 @@ pub trait Mutate: Inspect { /// Attempt to set the amount frozen under the given `id` to `amount`, iff this would increase /// the amount frozen under `id`. Do nothing otherwise. /// - /// Fail if the account of `who` has fewer freezable funds than `amount`, unless `fortitude` is - /// `Fortitude::Force`. + /// Fail if this would increase the amount frozen under `id` and the account of `who` has + /// fewer freezable funds than `amount`, unless `fortitude` is `Fortitude::Force`. fn ensure_frozen( asset: Self::AssetId, id: &Self::Id, @@ -115,8 +115,13 @@ pub trait Mutate: Inspect { fortitude: Fortitude, ) -> DispatchResult { let force = fortitude == Fortitude::Force; + // Only require freezable funds if this call would actually increase the amount frozen + // under `id`; otherwise `extend_freeze` is a no-op and must remain idempotent even when + // an existing (permitted) over-freeze exceeds the current freezable balance. ensure!( - force || Self::balance_freezable(asset.clone(), who) >= amount, + force || + amount <= Self::balance_frozen(asset.clone(), id, who) || + Self::balance_freezable(asset.clone(), who) >= amount, TokenError::FundsUnavailable ); Self::extend_freeze(asset, id, who, amount) @@ -133,7 +138,10 @@ pub trait Mutate: Inspect { let a = Self::balance_frozen(asset.clone(), id, who) .checked_sub(&amount) .ok_or(ArithmeticError::Underflow)?; - Self::set_frozen(asset, id, who, a, Fortitude::Polite) + // Reducing a freeze never increases exposure, so use the unchecked primitive: routing + // through `set_frozen(.., Polite)` would wrongly fail partial releases of a freeze that + // (legitimately) exceeds the current freezable balance. + Self::set_freeze(asset, id, who, a) } /// Increase the amount which is being frozen for a particular lock, failing in the case that @@ -150,3 +158,122 @@ pub trait Mutate: Inspect { Self::set_frozen(asset, id, who, a, Fortitude::Polite) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::traits::tokens::{ + DepositConsequence, Preservation, Provenance, WithdrawConsequence, + }; + use core::cell::RefCell; + + thread_local! { + static BALANCE: RefCell = RefCell::new(0); + static FROZEN: RefCell = RefCell::new(0); + } + + /// Minimal single-account, single-freeze-id asset that implements only the required + /// primitives, so the trait's default method bodies are what gets exercised. + struct TestAsset; + + impl super::super::Inspect for TestAsset { + type AssetId = u32; + type Balance = u64; + fn total_issuance(_: u32) -> u64 { + BALANCE.with(|b| *b.borrow()) + } + fn minimum_balance(_: u32) -> u64 { + 1 + } + fn total_balance(_: u32, _: &u64) -> u64 { + BALANCE.with(|b| *b.borrow()) + } + fn balance(_: u32, _: &u64) -> u64 { + BALANCE.with(|b| *b.borrow()) + } + fn reducible_balance(_: u32, _: &u64, _: Preservation, _: Fortitude) -> u64 { + BALANCE.with(|b| *b.borrow()) + } + fn can_deposit(_: u32, _: &u64, _: u64, _: Provenance) -> DepositConsequence { + DepositConsequence::Success + } + fn can_withdraw(_: u32, _: &u64, _: u64) -> WithdrawConsequence { + WithdrawConsequence::Success + } + fn asset_exists(_: u32) -> bool { + true + } + } + + impl Inspect for TestAsset { + type Id = (); + fn balance_frozen(_: u32, _: &(), _: &u64) -> u64 { + FROZEN.with(|f| *f.borrow()) + } + fn can_freeze(_: u32, _: &(), _: &u64) -> bool { + true + } + } + + impl Mutate for TestAsset { + fn set_freeze(_: u32, _: &(), _: &u64, amount: u64) -> DispatchResult { + FROZEN.with(|f| *f.borrow_mut() = amount); + Ok(()) + } + fn extend_freeze(_: u32, _: &(), _: &u64, amount: u64) -> DispatchResult { + FROZEN.with(|f| { + let mut f = f.borrow_mut(); + *f = (*f).max(amount); + }); + Ok(()) + } + fn thaw(_: u32, _: &(), _: &u64) -> DispatchResult { + FROZEN.with(|f| *f.borrow_mut() = 0); + Ok(()) + } + } + + fn setup(balance: u64, frozen: u64) { + BALANCE.with(|b| *b.borrow_mut() = balance); + FROZEN.with(|f| *f.borrow_mut() = frozen); + } + + #[test] + fn ensure_frozen_is_idempotent_for_existing_over_freeze() { + // A freeze above the freezable balance is an explicitly permitted state. Re-ensuring at + // or below the existing freeze does not increase exposure, so it must not fail even + // though the requested amount exceeds the freezable balance. + setup(100, 200); + assert_eq!(TestAsset::ensure_frozen(0, &(), &1, 150, Fortitude::Polite), Ok(())); + assert_eq!(TestAsset::balance_frozen(0, &(), &1), 200); + + // An actual increase beyond the freezable balance still requires `Force`. + assert_eq!( + TestAsset::ensure_frozen(0, &(), &1, 300, Fortitude::Polite), + Err(TokenError::FundsUnavailable.into()) + ); + assert_eq!(TestAsset::ensure_frozen(0, &(), &1, 300, Fortitude::Force), Ok(())); + assert_eq!(TestAsset::balance_frozen(0, &(), &1), 300); + + // A plain increase within the freezable balance also still works. + setup(100, 20); + assert_eq!(TestAsset::ensure_frozen(0, &(), &1, 80, Fortitude::Polite), Ok(())); + assert_eq!(TestAsset::balance_frozen(0, &(), &1), 80); + } + + #[test] + fn decrease_frozen_works_when_freeze_exceeds_freezable_balance() { + // Partially releasing an over-freeze reduces exposure and must succeed even while the + // remaining target is still above the freezable balance. + setup(100, 200); + assert_eq!(TestAsset::decrease_frozen(0, &(), &1, 50), Ok(())); + assert_eq!(TestAsset::balance_frozen(0, &(), &1), 150); + + // Underflow is still rejected. + assert_eq!( + TestAsset::decrease_frozen(0, &(), &1, 151), + Err(ArithmeticError::Underflow.into()) + ); + assert_eq!(TestAsset::balance_frozen(0, &(), &1), 150); + } +} From ca4a4bd85cec0f4cd2659e90d0025d5a16072f41 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 17:33:40 +0800 Subject: [PATCH 09/47] Use decode_all for Callback::curry and typed NFT attribute reads Co-authored-by: Cursor --- frame/support/src/traits/reality.rs | 41 +++++++++++++++- .../support/src/traits/tokens/nonfungible.rs | 4 +- .../src/traits/tokens/nonfungible_v2.rs | 8 +-- .../support/src/traits/tokens/nonfungibles.rs | 6 +-- .../src/traits/tokens/nonfungibles_v2.rs | 49 +++++++++++++++++-- 5 files changed, 92 insertions(+), 16 deletions(-) diff --git a/frame/support/src/traits/reality.rs b/frame/support/src/traits/reality.rs index 5538a3b1..c73a6d69 100644 --- a/frame/support/src/traits/reality.rs +++ b/frame/support/src/traits/reality.rs @@ -19,7 +19,7 @@ use core::marker::PhantomData; -use codec::{Decode, DecodeWithMemTracking, Encode, FullCodec, MaxEncodedLen}; +use codec::{Decode, DecodeAll, DecodeWithMemTracking, Encode, FullCodec, MaxEncodedLen}; use frame_support::{CloneNoBound, EqNoBound, Parameter, PartialEqNoBound}; use scale_info::TypeInfo; use sp_core::ConstU32; @@ -306,7 +306,10 @@ impl Callback { Self { pallet_index, call_index, phantom: PhantomData } } pub fn curry(&self, args: Params) -> Result { - (self.pallet_index, self.call_index, args).using_encoded(|mut d| Decode::decode(&mut d)) + // `decode_all` ensures the resulting call consumes the whole of `args`: a prefix decode + // would silently bind the callback to a call with a different signature than documented. + (self.pallet_index, self.call_index, args) + .using_encoded(|mut d| DecodeAll::decode_all(&mut d)) } } @@ -346,3 +349,37 @@ impl StatementOracle for () { Err(DispatchError::Unavailable) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[derive(Encode, Decode, PartialEq, Eq, Debug)] + enum TestRuntimeCall { + #[codec(index = 7)] + Oracle(OracleCall), + } + + #[derive(Encode, Decode, PartialEq, Eq, Debug)] + enum OracleCall { + #[codec(index = 3)] + OnJudgement { ticket: u32, judgement: u16 }, + } + + #[test] + fn curry_decodes_matching_signature() { + let callback = Callback::<(u32, u16), TestRuntimeCall>::from_parts(7, 3); + assert_eq!( + callback.curry((42, 5)), + Ok(TestRuntimeCall::Oracle(OracleCall::OnJudgement { ticket: 42, judgement: 5 })), + ); + } + + #[test] + fn curry_rejects_prefix_compatible_signature_mismatch() { + // The target call consumes only (u32, u16); the extra trailing u64 must make the curry + // fail rather than be silently ignored by a prefix decode. + let callback = Callback::<(u32, u16, u64), TestRuntimeCall>::from_parts(7, 3); + assert!(callback.curry((42, 5, 99)).is_err()); + } +} diff --git a/frame/support/src/traits/tokens/nonfungible.rs b/frame/support/src/traits/tokens/nonfungible.rs index 249f84b2..011433bf 100644 --- a/frame/support/src/traits/tokens/nonfungible.rs +++ b/frame/support/src/traits/tokens/nonfungible.rs @@ -27,7 +27,7 @@ use super::nonfungibles; use crate::{dispatch::DispatchResult, traits::Get}; use alloc::vec::Vec; -use codec::{Decode, Encode}; +use codec::{Decode, DecodeAll, Encode}; use sp_runtime::TokenError; /// Trait for providing an interface to a read-only NFT-like set of items. @@ -51,7 +51,7 @@ pub trait Inspect { /// By default this just attempts to use `attribute`. fn typed_attribute(item: &Self::ItemId, key: &K) -> Option { key.using_encoded(|d| Self::attribute(item, d)) - .and_then(|v| V::decode(&mut &v[..]).ok()) + .and_then(|v| V::decode_all(&mut &v[..]).ok()) } /// Returns `true` if the `item` may be transferred. diff --git a/frame/support/src/traits/tokens/nonfungible_v2.rs b/frame/support/src/traits/tokens/nonfungible_v2.rs index 5775162e..fbdb7851 100644 --- a/frame/support/src/traits/tokens/nonfungible_v2.rs +++ b/frame/support/src/traits/tokens/nonfungible_v2.rs @@ -30,7 +30,7 @@ use crate::{ traits::Get, }; use alloc::vec::Vec; -use codec::{Decode, Encode}; +use codec::{Decode, DecodeAll, Encode}; use sp_runtime::TokenError; /// Trait for providing an interface to a read-only NFT-like item. @@ -72,7 +72,7 @@ pub trait Inspect { /// By default this just attempts to use `attribute`. fn typed_attribute(item: &Self::ItemId, key: &K) -> Option { key.using_encoded(|d| Self::attribute(item, d)) - .and_then(|v| V::decode(&mut &v[..]).ok()) + .and_then(|v| V::decode_all(&mut &v[..]).ok()) } /// Returns the strongly-typed custom attribute value of `item` corresponding to `key`. @@ -84,7 +84,7 @@ pub trait Inspect { key: &K, ) -> Option { key.using_encoded(|d| Self::custom_attribute(account, item, d)) - .and_then(|v| V::decode(&mut &v[..]).ok()) + .and_then(|v| V::decode_all(&mut &v[..]).ok()) } /// Returns the strongly-typed system attribute value of `item` corresponding to `key`. @@ -92,7 +92,7 @@ pub trait Inspect { /// By default this just attempts to use `system_attribute`. fn typed_system_attribute(item: &Self::ItemId, key: &K) -> Option { key.using_encoded(|d| Self::system_attribute(item, d)) - .and_then(|v| V::decode(&mut &v[..]).ok()) + .and_then(|v| V::decode_all(&mut &v[..]).ok()) } /// Returns `true` if the `item` may be transferred. diff --git a/frame/support/src/traits/tokens/nonfungibles.rs b/frame/support/src/traits/tokens/nonfungibles.rs index 22358cf8..2e6bc5b9 100644 --- a/frame/support/src/traits/tokens/nonfungibles.rs +++ b/frame/support/src/traits/tokens/nonfungibles.rs @@ -29,7 +29,7 @@ use crate::dispatch::DispatchResult; use alloc::vec::Vec; -use codec::{Decode, Encode}; +use codec::{Decode, DecodeAll, Encode}; use sp_runtime::{DispatchError, TokenError}; /// Trait for providing an interface to many read-only NFT-like sets of items. @@ -73,7 +73,7 @@ pub trait Inspect { key: &K, ) -> Option { key.using_encoded(|d| Self::attribute(collection, item, d)) - .and_then(|v| V::decode(&mut &v[..]).ok()) + .and_then(|v| V::decode_all(&mut &v[..]).ok()) } /// Returns the attribute value of `collection` corresponding to `key`. @@ -91,7 +91,7 @@ pub trait Inspect { key: &K, ) -> Option { key.using_encoded(|d| Self::collection_attribute(collection, d)) - .and_then(|v| V::decode(&mut &v[..]).ok()) + .and_then(|v| V::decode_all(&mut &v[..]).ok()) } /// Returns `true` if the `item` of `collection` may be transferred. diff --git a/frame/support/src/traits/tokens/nonfungibles_v2.rs b/frame/support/src/traits/tokens/nonfungibles_v2.rs index edf1c2b8..23dc828a 100644 --- a/frame/support/src/traits/tokens/nonfungibles_v2.rs +++ b/frame/support/src/traits/tokens/nonfungibles_v2.rs @@ -29,7 +29,7 @@ use crate::dispatch::{DispatchResult, Parameter}; use alloc::vec::Vec; -use codec::{Decode, Encode}; +use codec::{Decode, DecodeAll, Encode}; use sp_runtime::{DispatchError, TokenError}; /// Trait for providing an interface to many read-only NFT-like sets of items. @@ -98,7 +98,7 @@ pub trait Inspect { key: &K, ) -> Option { key.using_encoded(|d| Self::attribute(collection, item, d)) - .and_then(|v| V::decode(&mut &v[..]).ok()) + .and_then(|v| V::decode_all(&mut &v[..]).ok()) } /// Returns the strongly-typed custom attribute value of `item` of `collection` corresponding to @@ -112,7 +112,7 @@ pub trait Inspect { key: &K, ) -> Option { key.using_encoded(|d| Self::custom_attribute(account, collection, item, d)) - .and_then(|v| V::decode(&mut &v[..]).ok()) + .and_then(|v| V::decode_all(&mut &v[..]).ok()) } /// Returns the strongly-typed system attribute value of `item` corresponding to `key` if @@ -126,7 +126,7 @@ pub trait Inspect { key: &K, ) -> Option { key.using_encoded(|d| Self::system_attribute(collection, item, d)) - .and_then(|v| V::decode(&mut &v[..]).ok()) + .and_then(|v| V::decode_all(&mut &v[..]).ok()) } /// Returns the attribute value of `collection` corresponding to `key`. @@ -144,7 +144,7 @@ pub trait Inspect { key: &K, ) -> Option { key.using_encoded(|d| Self::collection_attribute(collection, d)) - .and_then(|v| V::decode(&mut &v[..]).ok()) + .and_then(|v| V::decode_all(&mut &v[..]).ok()) } /// Returns `true` if the `item` of `collection` may be transferred. @@ -441,3 +441,42 @@ pub trait Trading: Inspect { /// Returns the item price of `item` or `None` if the item is not for sale. fn item_price(collection: &Self::CollectionId, item: &Self::ItemId) -> Option; } + +#[cfg(test)] +mod tests { + use super::*; + use core::cell::RefCell; + + thread_local! { + static ATTRIBUTE: RefCell>> = RefCell::new(None); + } + + /// Mock exposing only the raw `attribute` primitive, so the default `typed_attribute` body + /// is what gets exercised. + struct TestCollection; + + impl Inspect for TestCollection { + type ItemId = u32; + type CollectionId = u32; + fn owner(_: &u32, _: &u32) -> Option { + None + } + fn attribute(_: &u32, _: &u32, _: &[u8]) -> Option> { + ATTRIBUTE.with(|a| a.borrow().clone()) + } + } + + #[test] + fn typed_attribute_rejects_trailing_bytes() { + // A canonically encoded value decodes as expected. + ATTRIBUTE.with(|a| *a.borrow_mut() = Some(42u32.encode())); + assert_eq!(TestCollection::typed_attribute::<_, u32>(&0, &0, &b"key"), Some(42)); + + // Raw attribute writes are not bound to any type: a value with a valid prefix but + // trailing bytes must not be accepted as the typed value. + let mut noncanonical = 42u32.encode(); + noncanonical.extend_from_slice(b"junk"); + ATTRIBUTE.with(|a| *a.borrow_mut() = Some(noncanonical)); + assert_eq!(TestCollection::typed_attribute::<_, u32>(&0, &0, &b"key"), None); + } +} From 555c5758b223243f4d1a2400d705c866ba88d573 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 17:42:27 +0800 Subject: [PATCH 10/47] Fix ration denominator overflow and reject invalid SplitTwoWays configs at compile time Co-authored-by: Cursor --- frame/support/src/traits/tokens/imbalance.rs | 127 +++++++++++++++++- .../traits/tokens/imbalance/split_two_ways.rs | 14 +- 2 files changed, 137 insertions(+), 4 deletions(-) diff --git a/frame/support/src/traits/tokens/imbalance.rs b/frame/support/src/traits/tokens/imbalance.rs index ee0d7a81..7b58da98 100644 --- a/frame/support/src/traits/tokens/imbalance.rs +++ b/frame/support/src/traits/tokens/imbalance.rs @@ -79,13 +79,23 @@ pub trait Imbalance: Sized + TryDrop + Default + TryMerge { /// Consume `self` and return two independent instances; the amounts returned will be in /// approximately the same ratio as `first`:`second`. /// - /// NOTE: This requires up to `first + second` room for a multiply, and `first + second` should - /// fit into a `u32`. Overflow will safely saturate in both cases. + /// NOTE: This requires up to `first + second` room for a multiply. If `first + second` + /// overflows a `u32`, both are halved, which preserves the ratio up to rounding. The multiply + /// will safely saturate on overflow. fn ration(self, first: u32, second: u32) -> (Self, Self) where Balance: From + Saturating + Div, { - let total: u32 = first.saturating_add(second); + // If `first + second` would overflow, halve both: this keeps the ratio (up to rounding) + // while keeping the denominator exact. Saturating the denominator to `u32::MAX` instead + // would distort the split, e.g. `MAX:MAX` would send (almost) the whole imbalance to the + // first leg rather than half of it. + let (first, second) = if first.checked_add(second).is_none() { + (first >> 1, second >> 1) + } else { + (first, second) + }; + let total = first + second; if total == 0 { return (Self::zero(), Self::zero()) } @@ -250,3 +260,114 @@ impl TryMerge for () { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + use core::cell::RefCell; + + /// Minimal imbalance over `u64` so the trait's default method bodies are what gets + /// exercised. + #[derive(Default, Debug, PartialEq, Eq)] + struct TestImbalance(u64); + + impl TryDrop for TestImbalance { + fn try_drop(self) -> Result<(), Self> { + if self.0 == 0 { + Ok(()) + } else { + Err(self) + } + } + } + + impl TryMerge for TestImbalance { + fn try_merge(self, other: Self) -> Result { + Ok(Self(self.0 + other.0)) + } + } + + impl Imbalance for TestImbalance { + type Opposite = TestImbalance; + fn zero() -> Self { + Self(0) + } + fn drop_zero(self) -> Result<(), Self> { + self.try_drop() + } + fn split(self, amount: u64) -> (Self, Self) { + let first = self.0.min(amount); + (Self(first), Self(self.0 - first)) + } + fn extract(&mut self, amount: u64) -> Self { + let extracted = self.0.min(amount); + self.0 -= extracted; + Self(extracted) + } + fn merge(self, other: Self) -> Self { + Self(self.0 + other.0) + } + fn subsume(&mut self, other: Self) { + self.0 += other.0 + } + fn offset(self, other: Self::Opposite) -> SameOrOther { + if self.0 >= other.0 { + SameOrOther::Same(Self(self.0 - other.0)) + } else { + SameOrOther::Other(Self(other.0 - self.0)) + } + } + fn peek(&self) -> u64 { + self.0 + } + } + + #[test] + fn ration_splits_proportionally() { + let (a, b) = TestImbalance(600).ration(1, 2); + assert_eq!((a.peek(), b.peek()), (200, 400)); + + let (a, b) = TestImbalance(600).ration(0, 0); + assert_eq!((a.peek(), b.peek()), (0, 0)); + } + + #[test] + fn ration_keeps_ratio_when_sum_overflows() { + // `MAX:MAX` must behave as `1:1`. A denominator saturated to `u32::MAX` would instead + // compute `peek * MAX / MAX` and send the whole imbalance to the first leg. + let (a, b) = TestImbalance(1000).ration(u32::MAX, u32::MAX); + assert_eq!((a.peek(), b.peek()), (500, 500)); + + // Heavily skewed overflowing ratios keep their skew. + let (a, b) = TestImbalance(1000).ration(1, u32::MAX); + assert_eq!((a.peek(), b.peek()), (0, 1000)); + let (a, b) = TestImbalance(1000).ration(u32::MAX, 1); + assert_eq!((a.peek(), b.peek()), (1000, 0)); + } + + thread_local! { + static SPLIT_SINK: RefCell<(u64, u64)> = RefCell::new((0, 0)); + } + + struct Sink1; + impl OnUnbalanced for Sink1 { + fn on_nonzero_unbalanced(amount: TestImbalance) { + SPLIT_SINK.with(|s| s.borrow_mut().0 += amount.peek()); + } + } + struct Sink2; + impl OnUnbalanced for Sink2 { + fn on_nonzero_unbalanced(amount: TestImbalance) { + SPLIT_SINK.with(|s| s.borrow_mut().1 += amount.peek()); + } + } + + #[test] + fn split_two_ways_splits_by_configured_parts() { + SPLIT_SINK.with(|s| *s.borrow_mut() = (0, 0)); + as OnUnbalanced<_>>::on_unbalanced( + TestImbalance(100), + ); + assert_eq!(SPLIT_SINK.with(|s| *s.borrow()), (75, 25)); + } +} diff --git a/frame/support/src/traits/tokens/imbalance/split_two_ways.rs b/frame/support/src/traits/tokens/imbalance/split_two_ways.rs index d79ae562..6207bae9 100644 --- a/frame/support/src/traits/tokens/imbalance/split_two_ways.rs +++ b/frame/support/src/traits/tokens/imbalance/split_two_ways.rs @@ -26,6 +26,18 @@ pub struct SplitTwoWays, ); +impl + SplitTwoWays +{ + /// Evaluated at compile time for each instantiation that handles an imbalance: a ratio whose + /// sum is zero or overflows `u32` is a configuration bug that would otherwise panic + /// (divide-by-zero or add overflow) at runtime while disposing of a nonzero imbalance. + const TOTAL: u32 = match PART1.checked_add(PART2) { + Some(total) if total > 0 => total, + _ => panic!("`SplitTwoWays` requires `0 < PART1 + PART2 <= u32::MAX`"), + }; +} + impl< Balance: From + Saturating + Div, I: Imbalance, @@ -36,7 +48,7 @@ impl< > OnUnbalanced for SplitTwoWays { fn on_nonzero_unbalanced(amount: I) { - let total: u32 = PART1 + PART2; + let total: u32 = Self::TOTAL; let amount1 = amount.peek().saturating_mul(PART1.into()) / total.into(); let (imb1, imb2) = amount.split(amount1); Target1::on_unbalanced(imb1); From 5f0f0b908ef6463dccb7a96a5eef0f1f6e9d90e3 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 17:52:17 +0800 Subject: [PATCH 11/47] Propagate actual transfer debit and burn approved-transfer reaping dust Credit the full amount removed from the source in generic transfer helpers so expendable reaping cannot desync issuance from balances. Burn reaping dust in transfer_approved so delegates cannot move more than the approved amount to their chosen destination. Co-authored-by: Cursor --- .../src/traits/tokens/fungible/regular.rs | 20 +++++----- .../src/traits/tokens/fungibles/regular.rs | 21 ++++++----- pallets/assets/src/functions.rs | 2 +- pallets/assets/src/tests.rs | 37 ++++++++++++++++++- 4 files changed, 60 insertions(+), 20 deletions(-) diff --git a/frame/support/src/traits/tokens/fungible/regular.rs b/frame/support/src/traits/tokens/fungible/regular.rs index 2997d6b7..d78ad22f 100644 --- a/frame/support/src/traits/tokens/fungible/regular.rs +++ b/frame/support/src/traits/tokens/fungible/regular.rs @@ -324,18 +324,20 @@ where amount: Self::Balance, preservation: Preservation, ) -> Result { - let _extra = Self::can_withdraw(source, amount).into_result(preservation != Expendable)?; - Self::can_deposit(dest, amount, Extant).into_result()?; + let dust = Self::can_withdraw(source, amount).into_result(preservation != Expendable)?; + let actual = amount.saturating_add(dust); + Self::can_deposit(dest, actual, Extant).into_result()?; if source == dest { - return Ok(amount) + return Ok(actual) } - Self::decrease_balance(source, amount, BestEffort, preservation, Polite)?; - // This should never fail as we checked `can_deposit` earlier. But we do a best-effort - // anyway. - let _ = Self::increase_balance(dest, amount, BestEffort); - Self::done_transfer(source, dest, amount); - Ok(amount) + let debited = Self::decrease_balance(source, amount, BestEffort, preservation, Polite)?; + // Credit the amount actually removed from the source. When an expendable transfer reaps + // the source, `debited` can exceed `amount`; ignoring it desynchronizes issuance from + // account balances. + let _ = Self::increase_balance(dest, debited, BestEffort); + Self::done_transfer(source, dest, debited); + Ok(debited) } /// Simple infallible function to force an account to have a particular balance, good for use diff --git a/frame/support/src/traits/tokens/fungibles/regular.rs b/frame/support/src/traits/tokens/fungibles/regular.rs index 3985da78..974188e2 100644 --- a/frame/support/src/traits/tokens/fungibles/regular.rs +++ b/frame/support/src/traits/tokens/fungibles/regular.rs @@ -370,19 +370,22 @@ where amount: Self::Balance, preservation: Preservation, ) -> Result { - let _extra = Self::can_withdraw(asset.clone(), source, amount) + let dust = Self::can_withdraw(asset.clone(), source, amount) .into_result(preservation != Expendable)?; - Self::can_deposit(asset.clone(), dest, amount, Extant).into_result()?; + let actual = amount.saturating_add(dust); + Self::can_deposit(asset.clone(), dest, actual, Extant).into_result()?; if source == dest { - return Ok(amount) + return Ok(actual) } - Self::decrease_balance(asset.clone(), source, amount, BestEffort, preservation, Polite)?; - // This should never fail as we checked `can_deposit` earlier. But we do a best-effort - // anyway. - let _ = Self::increase_balance(asset.clone(), dest, amount, BestEffort); - Self::done_transfer(asset, source, dest, amount); - Ok(amount) + let debited = + Self::decrease_balance(asset.clone(), source, amount, BestEffort, preservation, Polite)?; + // Credit the amount actually removed from the source. When an expendable transfer reaps + // the source, `debited` can exceed `amount`; ignoring it desynchronizes issuance from + // account balances. + let _ = Self::increase_balance(asset.clone(), dest, debited, BestEffort); + Self::done_transfer(asset, source, dest, debited); + Ok(debited) } /// Simple infallible function to force an account to have a particular balance, good for use diff --git a/pallets/assets/src/functions.rs b/pallets/assets/src/functions.rs index b82b22d6..2e6a690b 100644 --- a/pallets/assets/src/functions.rs +++ b/pallets/assets/src/functions.rs @@ -1021,7 +1021,7 @@ impl, I: 'static> Pallet { let remaining = approved.amount.checked_sub(&amount).ok_or(Error::::Unapproved)?; - let f = TransferFlags { keep_alive: false, best_effort: false, burn_dust: false }; + let f = TransferFlags { keep_alive: false, best_effort: false, burn_dust: true }; owner_died = Self::transfer_and_die(id.clone(), owner, destination, amount, None, f)?.1; diff --git a/pallets/assets/src/tests.rs b/pallets/assets/src/tests.rs index 439e743c..264f9b38 100644 --- a/pallets/assets/src/tests.rs +++ b/pallets/assets/src/tests.rs @@ -23,8 +23,9 @@ use frame_support::{ assert_noop, assert_ok, dispatch::GetDispatchInfo, traits::{ + fungibles::Mutate, fungibles::InspectEnumerable, - tokens::{Preservation::Protect, Provenance}, + tokens::{Preservation::Expendable, Preservation::Protect, Provenance}, Currency, }, BoundedVec, @@ -78,6 +79,40 @@ fn transfer_should_never_burn() { }); } +#[test] +fn fungible_transfer_credits_reaped_dust() { + new_test_ext().execute_with(|| { + assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 10)); + assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100)); + assert_eq!(Assets::total_supply(0), 100); + + // An expendable transfer that reaps the source debits the full balance, not just the + // requested amount. The generic `fungibles::Mutate::transfer` path must credit that + // actual debit or total issuance no longer matches the sum of account balances. + assert_ok!(>::transfer(0, &1, &2, 91, Expendable)); + assert!(Assets::maybe_balance(0, 1).is_none()); + assert_eq!(Assets::balance(0, 2), 100); + assert_eq!(Assets::total_supply(0), 100); + }); +} + +#[test] +fn transfer_approved_burns_reaping_dust() { + new_test_ext().execute_with(|| { + assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 10)); + assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100)); + Balances::make_free_balance_be(&1, 2); + assert_ok!(Assets::approve_transfer(RuntimeOrigin::signed(1), 0, 2, 91)); + + // Reaping dust must be burned, not credited to the delegate's destination beyond the + // approved amount. + assert_ok!(Assets::transfer_approved(RuntimeOrigin::signed(2), 0, 1, 3, 91)); + assert!(Assets::maybe_balance(0, 1).is_none()); + assert_eq!(Assets::balance(0, 3), 91); + assert_eq!(Assets::total_supply(0), 91); + }); +} + #[test] fn basic_minting_should_work() { new_test_ext().execute_with(|| { From a15a98e7be5eb45040a73bfd882575c5e1510da9 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 18:00:56 +0800 Subject: [PATCH 12/47] fix tests --- frame/support/src/traits/misc.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/support/src/traits/misc.rs b/frame/support/src/traits/misc.rs index ec868bb7..16da4558 100644 --- a/frame/support/src/traits/misc.rs +++ b/frame/support/src/traits/misc.rs @@ -505,7 +505,7 @@ pub trait DefensiveMin { /// When the assumption is violated, the other value is returned and a defensive /// error is logged. With `debug_assertions` enabled, this also panics. /// - /// ```should_panic + /// ```no_run /// use frame_support::traits::DefensiveMin; /// assert_eq!(3, 4_u32.defensive_min(3_u32)); /// ``` @@ -524,7 +524,7 @@ pub trait DefensiveMin { /// When the assumption is violated, the other value is returned and a defensive /// error is logged. With `debug_assertions` enabled, this also panics. /// - /// ```should_panic + /// ```no_run /// use frame_support::traits::DefensiveMin; /// assert_eq!(4, 4_u32.defensive_strict_min(4_u32)); /// ``` @@ -573,7 +573,7 @@ pub trait DefensiveMax { /// When the assumption is violated, the other value is returned and a defensive /// error is logged. With `debug_assertions` enabled, this also panics. /// - /// ```should_panic + /// ```no_run /// use frame_support::traits::DefensiveMax; /// assert_eq!(5, 4_u32.defensive_max(5_u32)); /// ``` @@ -592,7 +592,7 @@ pub trait DefensiveMax { /// When the assumption is violated, the other value is returned and a defensive /// error is logged. With `debug_assertions` enabled, this also panics. /// - /// ```should_panic + /// ```no_run /// use frame_support::traits::DefensiveMax; /// assert_eq!(4, 4_u32.defensive_strict_max(4_u32)); /// ``` From f146867204851e93108ee97e2d27e23ad8148754 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 18:04:55 +0800 Subject: [PATCH 13/47] fmt --- frame/executive/src/lib.rs | 5 +---- frame/support-procedural/src/dynamic_params.rs | 6 ++---- frame/support/src/migrations.rs | 8 ++++++-- frame/support/src/storage/migration.rs | 5 +++-- frame/support/src/traits/tokens/fungible/regular.rs | 3 +-- frame/support/src/traits/tokens/fungibles/regular.rs | 10 ++++++++-- pallets/assets/src/tests.rs | 8 +++++--- 7 files changed, 26 insertions(+), 19 deletions(-) diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index 6447cf52..20d7f74b 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -952,10 +952,7 @@ where // root into the header. let zk_tree_root = new_header.zk_tree_root(); header.zk_tree_root().check_equal(zk_tree_root); - assert!( - header.zk_tree_root() == zk_tree_root, - "ZK tree root must match that calculated.", - ); + assert!(header.zk_tree_root() == zk_tree_root, "ZK tree root must match that calculated.",); } /// Check a given signed transaction for validity. This doesn't execute any diff --git a/frame/support-procedural/src/dynamic_params.rs b/frame/support-procedural/src/dynamic_params.rs index 466ff876..ec8cc1ba 100644 --- a/frame/support-procedural/src/dynamic_params.rs +++ b/frame/support-procedural/src/dynamic_params.rs @@ -598,10 +598,8 @@ mod tests { items .iter() .find_map(|i| match i { - syn::Item::Mod(m) => m - .attrs - .iter() - .find(|attr| attr.path().is_ident("dynamic_pallet_params")), + syn::Item::Mod(m) => + m.attrs.iter().find(|attr| attr.path().is_ident("dynamic_pallet_params")), _ => None, }) .unwrap() diff --git a/frame/support/src/migrations.rs b/frame/support/src/migrations.rs index a06c117a..34ab86c4 100644 --- a/frame/support/src/migrations.rs +++ b/frame/support/src/migrations.rs @@ -432,8 +432,12 @@ pub struct RemoveStorage< DbWeight: Get, Limit: Get, >(PhantomData<(P, S, DbWeight, Limit)>); -impl, S: Get<&'static str>, DbWeight: Get, Limit: Get> - frame_support::traits::OnRuntimeUpgrade for RemoveStorage +impl< + P: Get<&'static str>, + S: Get<&'static str>, + DbWeight: Get, + Limit: Get, + > frame_support::traits::OnRuntimeUpgrade for RemoveStorage { fn on_runtime_upgrade() -> frame_support::weights::Weight { let hashed_prefix = storage_prefix(P::get().as_bytes(), S::get().as_bytes()); diff --git a/frame/support/src/storage/migration.rs b/frame/support/src/storage/migration.rs index c9522ddc..67bccfb2 100644 --- a/frame/support/src/storage/migration.rs +++ b/frame/support/src/storage/migration.rs @@ -441,8 +441,9 @@ pub fn move_prefix_bounded( // so hand back a cursor so the caller can resume. The last moved key was drained, but // `next_key` still returns the lexicographically next key regardless. let last = iter.last_raw_key().to_vec(); - let more_remain = - sp_io::storage::next_key(&last).map(|n| n.starts_with(from_prefix)).unwrap_or(false); + let more_remain = sp_io::storage::next_key(&last) + .map(|n| n.starts_with(from_prefix)) + .unwrap_or(false); MovePrefixResult { moved, maybe_cursor: more_remain.then_some(last) } } diff --git a/frame/support/src/traits/tokens/fungible/regular.rs b/frame/support/src/traits/tokens/fungible/regular.rs index d78ad22f..1bc4bb76 100644 --- a/frame/support/src/traits/tokens/fungible/regular.rs +++ b/frame/support/src/traits/tokens/fungible/regular.rs @@ -475,8 +475,7 @@ pub trait Balanced: Inspect + Unbalanced { }, // Best-effort: cap the deposit to the remaining issuance headroom. BestEffort => { - let headroom = - Self::Balance::max_value().saturating_sub(Self::total_issuance()); + let headroom = Self::Balance::max_value().saturating_sub(Self::total_issuance()); value.min(headroom) }, }; diff --git a/frame/support/src/traits/tokens/fungibles/regular.rs b/frame/support/src/traits/tokens/fungibles/regular.rs index 974188e2..1d9d78bb 100644 --- a/frame/support/src/traits/tokens/fungibles/regular.rs +++ b/frame/support/src/traits/tokens/fungibles/regular.rs @@ -378,8 +378,14 @@ where return Ok(actual) } - let debited = - Self::decrease_balance(asset.clone(), source, amount, BestEffort, preservation, Polite)?; + let debited = Self::decrease_balance( + asset.clone(), + source, + amount, + BestEffort, + preservation, + Polite, + )?; // Credit the amount actually removed from the source. When an expendable transfer reaps // the source, `debited` can exceed `amount`; ignoring it desynchronizes issuance from // account balances. diff --git a/pallets/assets/src/tests.rs b/pallets/assets/src/tests.rs index 264f9b38..c8eed9a4 100644 --- a/pallets/assets/src/tests.rs +++ b/pallets/assets/src/tests.rs @@ -23,9 +23,11 @@ use frame_support::{ assert_noop, assert_ok, dispatch::GetDispatchInfo, traits::{ - fungibles::Mutate, - fungibles::InspectEnumerable, - tokens::{Preservation::Expendable, Preservation::Protect, Provenance}, + fungibles::{InspectEnumerable, Mutate}, + tokens::{ + Preservation::{Expendable, Protect}, + Provenance, + }, Currency, }, BoundedVec, From a0dd58e694099a2f7e817e33bc5aee14d264518b Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 18:10:26 +0800 Subject: [PATCH 14/47] Tolerate bounded key remainder in RemovePallet/RemoveStorage try-runtime checks Co-authored-by: Cursor --- frame/support/src/migrations.rs | 140 ++++++++++++++++++++++++++++---- 1 file changed, 126 insertions(+), 14 deletions(-) diff --git a/frame/support/src/migrations.rs b/frame/support/src/migrations.rs index 34ab86c4..413bc319 100644 --- a/frame/support/src/migrations.rs +++ b/frame/support/src/migrations.rs @@ -260,6 +260,26 @@ pub fn migrate_from_pallet_version_to_storage_version< Pallets::migrate(db_weight) } +/// Count keys under `prefix`, stopping once `up_to` keys have been seen. +/// +/// Only used by try-runtime hooks; the cap keeps the check bounded for prefixes holding many more +/// keys than the migration's per-upgrade limit. +#[cfg(feature = "try-runtime")] +fn count_prefixed_keys_up_to(prefix: &[u8], up_to: u32) -> u32 { + let mut count = 0u32; + let mut previous = prefix.to_vec(); + while count < up_to { + match sp_io::storage::next_key(&previous) { + Some(next) if next.starts_with(prefix) => { + count += 1; + previous = next; + }, + _ => break, + } + } + count +} + /// `RemovePallet` is a utility struct used to remove all storage items associated with a specific /// pallet. /// @@ -343,30 +363,41 @@ impl, DbWeight: Get, Limit: Get> #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { - use crate::storage::unhashed::contains_prefixed_key; + use codec::Encode; let hashed_prefix = twox_128(P::get().as_bytes()); - match contains_prefixed_key(&hashed_prefix) { + let key_count = count_prefixed_keys_up_to(&hashed_prefix, Limit::get().saturating_add(1)); + match key_count > 0 { true => log::info!("Found {} keys pre-removal 👀", P::get()), false => log::warn!( "Migration RemovePallet<{}> can be removed (no keys found pre-removal).", P::get() ), }; - Ok(alloc::vec::Vec::new()) + // Whether more keys exist than the migration is allowed to remove in one upgrade. + Ok((key_count > Limit::get()).encode()) } #[cfg(feature = "try-runtime")] - fn post_upgrade(_state: alloc::vec::Vec) -> Result<(), sp_runtime::TryRuntimeError> { + fn post_upgrade(state: alloc::vec::Vec) -> Result<(), sp_runtime::TryRuntimeError> { use crate::storage::unhashed::contains_prefixed_key; + use codec::Decode; + let over_limit = bool::decode(&mut &state[..]) + .map_err(|_| "RemovePallet post_upgrade failed to decode pre_upgrade state")?; let hashed_prefix = twox_128(P::get().as_bytes()); - match contains_prefixed_key(&hashed_prefix) { - true => { + match (contains_prefixed_key(&hashed_prefix), over_limit) { + (false, _) => log::info!("No {} keys found post-removal 🎉", P::get()), + (true, true) => log::warn!( + "{} has keys remaining post-removal because the prefix held more than the \ + per-upgrade limit of {} keys. Schedule further removals to clear the rest.", + P::get(), + Limit::get(), + ), + (true, false) => { log::error!("{} has keys remaining post-removal ❗", P::get()); return Err("Keys remaining post-removal, this should never happen 🚨".into()) }, - false => log::info!("No {} keys found post-removal 🎉", P::get()), }; Ok(()) } @@ -464,10 +495,11 @@ impl< #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { - use crate::storage::unhashed::contains_prefixed_key; + use codec::Encode; let hashed_prefix = storage_prefix(P::get().as_bytes(), S::get().as_bytes()); - match contains_prefixed_key(&hashed_prefix) { + let key_count = count_prefixed_keys_up_to(&hashed_prefix, Limit::get().saturating_add(1)); + match key_count > 0 { true => log::info!("Found `{}` `{}` keys pre-removal 👀", P::get(), S::get()), false => log::warn!( "Migration RemoveStorage<{}, {}> can be removed (no keys found pre-removal).", @@ -475,20 +507,31 @@ impl< S::get() ), }; - Ok(Default::default()) + // Whether more keys exist than the migration is allowed to remove in one upgrade. + Ok((key_count > Limit::get()).encode()) } #[cfg(feature = "try-runtime")] - fn post_upgrade(_state: alloc::vec::Vec) -> Result<(), sp_runtime::TryRuntimeError> { + fn post_upgrade(state: alloc::vec::Vec) -> Result<(), sp_runtime::TryRuntimeError> { use crate::storage::unhashed::contains_prefixed_key; + use codec::Decode; + let over_limit = bool::decode(&mut &state[..]) + .map_err(|_| "RemoveStorage post_upgrade failed to decode pre_upgrade state")?; let hashed_prefix = storage_prefix(P::get().as_bytes(), S::get().as_bytes()); - match contains_prefixed_key(&hashed_prefix) { - true => { + match (contains_prefixed_key(&hashed_prefix), over_limit) { + (false, _) => log::info!("No `{}` `{}` keys found post-removal 🎉", P::get(), S::get()), + (true, true) => log::warn!( + "`{}` `{}` has keys remaining post-removal because the prefix held more than the \ + per-upgrade limit of {} keys. Schedule further removals to clear the rest.", + P::get(), + S::get(), + Limit::get(), + ), + (true, false) => { log::error!("`{}` `{}` has keys remaining post-removal ❗", P::get(), S::get()); return Err("Keys remaining post-removal, this should never happen 🚨".into()) }, - false => log::info!("No `{}` `{}` keys found post-removal 🎉", P::get(), S::get()), }; Ok(()) } @@ -1280,6 +1323,75 @@ mod tests { }); } + /// The try-runtime checks must tolerate keys remaining when the prefix held more keys than + /// the per-upgrade `Limit`, since that is the documented (bounded) behaviour, while still + /// failing when keys remain unexpectedly. + #[test] + #[cfg(feature = "try-runtime")] + fn remove_pallet_try_runtime_tolerates_bounded_remainder() { + use crate::traits::OnRuntimeUpgrade; + use sp_core::ConstU32; + + type UnderLimit = RemovePallet>; + type OverLimit = RemovePallet>; + + let prefix = twox_128(RemovePalletTestName::get().as_bytes()); + + // More keys than `Limit`: keys legitimately remain, post_upgrade must accept. + let mut ext = sp_io::TestExternalities::default(); + ext.execute_with(|| fill_prefix(&prefix, 10)); + ext.commit_all().unwrap(); + ext.execute_with(|| { + let state = OverLimit::pre_upgrade().unwrap(); + OverLimit::on_runtime_upgrade(); + assert_eq!(count_prefixed_keys(&prefix), 6); + assert!(OverLimit::post_upgrade(state).is_ok()); + }); + + // Fewer keys than `Limit`: all keys must be gone, otherwise post_upgrade must fail. + let mut ext = sp_io::TestExternalities::default(); + ext.execute_with(|| fill_prefix(&prefix, 10)); + ext.commit_all().unwrap(); + ext.execute_with(|| { + let state = UnderLimit::pre_upgrade().unwrap(); + UnderLimit::on_runtime_upgrade(); + assert_eq!(count_prefixed_keys(&prefix), 0); + assert!(UnderLimit::post_upgrade(state.clone()).is_ok()); + + // Keys remaining without the over-limit justification is still a fatal error. + fill_prefix(&prefix, 1); + assert!(UnderLimit::post_upgrade(state).is_err()); + }); + } + + #[test] + #[cfg(feature = "try-runtime")] + fn remove_storage_try_runtime_tolerates_bounded_remainder() { + use crate::traits::OnRuntimeUpgrade; + use sp_core::ConstU32; + + type OverLimit = RemoveStorage< + RemovePalletTestName, + RemoveStorageTestStorage, + TestDbWeight, + ConstU32<4>, + >; + + let prefix = storage_prefix( + RemovePalletTestName::get().as_bytes(), + RemoveStorageTestStorage::get().as_bytes(), + ); + let mut ext = sp_io::TestExternalities::default(); + ext.execute_with(|| fill_prefix(&prefix, 10)); + ext.commit_all().unwrap(); + ext.execute_with(|| { + let state = OverLimit::pre_upgrade().unwrap(); + OverLimit::on_runtime_upgrade(); + assert_eq!(count_prefixed_keys(&prefix), 6); + assert!(OverLimit::post_upgrade(state).is_ok()); + }); + } + /// Count how many keys currently exist under `prefix`. fn count_prefixed_keys(prefix: &[u8]) -> u32 { let mut count = 0u32; From c1275fdf3f7709bb788ccf597df21d3c80d8d407 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 18:18:54 +0800 Subject: [PATCH 15/47] Return empty slice for unknown module in get_call_names instead of panicking Co-authored-by: Cursor --- .../src/construct_runtime/expand/call.rs | 5 ++++- frame/support/src/tests/mod.rs | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/frame/support-procedural/src/construct_runtime/expand/call.rs b/frame/support-procedural/src/construct_runtime/expand/call.rs index f2ec5594..44daf4cb 100644 --- a/frame/support-procedural/src/construct_runtime/expand/call.rs +++ b/frame/support-procedural/src/construct_runtime/expand/call.rs @@ -162,7 +162,10 @@ pub fn expand_outer_dispatch( <<#pallet_names as Callable<#runtime>>::RuntimeCall as GetCallName>::get_call_names(), )* - _ => unreachable!(), + // An unknown module name is a recoverable "not found" rather than an + // impossible state: this is public metadata API reachable with + // caller-supplied strings, so return an empty slice instead of panicking. + _ => &[], } } } diff --git a/frame/support/src/tests/mod.rs b/frame/support/src/tests/mod.rs index 43811ee4..dd7015fd 100644 --- a/frame/support/src/tests/mod.rs +++ b/frame/support/src/tests/mod.rs @@ -263,6 +263,23 @@ fn new_test_ext() -> TestExternalities { RuntimeGenesisConfig::default().build_storage().unwrap().into() } +#[test] +fn get_call_names_unknown_module_returns_empty_not_panic() { + use crate::traits::GetCallMetadata; + + // A known module resolves to its call names. + let modules = ::get_module_names(); + assert!(modules.contains(&"System")); + assert!(!::get_call_names("System").is_empty()); + + // `get_call_names` is public metadata API reachable with caller-supplied strings; an + // unknown module must be a recoverable empty result, not a panic. + assert_eq!( + ::get_call_names("DefinitelyMissingPallet"), + &[] as &[&str], + ); +} + trait Sorted { fn sorted(self) -> Self; } From 78bbad282ebdade7dfd67e8921d170289d051ab5 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 18:27:50 +0800 Subject: [PATCH 16/47] Stream task enumeration lazily instead of eagerly collecting into vectors Both the pallet-level Task::iter and the aggregated RuntimeTask::iter collected every task into heap vectors before yielding the first item. Task lists can be derived from attacker-growable storage, so eager materialization allowed unbounded allocation in off-chain workers. Co-authored-by: Cursor --- .../src/construct_runtime/expand/task.rs | 16 ++++-- .../src/pallet/expand/tasks.rs | 19 ++++--- frame/support/src/tests/mod.rs | 52 +++++++++++++++++++ 3 files changed, 77 insertions(+), 10 deletions(-) diff --git a/frame/support-procedural/src/construct_runtime/expand/task.rs b/frame/support-procedural/src/construct_runtime/expand/task.rs index b1cf4f85..6dc16b2d 100644 --- a/frame/support-procedural/src/construct_runtime/expand/task.rs +++ b/frame/support-procedural/src/construct_runtime/expand/task.rs @@ -98,7 +98,9 @@ pub fn expand_outer_task( #[automatically_derived] impl #scrate::traits::Task for RuntimeTask { - type Enumeration = #prelude::IntoIter; + type Enumeration = #scrate::__private::Box< + dyn #prelude::Iterator + >; fn is_valid(&self) -> bool { match self { @@ -141,12 +143,18 @@ pub fn expand_outer_task( } fn iter() -> Self::Enumeration { - let mut all_tasks = Vec::new(); + // Stream the pallet task iterators lazily instead of collecting them into + // heap vectors: task sets can be derived from attacker-growable state, so + // eager materialization allows unbounded allocation before the first yield. + let mut all_tasks = + #scrate::__private::Vec::::new(); #( #cfg_attrs - all_tasks.extend(<#task_types>::iter().map(RuntimeTask::from).collect::>()); + all_tasks.push(#scrate::__private::Box::new( + <#task_types>::iter().map(RuntimeTask::from), + )); )* - all_tasks.into_iter() + #scrate::__private::Box::new(all_tasks.into_iter().flatten()) } } diff --git a/frame/support-procedural/src/pallet/expand/tasks.rs b/frame/support-procedural/src/pallet/expand/tasks.rs index 6cd2436c..f2f0f777 100644 --- a/frame/support-procedural/src/pallet/expand/tasks.rs +++ b/frame/support-procedural/src/pallet/expand/tasks.rs @@ -166,15 +166,22 @@ impl TasksDef { impl #impl_generics #frame_support::traits::Task for #enum_use { - type Enumeration = #frame_support::__private::IntoIter<#enum_use>; + type Enumeration = #frame_support::__private::Box< + dyn #frame_support::traits::tasks::__private::Iterator + >; fn iter() -> Self::Enumeration { - let mut all_tasks = #frame_support::__private::vec![]; - #(all_tasks - .extend(#task_iters.map(|(#(#task_arg_names),*)| #enum_ident::#task_fn_idents { #(#task_arg_names: #task_arg_names.clone()),* }) - .collect::<#frame_support::__private::Vec<_>>()); + // Stream the task lists lazily instead of collecting them into heap + // vectors: task lists can be derived from attacker-growable state, so + // eager materialization allows unbounded allocation before the first + // yield. + let mut all_tasks = + #frame_support::__private::Vec::::new(); + #(all_tasks.push(#frame_support::__private::Box::new( + #task_iters.map(|(#(#task_arg_names),*)| #enum_ident::#task_fn_idents { #(#task_arg_names: #task_arg_names.clone()),* }) + )); )* - all_tasks.into_iter() + #frame_support::__private::Box::new(all_tasks.into_iter().flatten()) } fn task_index(&self) -> u32 { diff --git a/frame/support/src/tests/mod.rs b/frame/support/src/tests/mod.rs index dd7015fd..21b14602 100644 --- a/frame/support/src/tests/mod.rs +++ b/frame/support/src/tests/mod.rs @@ -215,6 +215,23 @@ pub mod frame_system { #[pallet::storage] pub type Numbers = StorageMap<_, Twox64Concat, u32, u32, OptionQuery>; + #[pallet::tasks_experimental] + impl Pallet { + /// Add a number into the total and remove it. + #[pallet::task_list(Numbers::::iter_keys())] + #[pallet::task_condition(|i| Numbers::::contains_key(i))] + #[pallet::task_weight(crate::weights::Weight::zero())] + #[pallet::task_index(0)] + pub fn add_number_into_total(i: u32) -> crate::dispatch::DispatchResult { + let v = Numbers::::take(i).ok_or(Error::::InvalidTask)?; + Total::::mutate(|(total_keys, total_values)| { + *total_keys += i; + *total_values += v; + }); + Ok(()) + } + } + pub mod pallet_prelude { pub type OriginFor = ::RuntimeOrigin; @@ -263,6 +280,41 @@ fn new_test_ext() -> TestExternalities { RuntimeGenesisConfig::default().build_storage().unwrap().into() } +#[test] +fn runtime_task_enumeration_is_lazy() { + use crate::traits::Task as TaskTrait; + + new_test_ext().execute_with(|| { + for i in 0..5u32 { + frame_system::Numbers::::insert(i, i); + } + + // Create the enumeration first, then clear the backing storage. A lazy + // enumeration observes the cleared state; an eager implementation would have + // collected all five tasks into heap vectors before yielding any of them. + let mut tasks = ::iter(); + let _ = frame_system::Numbers::::clear(u32::MAX, None); + assert_eq!(tasks.next(), None); + }); +} + +#[test] +fn runtime_task_enumeration_yields_pallet_tasks() { + use crate::traits::Task as TaskTrait; + + new_test_ext().execute_with(|| { + frame_system::Numbers::::insert(7, 3); + + let tasks = ::iter().collect::>(); + assert_eq!( + tasks, + vec![RuntimeTask::System(frame_system::Task::::AddNumberIntoTotal { + i: 7 + })] + ); + }); +} + #[test] fn get_call_names_unknown_module_returns_empty_not_panic() { use crate::traits::GetCallMetadata; From 9fbbf73289cefe7394f03cdcc149a2f02bda4850 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 18:33:25 +0800 Subject: [PATCH 17/47] Initialize new-pallet storage version after migrations instead of before before_all_runtime_migrations pre-seeded the on-chain storage version for any pallet with an empty prefix, running before the runtime's custom migrations. A version-gated migration importing state into a new pallet prefix (rename/split/import) then saw the version as already current and was silently skipped, stranding state under the old namespace. The version is now initialized after the pallet's own on_runtime_upgrade, and only if the version key is still absent. Co-authored-by: Cursor --- .../src/pallet/expand/hooks.rs | 42 ++++++---- frame/support/src/tests/mod.rs | 77 +++++++++++++++++++ 2 files changed, 103 insertions(+), 16 deletions(-) diff --git a/frame/support-procedural/src/pallet/expand/hooks.rs b/frame/support-procedural/src/pallet/expand/hooks.rs index c31ddd8a..e1a2310a 100644 --- a/frame/support-procedural/src/pallet/expand/hooks.rs +++ b/frame/support-procedural/src/pallet/expand/hooks.rs @@ -214,24 +214,16 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream { for #pallet_ident<#type_use_gen> #where_clause { fn before_all_runtime_migrations() -> #frame_support::weights::Weight { - use #frame_support::traits::{Get, PalletInfoAccess}; - use #frame_support::__private::hashing::twox_128; - use #frame_support::storage::unhashed::contains_prefixed_key; #frame_support::__private::sp_tracing::enter_span!( #frame_support::__private::sp_tracing::trace_span!("before_all") ); - // Check if the pallet has any keys set, including the storage version. If there are - // no keys set, the pallet was just added to the runtime and needs to have its - // version initialized. - let pallet_hashed_prefix = ::name_hash(); - let exists = contains_prefixed_key(&pallet_hashed_prefix); - if !exists { - #initialize_on_chain_storage_version - ::DbWeight::get().reads_writes(1, 1) - } else { - ::DbWeight::get().reads(1) - } + // The on-chain storage version of a newly added pallet is initialized + // *after* all migrations ran (see `on_runtime_upgrade` below). Seeding it + // here would run before the runtime's custom migrations and cause + // version-gated migrations targeting a new (still empty) pallet prefix, + // e.g. renames or imports from another namespace, to be silently skipped. + #frame_support::weights::Weight::zero() } } @@ -240,6 +232,7 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream { for #pallet_ident<#type_use_gen> #where_clause { fn on_runtime_upgrade() -> #frame_support::weights::Weight { + use #frame_support::traits::Get; #frame_support::__private::sp_tracing::enter_span!( #frame_support::__private::sp_tracing::trace_span!("on_runtime_update") ); @@ -247,11 +240,28 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream { // log info about the upgrade. #log_runtime_upgrade - < + let mut weight = < Self as #frame_support::traits::Hooks< #frame_system::pallet_prelude::BlockNumberFor:: > - >::on_runtime_upgrade() + >::on_runtime_upgrade(); + + // If the storage-version key is still absent after every migration had a + // chance to run, the pallet is genuinely new: initialize its on-chain + // storage version. Doing this after the migrations (instead of in + // `before_all_runtime_migrations`) keeps version-gated migrations that + // target a new pallet prefix from being skipped. + if !#frame_support::traits::StorageVersion::exists::() { + #initialize_on_chain_storage_version + weight = weight.saturating_add( + ::DbWeight::get().reads_writes(1, 1) + ); + } else { + weight = weight + .saturating_add(::DbWeight::get().reads(1)); + } + + weight } #frame_support::try_runtime_enabled! { diff --git a/frame/support/src/tests/mod.rs b/frame/support/src/tests/mod.rs index 21b14602..87dda338 100644 --- a/frame/support/src/tests/mod.rs +++ b/frame/support/src/tests/mod.rs @@ -242,6 +242,27 @@ pub mod frame_system { } } +/// A minimal second pallet with a non-zero in-code storage version, used to test the +/// interaction between new-pallet storage-version initialization and version-gated +/// migrations. +#[pallet] +pub mod pallet2 { + use super::frame_system; + use crate::pallet_prelude::*; + + pub const STORAGE_VERSION: StorageVersion = StorageVersion::new(3); + + #[pallet::pallet] + #[pallet::storage_version(STORAGE_VERSION)] + pub struct Pallet(_); + + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::storage] + pub type Values = StorageMap<_, Twox64Concat, u32, u32, OptionQuery>; +} + type BlockNumber = u32; type AccountId = u32; type Header = generic::Header; @@ -267,6 +288,9 @@ mod runtime { #[runtime::pallet_index(0)] pub type System = self::frame_system; + + #[runtime::pallet_index(1)] + pub type Example2 = self::pallet2; } #[crate::derive_impl(self::frame_system::config_preludes::TestDefaultConfig as self::frame_system::DefaultConfig)] @@ -276,10 +300,63 @@ impl Config for Runtime { type ExampleConstant = (); } +impl pallet2::Config for Runtime {} + fn new_test_ext() -> TestExternalities { RuntimeGenesisConfig::default().build_storage().unwrap().into() } +#[test] +fn new_pallet_storage_version_initialized_after_migrations_not_before() { + use crate::{ + migrations::VersionedMigration, + traits::{ + BeforeAllRuntimeMigrations, GetStorageVersion, OnRuntimeUpgrade, StorageVersion, + UncheckedOnRuntimeUpgrade, + }, + weights::Weight, + }; + + /// A migration importing state into the (new) `pallet2` prefix, gated on version 0. + pub struct ImportIntoPallet2; + impl UncheckedOnRuntimeUpgrade for ImportIntoPallet2 { + fn on_runtime_upgrade() -> Weight { + pallet2::Values::::insert(1, 100); + Weight::zero() + } + } + type Migration = VersionedMigration<0, 3, ImportIntoPallet2, pallet2::Pallet, ()>; + + // Empty state: the pallet was just added to the runtime, no genesis ran for it. + TestExternalities::default().execute_with(|| { + assert!(!StorageVersion::exists::>()); + + // Mirrors `Executive::execute_on_runtime_upgrade` ordering: + // 1. `before_all_runtime_migrations` must not pre-seed the storage version, + // otherwise the version-gated migration below is silently skipped. + as BeforeAllRuntimeMigrations>::before_all_runtime_migrations(); + assert!(!StorageVersion::exists::>()); + + // 2. The custom (version-gated) migration runs and observes version 0. + Migration::on_runtime_upgrade(); + assert_eq!(pallet2::Values::::get(1), Some(100), "migration must not be skipped"); + assert_eq!(pallet2::Pallet::::on_chain_storage_version(), 3); + + // 3. The pallet's own hook must not clobber the version set by the migration. + as OnRuntimeUpgrade>::on_runtime_upgrade(); + assert_eq!(pallet2::Pallet::::on_chain_storage_version(), 3); + }); + + // A genuinely new pallet with no migration still gets its version initialized once + // all migrations had the chance to run. + TestExternalities::default().execute_with(|| { + as BeforeAllRuntimeMigrations>::before_all_runtime_migrations(); + as OnRuntimeUpgrade>::on_runtime_upgrade(); + assert!(StorageVersion::exists::>()); + assert_eq!(pallet2::Pallet::::on_chain_storage_version(), 3); + }); +} + #[test] fn runtime_task_enumeration_is_lazy() { use crate::traits::Task as TaskTrait; From a15577341df8b337015ff2eed09c3c7d1d785e8b Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 18:41:25 +0800 Subject: [PATCH 18/47] Reject trailing tokens in pallet::call attributes instead of ignoring them FunctionAttr::parse consumed only the leading value of weight/call_index/ feeless_if/authorize/weight_of_authorize attributes and returned without checking the buffers were exhausted, so malformed attributes compiled with trailing tokens silently dropped. This could silently truncate a call's weight metadata and underprice a dispatchable. Co-authored-by: Cursor --- .../src/pallet/parse/call.rs | 53 +++++++---- .../src/pallet/parse/tests/calls.rs | 92 +++++++++++++++++++ .../src/pallet/parse/tests/mod.rs | 1 + 3 files changed, 130 insertions(+), 16 deletions(-) create mode 100644 frame/support-procedural/src/pallet/parse/tests/calls.rs diff --git a/frame/support-procedural/src/pallet/parse/call.rs b/frame/support-procedural/src/pallet/parse/call.rs index 851b75e3..02642ef2 100644 --- a/frame/support-procedural/src/pallet/parse/call.rs +++ b/frame/support-procedural/src/pallet/parse/call.rs @@ -144,12 +144,26 @@ impl syn::parse::Parse for FunctionAttr { content.parse::()?; content.parse::()?; + // Reject attributes with unparsed trailing tokens instead of silently ignoring + // them: attributes like `#[pallet::weight(...)]` define security-critical dispatch + // metadata, so accepting only a leading prefix of the attribute could silently + // underprice a call. + fn ensure_empty(stream: syn::parse::ParseStream) -> syn::Result<()> { + if stream.is_empty() { + Ok(()) + } else { + Err(stream.error("unexpected trailing tokens in pallet attribute")) + } + } + let lookahead = content.lookahead1(); - if lookahead.peek(keyword::weight) { + let attr = if lookahead.peek(keyword::weight) { content.parse::()?; let weight_content; syn::parenthesized!(weight_content in content); - Ok(FunctionAttr::Weight(weight_content.parse::()?)) + let weight = weight_content.parse::()?; + ensure_empty(&weight_content)?; + FunctionAttr::Weight(weight) } else if lookahead.peek(keyword::call_index) { content.parse::()?; let call_index_content; @@ -159,33 +173,40 @@ impl syn::parse::Parse for FunctionAttr { let msg = "Number literal must not have a suffix"; return Err(syn::Error::new(index.span(), msg)); } - Ok(FunctionAttr::CallIndex(index.base10_parse()?)) + ensure_empty(&call_index_content)?; + FunctionAttr::CallIndex(index.base10_parse()?) } else if lookahead.peek(keyword::feeless_if) { content.parse::()?; let closure_content; syn::parenthesized!(closure_content in content); - Ok(FunctionAttr::FeelessIf( - closure_content.span(), - closure_content.parse::().map_err(|e| { - let msg = "Invalid feeless_if attribute: expected a closure"; - let mut err = syn::Error::new(closure_content.span(), msg); - err.combine(e); - err - })?, - )) + let closure = closure_content.parse::().map_err(|e| { + let msg = "Invalid feeless_if attribute: expected a closure"; + let mut err = syn::Error::new(closure_content.span(), msg); + err.combine(e); + err + })?; + ensure_empty(&closure_content)?; + FunctionAttr::FeelessIf(closure_content.span(), closure) } else if lookahead.peek(keyword::authorize) { content.parse::()?; let closure_content; syn::parenthesized!(closure_content in content); - Ok(FunctionAttr::Authorize(closure_content.parse::()?)) + let expr = closure_content.parse::()?; + ensure_empty(&closure_content)?; + FunctionAttr::Authorize(expr) } else if lookahead.peek(keyword::weight_of_authorize) { content.parse::()?; let closure_content; syn::parenthesized!(closure_content in content); - Ok(FunctionAttr::WeightOfAuthorize(closure_content.parse::()?)) + let expr = closure_content.parse::()?; + ensure_empty(&closure_content)?; + FunctionAttr::WeightOfAuthorize(expr) } else { - Err(lookahead.error()) - } + return Err(lookahead.error()); + }; + + ensure_empty(&content)?; + Ok(attr) } } diff --git a/frame/support-procedural/src/pallet/parse/tests/calls.rs b/frame/support-procedural/src/pallet/parse/tests/calls.rs new file mode 100644 index 00000000..320d2543 --- /dev/null +++ b/frame/support-procedural/src/pallet/parse/tests/calls.rs @@ -0,0 +1,92 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use syn::parse_quote; + +#[test] +fn test_weight_with_trailing_tokens_is_rejected() { + assert_pallet_parse_error! { + #[manifest_dir("../examples/basic")] + #[error_regex("unexpected trailing tokens in pallet attribute")] + #[frame_support::pallet] + pub mod pallet { + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::pallet] + pub struct Pallet(_); + + #[pallet::call] + impl Pallet { + #[pallet::call_index(0)] + #[pallet::weight(Weight::zero(), DispatchClass::Operational)] + pub fn noop(origin: OriginFor) -> DispatchResult { + Ok(()) + } + } + } + } +} + +#[test] +fn test_call_index_with_trailing_tokens_is_rejected() { + assert_pallet_parse_error! { + #[manifest_dir("../examples/basic")] + #[error_regex("unexpected trailing tokens in pallet attribute")] + #[frame_support::pallet] + pub mod pallet { + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::pallet] + pub struct Pallet(_); + + #[pallet::call] + impl Pallet { + #[pallet::call_index(0, 1)] + #[pallet::weight(Weight::zero())] + pub fn noop(origin: OriginFor) -> DispatchResult { + Ok(()) + } + } + } + } +} + +#[test] +fn test_well_formed_call_attributes_parse() { + assert_pallet_parses! { + #[manifest_dir("../examples/basic")] + #[frame_support::pallet] + pub mod pallet { + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::pallet] + pub struct Pallet(_); + + #[pallet::call] + impl Pallet { + #[pallet::call_index(0)] + #[pallet::weight(Weight::zero())] + pub fn noop(origin: OriginFor) -> DispatchResult { + Ok(()) + } + } + } + }; +} diff --git a/frame/support-procedural/src/pallet/parse/tests/mod.rs b/frame/support-procedural/src/pallet/parse/tests/mod.rs index 86caf1e9..ad0f3720 100644 --- a/frame/support-procedural/src/pallet/parse/tests/mod.rs +++ b/frame/support-procedural/src/pallet/parse/tests/mod.rs @@ -220,6 +220,7 @@ pub fn simulate_manifest_dir, F: FnOnce() + std::panic result.unwrap(); } +mod calls; mod tasks; #[test] From dddddb73173cef52dab4cda3230adb8282453a3c Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 18:41:31 +0800 Subject: [PATCH 19/47] Bound view-function input decoding with a mem limit and full-consumption check The shared ViewFunction::execute path used plain DecodeAll, bypassing the mem-tracked decode derived on the generated request structs. View functions are reachable via the RuntimeViewFunction runtime API with attacker-supplied input, so decoding is now bounded by MAX_VIEW_FUNCTION_DECODE_MEM to prevent decode-bomb allocation, while still requiring the whole input be consumed. Co-authored-by: Cursor --- frame/support/src/view_functions.rs | 91 ++++++++++++++++++++++++++++- 1 file changed, 88 insertions(+), 3 deletions(-) diff --git a/frame/support/src/view_functions.rs b/frame/support/src/view_functions.rs index cba451b5..259ccdcf 100644 --- a/frame/support/src/view_functions.rs +++ b/frame/support/src/view_functions.rs @@ -18,7 +18,7 @@ //! Traits for querying pallet view functions. use alloc::vec::Vec; -use codec::{Decode, DecodeAll, Encode, Output}; +use codec::{Decode, DecodeWithMemLimit, Encode, Output}; use scale_info::TypeInfo; use sp_runtime::RuntimeDebug; @@ -98,9 +98,16 @@ pub trait ViewFunctionIdSuffix { const SUFFIX: [u8; 16]; } +/// Maximum heap memory a single view-function input is allowed to allocate while decoding. +/// +/// View functions are reachable through the `RuntimeViewFunction` runtime API with +/// attacker-controlled input, so decoding is bounded to prevent a small crafted request from +/// forcing a disproportionately large allocation (a decode "bomb"). +pub const MAX_VIEW_FUNCTION_DECODE_MEM: usize = 16 * 1024 * 1024; + /// Automatically implemented for each pallet view function method by the macro /// [`pallet`](crate::pallet). -pub trait ViewFunction: DecodeAll { +pub trait ViewFunction: DecodeWithMemLimit { fn id() -> ViewFunctionId; type ReturnType: Encode; @@ -110,7 +117,13 @@ pub trait ViewFunction: DecodeAll { input: &mut &[u8], output: &mut O, ) -> Result<(), ViewFunctionDispatchError> { - let view_function = Self::decode_all(input)?; + // Use the mem-tracked decode (bounded by `MAX_VIEW_FUNCTION_DECODE_MEM`) instead of a + // plain `DecodeAll`, and still require the whole input to be consumed. + let view_function = + Self::decode_with_mem_limit(input, MAX_VIEW_FUNCTION_DECODE_MEM)?; + if !input.is_empty() { + return Err(ViewFunctionDispatchError::Codec) + } let result = view_function.invoke(); Encode::encode_to(&result, output); Ok(()) @@ -132,3 +145,75 @@ pub mod runtime_api { } } } + +#[cfg(test)] +mod tests { + use super::*; + use codec::{Decode, DecodeWithMemTracking}; + + #[derive(Encode, Decode, DecodeWithMemTracking)] + struct DoubleIt { + value: u32, + } + + impl ViewFunction for DoubleIt { + fn id() -> ViewFunctionId { + ViewFunctionId { prefix: [0u8; 16], suffix: [0u8; 16] } + } + type ReturnType = u32; + + fn invoke(self) -> Self::ReturnType { + self.value.wrapping_mul(2) + } + } + + #[test] + fn execute_decodes_invokes_and_encodes() { + let input = 21u32.encode(); + let mut output = Vec::new(); + DoubleIt::execute(&mut &input[..], &mut output).unwrap(); + assert_eq!(u32::decode(&mut &output[..]).unwrap(), 42); + } + + #[test] + fn execute_rejects_trailing_bytes() { + // A valid `u32` followed by extra trailing bytes must be rejected rather than + // silently decoded from a prefix of the input. + let mut input = 21u32.encode(); + input.extend_from_slice(&[0xff, 0xff]); + let mut output = Vec::new(); + assert!(matches!( + DoubleIt::execute(&mut &input[..], &mut output), + Err(ViewFunctionDispatchError::Codec) + )); + } + + #[test] + fn execute_bounds_decode_allocation() { + // A `Vec` view function argument whose declared length would allocate far more + // than the mem limit is rejected instead of being decoded. + #[derive(Encode, Decode, DecodeWithMemTracking)] + struct TakesVec { + data: Vec, + } + impl ViewFunction for TakesVec { + fn id() -> ViewFunctionId { + ViewFunctionId { prefix: [1u8; 16], suffix: [1u8; 16] } + } + type ReturnType = u32; + fn invoke(self) -> Self::ReturnType { + self.data.len() as u32 + } + } + + // Encode a compact length larger than the mem limit, then provide that many bytes. + let len = (MAX_VIEW_FUNCTION_DECODE_MEM + 1) as u32; + let mut input = codec::Compact(len).encode(); + input.extend(core::iter::repeat(0u8).take(len as usize)); + let mut output = Vec::new(); + assert!(matches!( + TakesVec::execute(&mut &input[..], &mut output), + Err(ViewFunctionDispatchError::Codec) + )); + } +} From 48d0b8d65ac8d87a67264299b0a1edce74b417ce Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 18:45:40 +0800 Subject: [PATCH 20/47] Make genesis field retention use serde-exact camelCase comparison InitializedField::eq approximated serde's camelCase rename with an underscore-insensitive, case-insensitive match that was not injective, so distinct genesis fields like a_b (=> aB) and ab (=> ab) compared equal and retain_initialized_fields could keep an unselected default field. The comparison now uses serde's exact camelCase rule so distinct identifiers never collide. Co-authored-by: Cursor --- frame/support/src/generate_genesis_config.rs | 157 ++++++++++++------- 1 file changed, 102 insertions(+), 55 deletions(-) diff --git a/frame/support/src/generate_genesis_config.rs b/frame/support/src/generate_genesis_config.rs index 283840d7..c54281e1 100644 --- a/frame/support/src/generate_genesis_config.rs +++ b/frame/support/src/generate_genesis_config.rs @@ -64,19 +64,52 @@ impl<'a> InitializedField<'a> { } } +/// Converts a single `snake_case` identifier segment to `camelCase`, exactly matching +/// serde's `#[serde(rename_all = "camelCase")]` field-renaming rule. +/// +/// Serde first produces `PascalCase` (uppercase the first character of every +/// underscore-separated word, dropping the underscores) and then lowercases the first +/// character of the result. +fn to_camel_case_segment(segment: &str) -> String { + let mut pascal = String::new(); + let mut capitalize = true; + for ch in segment.chars() { + if ch == '_' { + capitalize = true; + } else if capitalize { + pascal.push(ch.to_ascii_uppercase()); + capitalize = false; + } else { + pascal.push(ch); + } + } + match pascal.char_indices().nth(1) { + Some((split, _)) => pascal[..split].to_ascii_lowercase() + &pascal[split..], + None => pascal.to_ascii_lowercase(), + } +} + +/// Converts a dotted field path (e.g. `outer.inner_field`) to its `camelCase` form, +/// converting each `.`-separated segment individually. +fn to_camel_case_path(path: &str) -> String { + let mut out = String::new(); + for (i, segment) in path.split('.').enumerate() { + if i > 0 { + out.push('.'); + } + out.push_str(&to_camel_case_segment(segment)); + } + out +} + impl PartialEq for InitializedField<'_> { fn eq(&self, other: &String) -> bool { - #[inline] - /// We need to respect the `camelCase` naming for field names. This means that - /// `"camelCaseKey"` should be considered equal to `"camel_case_key"`. This - /// function implements this comparison. - fn compare_keys(ident_chars: core::str::Chars, camel_chars: core::str::Chars) -> bool { - ident_chars - .filter(|c| *c != '_') - .map(|c| c.to_ascii_uppercase()) - .eq(camel_chars.map(|c| c.to_ascii_uppercase())) - } - *self.1 == *other || compare_keys(self.1.chars(), other.chars()) + // A JSON key matches the recorded identifier either exactly (structs without a + // `rename_all` attribute serialize field names verbatim) or as its `camelCase` + // rename. The camelCase comparison uses serde's exact rule rather than an + // underscore-insensitive match, so distinct identifiers such as `a_b` (=> `aB`) + // and `ab` (=> `ab`) never compare equal and retention stays injective. + *self.1 == *other || to_camel_case_path(&self.1) == *other } } @@ -1183,53 +1216,67 @@ mod retain_keys_test { use super::*; use serde_json::json; - macro_rules! check_initialized_field_eq_cc( - ( $s:literal ) => { - let field = InitializedField::full($s); - let cc = inflector::cases::camelcase::to_camel_case($s); - assert_eq!(field,cc); - } ; - ( &[ $f:literal $(, $r:literal)* ]) => { - let field = InitializedField::full( - concat!( $f $(,".",$r)+ ) - ); - let cc = [ $f $(,$r)+ ].into_iter() - .map(|s| inflector::cases::camelcase::to_camel_case(s)) - .collect::>() - .join("."); - assert_eq!(field,cc); - } ; - ); + // Ground truth is serde's own `rename_all = "camelCase"` rule (see + // `serde_derive`'s `RenameRule::CamelCase`): PascalCase, then lowercase the first char. + #[track_caller] + fn assert_field_eq_cc(ident: &str, expected_camel: &str) { + let field = InitializedField::full(ident); + // A field is equal to its camelCase rename... + assert_eq!(field, expected_camel.to_string(), "{ident} should equal {expected_camel}"); + // ...and to its verbatim (non-renamed) form. + assert_eq!(field, ident.to_string(), "{ident} should equal itself"); + } #[test] fn test_initialized_field_eq_cc_string() { - check_initialized_field_eq_cc!("a_"); - check_initialized_field_eq_cc!("abc"); - check_initialized_field_eq_cc!("aBc"); - check_initialized_field_eq_cc!("aBC"); - check_initialized_field_eq_cc!("ABC"); - check_initialized_field_eq_cc!("2abs"); - check_initialized_field_eq_cc!("2Abs"); - check_initialized_field_eq_cc!("2ABs"); - check_initialized_field_eq_cc!("2aBs"); - check_initialized_field_eq_cc!("AlreadyCamelCase"); - check_initialized_field_eq_cc!("alreadyCamelCase"); - check_initialized_field_eq_cc!("C"); - check_initialized_field_eq_cc!("1a"); - check_initialized_field_eq_cc!("_1a"); - check_initialized_field_eq_cc!("a_b"); - check_initialized_field_eq_cc!("_a_b"); - check_initialized_field_eq_cc!("a___b"); - check_initialized_field_eq_cc!("__a_b"); - check_initialized_field_eq_cc!("_a___b_C"); - check_initialized_field_eq_cc!("__A___B_C"); - check_initialized_field_eq_cc!(&["a_b", "b_c"]); - check_initialized_field_eq_cc!(&["al_pha", "_a___b_C"]); - check_initialized_field_eq_cc!(&["al_pha_", "_a___b_C"]); - check_initialized_field_eq_cc!(&["first_field", "al_pha_", "_a___b_C"]); - check_initialized_field_eq_cc!(&["al_pha_", "__2nd_field", "_a___b_C"]); - check_initialized_field_eq_cc!(&["al_pha_", "__2nd3and_field", "_a___b_C"]); - check_initialized_field_eq_cc!(&["_a1", "_a2", "_a3_"]); + assert_field_eq_cc("a_", "a"); + assert_field_eq_cc("abc", "abc"); + assert_field_eq_cc("aBc", "aBc"); + assert_field_eq_cc("aBC", "aBC"); + assert_field_eq_cc("ABC", "aBC"); + assert_field_eq_cc("2abs", "2abs"); + assert_field_eq_cc("2Abs", "2Abs"); + assert_field_eq_cc("2ABs", "2ABs"); + assert_field_eq_cc("2aBs", "2aBs"); + assert_field_eq_cc("AlreadyCamelCase", "alreadyCamelCase"); + assert_field_eq_cc("alreadyCamelCase", "alreadyCamelCase"); + assert_field_eq_cc("C", "c"); + assert_field_eq_cc("1a", "1a"); + assert_field_eq_cc("_1a", "1a"); + assert_field_eq_cc("a_b", "aB"); + assert_field_eq_cc("_a_b", "aB"); + assert_field_eq_cc("a___b", "aB"); + assert_field_eq_cc("__a_b", "aB"); + assert_field_eq_cc("_a___b_C", "aBC"); + assert_field_eq_cc("__A___B_C", "aBC"); + assert_field_eq_cc("a_b.b_c", "aB.bC"); + assert_field_eq_cc("al_pha._a___b_C", "alPha.aBC"); + assert_field_eq_cc("al_pha_._a___b_C", "alPha.aBC"); + assert_field_eq_cc("first_field.al_pha_._a___b_C", "firstField.alPha.aBC"); + } + + #[test] + fn test_camel_case_comparison_is_injective() { + // The reported collision: `a_b` (=> `aB`) must not match the JSON key `ab`, and + // `ab` must not match the JSON key `aB`. + assert_ne!(InitializedField::full("a_b"), "ab".to_string()); + assert_ne!(InitializedField::full("ab"), "aB".to_string()); + // Sanity: each still matches its own canonical camelCase / verbatim form. + assert_eq!(InitializedField::full("a_b"), "aB".to_string()); + assert_eq!(InitializedField::full("ab"), "ab".to_string()); + } + + #[test] + fn retain_does_not_keep_colliding_unselected_field() { + // Serialized default object exposes both `aB` (from `a_b`) and `ab` (from `ab`). + let mut v = json!({ + "aB": 7, + "ab": true, + }); + // Only `a_b` was initialized. + retain_initialized_fields(&mut v, &[InitializedField::full("a_b")], String::default()); + // The unrelated `ab` field must be pruned, not retained by an ambiguous match. + assert_eq!(v, json!({ "aB": 7 })); } #[test] From 2da9c25fe0c939794fea53b71fa6e20d909b9440 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 18:46:56 +0800 Subject: [PATCH 21/47] Clear undecodable explicit child-storage entries in take() child::take conditioned key removal on a successful typed decode, so a corrupted (undecodable) explicit entry returned None but was left in storage. This broke the take_or*/take_or_default/take_or_else contract that no explicit entry remains on return. take() now removes any explicit entry, decodable or not. Co-authored-by: Cursor --- frame/support/src/storage/child.rs | 58 +++++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/frame/support/src/storage/child.rs b/frame/support/src/storage/child.rs index 7109e921..7e3f0a0f 100644 --- a/frame/support/src/storage/child.rs +++ b/frame/support/src/storage/child.rs @@ -80,8 +80,13 @@ pub fn put(child_info: &ChildInfo, key: &[u8], value: &T) { /// Remove `key` from storage, returning its value if it had an explicit entry or `None` otherwise. pub fn take(child_info: &ChildInfo, key: &[u8]) -> Option { + // Remove any explicit entry, even one whose bytes fail to decode (in which case `get` + // returns `None`). Conditioning the removal on a successful decode would leave a + // corrupted entry in place and break the `take_or*` contract that no explicit entry + // remains on return. + let had_explicit_entry = exists(child_info, key); let r = get(child_info, key); - if r.is_some() { + if had_explicit_entry { kill(child_info, key); } r @@ -237,3 +242,54 @@ pub fn len(child_info: &ChildInfo, key: &[u8]) -> Option { }, } } + +#[cfg(test)] +mod tests { + use super::*; + use sp_io::TestExternalities; + + #[test] + fn take_removes_undecodable_explicit_entry() { + TestExternalities::new_empty().execute_with(|| { + let child_info = ChildInfo::new_default(b"test-child"); + let key = b"slot"; + // `[0x02]` is not a valid SCALE `bool` (only `0x00`/`0x01` decode). + put_raw(&child_info, key, &[0x02]); + assert!(exists(&child_info, key)); + + // `take` on a corrupted entry returns `None` (decode fails) but must still + // clear the explicit entry. + let taken = take::(&child_info, key); + assert_eq!(taken, None); + assert!(!exists(&child_info, key), "corrupt explicit entry must be removed by take"); + }); + } + + #[test] + fn take_or_clears_corrupt_entry_and_returns_default() { + TestExternalities::new_empty().execute_with(|| { + let child_info = ChildInfo::new_default(b"test-child"); + let key = b"one-shot"; + put_raw(&child_info, key, &[0x02]); + + // `take_or` must honor its contract: no explicit entry remains afterwards. + assert!(!take_or::(&child_info, key, false)); + assert!(!exists(&child_info, key)); + // A subsequent emptiness check now sees the slot as free. + assert!(!exists(&child_info, key)); + }); + } + + #[test] + fn take_still_returns_and_clears_valid_entry() { + TestExternalities::new_empty().execute_with(|| { + let child_info = ChildInfo::new_default(b"test-child"); + let key = b"slot"; + put(&child_info, key, &123u32); + assert_eq!(take::(&child_info, key), Some(123)); + assert!(!exists(&child_info, key)); + // Taking an absent key returns `None` and remains absent. + assert_eq!(take::(&child_info, key), None); + }); + } +} From a786dcf955f127c76b335f2391ecf3c95b1e39f5 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 18:49:07 +0800 Subject: [PATCH 22/47] Account for overlay-only removals in child::clear_storage counters The legacy storage_kill host call only reports backend deletions, so overlay-resident child keys it also deletes were reported as zero unique/ loops. Callers using those counters for weight or cleanup accounting could be told no work occurred. clear_storage now counts visible keys before and after and folds overlay-only removals into unique/loops. Co-authored-by: Cursor --- frame/support/src/storage/child.rs | 48 +++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/frame/support/src/storage/child.rs b/frame/support/src/storage/child.rs index 7e3f0a0f..4d09f628 100644 --- a/frame/support/src/storage/child.rs +++ b/frame/support/src/storage/child.rs @@ -186,6 +186,25 @@ pub fn clear_storage( maybe_limit: Option, _maybe_cursor: Option<&[u8]>, ) -> MultiRemovalResults { + // The legacy `storage_kill` host function only reports keys removed from the backend; it + // also deletes overlay-resident keys (those written earlier in the current block) but does + // not count them. Count the visible keys before and after so those overlay-only removals + // are still reflected in `unique`/`loops`, which callers may use for weight or cleanup + // accounting. + fn key_count(child_info: &ChildInfo) -> u32 { + let mut count: u32 = if exists(child_info, &[]) { 1 } else { 0 }; + let mut previous_key = Vec::new(); + while let Some(next_key) = + sp_io::default_child_storage::next_key(child_info.storage_key(), &previous_key) + { + count = count.saturating_add(1); + previous_key = next_key; + } + count + } + + let keys_before = key_count(child_info); + // TODO: Once the network has upgraded to include the new host functions, this code can be // enabled. // sp_io::default_child_storage::storage_kill(prefix, maybe_limit, maybe_cursor) @@ -198,7 +217,14 @@ pub fn clear_storage( AllRemoved(db) => (None, db), SomeRemaining(db) => (Some(child_info.storage_key().to_vec()), db), }; - MultiRemovalResults { maybe_cursor, backend, unique: backend, loops: backend } + + // Overlay-only removals = total keys deleted (before - after) minus those the host call + // already accounted for in `backend`. + let keys_after = key_count(child_info); + let overlay = keys_before.saturating_sub(keys_after).saturating_sub(backend); + let total = backend.saturating_add(overlay); + + MultiRemovalResults { maybe_cursor, backend, unique: total, loops: total } } /// Ensure `key` has no explicit entry in storage. @@ -280,6 +306,26 @@ mod tests { }); } + #[test] + fn clear_storage_counts_overlay_only_removals() { + TestExternalities::new_empty().execute_with(|| { + let child_info = ChildInfo::new_default(b"overlay-child"); + // Stage several keys in the overlay within this block. + for i in 0u8..5 { + put_raw(&child_info, &[i], &[i]); + } + + // Even with a backend limit of zero, the overlay keys are deleted and must be + // reported in `unique`/`loops` instead of being counted as zero work. + let res = clear_storage(&child_info, Some(0), None); + assert_eq!(res.unique, 5); + assert_eq!(res.loops, 5); + for i in 0u8..5 { + assert!(!exists(&child_info, &[i])); + } + }); + } + #[test] fn take_still_returns_and_clears_valid_entry() { TestExternalities::new_empty().execute_with(|| { From 62129442e88db6c7942a5c2e55548dd96bd339a0 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 18:50:41 +0800 Subject: [PATCH 23/47] Reject overlapping prefixes in move_prefix to avoid self-reprocessing move_prefix only guarded the from==to case. If to_prefix nests inside from_prefix (or vice versa), keys written during the drain fall back inside the iterator's traversal domain, causing the move to reprocess its own output (unbounded work) or terminate leaving values under the source prefix. The prefixes must now be disjoint (neither a prefix of the other). Co-authored-by: Cursor --- frame/support/src/storage/migration.rs | 31 ++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/frame/support/src/storage/migration.rs b/frame/support/src/storage/migration.rs index 67bccfb2..3af7baa9 100644 --- a/frame/support/src/storage/migration.rs +++ b/frame/support/src/storage/migration.rs @@ -416,6 +416,17 @@ pub fn move_prefix_bounded( return MovePrefixResult { moved: 0, maybe_cursor: None } } + // The source and destination prefixes must be disjoint. If either is a prefix of the + // other, the destination keys written during the drain can fall back inside the source + // traversal domain, causing the move to reprocess its own output (unbounded work) or to + // terminate leaving values under the source prefix (silent partial migration). A bad + // prefix pair is a migration-author error, so fail loudly rather than corrupt storage. + assert!( + !to_prefix.starts_with(from_prefix) && !from_prefix.starts_with(to_prefix), + "move_prefix: `from_prefix` and `to_prefix` must be disjoint (neither may be a prefix \ + of the other), otherwise the move would overlap its own traversal domain", + ); + let previous_key = maybe_cursor.map(|c| c.to_vec()).unwrap_or_else(|| from_prefix.to_vec()); let mut iter = PrefixIterator::<(Vec, Vec)>::new( from_prefix.to_vec(), @@ -511,6 +522,26 @@ mod tests { }) } + #[test] + #[should_panic(expected = "must be disjoint")] + fn move_prefix_rejects_destination_nested_in_source() { + TestExternalities::new_empty().execute_with(|| { + // `to_prefix` starts with `from_prefix`: moved keys would re-enter the source + // traversal domain and get reprocessed. This must be rejected. + crate::storage::unhashed::put_raw(b"aab", b"v"); + move_prefix(b"a", b"aa"); + }) + } + + #[test] + #[should_panic(expected = "must be disjoint")] + fn move_prefix_rejects_source_nested_in_destination() { + TestExternalities::new_empty().execute_with(|| { + crate::storage::unhashed::put_raw(b"aab", b"v"); + move_prefix(b"aa", b"a"); + }) + } + #[test] fn test_move_prefix_bounded_moves_in_chunks() { TestExternalities::new_empty().execute_with(|| { From a643ba379b9b393c721dcfe164bd6155cfae08d8 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 18:59:58 +0800 Subject: [PATCH 24/47] Enforce bounded try_append against effective query default The bounded try_append helpers treated a missing decode_len as an empty collection. For ValueQuery storage with a non-empty OnEmpty default, the effective value may already be at capacity, so appends could bypass the bound (and the default's membership assumptions). StorageTryAppend now exposes the value length, and all try_append paths (value, map, double map, n-map, counted map) fall back to the effective query-kind default when no explicit entry exists. Co-authored-by: Cursor --- frame/support/src/storage/bounded_vec.rs | 4 + frame/support/src/storage/mod.rs | 119 +++++++++++++++++- .../support/src/storage/types/counted_map.rs | 36 +++++- frame/support/src/storage/weak_bounded_vec.rs | 4 + 4 files changed, 158 insertions(+), 5 deletions(-) diff --git a/frame/support/src/storage/bounded_vec.rs b/frame/support/src/storage/bounded_vec.rs index 06c58915..dc13116e 100644 --- a/frame/support/src/storage/bounded_vec.rs +++ b/frame/support/src/storage/bounded_vec.rs @@ -30,6 +30,10 @@ impl> StorageTryAppend for BoundedVec { fn bound() -> usize { S::get() as usize } + + fn len(value: &Self) -> usize { + value.len() + } } #[cfg(test)] diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 61939256..22cd6e5f 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -1583,6 +1583,33 @@ impl StorageAppend for Digest {} /// This trait is sealed. pub trait StorageTryAppend: StorageDecodeLength + private::Sealed { fn bound() -> usize; + + /// The number of items in `value`. + /// + /// Used by the bounded `try_append` implementations to enforce the bound against the + /// effective query value (e.g. a non-empty `OnEmpty` default) when no explicit entry is + /// present in storage and `decode_len` therefore returns `None`. + fn len(value: &Self) -> usize; +} + +/// Compute the length of the *effective* value of a bounded storage entry that has no explicit +/// (decodable) trie entry, given the query-kind conversions of its storage type. +/// +/// A missing explicit entry is not necessarily an empty collection: `ValueQuery` storage falls +/// back to its `OnEmpty` default, which may be non-empty or even at capacity. Bounded appends +/// must enforce their bound against that effective default instead of assuming zero. +fn effective_absent_len( + from_optional_value_to_query: FromOptional, + from_query_to_optional_value: ToOptional, +) -> usize +where + T: StorageTryAppend, + FromOptional: FnOnce(Option) -> Query, + ToOptional: FnOnce(Query) -> Option, +{ + from_query_to_optional_value(from_optional_value_to_query(None)) + .map(|value| >::len(&value)) + .unwrap_or(0) } /// Storage value that is capable of [`StorageTryAppend`]. @@ -1601,7 +1628,15 @@ where { fn try_append>(item: LikeI) -> Result<(), ()> { let bound = T::bound(); - let current = Self::decode_len().unwrap_or_default(); + // `decode_len` only sees explicitly stored entries; with no (decodable) entry the + // effective value is the query-kind default, which may be non-empty. Enforce the + // bound against that effective value instead of assuming it is empty. + let current = Self::decode_len().unwrap_or_else(|| { + effective_absent_len::( + Self::from_optional_value_to_query, + Self::from_query_to_optional_value, + ) + }); if current < bound { // NOTE: we cannot reuse the implementation for `Vec` here because we never want to // mark `BoundedVec` as `StorageAppend`. @@ -1637,7 +1672,14 @@ where item: LikeI, ) -> Result<(), ()> { let bound = T::bound(); - let current = Self::decode_len(key.clone()).unwrap_or_default(); + // See `TryAppendValue::try_append`: a missing entry may still have a non-empty + // query-kind default, so the bound is enforced against the effective value. + let current = Self::decode_len(key.clone()).unwrap_or_else(|| { + effective_absent_len::( + Self::from_optional_value_to_query, + Self::from_query_to_optional_value, + ) + }); if current < bound { let key = Self::storage_map_final_key(key); sp_io::storage::append(&key, item.encode()); @@ -1682,7 +1724,14 @@ where item: LikeI, ) -> Result<(), ()> { let bound = T::bound(); - let current = Self::decode_len(key1.clone(), key2.clone()).unwrap_or_default(); + // See `TryAppendValue::try_append`: a missing entry may still have a non-empty + // query-kind default, so the bound is enforced against the effective value. + let current = Self::decode_len(key1.clone(), key2.clone()).unwrap_or_else(|| { + effective_absent_len::( + Self::from_optional_value_to_query, + Self::from_query_to_optional_value, + ) + }); if current < bound { let double_map_key = Self::storage_double_map_final_key(key1, key2); sp_io::storage::append(&double_map_key, item.encode()); @@ -1722,7 +1771,14 @@ where item: LikeI, ) -> Result<(), ()> { let bound = T::bound(); - let current = Self::decode_len(key.clone()).unwrap_or_default(); + // See `TryAppendValue::try_append`: a missing entry may still have a non-empty + // query-kind default, so the bound is enforced against the effective value. + let current = Self::decode_len(key.clone()).unwrap_or_else(|| { + effective_absent_len::( + Self::from_optional_value_to_query, + Self::from_query_to_optional_value, + ) + }); if current < bound { let key = Self::storage_n_map_final_key::(key); sp_io::storage::append(&key, item.encode()); @@ -2098,6 +2154,61 @@ mod test { }); } + pub struct FullDefault; + impl crate::traits::Get>> for FullDefault { + fn get() -> BoundedVec> { + vec![1, 2, 3].try_into().expect("fits the bound") + } + } + + struct WithDefaultPrefix; + impl crate::traits::StorageInstance for WithDefaultPrefix { + const STORAGE_PREFIX: &'static str = "with_default"; + fn pallet_prefix() -> &'static str { + "test" + } + } + + // `#[storage_alias]` does not support an `OnEmpty` parameter, so define these directly. + type FooWithDefault = types::StorageValue< + WithDefaultPrefix, + BoundedVec>, + types::ValueQuery, + FullDefault, + >; + type FooMapWithDefault = types::StorageMap< + WithDefaultPrefix, + Twox128, + u32, + BoundedVec>, + types::ValueQuery, + FullDefault, + >; + + #[test] + fn try_append_respects_non_empty_default_of_missing_entry() { + TestExternalities::default().execute_with(|| { + // No explicit entry exists, but the effective `ValueQuery` value is the full + // `OnEmpty` default: `try_append` must respect the bound of that effective value + // instead of treating the missing entry as an empty collection. + assert_eq!(FooWithDefault::get().len(), 3); + assert!(FooWithDefault::try_append(4).is_err()); + + assert_eq!(FooMapWithDefault::get(1).len(), 3); + assert!(FooMapWithDefault::try_append(1, 4).is_err()); + + // Explicitly stored values below the bound can still be appended to. + let bounded: BoundedVec> = vec![1].try_into().unwrap(); + FooWithDefault::put(bounded.clone()); + assert_ok!(FooWithDefault::try_append(2)); + assert_eq!(FooWithDefault::get().into_inner(), vec![1, 2]); + + FooMapWithDefault::insert(1, bounded); + assert_ok!(FooMapWithDefault::try_append(1, 2)); + assert_eq!(FooMapWithDefault::get(1).into_inner(), vec![1, 2]); + }); + } + #[test] fn try_append_works() { TestExternalities::default().execute_with(|| { diff --git a/frame/support/src/storage/types/counted_map.rs b/frame/support/src/storage/types/counted_map.rs index 872c6679..c136226b 100644 --- a/frame/support/src/storage/types/counted_map.rs +++ b/frame/support/src/storage/types/counted_map.rs @@ -410,7 +410,15 @@ where Value: StorageTryAppend, { let bound = Value::bound(); - let current = ::Map::decode_len(Ref::from(&key)).unwrap_or_default(); + // See `TryAppendValue::try_append`: a missing entry may still have a non-empty + // query-kind default, so the bound is enforced against the effective value. + let current = + ::Map::decode_len(Ref::from(&key)).unwrap_or_else(|| { + crate::storage::effective_absent_len::( + QueryKind::from_optional_value_to_query, + QueryKind::from_query_to_optional_value, + ) + }); if current < bound { CounterFor::::mutate(|value| value.saturating_inc()); let key = ::Map::hashed_key_for(key); @@ -1129,6 +1137,32 @@ mod test { }) } + #[test] + fn try_append_respects_non_empty_default_of_missing_entry() { + struct FullBoundedDefault; + impl crate::traits::Get>> for FullBoundedDefault { + fn get() -> BoundedVec> { + vec![1, 2, 3].try_into().expect("fits the bound") + } + } + type B = CountedStorageMap< + Prefix, + Twox64Concat, + u16, + BoundedVec>, + ValueQuery, + FullBoundedDefault, + >; + + TestExternalities::default().execute_with(|| { + // No explicit entry exists, but the effective `ValueQuery` value is the full + // `OnEmpty` default: appending must be rejected and the counter untouched. + assert_eq!(B::get(0).len(), 3); + assert!(B::try_append(0, 4).is_err()); + assert_eq!(B::count(), 0); + }) + } + #[test] fn migrate_keys_works() { type A = CountedStorageMap; diff --git a/frame/support/src/storage/weak_bounded_vec.rs b/frame/support/src/storage/weak_bounded_vec.rs index 41d27b51..cf9d6cd8 100644 --- a/frame/support/src/storage/weak_bounded_vec.rs +++ b/frame/support/src/storage/weak_bounded_vec.rs @@ -30,6 +30,10 @@ impl> StorageTryAppend for WeakBoundedVec { fn bound() -> usize { S::get() as usize } + + fn len(value: &Self) -> usize { + value.len() + } } #[cfg(test)] From acb12854fdc266be4048f6b0f088501d571194e3 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 19:03:16 +0800 Subject: [PATCH 25/47] Deduplicate BTreeSet storage appends StorageAppend is now a behavior trait instead of a pure marker. Vec and Digest keep the raw sp_io append, but BTreeSet decodes the stored set, inserts the item, and writes the canonical representation back. Raw appends could previously store duplicate entries that a decoded set silently deduplicates, letting repeated appends of the same element grow state and decode cost without changing the logical set. Co-authored-by: Cursor --- .../src/storage/generator/double_map.rs | 2 +- frame/support/src/storage/generator/map.rs | 2 +- frame/support/src/storage/generator/nmap.rs | 2 +- frame/support/src/storage/generator/value.rs | 2 +- frame/support/src/storage/mod.rs | 51 ++++++++++++++++--- 5 files changed, 49 insertions(+), 10 deletions(-) diff --git a/frame/support/src/storage/generator/double_map.rs b/frame/support/src/storage/generator/double_map.rs index a9116f1f..a60a00d9 100644 --- a/frame/support/src/storage/generator/double_map.rs +++ b/frame/support/src/storage/generator/double_map.rs @@ -318,7 +318,7 @@ where V: StorageAppend, { let final_key = Self::storage_double_map_final_key(k1, k2); - sp_io::storage::append(&final_key, item.encode()); + V::append(&final_key, item); } fn migrate_keys< diff --git a/frame/support/src/storage/generator/map.rs b/frame/support/src/storage/generator/map.rs index 2d1f6c9f..041c28e1 100644 --- a/frame/support/src/storage/generator/map.rs +++ b/frame/support/src/storage/generator/map.rs @@ -295,7 +295,7 @@ impl> storage::StorageMap V: StorageAppend, { let key = Self::storage_map_final_key(key); - sp_io::storage::append(&key, item.encode()); + V::append(&key, item); } fn migrate_key>(key: KeyArg) -> Option { diff --git a/frame/support/src/storage/generator/nmap.rs b/frame/support/src/storage/generator/nmap.rs index 9083aba9..f007e868 100755 --- a/frame/support/src/storage/generator/nmap.rs +++ b/frame/support/src/storage/generator/nmap.rs @@ -287,7 +287,7 @@ where V: StorageAppend, { let final_key = Self::storage_n_map_final_key::(key); - sp_io::storage::append(&final_key, item.encode()); + V::append(&final_key, item); } fn migrate_keys(key: KArg, hash_fns: K::HArg) -> Option diff --git a/frame/support/src/storage/generator/value.rs b/frame/support/src/storage/generator/value.rs index 21166b39..aa85d881 100644 --- a/frame/support/src/storage/generator/value.rs +++ b/frame/support/src/storage/generator/value.rs @@ -156,6 +156,6 @@ impl> storage::StorageValue for G { T: StorageAppend, { let key = Self::storage_value_final_key(); - sp_io::storage::append(&key, item.encode()); + T::append(&key, item); } } diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 22cd6e5f..a63ae708 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -1465,10 +1465,19 @@ pub trait StoragePrefixedMap { } } -/// Marker trait that will be implemented for types that support the `storage::append` api. +/// Trait that will be implemented for types that support the `storage::append` api. /// /// This trait is sealed. -pub trait StorageAppend: private::Sealed {} +pub trait StorageAppend: private::Sealed { + /// Append `item` to the storage entry at `key`. + /// + /// The default implementation performs a raw SCALE append, which is only sound for + /// sequence-like encodings whose raw representation cannot violate the container's + /// invariants (e.g. `Vec`). Containers with uniqueness invariants must override this. + fn append>(key: &[u8], item: EncodeLikeItem) { + sp_io::storage::append(key, item.encode()); + } +} /// It is expected that the length is at the beginning of the encoded object /// and that the length is a `Compact`. @@ -1562,7 +1571,20 @@ mod private { impl StorageAppend for Vec {} impl StorageDecodeLength for Vec {} -impl StorageAppend for BTreeSet {} +impl StorageAppend for BTreeSet { + fn append>(key: &[u8], item: EncodeLikeItem) { + // A raw SCALE append can store duplicate raw entries that a decoded `BTreeSet` + // silently deduplicates, letting repeated appends of the same element grow state and + // decode cost without changing the logical set. Instead, decode the stored set, + // insert the item, and write the canonical deduplicated representation back. + let mut value: Self = unhashed::get(key).unwrap_or_default(); + let item = item + .using_encoded(|mut encoded| T::decode(&mut encoded)) + .expect("an `EncodeLike` value must encode to a decodable `T`; qed"); + value.insert(item); + unhashed::put(key, &value); + } +} impl StorageDecodeNonDedupLength for BTreeSet {} // Blanket implementation StorageDecodeNonDedupLength for all types that are StorageDecodeLength. @@ -2316,6 +2338,20 @@ mod test { }); } + #[test] + fn btree_set_append_deduplicates() { + TestExternalities::default().execute_with(|| { + FooSet::append(4); + FooSet::append(4); // duplicate value + FooSet::append(5); + + // The append path canonicalizes the set: the duplicate raw entry is not stored, so + // repeated appends of the same element cannot grow state. + assert_eq!(FooSet::decode_non_dedup_len().unwrap(), 2); + assert_eq!(FooSet::get().unwrap(), BTreeSet::from([4, 5])); + }); + } + #[docify::export] #[test] fn btree_set_decode_non_dedup_len() { @@ -2323,13 +2359,16 @@ mod test { type Store = StorageValue>; TestExternalities::default().execute_with(|| { - Store::append(4); - Store::append(4); // duplicate value - Store::append(5); + // A raw (non-canonical) encoding, e.g. written by a legacy runtime, can contain + // duplicate entries that a decoded `BTreeSet` deduplicates. + sp_io::storage::append(&Store::hashed_key(), 4u32.encode()); + sp_io::storage::append(&Store::hashed_key(), 4u32.encode()); // duplicate value + sp_io::storage::append(&Store::hashed_key(), 5u32.encode()); let length_with_dup_items = 3; assert_eq!(Store::decode_non_dedup_len().unwrap(), length_with_dup_items); + assert_eq!(Store::get().unwrap().len(), 2); }); } } From b862ce457a79cefa3371ca231cecec8b03cb3a58 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 19:05:32 +0800 Subject: [PATCH 26/47] Do not kill counted N-map counter after a partial clear_prefix A None cursor from clear_prefix only means the selected partial prefix is exhausted, not that the whole map is empty. Killing the counter reported a non-empty map as count()==0, letting quota/emptiness checks be bypassed. Subtract the removed entries instead. Co-authored-by: Cursor --- .../support/src/storage/types/counted_nmap.rs | 39 +++++++++++++++++-- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/frame/support/src/storage/types/counted_nmap.rs b/frame/support/src/storage/types/counted_nmap.rs index 267ffd92..e473284a 100644 --- a/frame/support/src/storage/types/counted_nmap.rs +++ b/frame/support/src/storage/types/counted_nmap.rs @@ -269,10 +269,10 @@ where Key: HasKeyPrefix, { let result = ::Map::clear_prefix(partial_key, limit, maybe_cursor); - match result.maybe_cursor { - None => CounterFor::::kill(), - Some(_) => CounterFor::::mutate(|x| x.saturating_reduce(result.unique)), - } + // A `None` cursor only means that no items remain under *this* partial prefix; other + // prefixes may still hold entries, so the counter must be decremented by the number of + // removed items rather than killed (which would report a non-empty map as empty). + CounterFor::::mutate(|x| x.saturating_reduce(result.unique)); result } @@ -710,6 +710,37 @@ mod test { } } + #[test] + fn clear_prefix_keeps_count_of_other_prefixes() { + type A = CountedStorageNMap< + Prefix, + (NMapKey, NMapKey), + u32, + OptionQuery, + >; + + let mut ext = TestExternalities::default(); + ext.execute_with(|| { + A::insert((1, 10), 100); + A::insert((1, 11), 101); + A::insert((2, 20), 200); + assert_eq!(A::count(), 3); + }); + // Commit to the backend so the legacy `kill_prefix` host function reports the + // removals in `unique`. + ext.commit_all().unwrap(); + ext.execute_with(|| { + // Clearing one partial prefix to completion (cursor `None`) must only subtract the + // removed entries, not reset the global counter: entries under other prefixes + // remain in the map. + let res = A::clear_prefix((1,), u32::MAX, None); + assert!(res.maybe_cursor.is_none()); + assert_eq!(res.unique, 2); + assert_eq!(A::count(), 1); + assert_eq!(A::get((2, 20)), Some(200)); + }); + } + #[test] fn test_1_key() { type A = CountedStorageNMap, u32, OptionQuery>; From bdec3d1fe417ac58c862c0c79cfe721ea1ae6fd4 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 19:08:32 +0800 Subject: [PATCH 27/47] Enforce MaxEncodedLen bound on WrapperKeepOpaque WrapperKeepOpaque advertised a MaxEncodedLen derived from T but stored an unbounded Vec, so attacker-controlled bytes could exceed the max_size used for bounded-storage and PoV accounting. Decode now rejects opaque data longer than T::max_encoded_len(), a checked try_from_encoded constructor is added, and the unchecked from_encoded is deprecated. Co-authored-by: Cursor --- frame/support/src/traits/misc.rs | 49 ++++++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/frame/support/src/traits/misc.rs b/frame/support/src/traits/misc.rs index 16da4558..221f2652 100644 --- a/frame/support/src/traits/misc.rs +++ b/frame/support/src/traits/misc.rs @@ -1119,11 +1119,31 @@ impl WrapperKeepOpaque { } /// Create from the given encoded `data`. + /// + /// # Warning + /// + /// This does not enforce the [`MaxEncodedLen`] bound advertised by this type: `data` longer + /// than [`Self::max_encoded_len`] produces a value that violates the bound and will fail to + /// decode again. Prefer [`Self::try_from_encoded`], which rejects oversized input. + #[deprecated(note = "use `try_from_encoded`, which enforces the `MaxEncodedLen` bound")] pub fn from_encoded(data: Vec) -> Self { Self { data, _phantom: core::marker::PhantomData } } } +impl WrapperKeepOpaque { + /// Create from the given encoded `data`, enforcing the advertised [`MaxEncodedLen`] bound. + /// + /// Returns `None` if `data` is longer than the maximum encoded length of `T`, since such a + /// value would exceed the `max_size` reported for bounded storage and PoV accounting. + pub fn try_from_encoded(data: Vec) -> Option { + (data.len() <= T::max_encoded_len()).then(|| Self { + data, + _phantom: core::marker::PhantomData, + }) + } +} + impl EncodeLike for WrapperKeepOpaque {} impl EncodeLike> for WrapperKeepOpaque {} @@ -1145,9 +1165,17 @@ impl Encode for WrapperKeepOpaque { } } -impl Decode for WrapperKeepOpaque { +impl Decode for WrapperKeepOpaque { fn decode(input: &mut I) -> Result { - Ok(Self { data: Vec::::decode(input)?, _phantom: core::marker::PhantomData }) + let data = Vec::::decode(input)?; + // Enforce the bound advertised via `MaxEncodedLen`: the opaque bytes are the encoding of + // `T`, so they can never legitimately exceed `T::max_encoded_len()`. Rejecting oversized + // input keeps decoded values (e.g. read back from storage or an extrinsic) within the + // `max_size` used for bounded-storage and PoV accounting. + if data.len() > T::max_encoded_len() { + return Err("WrapperKeepOpaque: encoded data exceeds `T::max_encoded_len()`".into()) + } + Ok(Self { data, _phantom: core::marker::PhantomData }) } fn skip(input: &mut I) -> Result<(), codec::Error> { @@ -1495,6 +1523,23 @@ mod test { WrapperOpaque::::decode(&mut &data[..]).unwrap(); } + #[test] + fn keep_opaque_wrapper_enforces_max_encoded_len() { + // The advertised bound is derived from `T`; opaque bytes longer than + // `T::max_encoded_len()` must be rejected on decode so decoded values never exceed the + // `max_size` used for bounded storage / PoV accounting. + let bound = >::max_encoded_len(); + + let oversized: Vec = vec![0x41; bound + 32]; + let encoded = oversized.encode(); + assert!(WrapperKeepOpaque::::decode(&mut &encoded[..]).is_err()); + + // The checked constructor rejects oversized bytes and accepts in-bound bytes. + assert!(WrapperKeepOpaque::::try_from_encoded(oversized).is_none()); + let within_bound = 3u32.encode(); + assert!(WrapperKeepOpaque::::try_from_encoded(within_bound).is_some()); + } + #[test] fn defensive_min_works() { assert_eq!(10, 10_u32.defensive_min(11_u32)); From e83b2f50ce6877937e1bf878a70974422c43226f Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 19:13:37 +0800 Subject: [PATCH 28/47] Drop noted preimage when scheduling fails StorePreimage::bound notes a preimage for oversized calls before the scheduler runs its fallible checks. If resolve_time or place_task failed, do_schedule/do_schedule_named returned an error without undoing the note, leaving an unowned system-requested preimage as state bloat. Both now drop the bounded call's preimage on every failure path, mirroring cancellation cleanup. Co-authored-by: Cursor --- pallets/scheduler/src/lib.rs | 39 ++++++++++++++++++++++++++++++---- pallets/scheduler/src/tests.rs | 29 +++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/pallets/scheduler/src/lib.rs b/pallets/scheduler/src/lib.rs index 55dcef8e..e0971094 100644 --- a/pallets/scheduler/src/lib.rs +++ b/pallets/scheduler/src/lib.rs @@ -754,10 +754,26 @@ impl Pallet { origin: T::PalletsOrigin, call: BoundedCallOf, ) -> Result, DispatchError> { - let when = Self::resolve_time(when)?; + // `call` may already carry a preimage noted by `StorePreimage::bound` (called by the + // scheduler entrypoints before this). If any later fallible step fails, drop that + // preimage so a failed schedule cannot leave an unowned, system-requested preimage + // behind as state bloat. This mirrors the cleanup performed on cancellation. + let when = match Self::resolve_time(when) { + Ok(when) => when, + Err(e) => { + T::Preimages::drop(&call); + return Err(e) + }, + }; let lookup_hash = call.lookup_hash(); let task = Scheduled { maybe_id: None, priority, call, origin, _phantom: PhantomData }; - let res = Self::place_task(when, task).map_err(|x| x.0)?; + let res = match Self::place_task(when, task) { + Ok(res) => res, + Err((e, task)) => { + T::Preimages::drop(&task.call); + return Err(e) + }, + }; if let Some(hash) = lookup_hash { T::Preimages::request(&hash); } @@ -826,14 +842,29 @@ impl Pallet { origin: T::PalletsOrigin, call: BoundedCallOf, ) -> Result, DispatchError> { + // See `do_schedule`: drop any preimage noted by `StorePreimage::bound` on every fallible + // path so a failed schedule cannot leave an unowned, system-requested preimage behind. if Lookup::::contains_key(id) { + T::Preimages::drop(&call); return Err(Error::::FailedToSchedule.into()); } - let when = Self::resolve_time(when)?; + let when = match Self::resolve_time(when) { + Ok(when) => when, + Err(e) => { + T::Preimages::drop(&call); + return Err(e) + }, + }; let lookup_hash = call.lookup_hash(); let task = Scheduled { maybe_id: Some(id), priority, call, origin, _phantom: Default::default() }; - let res = Self::place_task(when, task).map_err(|x| x.0)?; + let res = match Self::place_task(when, task) { + Ok(res) => res, + Err((e, task)) => { + T::Preimages::drop(&task.call); + return Err(e) + }, + }; if let Some(hash) = lookup_hash { T::Preimages::request(&hash); } diff --git a/pallets/scheduler/src/tests.rs b/pallets/scheduler/src/tests.rs index 9768af7c..7a966604 100644 --- a/pallets/scheduler/src/tests.rs +++ b/pallets/scheduler/src/tests.rs @@ -752,6 +752,35 @@ fn reschedule_named_works() { }); } +#[test] +fn failed_schedule_drops_noted_preimage() { + new_test_ext().execute_with(|| { + run_to_block(1); + + // A call large enough to be stored as a `Lookup` preimage rather than inline. + let call = + RuntimeCall::System(frame_system::Call::remark { remark: vec![0u8; 1024] }); + let bounded = Preimage::bound(call).unwrap(); + assert!(bounded.lookup_needed(), "the call must be noted as a preimage"); + // `bound` has already noted the preimage. + assert!(Preimage::have(&bounded)); + + // Scheduling at a past target fails in `resolve_time`, after the preimage was noted. + // Note: this deliberately mutates storage (it drops the preimage), so `assert_noop!` + // is not applicable here. + assert_err!( + Scheduler::do_schedule(DispatchTime::At(1), 127, root(), bounded.clone()), + Error::::TargetBlockNumberInPast, + ); + + // The failed schedule must not leave the noted preimage behind. + assert!( + !Preimage::have(&bounded), + "a failed schedule must drop the preimage noted by `bound`", + ); + }); +} + #[test] fn cancel_named_scheduling_works_with_normal_cancel() { new_test_ext().execute_with(|| { From 1faa06f4281c0b567d5382914211c05d58946be3 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 19:14:58 +0800 Subject: [PATCH 29/47] Document upgrade-instability of Callback index pair Callback stores only (pallet_index, call_index) and resolves them against the call layout present when curry runs, so persisting a caller-provided callback across a runtime upgrade can decode into a different call. No in-repo consumer currently persists such callbacks, so this documents the constraint for future StatementOracle implementations rather than changing the (unused) type's encoding. Co-authored-by: Cursor --- frame/support/src/traits/reality.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/frame/support/src/traits/reality.rs b/frame/support/src/traits/reality.rs index c73a6d69..f8ef6087 100644 --- a/frame/support/src/traits/reality.rs +++ b/frame/support/src/traits/reality.rs @@ -290,6 +290,18 @@ pub enum Statement { /// Describes the location within the runtime of a callback, along with other type information such /// as parameters passed into the callback. +/// +/// # Warning: not stable across runtime upgrades +/// +/// A `Callback` records only the raw `(pallet_index, call_index)` pair, not the pallet/call name, +/// runtime version, or metadata hash. [`Self::curry`] resolves those indexes against the call +/// layout that exists *when it runs*, not when the callback was created. If a runtime upgrade +/// reorders pallets or call variants, a stored callback can decode into a different call than +/// intended. Implementations of asynchronous consumers such as [`StatementOracle`] must therefore +/// not persist a caller-provided `Callback` across a runtime upgrade (e.g. resolve pending +/// judgements before applying an upgrade, or bind the callback to a version/identity that is +/// revalidated on resolution). Callbacks selected by trusted code within a single runtime version +/// are unaffected. #[derive( CloneNoBound, PartialEqNoBound, EqNoBound, RuntimeDebug, Encode, Decode, MaxEncodedLen, TypeInfo, )] From 0695095b57b20772fba9f2968087f4b747c34da9 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 19:18:27 +0800 Subject: [PATCH 30/47] Reject zero-cost lone freeze/hold consideration tickets LoneFreezeConsideration/LoneHoldConsideration encode no ticket state, so a zero-footprint ticket leaves no freeze/hold marker yet its drop/burn clears the whole reason bucket. A stale zero ticket could therefore thaw, release, or burn a newer paid ticket's collateral. new/update now reject a zero converted amount so every live ticket corresponds to a non-empty bucket. Co-authored-by: Cursor --- .../support/src/traits/tokens/fungible/mod.rs | 23 +++++++++--- pallets/balances/src/tests/fungible_tests.rs | 36 ++++++++++++------- 2 files changed, 43 insertions(+), 16 deletions(-) diff --git a/frame/support/src/traits/tokens/fungible/mod.rs b/frame/support/src/traits/tokens/fungible/mod.rs index 31f8e015..dd994d0e 100644 --- a/frame/support/src/traits/tokens/fungible/mod.rs +++ b/frame/support/src/traits/tokens/fungible/mod.rs @@ -359,10 +359,17 @@ impl< { fn new(who: &A, footprint: Fp) -> Result { ensure!(Fx::balance_frozen(&Rx::get(), who).is_zero(), DispatchError::Unavailable); - Fx::set_frozen(&Rx::get(), who, D::convert(footprint), Polite).map(|_| Self(PhantomData)) + // A zero-cost ticket leaves no freeze marker, so it is indistinguishable from a later + // paid ticket for the same account/reason. Since `drop` thaws the whole reason bucket, + // such a stale zero ticket could later clear a newer ticket's freeze. Reject it. + let amount = D::convert(footprint); + ensure!(!amount.is_zero(), DispatchError::Unavailable); + Fx::set_frozen(&Rx::get(), who, amount, Polite).map(|_| Self(PhantomData)) } fn update(self, who: &A, footprint: Fp) -> Result { - Fx::set_frozen(&Rx::get(), who, D::convert(footprint), Polite).map(|_| Self(PhantomData)) + let amount = D::convert(footprint); + ensure!(!amount.is_zero(), DispatchError::Unavailable); + Fx::set_frozen(&Rx::get(), who, amount, Polite).map(|_| Self(PhantomData)) } fn drop(self, who: &A) -> Result<(), DispatchError> { Fx::thaw(&Rx::get(), who).map(|_| ()) @@ -404,10 +411,18 @@ impl< { fn new(who: &A, footprint: Fp) -> Result { ensure!(F::balance_on_hold(&R::get(), who).is_zero(), DispatchError::Unavailable); - F::set_on_hold(&R::get(), who, D::convert(footprint)).map(|_| Self(PhantomData)) + // A zero-cost ticket leaves no hold marker, so it is indistinguishable from a later paid + // ticket for the same account/reason. Since `drop`/`burn` operate on the whole reason + // bucket, such a stale zero ticket could later release or burn a newer ticket's hold. + // Reject it. + let amount = D::convert(footprint); + ensure!(!amount.is_zero(), DispatchError::Unavailable); + F::set_on_hold(&R::get(), who, amount).map(|_| Self(PhantomData)) } fn update(self, who: &A, footprint: Fp) -> Result { - F::set_on_hold(&R::get(), who, D::convert(footprint)).map(|_| Self(PhantomData)) + let amount = D::convert(footprint); + ensure!(!amount.is_zero(), DispatchError::Unavailable); + F::set_on_hold(&R::get(), who, amount).map(|_| Self(PhantomData)) } fn drop(self, who: &A) -> Result<(), DispatchError> { F::release_all(&R::get(), who, BestEffort).map(|_| ()) diff --git a/pallets/balances/src/tests/fungible_tests.rs b/pallets/balances/src/tests/fungible_tests.rs index 2c27f81e..82210b61 100644 --- a/pallets/balances/src/tests/fungible_tests.rs +++ b/pallets/balances/src/tests/fungible_tests.rs @@ -806,7 +806,12 @@ fn lone_freeze_consideration_works() { >; let who = 4; - let zero_ticket = Consideration::new(&who, Footprint::from_parts(0, 0)).unwrap(); + // A zero-footprint ticket is rejected: it would leave no freeze marker and could + // later thaw a newer ticket's freeze under the same reason. + assert_noop!( + Consideration::new(&who, Footprint::from_parts(0, 0)), + sp_runtime::DispatchError::Unavailable, + ); assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 0); let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap(); @@ -818,11 +823,12 @@ fn lone_freeze_consideration_works() { let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap(); assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 4); - assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), zero_ticket); - assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 0); - - let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap(); - assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 10); + // Updating to a zero footprint is likewise rejected and leaves the freeze intact. + assert_noop!( + ticket.clone().update(&who, Footprint::from_parts(0, 0)), + sp_runtime::DispatchError::Unavailable, + ); + assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 4); let _ = ticket.drop(&who).unwrap(); assert_eq!(Balances::balance_frozen(&TestId::Foo, &who), 0); @@ -844,7 +850,12 @@ fn lone_hold_consideration_works() { >; let who = 4; - let zero_ticket = Consideration::new(&who, Footprint::from_parts(0, 0)).unwrap(); + // A zero-footprint ticket is rejected: it would leave no hold marker and could + // later release/burn a newer ticket's hold under the same reason. + assert_noop!( + Consideration::new(&who, Footprint::from_parts(0, 0)), + sp_runtime::DispatchError::Unavailable, + ); assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 0); let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap(); @@ -871,11 +882,12 @@ fn lone_hold_consideration_works() { let ticket = ticket.update(&who, Footprint::from_parts(4, 1)).unwrap(); assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 4); - assert_eq!(ticket.update(&who, Footprint::from_parts(0, 0)).unwrap(), zero_ticket); - assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 0); - - let ticket = Consideration::new(&who, Footprint::from_parts(10, 1)).unwrap(); - assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 10); + // Updating to a zero footprint is likewise rejected and leaves the hold intact. + assert_noop!( + ticket.clone().update(&who, Footprint::from_parts(0, 0)), + sp_runtime::DispatchError::Unavailable, + ); + assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 4); let _ = ticket.drop(&who).unwrap(); assert_eq!(Balances::balance_on_hold(&TestId::Foo, &who), 0); From 45ce6720754a4ae20a7305f04537cbbfebd78f83 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 19:26:19 +0800 Subject: [PATCH 31/47] fix(fungible/fungibles): enforce dest existence for OnHold transfers `transfer_on_hold` documents that in `Restriction::OnHold` mode the destination account must already exist, but the default implementation never checked this: `can_deposit(.., Extant)` only concerns issuance accounting, so a held balance could be credited to a non-existent account, creating orphan hold state with no underlying account. Both the `fungible` and `fungibles` default implementations now reject `OnHold` transfers to non-existent destinations with `TokenError::CannotCreate`. Co-authored-by: Cursor --- .../src/traits/tokens/fungible/hold.rs | 8 +++++ .../src/traits/tokens/fungibles/hold.rs | 11 ++++++ pallets/balances/src/tests/fungible_tests.rs | 36 ++++++++++++++++++- 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/frame/support/src/traits/tokens/fungible/hold.rs b/frame/support/src/traits/tokens/fungible/hold.rs index 356a2625..e692806b 100644 --- a/frame/support/src/traits/tokens/fungible/hold.rs +++ b/frame/support/src/traits/tokens/fungible/hold.rs @@ -362,6 +362,14 @@ pub trait Mutate: ensure!(amount <= have, TokenError::FundsUnavailable); } + // In `OnHold` mode the destination keeps the funds on hold and must already exist: + // `can_deposit(.., Extant)` only concerns issuance accounting, not account existence, so + // without this check a held balance could be credited to a non-existent account, leaving + // hold state with no underlying account. + if mode == OnHold { + ensure!(!Self::total_balance(dest).is_zero(), TokenError::CannotCreate); + } + // We want to make sure we can deposit the amount in advance. If we can't then something is // very wrong. ensure!(Self::can_deposit(dest, amount, Extant) == Success, TokenError::CannotCreate); diff --git a/frame/support/src/traits/tokens/fungibles/hold.rs b/frame/support/src/traits/tokens/fungibles/hold.rs index 026bfc87..ce977c01 100644 --- a/frame/support/src/traits/tokens/fungibles/hold.rs +++ b/frame/support/src/traits/tokens/fungibles/hold.rs @@ -403,6 +403,17 @@ pub trait Mutate: ensure!(amount <= have, TokenError::FundsUnavailable); } + // In `OnHold` mode the destination keeps the funds on hold and must already exist: + // `can_deposit(.., Extant)` only concerns issuance accounting, not account existence, so + // without this check a held balance could be credited to a non-existent account, leaving + // hold state with no underlying asset account. + if mode == OnHold { + ensure!( + !Self::total_balance(asset.clone(), dest).is_zero(), + TokenError::CannotCreate + ); + } + // We want to make sure we can deposit the amount in advance. If we can't then something is // very wrong. ensure!( diff --git a/pallets/balances/src/tests/fungible_tests.rs b/pallets/balances/src/tests/fungible_tests.rs index 82210b61..bd46f908 100644 --- a/pallets/balances/src/tests/fungible_tests.rs +++ b/pallets/balances/src/tests/fungible_tests.rs @@ -23,7 +23,7 @@ use frame_support::traits::{ Fortitude::{Force, Polite}, Precision::{BestEffort, Exact}, Preservation::{Expendable, Preserve, Protect}, - Restriction::Free, + Restriction::{Free, OnHold}, }, Consideration, Footprint, LinearStoragePrice, MaybeConsideration, }; @@ -280,6 +280,40 @@ fn frozen_hold_balance_cannot_be_moved_without_force() { }); } +#[test] +fn transfer_on_hold_onhold_requires_existing_dest() { + ExtBuilder::default() + .existential_deposit(1) + .monied(true) + .build_and_execute_with(|| { + assert_ok!(Balances::hold(&TestId::Foo, &1, 7)); + let dest = 99u64; // does not exist + assert_eq!(Balances::total_balance(&dest), 0); + + // `OnHold` mode keeps the funds on hold in `dest`, which must already exist. A + // non-existent destination must be rejected rather than gaining orphan hold state. + assert_noop!( + Balances::transfer_on_hold(&TestId::Foo, &1, &dest, 5, Exact, OnHold, Force), + TokenError::CannotCreate, + ); + assert_eq!(Balances::balance_on_hold(&TestId::Foo, &1), 7); + assert_eq!(Balances::total_balance(&dest), 0); + + // `Free` mode may create the destination, so it still succeeds. + assert_ok!(Balances::transfer_on_hold( + &TestId::Foo, + &1, + &dest, + 5, + Exact, + Free, + Force + )); + assert_eq!(Balances::balance_on_hold(&TestId::Foo, &1), 2); + assert_eq!(Balances::free_balance(dest), 5); + }); +} + #[test] fn transfer_and_hold() { ExtBuilder::default() From 42fd029a552df6a193d0579333010d0a6d070c2e Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 19:29:11 +0800 Subject: [PATCH 32/47] docs(frame-system): document signer-length assumption of extension weights `check_non_zero_sender()` and `check_nonce()` return constant weights while their implementations do work proportional to the encoded signer length (scanning the encoded `AccountId` and encoding it into transaction-pool dependency tags). That work is bounded by `AccountId::max_encoded_len()`, so the constant model is only sound when the weights are benchmarked against the runtime's actual `AccountId` with a maximal-length signer. Document this requirement on `WeightInfo` so runtimes with variable-length account identifiers regenerate their weights instead of reusing values benchmarked with short signers. Co-authored-by: Cursor --- pallets/frame-system/src/extensions/weights.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/pallets/frame-system/src/extensions/weights.rs b/pallets/frame-system/src/extensions/weights.rs index a9c21e7b..5e872c41 100644 --- a/pallets/frame-system/src/extensions/weights.rs +++ b/pallets/frame-system/src/extensions/weights.rs @@ -50,6 +50,16 @@ use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}; use core::marker::PhantomData; /// Weight functions needed for `frame_system_extensions`. +/// +/// # Note on signer-dependent extensions +/// +/// [`Self::check_non_zero_sender`] and [`Self::check_nonce`] model a *constant* cost, yet their +/// implementations do work proportional to the SCALE-encoded length of the signer (scanning the +/// encoded `AccountId`, encoding it into transaction-pool dependency tags). That work is bounded +/// by `AccountId::max_encoded_len()`, so the constant model is sound only if the benchmarks that +/// generate these values are run with the runtime's actual `AccountId` type using a +/// maximal-length (worst-case) signer. Runtimes with variable-length account identifiers must +/// regenerate these weights accordingly rather than reuse values benchmarked with short signers. pub trait WeightInfo { fn check_genesis() -> Weight; fn check_mortality_mortal_transaction() -> Weight; From 4bd4714f025d1e45ded2a402f141c7b95d2e47a0 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 19:34:21 +0800 Subject: [PATCH 33/47] fix(frame-system): subtract base_block from derived max_extrinsic caps `BlockWeightsBuilder::build` derived `max_extrinsic` from a class's `max_total` minus the average initialization cost and the class base extrinsic weight, but ignored `base_block`, which every block consumes before any extrinsic runs. For classes whose `max_total` equals `max_block` (e.g. the operational class from the default builders), a transaction at the advertised cap passed pool validation yet could never be included: `base_block + base_extrinsic + weight` exceeded `max_block` and overflowed into (and past) the reserved allowance, creating a valid-but-unincludable transaction class. The derivation now subtracts `base_block` as well, keeping the validation-time cap consistent with the block-building capacity model. Co-authored-by: Cursor --- .../src/extensions/check_weight.rs | 25 ++++++----- pallets/frame-system/src/limits.rs | 41 +++++++++++++++++++ 2 files changed, 55 insertions(+), 11 deletions(-) diff --git a/pallets/frame-system/src/extensions/check_weight.rs b/pallets/frame-system/src/extensions/check_weight.rs index 16522611..98675e6e 100644 --- a/pallets/frame-system/src/extensions/check_weight.rs +++ b/pallets/frame-system/src/extensions/check_weight.rs @@ -383,13 +383,13 @@ mod tests { fn operational_extrinsic_limited_by_operational_space_limit() { new_test_ext().execute_with(|| { let weights = block_weights(); - let operational_limit = weights + // The derived cap already accounts for `base_block`, the class base extrinsic weight + // and the average initialization cost, so it is the largest single-extrinsic weight + // admitted by validation. + let call_weight = weights .get(DispatchClass::Operational) - .max_total - .unwrap_or_else(|| weights.max_block); - let base_weight = weights.get(DispatchClass::Operational).base_extrinsic; - - let call_weight = operational_limit - base_weight; + .max_extrinsic + .expect("mock sets `avg_block_initialization`, so the cap is derived; qed"); let okay = DispatchInfo { call_weight, class: DispatchClass::Operational, @@ -609,14 +609,17 @@ mod tests { #[test] fn signed_ext_check_weight_works_normal_tx() { new_test_ext().execute_with(|| { - let normal_limit = normal_weight_limit(); + // The derived per-extrinsic cap accounts for `base_block`, the class base extrinsic + // weight and the average initialization cost. + let max_extrinsic = block_weights() + .get(DispatchClass::Normal) + .max_extrinsic + .expect("mock sets `avg_block_initialization`, so the cap is derived; qed"); let small = DispatchInfo { call_weight: Weight::from_parts(100, 0), ..Default::default() }; - let base_extrinsic = block_weights().get(DispatchClass::Normal).base_extrinsic; - let medium = - DispatchInfo { call_weight: normal_limit - base_extrinsic, ..Default::default() }; + let medium = DispatchInfo { call_weight: max_extrinsic, ..Default::default() }; let big = DispatchInfo { - call_weight: normal_limit - base_extrinsic + Weight::from_parts(1, 0), + call_weight: max_extrinsic + Weight::from_parts(1, 0), ..Default::default() }; let len = 0_usize; diff --git a/pallets/frame-system/src/limits.rs b/pallets/frame-system/src/limits.rs index ab5a98a6..dfa77709 100644 --- a/pallets/frame-system/src/limits.rs +++ b/pallets/frame-system/src/limits.rs @@ -414,12 +414,18 @@ impl BlockWeightsBuilder { } // compute max size of single extrinsic if let Some(init_weight) = init_cost.map(|rate| rate * weights.max_block) { + let base_block = weights.base_block; for class in DispatchClass::all() { let per_class = weights.per_class.get_mut(*class); if per_class.max_extrinsic.is_none() && init_cost.is_some() { + // Every block starts with `base_block` already consumed (in addition to the + // average initialization cost), so it must be subtracted as well; otherwise an + // extrinsic at the derived cap validates against `max_extrinsic` but can never + // be included, as `base_block + base_extrinsic + weight` exceeds `max_block`. per_class.max_extrinsic = per_class .max_total .map(|x| x.saturating_sub(init_weight)) + .map(|x| x.saturating_sub(base_block)) .map(|x| x.saturating_sub(per_class.base_extrinsic)); } } @@ -447,4 +453,39 @@ mod tests { fn default_weights_are_valid() { BlockWeights::default().validate().unwrap(); } + + #[test] + fn derived_max_extrinsic_accounts_for_base_block() { + let base_block = Weight::from_parts(100, 5); + let base_extrinsic = Weight::from_parts(10, 1); + let max_total = Weight::from_parts(1_000, 100); + let init_rate = Perbill::from_percent(1); + + let weights = BlockWeights::builder() + .base_block(base_block) + .for_class(DispatchClass::all(), |w| { + w.base_extrinsic = base_extrinsic; + }) + .for_class(DispatchClass::non_mandatory(), |w| { + w.max_total = Some(max_total); + }) + .avg_block_initialization(init_rate) + .build() + .unwrap(); + + // A block starts with `base_block` (and on average `init_rate * max_block` of + // initialization) already consumed, so an extrinsic at the derived cap must still fit + // within `max_block` on top of those. + let max_extrinsic = weights.get(DispatchClass::Normal).max_extrinsic.unwrap(); + let consumed = weights + .base_block + .saturating_add(init_rate * weights.max_block) + .saturating_add(base_extrinsic) + .saturating_add(max_extrinsic); + assert!( + consumed.all_lte(weights.max_block), + "an extrinsic at the derived cap must fit in the block: {consumed:?} > {:?}", + weights.max_block, + ); + } } From 4aba38388f4043afc82751c1260f778129112f42 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 19:37:39 +0800 Subject: [PATCH 34/47] fix(frame-system): actually drain block weight on runtime upgrades MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `set_code`, `set_code_without_checks` and `apply_authorized_upgrade` tried to consume the rest of the block by returning `max_block` as post-dispatch `actual_weight`, but `calc_actual_weight` caps the post-dispatch weight at the static pre-dispatch weight (~0.17s of ref-time), so the returned value was silently ignored and further transactions could still be included after the code update in the same block — executing old-runtime transactions on top of an accepted upgrade and letting an attacker shape the migration starting state. The upgrade dispatchables now register the remaining block weight directly via `register_extra_weight_unchecked`, so the block really is drained regardless of the static call weight. Co-authored-by: Cursor --- pallets/frame-system/src/lib.rs | 20 ++++++++++++++++++++ pallets/frame-system/src/tests.rs | 31 +++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/pallets/frame-system/src/lib.rs b/pallets/frame-system/src/lib.rs index 7de51adb..898c653e 100644 --- a/pallets/frame-system/src/lib.rs +++ b/pallets/frame-system/src/lib.rs @@ -743,6 +743,7 @@ pub mod pallet { Self::can_set_code(&code, true).into_result()?; T::OnSetCode::set_code(code)?; // consume the rest of the block to prevent further transactions + Self::drain_remaining_block_weight(); Ok(Some(T::BlockWeights::get().max_block).into()) } @@ -759,6 +760,8 @@ pub mod pallet { ensure_root(origin)?; Self::can_set_code(&code, false).into_result()?; T::OnSetCode::set_code(code)?; + // consume the rest of the block to prevent further transactions + Self::drain_remaining_block_weight(); Ok(Some(T::BlockWeights::get().max_block).into()) } @@ -912,6 +915,9 @@ pub mod pallet { }; T::OnSetCode::set_code(code)?; + // consume the rest of the block to prevent further transactions + Self::drain_remaining_block_weight(); + Ok(PostDispatchInfo { // consume the rest of the block to prevent further transactions actual_weight: Some(T::BlockWeights::get().max_block), @@ -1932,6 +1938,20 @@ impl Pallet { }); } + /// Consume all remaining weight of the current block, preventing any further transaction + /// from being included after the current one. + /// + /// Used by the runtime-upgrade dispatchables: returning `max_block` as post-dispatch + /// `actual_weight` is not enough, since post-dispatch weight is capped at the (much smaller) + /// static pre-dispatch weight by `calc_actual_weight`. Registering the missing weight + /// directly ensures the block really is drained. + fn drain_remaining_block_weight() { + let max_block = T::BlockWeights::get().max_block; + let remaining = max_block.saturating_sub(Self::block_weight().total()); + // `Operational` so the consumption also exceeds any operational `reserved` allowance. + Self::register_extra_weight_unchecked(remaining, DispatchClass::Operational); + } + /// Start the execution of a particular block. /// /// # Panics diff --git a/pallets/frame-system/src/tests.rs b/pallets/frame-system/src/tests.rs index 710879f8..c7c7d50a 100644 --- a/pallets/frame-system/src/tests.rs +++ b/pallets/frame-system/src/tests.rs @@ -659,6 +659,37 @@ fn set_code_checks_works() { } } +#[test] +fn set_code_drains_remaining_block_weight() { + struct ReadRuntimeVersion(Vec); + + impl sp_core::traits::ReadRuntimeVersion for ReadRuntimeVersion { + fn read_runtime_version( + &self, + _wasm_code: &[u8], + _ext: &mut dyn sp_externalities::Externalities, + ) -> Result, String> { + Ok(self.0.clone()) + } + } + + let version = RuntimeVersion { spec_name: "test".into(), spec_version: 2, ..Default::default() }; + let read_runtime_version = ReadRuntimeVersion(version.encode()); + + let mut ext = new_test_ext(); + ext.register_extension(sp_core::traits::ReadRuntimeVersionExt::new(read_runtime_version)); + ext.execute_with(|| { + let max_block = ::BlockWeights::get().max_block; + assert!(System::block_weight().total().all_lt(max_block)); + + // The post-dispatch `actual_weight` returned by `set_code` is capped at the static + // pre-dispatch weight, so the block must be drained by direct weight registration. + assert_ok!(System::set_code(RawOrigin::Root.into(), vec![1, 2, 3, 4])); + + assert_eq!(System::block_weight().total(), max_block); + }); +} + fn assert_runtime_updated_digest(num: usize) { assert_eq!( System::digest() From 5ad79e37e3d2f7b66d1c448564ab4e908ce83db0 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 19:50:25 +0800 Subject: [PATCH 35/47] fix(utility/wormhole): reveal as_derivative pseudonyms to the soundness counter `as_derivative` pseudonym accounts receive tracked credits but never sign an extrinsic themselves (their controller signs), so their nonce stays zero forever: every credit to them was added to `PotentialWormholeBalance` and nothing ever subtracted it. An attacker could bounce fixed capital between derivative addresses to inflate the wormhole soundness pool without bound, weakening the `TotalWormholeExits <= PotentialWormholeBalance` backstop. Mirror the multisig treatment: on the first use of a pseudonym, `as_derivative` now reveals it via a new narrow `qp_wormhole::AddressRevealer` hook (deducting its balance from the pool) and records it in `KnownDerivatives`; the runtime's `NonWormholeAccounts` then excludes known pseudonyms so later receipts are not counted either. Co-authored-by: Cursor --- Cargo.lock | 1 + pallets/multisig/src/mock.rs | 1 + .../reversible-transfers/src/tests/mock.rs | 1 + pallets/utility/Cargo.toml | 2 + pallets/utility/src/lib.rs | 38 +++++++++- pallets/utility/src/tests.rs | 50 +++++++++++++ pallets/wormhole/src/lib.rs | 10 +++ primitives/wormhole/src/lib.rs | 14 ++++ runtime/src/configs/mod.rs | 8 +++ runtime/src/transaction_extensions.rs | 71 +++++++++++++++++++ 10 files changed, 195 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index ab3fbabd..1fd6e8b8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6815,6 +6815,7 @@ dependencies = [ "pallet-timestamp", "parity-scale-codec", "qp-high-security", + "qp-wormhole", "scale-info", "sp-core", "sp-io", diff --git a/pallets/multisig/src/mock.rs b/pallets/multisig/src/mock.rs index fe2066ee..63bcb79b 100644 --- a/pallets/multisig/src/mock.rs +++ b/pallets/multisig/src/mock.rs @@ -374,6 +374,7 @@ impl pallet_utility::Config for Test { type PalletsOrigin = OriginCaller; type WeightInfo = (); type HighSecurity = (); + type AddressRevealer = (); } pub fn new_test_ext() -> sp_io::TestExternalities { diff --git a/pallets/reversible-transfers/src/tests/mock.rs b/pallets/reversible-transfers/src/tests/mock.rs index bd2f4e5b..1daab458 100644 --- a/pallets/reversible-transfers/src/tests/mock.rs +++ b/pallets/reversible-transfers/src/tests/mock.rs @@ -366,6 +366,7 @@ impl pallet_utility::Config for Test { type PalletsOrigin = OriginCaller; type WeightInfo = (); type HighSecurity = (); + type AddressRevealer = (); } // Build genesis storage according to the mock runtime. diff --git a/pallets/utility/Cargo.toml b/pallets/utility/Cargo.toml index 7be28a14..916aaac0 100644 --- a/pallets/utility/Cargo.toml +++ b/pallets/utility/Cargo.toml @@ -21,6 +21,7 @@ frame-benchmarking = { optional = true, workspace = true } frame-support.workspace = true frame-system.workspace = true qp-high-security = { path = "../../primitives/high-security", default-features = false } +qp-wormhole = { workspace = true, default-features = false } scale-info = { features = ["derive"], workspace = true } sp-core.workspace = true sp-io.workspace = true @@ -46,6 +47,7 @@ std = [ "frame-support/std", "frame-system/std", "qp-high-security/std", + "qp-wormhole/std", "scale-info/std", "sp-core/std", "sp-io/std", diff --git a/pallets/utility/src/lib.rs b/pallets/utility/src/lib.rs index 6152de57..207d1706 100644 --- a/pallets/utility/src/lib.rs +++ b/pallets/utility/src/lib.rs @@ -114,8 +114,22 @@ pub mod pallet { Self::AccountId, ::RuntimeCall, >; + + /// Handle into the wormhole soundness counter. An `as_derivative` pseudonym spends + /// through its controller and never signs an extrinsic itself, so it would otherwise stay + /// "ambiguous" (nonce 0) forever while receiving tracked credits. On first use of a + /// pseudonym it is revealed here (deducting its balance from the potential wormhole + /// pool) and remembered in [`KnownDerivatives`], so it can be excluded from the ambiguous + /// set afterwards. + type AddressRevealer: qp_wormhole::AddressRevealer; } + /// Derivative pseudonym accounts that have been used with [`Pallet::as_derivative`] at least + /// once and have therefore been revealed to the wormhole soundness counter. + #[pallet::storage] + pub type KnownDerivatives = + StorageMap<_, Blake2_128Concat, T::AccountId, (), OptionQuery>; + #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event { @@ -267,6 +281,9 @@ pub mod pallet { T::WeightInfo::as_derivative() // AccountData for inner call origin accountdata. .saturating_add(T::DbWeight::get().reads_writes(1, 1)) + // First-use derivative reveal: `KnownDerivatives` read + insert and the + // wormhole pool write. + .saturating_add(T::DbWeight::get().reads_writes(1, 2)) .saturating_add(dispatch_info.call_weight), dispatch_info.class, ) @@ -283,12 +300,25 @@ pub mod pallet { T::HighSecurity::is_call_allowed(&pseudonym, &call), Error::::CallNotAllowedForHighSecurity ); + // A pseudonym spends via its controller and never signs itself, so it never reveals + // itself to the wormhole soundness counter the way regular accounts do on their first + // signature. Reveal it on first use here (deducting its balance from the potential + // wormhole pool) and remember it so the runtime can exclude it from the ambiguous set + // afterwards; otherwise funds bounced between derivative pseudonyms would inflate the + // pool without bound. + if !KnownDerivatives::::contains_key(&pseudonym) { + KnownDerivatives::::insert(&pseudonym, ()); + >::reveal_address( + pseudonym.clone(), + ); + } origin.set_caller_from(frame_system::RawOrigin::Signed(pseudonym)); let info = call.get_dispatch_info(); let result = call.dispatch(origin); // Always take into account the base weight of this call. let mut weight = T::WeightInfo::as_derivative() - .saturating_add(T::DbWeight::get().reads_writes(1, 1)); + .saturating_add(T::DbWeight::get().reads_writes(1, 1)) + .saturating_add(T::DbWeight::get().reads_writes(1, 2)); // Add the real weight of the dispatch. weight = weight.saturating_add(extract_actual_weight(&result, &info)); result @@ -599,6 +629,12 @@ pub mod pallet { } impl Pallet { + /// Whether `account` is a known [`Pallet::as_derivative`] pseudonym, i.e. it has been + /// used (and revealed to the wormhole soundness counter) at least once. + pub fn is_derivative(account: &T::AccountId) -> bool { + KnownDerivatives::::contains_key(account) + } + /// Get the accumulated `weight` and the dispatch class for the given `calls`. fn weight_and_dispatch_class( calls: &[::RuntimeCall], diff --git a/pallets/utility/src/tests.rs b/pallets/utility/src/tests.rs index a1337947..4ed4a403 100644 --- a/pallets/utility/src/tests.rs +++ b/pallets/utility/src/tests.rs @@ -22,6 +22,7 @@ use super::*; use crate as utility; +use core::cell::RefCell; use frame_support::{ assert_err_ignore_postinfo, assert_noop, assert_ok, derive_impl, dispatch::{DispatchErrorWithPostInfo, Pays}, @@ -225,6 +226,20 @@ impl Config for Test { type PalletsOrigin = OriginCaller; type WeightInfo = (); type HighSecurity = qp_high_security::testing::TestHighSecurity; + type AddressRevealer = MockAddressRevealer; +} + +thread_local! { + /// Records addresses passed to `AddressRevealer::reveal_address`, so tests can assert a + /// derivative pseudonym is revealed to the wormhole soundness counter on first use. + pub static REVEALED_ADDRESSES: RefCell> = const { RefCell::new(Vec::new()) }; +} + +pub struct MockAddressRevealer; +impl qp_wormhole::AddressRevealer for MockAddressRevealer { + fn reveal_address(account: u64) { + REVEALED_ADDRESSES.with(|a| a.borrow_mut().push(account)); + } } /// High-security accounts in tests may only dispatch `System::remark`. @@ -299,6 +314,41 @@ fn as_derivative_works() { }); } +#[test] +fn as_derivative_reveals_pseudonym_on_first_use_only() { + new_test_ext().execute_with(|| { + REVEALED_ADDRESSES.with(|a| a.borrow_mut().clear()); + let sub_1_0 = derivative_account_id(1u64, 0); + assert!(!Utility::is_derivative(&sub_1_0)); + + // First use reveals the pseudonym to the wormhole soundness counter and marks it known. + assert_ok!(Utility::as_derivative( + RuntimeOrigin::signed(1), + 0, + Box::new(RuntimeCall::System(frame_system::Call::remark { remark: vec![] })), + )); + assert!(Utility::is_derivative(&sub_1_0)); + assert_eq!(REVEALED_ADDRESSES.with(|a| a.borrow().clone()), vec![sub_1_0]); + + // A second use must not reveal again. + assert_ok!(Utility::as_derivative( + RuntimeOrigin::signed(1), + 0, + Box::new(RuntimeCall::System(frame_system::Call::remark { remark: vec![] })), + )); + assert_eq!(REVEALED_ADDRESSES.with(|a| a.borrow().clone()), vec![sub_1_0]); + + // A different index is a different pseudonym and reveals separately. + let sub_1_1 = derivative_account_id(1u64, 1); + assert_ok!(Utility::as_derivative( + RuntimeOrigin::signed(1), + 1, + Box::new(RuntimeCall::System(frame_system::Call::remark { remark: vec![] })), + )); + assert_eq!(REVEALED_ADDRESSES.with(|a| a.borrow().clone()), vec![sub_1_0, sub_1_1]); + }); +} + #[test] fn as_derivative_handles_weight_refund() { new_test_ext().execute_with(|| { diff --git a/pallets/wormhole/src/lib.rs b/pallets/wormhole/src/lib.rs index 2ce139ad..ecb7f7d1 100644 --- a/pallets/wormhole/src/lib.rs +++ b/pallets/wormhole/src/lib.rs @@ -806,4 +806,14 @@ pub mod pallet { Self::reveal_account(&account.into()); } } + + // Narrow reveal-only handle for pallets without balance/asset types (e.g. `pallet-utility` + // revealing `as_derivative` pseudonyms). + impl qp_wormhole::AddressRevealer<::AccountId> + for Pallet + { + fn reveal_address(account: ::AccountId) { + Self::reveal_account(&account); + } + } } diff --git a/primitives/wormhole/src/lib.rs b/primitives/wormhole/src/lib.rs index 66f2644f..22483ef8 100644 --- a/primitives/wormhole/src/lib.rs +++ b/primitives/wormhole/src/lib.rs @@ -94,6 +94,20 @@ pub trait TransferProofRecorder { fn reveal_address(account: AccountId); } +/// Narrow handle into the wormhole soundness counter for pallets that only need to reveal +/// addresses (see [`TransferProofRecorder::reveal_address`]) and have no notion of balances or +/// assets, e.g. `pallet-utility` revealing `as_derivative` pseudonyms on first use. +pub trait AddressRevealer { + /// Reveal `account` to the wormhole soundness counter, removing its current balance from the + /// pool of value that could be exited via the wormhole. + fn reveal_address(account: AccountId); +} + +/// No-op revealer for tests and runtimes without a wormhole. +impl AddressRevealer for () { + fn reveal_address(_account: AccountId) {} +} + /// Derive a wormhole address from a 32-byte inner_digest (already hashed). /// /// This hashes the inner_digest using Poseidon to get the wormhole account address. diff --git a/runtime/src/configs/mod.rs b/runtime/src/configs/mod.rs index 1c921ea9..762b04bd 100644 --- a/runtime/src/configs/mod.rs +++ b/runtime/src/configs/mod.rs @@ -464,6 +464,7 @@ impl pallet_utility::Config for Runtime { type PalletsOrigin = OriginCaller; type WeightInfo = pallet_utility::weights::SubstrateWeight; type HighSecurity = HighSecurityConfig; + type AddressRevealer = Wormhole; } parameter_types! { @@ -706,6 +707,13 @@ impl Contains for NonWormholeAccounts { return true; } + // `as_derivative` pseudonyms likewise spend through their controller and never sign. + // The utility pallet reveals a pseudonym on its first use (deducting its balance from + // the pool); afterwards it is excluded here so later receipts don't inflate the pool. + if pallet_utility::Pallet::::is_derivative(account) { + return true; + } + // The configured treasury account receives a block reward every block but is a keyless // governance account (a multisig that need not be registered in the multisig pallet), not // a wormhole deposit. In this fork the treasury address is configurable, so we read the diff --git a/runtime/src/transaction_extensions.rs b/runtime/src/transaction_extensions.rs index 26b8f9c2..57d01863 100644 --- a/runtime/src/transaction_extensions.rs +++ b/runtime/src/transaction_extensions.rs @@ -1578,4 +1578,75 @@ mod tests { ); }); } + + // --- derivative pseudonyms must not inflate the pool without bound --- + // An `as_derivative` pseudonym never signs (its controller does), so before the reveal-on-use + // fix an attacker could bounce fixed capital between derivative addresses, adding the amount + // to `PotentialWormholeBalance` on every hop while never subtracting anything. + #[test] + fn counter_derivative_hops_do_not_inflate_pool() { + new_test_ext().execute_with(|| { + System::set_block_number(1); + let controller = intg_account(70); + fund(&controller, SENDER_BAL); + mark_revealed(&controller); + pallet_wormhole::PotentialWormholeBalance::::put(POOL_BASE); + + let d0 = pallet_utility::derivative_account_id(controller.clone(), 0u16); + let d1 = pallet_utility::derivative_account_id(controller.clone(), 1u16); + + // Fund derivative 0: it has never been used, so it looks ambiguous and is counted. + let funding = 100 * UNIT; + run_transfer(&controller, &d0, funding); + assert_eq!(pool(), POOL_BASE + funding, "credit to an unused pseudonym is counted"); + + // Hop funds from derivative 0 to derivative 1. First use of derivative 0 reveals it + // (subtracting its whole balance), while the credit to (still unused) derivative 1 + // is counted — so the hop nets to the hopped amount, not a double count. + let hop = 40 * UNIT; + let inner = transfer_call(&d1, hop); + let call = RuntimeCall::Utility(pallet_utility::Call::as_derivative { + index: 0, + call: alloc::boxed::Box::new(inner.clone()), + }); + run_lifecycle(&controller, call, || { + assert_ok!(Utility::as_derivative( + RuntimeOrigin::signed(controller.clone()), + 0, + alloc::boxed::Box::new(inner), + )); + }); + assert_eq!( + pool(), + POOL_BASE + hop, + "first use reveals the pseudonym: its counted funding is subtracted" + ); + assert!( + !pallet_wormhole::Pallet::::is_ambiguous_account(&d0), + "a used pseudonym must no longer be treated as ambiguous" + ); + + // Hop back from derivative 1 to derivative 0. Derivative 1 reveals on its first use + // (-hop), and the credit to the already-revealed derivative 0 is NOT counted, so the + // pool returns to its baseline instead of growing on every bounce. + let hop_back = 20 * UNIT; + let inner = transfer_call(&d0, hop_back); + let call = RuntimeCall::Utility(pallet_utility::Call::as_derivative { + index: 1, + call: alloc::boxed::Box::new(inner.clone()), + }); + run_lifecycle(&controller, call, || { + assert_ok!(Utility::as_derivative( + RuntimeOrigin::signed(controller.clone()), + 1, + alloc::boxed::Box::new(inner), + )); + }); + assert_eq!( + pool(), + POOL_BASE, + "bouncing fixed capital between pseudonyms must not inflate the pool" + ); + }); + } } From 4c1051b4014caa03e9d961f36ae6d2b12ddfe390 Mon Sep 17 00:00:00 2001 From: illuzen Date: Thu, 2 Jul 2026 19:53:21 +0800 Subject: [PATCH 36/47] fix(frame-metadata): validate META_RESERVED before decoding the payload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `RuntimeMetadataPrefixed` used a derived `Decode`, so an invalid blob was only rejectable after the entire `(u32, RuntimeMetadata)` tuple — potentially an attacker-sized structure of nested vectors, maps and strings — had been fully materialized. Decode now checks the reserved 'meta' magic after the first four bytes and fails fast on a mismatch. Also document on the type that metadata decoding has no schema-level caps beyond input-length proportionality, so callers decoding untrusted blobs must bound the input size up front. Co-authored-by: Cursor --- frame/metadata/src/lib.rs | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/frame/metadata/src/lib.rs b/frame/metadata/src/lib.rs index eb626b68..5791968c 100644 --- a/frame/metadata/src/lib.rs +++ b/frame/metadata/src/lib.rs @@ -83,11 +83,31 @@ pub mod v16; pub const META_RESERVED: u32 = 0x6174656d; // 'meta' warning for endianness. /// Metadata prefixed by a u32 for reserved usage +/// +/// # Decoding untrusted input +/// +/// [`Decode`] rejects blobs whose prefix is not [`META_RESERVED`] after reading only the first +/// four bytes. Beyond that, decoding materializes unbounded owned containers (`Vec`s, maps, +/// strings) whose size is proportional to the input, with no schema-level caps: callers decoding +/// metadata from an untrusted source must bound the input size *before* decoding. #[derive(Eq, Encode, PartialEq, Debug)] -#[cfg_attr(feature = "decode", derive(Decode))] #[cfg_attr(feature = "serde_full", derive(Serialize))] pub struct RuntimeMetadataPrefixed(pub u32, pub RuntimeMetadata); +#[cfg(feature = "decode")] +impl Decode for RuntimeMetadataPrefixed { + fn decode(input: &mut I) -> Result { + // Validate the reserved prefix *before* decoding the payload, so a blob that is not + // runtime metadata is rejected after reading four bytes instead of after fully + // materializing an attacker-sized metadata structure. + let prefix = u32::decode(input)?; + if prefix != META_RESERVED { + return Err("Invalid metadata prefix: expected `META_RESERVED` ('meta')".into()) + } + Ok(Self(prefix, RuntimeMetadata::decode(input)?)) + } +} + impl From for Vec { fn from(value: RuntimeMetadataPrefixed) -> Self { value.encode() @@ -272,4 +292,19 @@ mod test { Decode::decode(&mut load_metadata(14).as_slice()).unwrap(); assert!(matches!(meta.1, RuntimeMetadata::V14(_))); } + + #[test] + fn should_reject_invalid_prefix_before_decoding_payload() { + // A wrong magic followed by a huge claimed payload: the prefix check must fail after + // the first four bytes, without attempting to materialize the payload. + let mut blob = 0xdeadbeef_u32.encode(); + blob.extend_from_slice(&load_metadata(14)[4..]); + let res = RuntimeMetadataPrefixed::decode(&mut blob.as_slice()); + assert!(res.is_err()); + + // A valid prefix still decodes. + let meta: RuntimeMetadataPrefixed = + Decode::decode(&mut load_metadata(14).as_slice()).unwrap(); + assert_eq!(meta.0, META_RESERVED); + } } From de06ef85dad3d674c638506def5159008cd8fa32 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 3 Jul 2026 09:54:16 +0800 Subject: [PATCH 37/47] fix(frame-support-procedural): reserve counter prefixes for counted N-maps in duplicate check Co-authored-by: Cursor --- frame/support-procedural/src/pallet/expand/storage.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frame/support-procedural/src/pallet/expand/storage.rs b/frame/support-procedural/src/pallet/expand/storage.rs index a0951a25..96799731 100644 --- a/frame/support-procedural/src/pallet/expand/storage.rs +++ b/frame/support-procedural/src/pallet/expand/storage.rs @@ -66,7 +66,10 @@ fn check_prefix_duplicates( return Err(err); } - if let Metadata::CountedMap { .. } = storage_def.metadata { + // Counted maps and counted N-maps both generate an auxiliary counter storage instance under + // the derived `CounterFor...` prefix, so that namespace must be reserved for both kinds; + // otherwise another storage item could silently alias the counter's physical key. + if let Metadata::CountedMap { .. } | Metadata::CountedNMap { .. } = storage_def.metadata { let counter_prefix = counter_prefix(&prefix); let counter_dup_err = syn::Error::new( storage_def.prefix_span(), From 50d1f3f4c8916dcc8a21eda31eec2f9c9b5d3a64 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 3 Jul 2026 11:08:43 +0800 Subject: [PATCH 38/47] fix(frame-support): resume clear_prefix from the supplied cursor instead of fabricating progress Co-authored-by: Cursor --- frame/support/src/storage/unhashed.rs | 72 ++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-) diff --git a/frame/support/src/storage/unhashed.rs b/frame/support/src/storage/unhashed.rs index 495c50ca..6a9541e6 100644 --- a/frame/support/src/storage/unhashed.rs +++ b/frame/support/src/storage/unhashed.rs @@ -140,12 +140,41 @@ pub fn kill_prefix(prefix: &[u8], limit: Option) -> sp_io::KillStorageResul pub fn clear_prefix( prefix: &[u8], maybe_limit: Option, - _maybe_cursor: Option<&[u8]>, + maybe_cursor: Option<&[u8]>, ) -> sp_io::MultiRemovalResults { + use sp_io::{KillStorageResult::*, MultiRemovalResults}; + + // The legacy `kill_prefix` host function has no cursor support and, when called repeatedly + // with a limit in the same block, keeps re-reporting the same backend keys. To honour the + // documented cursor contract, resumed calls walk `next_key` from the supplied cursor + // (which observes the overlay, so keys already deleted this block are skipped) and delete + // keys one by one, returning a cursor that reflects actual progress. + if let Some(cursor) = maybe_cursor { + let limit = maybe_limit.unwrap_or(u32::MAX); + let mut previous_key = cursor.to_vec(); + let mut unique = 0u32; + + while unique < limit { + match sp_io::storage::next_key(&previous_key) { + Some(next_key) if next_key.starts_with(prefix) => { + sp_io::storage::clear(&next_key); + previous_key = next_key; + unique = unique.saturating_add(1); + }, + _ => break, + } + } + + let maybe_cursor = match sp_io::storage::next_key(&previous_key) { + Some(next_key) if next_key.starts_with(prefix) => Some(previous_key), + _ => None, + }; + return MultiRemovalResults { maybe_cursor, backend: unique, unique, loops: unique } + } + // TODO: Once the network has upgraded to include the new host functions, this code can be // enabled. // sp_io::storage::clear_prefix(prefix, maybe_limit, maybe_cursor) - use sp_io::{KillStorageResult::*, MultiRemovalResults}; #[allow(deprecated)] let (maybe_cursor, i) = match kill_prefix(prefix, maybe_limit) { AllRemoved(i) => (None, i), @@ -177,3 +206,42 @@ pub fn get_raw(key: &[u8]) -> Option> { pub fn put_raw(key: &[u8], value: &[u8]) { sp_io::storage::set(key, value) } + +#[cfg(test)] +mod tests { + use super::*; + use sp_io::TestExternalities; + + #[test] + fn clear_prefix_resumes_from_cursor_instead_of_repeating_the_same_batch() { + let mut ext = TestExternalities::default(); + let prefix = b"test_prefix:"; + let key = |i: u8| [&prefix[..], &[i]].concat(); + + ext.execute_with(|| { + for i in 0u8..4 { + put_raw(&key(i), &[i]); + } + }); + // Move the keys into the backend so the limited first call leaves a remainder. + ext.commit_all().unwrap(); + + ext.execute_with(|| { + let first = clear_prefix(prefix, Some(2), None); + assert_eq!(first.unique, 2); + let first_cursor = + first.maybe_cursor.expect("entries remain after first partial clear"); + + // The resumed call must make real progress past the keys already deleted in + // this block rather than re-reporting the same backend batch. + let second = clear_prefix(prefix, Some(2), Some(&first_cursor)); + assert_eq!(second.unique, 2); + assert!(second.maybe_cursor.is_none(), "all entries are gone after the second call"); + + assert!(!contains_prefixed_key(prefix)); + for i in 0u8..4 { + assert!(!exists(&key(i)), "key {} must be deleted", i); + } + }); + } +} From 168066519d0a2389ba7bbc2e854fc8ebde4c1e55 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 3 Jul 2026 11:48:08 +0800 Subject: [PATCH 39/47] fix(frame-support): reconcile counted storage counters against actual key existence transitions Co-authored-by: Cursor --- .../support/src/storage/types/counted_map.rs | 66 +++++++++++++- .../support/src/storage/types/counted_nmap.rs | 88 +++++++++++++++++-- 2 files changed, 146 insertions(+), 8 deletions(-) diff --git a/frame/support/src/storage/types/counted_map.rs b/frame/support/src/storage/types/counted_map.rs index c136226b..441bbb53 100644 --- a/frame/support/src/storage/types/counted_map.rs +++ b/frame/support/src/storage/types/counted_map.rs @@ -325,7 +325,18 @@ where pub fn migrate_key>( key: KeyArg, ) -> Option { - ::Map::migrate_key::(key) + // If the same logical key already has an entry under the current hasher, migrating the + // old-hasher entry overwrites it and collapses two stored keys into one, so the counter + // must be reconciled against the actual pre-migration existence of both entries. + let old_hashed = key.using_encoded(OldHasher::hash); + let new_hashed = key.using_encoded(Hasher::hash); + let collapses = old_hashed.as_ref() != new_hashed.as_ref() && + ::Map::contains_key(Ref::from(&key)); + let value = ::Map::migrate_key::(key); + if value.is_some() && collapses { + CounterFor::::mutate(|value| value.saturating_dec()); + } + value } /// Remove all values in the map. @@ -420,7 +431,11 @@ where ) }); if current < bound { - CounterFor::::mutate(|value| value.saturating_inc()); + // The counter tracks the number of keys in the map, so it must only grow when the + // append creates a previously absent key, not on every successful append. + if !::Map::contains_key(Ref::from(&key)) { + CounterFor::::mutate(|value| value.saturating_inc()); + } let key = ::Map::hashed_key_for(key); sp_io::storage::append(&key, item.encode()); Ok(()) @@ -1174,6 +1189,53 @@ mod test { }) } + #[test] + fn try_append_increments_counter_only_for_new_keys() { + type B = CountedStorageMap>>; + TestExternalities::default().execute_with(|| { + B::try_append(0, 1).unwrap(); + assert_eq!(B::count(), 1); + // Repeated appends to an existing key must not inflate the key count. + B::try_append(0, 2).unwrap(); + B::try_append(0, 3).unwrap(); + assert_eq!(B::count(), 1); + B::try_append(1, 1).unwrap(); + assert_eq!(B::count(), 2); + }) + } + + #[test] + fn migrate_key_reconciles_counter_when_entries_collapse() { + type A = CountedStorageMap; + type B = CountedStorageMap; + TestExternalities::default().execute_with(|| { + // The same logical key exists under both the old and the current hasher. + A::insert(1, 1); + B::insert(1, 2); + assert_eq!(B::count(), 2); + + // Migration overwrites the current-hasher entry: two stored keys collapse into one + // and the counter must follow. + assert_eq!(B::migrate_key::(1), Some(1)); + assert_eq!(B::get(1), Some(1)); + assert_eq!(B::count(), 1); + + // A degenerate migration with the current hasher rewrites the same physical key and + // must not change the count. + assert_eq!(B::migrate_key::(1), Some(1)); + assert_eq!(B::count(), 1); + + // Migrating a key that only exists under the old hasher moves it without changing + // the count, and migrating a missing key is a no-op. + A::insert(2, 4); + assert_eq!(B::count(), 2); + assert_eq!(B::migrate_key::(2), Some(4)); + assert_eq!(B::count(), 2); + assert_eq!(B::migrate_key::(3), None); + assert_eq!(B::count(), 2); + }) + } + #[test] fn translate_values() { type A = CountedStorageMap; diff --git a/frame/support/src/storage/types/counted_nmap.rs b/frame/support/src/storage/types/counted_nmap.rs index e473284a..18aed850 100644 --- a/frame/support/src/storage/types/counted_nmap.rs +++ b/frame/support/src/storage/types/counted_nmap.rs @@ -181,16 +181,16 @@ where } /// Store or remove the value to be associated with `key` so that `get` returns the `query`. - /// It decrements the counter when the value is removed. + /// It updates the counter when a key is actually added to or removed from the map. pub fn set + TupleToEncodedIter>( key: KArg, query: QueryKind::Query, ) { let option = QueryKind::from_query_to_optional_value(query); - if option.is_none() { - CounterFor::::mutate(|value| value.saturating_dec()); - } - ::Map::set(key, QueryKind::from_optional_value_to_query(option)) + // The counter must reflect actual key existence transitions: writing a value can create + // a previously absent key and removing one only shrinks the map if the key existed. + // `try_mutate_exists` reconciles the counter against both states. + Self::mutate_exists(key, |value| *value = option); } /// Take a value from storage, removing it afterwards. @@ -396,7 +396,24 @@ where where KArg: EncodeLikeTuple + TupleToEncodedIter, { - ::Map::migrate_keys::<_>(key, hash_fns) + // This reimplements the raw map migration (rather than delegating) because the counter + // must be reconciled when the same logical key already has an entry under the current + // hashers: migrating then collapses two stored keys into one. The single-use `hash_fns` + // closures cannot be reused for both a collision check and a delegated call. + let old_key = { + let mut old_key = Self::map_storage_final_prefix(); + old_key.extend_from_slice(&Key::migrate_key(&key, hash_fns)); + old_key + }; + let new_key = ::Map::hashed_key_for(key); + let value = crate::storage::unhashed::take::(&old_key); + if let Some(value) = &value { + if old_key != new_key && crate::storage::unhashed::exists(&new_key) { + CounterFor::::mutate(|value| value.saturating_dec()); + } + crate::storage::unhashed::put(&new_key, value); + } + value } /// Attempt to remove all items from the map. @@ -741,6 +758,65 @@ mod test { }); } + #[test] + fn set_reconciles_counter_with_actual_key_existence() { + type A = CountedStorageNMap, u32, OptionQuery>; + + TestExternalities::default().execute_with(|| { + // Setting a value on an absent key creates it and must increment the count. + A::set((1,), Some(10)); + assert_eq!(A::count(), 1); + // Overwriting an existing key must not change the count. + A::set((1,), Some(11)); + assert_eq!(A::get((1,)), Some(11)); + assert_eq!(A::count(), 1); + // Removing an absent key must not decrement the count. + A::set((2,), None); + assert_eq!(A::count(), 1); + // Removing an existing key decrements. + A::set((1,), None); + assert!(!A::contains_key((1,))); + assert_eq!(A::count(), 0); + }) + } + + #[test] + fn migrate_keys_reconciles_counter_when_entries_collapse() { + type A = CountedStorageNMap, u32, OptionQuery>; + type B = CountedStorageNMap, u32, ValueQuery>; + + TestExternalities::default().execute_with(|| { + // The same logical key exists under both the old and the current hashers. + B::insert((2,), 10); + A::insert((2,), 20); + assert_eq!(A::count(), 2); + + // Migration overwrites the current-hasher entry: two stored keys collapse into one + // and the counter must follow. + assert_eq!( + A::migrate_keys((2,), (Box::new(|key| Blake2_256::hash(key).to_vec()),)), + Some(10) + ); + assert_eq!(A::get((2,)), Some(10)); + assert_eq!(A::count(), 1); + + // A degenerate migration with the current hasher rewrites the same physical key and + // must not change the count. + assert_eq!( + A::migrate_keys((2,), (Box::new(|key| Blake2_128Concat::hash(key)),)), + Some(10) + ); + assert_eq!(A::count(), 1); + + // Migrating a missing key is a no-op. + assert_eq!( + A::migrate_keys((3,), (Box::new(|key| Blake2_256::hash(key).to_vec()),)), + None + ); + assert_eq!(A::count(), 1); + }) + } + #[test] fn test_1_key() { type A = CountedStorageNMap, u32, OptionQuery>; From 02d74c2a5e82a1e937dd2ed14d46cdb277fa820b Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 3 Jul 2026 11:59:53 +0800 Subject: [PATCH 40/47] fix(frame-support): update counted map/N-map counters when draining through key and value iterators Co-authored-by: Cursor --- .../src/storage/generator/double_map.rs | 2 + frame/support/src/storage/generator/map.rs | 1 + frame/support/src/storage/generator/nmap.rs | 2 + frame/support/src/storage/mod.rs | 46 ++++++++++++- .../support/src/storage/types/counted_map.rs | 19 +++++- .../support/src/storage/types/counted_nmap.rs | 67 +++++++++++++++---- 6 files changed, 118 insertions(+), 19 deletions(-) diff --git a/frame/support/src/storage/generator/double_map.rs b/frame/support/src/storage/generator/double_map.rs index a60a00d9..fbbf1f07 100644 --- a/frame/support/src/storage/generator/double_map.rs +++ b/frame/support/src/storage/generator/double_map.rs @@ -396,6 +396,7 @@ where let mut key_material = G::Hasher2::reverse(raw_key_without_prefix); K2::decode(&mut key_material) }, + phantom: Default::default(), } } @@ -450,6 +451,7 @@ where let k2 = K2::decode(&mut k2_material)?; Ok((k1, k2)) }, + phantom: Default::default(), } } diff --git a/frame/support/src/storage/generator/map.rs b/frame/support/src/storage/generator/map.rs index 041c28e1..b18b502a 100644 --- a/frame/support/src/storage/generator/map.rs +++ b/frame/support/src/storage/generator/map.rs @@ -114,6 +114,7 @@ where let mut key_material = G::Hasher::reverse(raw_key_without_prefix); K::decode(&mut key_material) }, + phantom: Default::default(), } } diff --git a/frame/support/src/storage/generator/nmap.rs b/frame/support/src/storage/generator/nmap.rs index f007e868..32814247 100755 --- a/frame/support/src/storage/generator/nmap.rs +++ b/frame/support/src/storage/generator/nmap.rs @@ -356,6 +356,7 @@ impl> previous_key: prefix, drain: false, closure: K::decode_partial_key, + phantom: Default::default(), } } @@ -412,6 +413,7 @@ impl> let (final_key, _) = K::decode_final_key(raw_key_without_prefix)?; Ok(final_key) }, + phantom: Default::default(), } } diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index a63ae708..34ea4d61 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -1132,7 +1132,9 @@ impl Iterator for PrefixIterator { +/// +/// If draining, then the hook `OnRemoval::on_removal` is called after each removal. +pub struct KeyPrefixIterator { prefix: Vec, previous_key: Vec, /// If true then value are removed while iterating @@ -1140,6 +1142,20 @@ pub struct KeyPrefixIterator { /// Function that take `raw_key_without_prefix` and decode `T`. /// `raw_key_without_prefix` is the raw storage key without the prefix iterated on. closure: fn(&[u8]) -> Result, + phantom: core::marker::PhantomData, +} + +impl KeyPrefixIterator { + /// Converts to the same iterator but with the different 'OnRemoval' type + pub fn convert_on_removal(self) -> KeyPrefixIterator { + KeyPrefixIterator:: { + prefix: self.prefix, + previous_key: self.previous_key, + drain: self.drain, + closure: self.closure, + phantom: Default::default(), + } + } } impl KeyPrefixIterator { @@ -1150,14 +1166,25 @@ impl KeyPrefixIterator { /// a `Result` containing the decoded key type `T` if successful, and a `codec::Error` on /// failure. The `&[u8]` argument represents the raw, undecoded key without the prefix of the /// current item. + /// + /// The returned iterator uses the no-op `OnRemoval` hook; use [`Self::convert_on_removal`] to + /// attach a different one. pub fn new( prefix: Vec, previous_key: Vec, decode_fn: fn(&[u8]) -> Result, ) -> Self { - KeyPrefixIterator { prefix, previous_key, drain: false, closure: decode_fn } + KeyPrefixIterator { + prefix, + previous_key, + drain: false, + closure: decode_fn, + phantom: Default::default(), + } } +} +impl KeyPrefixIterator { /// Get the last key that has been iterated upon and return it. pub fn last_raw_key(&self) -> &[u8] { &self.previous_key @@ -1180,7 +1207,7 @@ impl KeyPrefixIterator { } } -impl Iterator for KeyPrefixIterator { +impl Iterator for KeyPrefixIterator { type Item = T; fn next(&mut self) -> Option { @@ -1191,7 +1218,20 @@ impl Iterator for KeyPrefixIterator { if let Some(next) = maybe_next { self.previous_key = next; if self.drain { + // The raw value is read before removal so the `OnRemoval` hook (e.g. the + // counted map counter update) can observe what was deleted. + let raw_value = match unhashed::get_raw(&self.previous_key) { + Some(raw_value) => raw_value, + None => { + log::error!( + "next_key returned a key with no value at {:?}", + self.previous_key, + ); + continue + }, + }; unhashed::kill(&self.previous_key); + OnRemoval::on_removal(&self.previous_key, &raw_value); } let raw_key_without_prefix = &self.previous_key[self.prefix.len()..]; diff --git a/frame/support/src/storage/types/counted_map.rs b/frame/support/src/storage/types/counted_map.rs index 441bbb53..03909a8c 100644 --- a/frame/support/src/storage/types/counted_map.rs +++ b/frame/support/src/storage/types/counted_map.rs @@ -515,8 +515,9 @@ where /// Enumerate all keys in the counted map. /// /// If you alter the map while doing this, you'll get undefined results. - pub fn iter_keys() -> crate::storage::KeyPrefixIterator { - ::Map::iter_keys() + pub fn iter_keys( + ) -> crate::storage::KeyPrefixIterator> { + ::Map::iter_keys().convert_on_removal() } } @@ -1189,6 +1190,20 @@ mod test { }) } + #[test] + fn iter_keys_drain_updates_count() { + type A = CountedStorageMap; + TestExternalities::default().execute_with(|| { + A::insert(1, 10); + A::insert(2, 20); + assert_eq!(A::count(), 2); + + assert_eq!(A::iter_keys().drain().count(), 2); + assert_eq!(A::count(), 0); + assert_eq!(A::iter().count(), 0); + }) + } + #[test] fn try_append_increments_counter_only_for_new_keys() { type B = CountedStorageMap>>; diff --git a/frame/support/src/storage/types/counted_nmap.rs b/frame/support/src/storage/types/counted_nmap.rs index 18aed850..1e1ee589 100644 --- a/frame/support/src/storage/types/counted_nmap.rs +++ b/frame/support/src/storage/types/counted_nmap.rs @@ -277,11 +277,13 @@ where } /// Iterate over values that share the first key. - pub fn iter_prefix_values(partial_key: KP) -> PrefixIterator + pub fn iter_prefix_values( + partial_key: KP, + ) -> PrefixIterator> where Key: HasKeyPrefix, { - ::Map::iter_prefix_values(partial_key) + ::Map::iter_prefix_values(partial_key).convert_on_removal() } /// Mutate the value under the given keys. @@ -451,8 +453,8 @@ where /// Iter over all value of the storage. /// /// NOTE: If a value failed to decode because storage is corrupted then it is skipped. - pub fn iter_values() -> crate::storage::PrefixIterator { - ::Map::iter_values() + pub fn iter_values() -> crate::storage::PrefixIterator> { + ::Map::iter_values().convert_on_removal() } /// Translate the values of all elements by a function `f`, in the map in no particular order. @@ -512,11 +514,14 @@ where /// undefined results. pub fn iter_prefix( kp: KP, - ) -> crate::storage::PrefixIterator<(>::Suffix, Value)> + ) -> crate::storage::PrefixIterator< + (>::Suffix, Value), + OnRemovalCounterUpdate, + > where Key: HasReversibleKeyPrefix, { - ::Map::iter_prefix(kp) + ::Map::iter_prefix(kp).convert_on_removal() } /// Enumerate all elements in the map with prefix key `kp` after a specified `starting_raw_key` @@ -543,11 +548,14 @@ where /// undefined results. pub fn iter_key_prefix( kp: KP, - ) -> crate::storage::KeyPrefixIterator<>::Suffix> + ) -> crate::storage::KeyPrefixIterator< + >::Suffix, + OnRemovalCounterUpdate, + > where Key: HasReversibleKeyPrefix, { - ::Map::iter_key_prefix(kp) + ::Map::iter_key_prefix(kp).convert_on_removal() } /// Enumerate all suffix keys in the map with prefix key `kp` after a specified @@ -558,11 +566,14 @@ where pub fn iter_key_prefix_from( kp: KP, starting_raw_key: Vec, - ) -> crate::storage::KeyPrefixIterator<>::Suffix> + ) -> crate::storage::KeyPrefixIterator< + >::Suffix, + OnRemovalCounterUpdate, + > where Key: HasReversibleKeyPrefix, { - ::Map::iter_key_prefix_from(kp, starting_raw_key) + ::Map::iter_key_prefix_from(kp, starting_raw_key).convert_on_removal() } /// Remove all elements from the map with prefix key `kp` and iterate through them in no @@ -602,8 +613,9 @@ where /// Enumerate all keys in the map in no particular order. /// /// If you add or remove values to the map while doing this, you'll get undefined results. - pub fn iter_keys() -> crate::storage::KeyPrefixIterator { - ::Map::iter_keys() + pub fn iter_keys( + ) -> crate::storage::KeyPrefixIterator> { + ::Map::iter_keys().convert_on_removal() } /// Enumerate all keys in the map after a specified `starting_raw_key` in no particular order. @@ -611,8 +623,8 @@ where /// If you add or remove values to the map while doing this, you'll get undefined results. pub fn iter_keys_from( starting_raw_key: Vec, - ) -> crate::storage::KeyPrefixIterator { - ::Map::iter_keys_from(starting_raw_key) + ) -> crate::storage::KeyPrefixIterator> { + ::Map::iter_keys_from(starting_raw_key).convert_on_removal() } /// Remove all elements from the map and iterate through them in no particular order. @@ -758,6 +770,33 @@ mod test { }); } + #[test] + fn drainable_iterator_variants_update_count() { + type A = CountedStorageNMap< + Prefix, + (NMapKey, NMapKey), + u32, + OptionQuery, + >; + + TestExternalities::default().execute_with(|| { + A::insert((3, 30), 11); + A::insert((3, 31), 12); + A::insert((4, 40), 13); + assert_eq!(A::count(), 3); + + // Draining through a value-only prefix iterator must decrement the counter. + assert_eq!(A::iter_prefix_values((3,)).drain().count(), 2); + assert_eq!(A::count(), 1); + assert_eq!(A::iter().collect::>(), vec![((4, 40), 13)]); + + // Draining through the key iterator must decrement the counter as well. + assert_eq!(A::iter_keys().drain().collect::>(), vec![(4, 40)]); + assert_eq!(A::count(), 0); + assert_eq!(A::iter().count(), 0); + }); + } + #[test] fn set_reconciles_counter_with_actual_key_existence() { type A = CountedStorageNMap, u32, OptionQuery>; From 8fc3a0195b2183c8bbe00eb3735af0a163424c2e Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 3 Jul 2026 12:05:38 +0800 Subject: [PATCH 41/47] fix(runtime/wormhole): weight as_derivative transfers and reconcile uncounted proof-recording work post-dispatch Co-authored-by: Cursor --- runtime/src/transaction_extensions.rs | 131 +++++++++++++++++++++++--- 1 file changed, 116 insertions(+), 15 deletions(-) diff --git a/runtime/src/transaction_extensions.rs b/runtime/src/transaction_extensions.rs index 57d01863..23db7fbd 100644 --- a/runtime/src/transaction_extensions.rs +++ b/runtime/src/transaction_extensions.rs @@ -106,12 +106,31 @@ impl WormholeProofRecorderExtension Self(PhantomData) } + /// Weight charged per recorded transfer proof. + /// + /// Per recorded transfer, `record_transfer` touches (worst case): + /// reads: TransferCount (1) + the `is_ambiguous_account` lookup on the recipient, + /// which reads the recipient nonce (1) and, when nonce == 0, the + /// `NonWormholeAccounts` membership checks `Multisigs` (1) and the configured + /// `TreasuryAccount` (1) => 4 reads. + /// writes: TransferProof / ZK-tree leaf (1) + TransferCount (1) + the conditional + /// `PotentialWormholeBalance` deposit add (1) => 3 writes. + fn per_transfer_weight() -> Weight { + T::DbWeight::get().reads_writes(4, 3) + } + fn count_transfers(call: &RuntimeCall) -> u64 { // NOTE: this must stay in sync with the events matched by `record_proofs_from_events_since` // — we only weight calls whose emitted events we actually record. In particular // `Balances::force_set_balance` is deliberately NOT counted here: it emits `BalanceSet` // (an absolute set, not a transfer/mint), which we cannot turn into a transfer proof and // therefore never record. See the soundness-counter caveat on `reduce_potential_balance`. + // + // Wrappers whose inner call is stored on-chain rather than in the submitted call + // (`Multisig::execute`, `ReversibleTransfers::recover_funds`, ...) cannot be counted + // statically. Proof-recording work they trigger is reconciled in `post_dispatch`, which + // registers any weight shortfall against the block via + // `register_extra_weight_unchecked`. match call { RuntimeCall::Balances(pallet_balances::Call::transfer_keep_alive { .. }) | RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death { .. }) | @@ -130,6 +149,7 @@ impl WormholeProofRecorderExtension RuntimeCall::Utility(pallet_utility::Call::dispatch_as { call, .. }) | RuntimeCall::Utility(pallet_utility::Call::with_weight { call, .. }) | + RuntimeCall::Utility(pallet_utility::Call::as_derivative { call, .. }) | RuntimeCall::Recovery(pallet_recovery::Call::as_recovered { call, .. }) => Self::count_transfers(call), @@ -143,7 +163,10 @@ impl WormholeProofRecorderExtension /// /// `event_count_before` is the value from `frame_system::Pallet::event_count()` /// captured in `prepare()`. - fn record_proofs_from_events_since(event_count_before: u32) { + /// + /// Returns the number of transfer proofs recorded, so callers can reconcile the actual + /// proof-recording work against the statically charged weight. + fn record_proofs_from_events_since(event_count_before: u32) -> u64 { // IMPORTANT: We must collect all transfers FIRST before calling record_transfer_proof, // because record_transfer_proof deposits new events which would invalidate the // stream_iter iterator (causing "Corrupted state" errors). @@ -191,18 +214,23 @@ impl WormholeProofRecorderExtension .collect(); // Now record the proofs - this is safe because we're no longer iterating over Events + let recorded = transfers_to_record.len() as u64; for (asset_id, from, to, amount) in transfers_to_record { >::record_transfer_proof( asset_id, from, to, amount, ); } + recorded } } impl TransactionExtension for WormholeProofRecorderExtension { - type Pre = u32; + /// `(event_count_snapshot, statically_charged_transfer_count)`. The snapshot bounds the + /// event scan in `post_dispatch`; the charged count lets `post_dispatch` reconcile actual + /// proof-recording work against the weight reserved by `weight()`. + type Pre = (u32, u64); /// Carries the pre-fee balance captured in `validate` for a first-time signer that must be /// revealed (subtracted from the potential wormhole pool). `None` when there is nothing to /// reveal. The actual subtraction is committed in `prepare`. @@ -214,14 +242,7 @@ impl TransactionEx fn weight(&self, call: &RuntimeCall) -> Weight { let n = Self::count_transfers(call); let transfer_weight = if n > 0 { - // Per recorded transfer, `record_transfer` touches (worst case): - // reads: TransferCount (1) + the `is_ambiguous_account` lookup on the recipient, - // which reads the recipient nonce (1) and, when nonce == 0, the - // `NonWormholeAccounts` membership checks `Multisigs` (1) and the configured - // `TreasuryAccount` (1) => 4 reads. - // writes: TransferProof / ZkTree leaf (1) + TransferCount (1) + the conditional - // `PotentialWormholeBalance` deposit add (1) => 3 writes. - T::DbWeight::get().reads_writes(4 * n, 3 * n) + Self::per_transfer_weight().saturating_mul(n) } else { Weight::zero() }; @@ -239,7 +260,7 @@ impl TransactionEx self, val: Self::Val, _origin: &sp_runtime::traits::DispatchOriginOf, - _call: &RuntimeCall, + call: &RuntimeCall, _info: &sp_runtime::traits::DispatchInfoOf, _len: usize, ) -> Result { @@ -251,8 +272,9 @@ impl TransactionEx } // Snapshot current event count so we only process events added by this tx - // (and any events from previous txs in the same block). - Ok(frame_system::Pallet::::event_count()) + // (and any events from previous txs in the same block), and remember how many transfer + // proofs were statically charged for so post_dispatch can reconcile. + Ok((frame_system::Pallet::::event_count(), Self::count_transfers(call))) } fn validate( @@ -292,7 +314,7 @@ impl TransactionEx fn post_dispatch( pre: Self::Pre, - _info: &DispatchInfoOf, + info: &DispatchInfoOf, _post_info: &mut PostDispatchInfoOf, _len: usize, result: &DispatchResult, @@ -300,7 +322,21 @@ impl TransactionEx // Only record proofs if the transaction succeeded. // Use the event count snapshot from prepare() to avoid duplicate recording. if result.is_ok() { - Self::record_proofs_from_events_since(pre); + let (event_count_before, charged_transfers) = pre; + let recorded = Self::record_proofs_from_events_since(event_count_before); + + // Wrappers that dispatch inner calls stored on-chain (`Multisig::execute`, + // `ReversibleTransfers::recover_funds`, ...) can emit transfer events the static + // `count_transfers` matcher cannot see, so the proof-recording work above may exceed + // the weight reserved by `weight()`. Register the shortfall against the block so + // block-weight based DoS protection stays sound even when the static count drifts. + if recorded > charged_transfers { + frame_system::Pallet::::register_extra_weight_unchecked( + Self::per_transfer_weight() + .saturating_mul(recorded.saturating_sub(charged_transfers)), + info.class, + ); + } } Ok(()) @@ -723,6 +759,71 @@ mod tests { }); } + #[test] + fn wormhole_proof_recorder_counts_as_derivative_wrapped_transfers() { + new_test_ext().execute_with(|| { + let ext = WormholeProofRecorderExtension::::new(); + let reveal_weight = + ::DbWeight::get().reads_writes(4, 1); + + // A transfer hidden behind `as_derivative` (possibly batched) must be charged the + // same per-transfer weight as a direct transfer. + let wrapped = RuntimeCall::Utility(pallet_utility::Call::as_derivative { + index: 0, + call: boxed(RuntimeCall::Utility(pallet_utility::Call::batch { + calls: vec![ + non_whitelisted_transfer(), + non_whitelisted_transfer(), + ], + })), + }); + let weight = as TransactionExtension< + RuntimeCall, + >>::weight(&ext, &wrapped); + assert_eq!( + weight, + WormholeProofRecorderExtension::::per_transfer_weight() + .saturating_mul(2) + .saturating_add(reveal_weight), + "as_derivative-wrapped transfers must be statically counted" + ); + }); + } + + #[test] + fn wormhole_proof_recorder_registers_extra_weight_for_uncounted_transfers() { + new_test_ext().execute_with(|| { + System::set_block_number(1); + + // The presented call is opaque to the static matcher (like `Multisig::execute` or + // `ReversibleTransfers::recover_funds`, whose inner call lives on-chain), but the + // dispatch emits a real transfer event that post_dispatch must record. + let opaque_call = + RuntimeCall::System(frame_system::Call::remark { remark: vec![1] }); + assert_eq!( + WormholeProofRecorderExtension::::count_transfers(&opaque_call), + 0 + ); + + let weight_before = frame_system::Pallet::::block_weight().total(); + + run_lifecycle(&alice(), opaque_call, || { + assert_ok!(Balances::transfer_keep_alive( + RuntimeOrigin::signed(alice()), + MultiAddress::Id(bob()), + EXISTENTIAL_DEPOSIT * 50, + )); + }); + + let weight_after = frame_system::Pallet::::block_weight().total(); + assert_eq!( + weight_after.saturating_sub(weight_before), + WormholeProofRecorderExtension::::per_transfer_weight(), + "the uncounted recorded transfer must be registered as extra block weight" + ); + }); + } + #[test] fn wormhole_proof_recorder_extension_prepare_succeeds() { new_test_ext().execute_with(|| { From daa52b4f3252507a6b69e0d3f3a83d8169579992 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 3 Jul 2026 14:38:13 +0800 Subject: [PATCH 42/47] fmt --- .../support/src/storage/types/counted_map.rs | 16 +++++++-------- .../support/src/storage/types/counted_nmap.rs | 4 ++-- frame/support/src/tests/mod.rs | 8 +++----- frame/support/src/traits/misc.rs | 6 ++---- .../src/traits/tokens/fungibles/hold.rs | 5 +---- frame/support/src/view_functions.rs | 3 +-- pallets/balances/src/tests/fungible_tests.rs | 10 +--------- pallets/frame-system/src/tests.rs | 3 ++- pallets/scheduler/src/tests.rs | 3 +-- pallets/wormhole/src/lib.rs | 4 +--- runtime/src/transaction_extensions.rs | 20 +++++-------------- 11 files changed, 26 insertions(+), 56 deletions(-) diff --git a/frame/support/src/storage/types/counted_map.rs b/frame/support/src/storage/types/counted_map.rs index 03909a8c..fc8974dc 100644 --- a/frame/support/src/storage/types/counted_map.rs +++ b/frame/support/src/storage/types/counted_map.rs @@ -423,13 +423,12 @@ where let bound = Value::bound(); // See `TryAppendValue::try_append`: a missing entry may still have a non-empty // query-kind default, so the bound is enforced against the effective value. - let current = - ::Map::decode_len(Ref::from(&key)).unwrap_or_else(|| { - crate::storage::effective_absent_len::( - QueryKind::from_optional_value_to_query, - QueryKind::from_query_to_optional_value, - ) - }); + let current = ::Map::decode_len(Ref::from(&key)).unwrap_or_else(|| { + crate::storage::effective_absent_len::( + QueryKind::from_optional_value_to_query, + QueryKind::from_query_to_optional_value, + ) + }); if current < bound { // The counter tracks the number of keys in the map, so it must only grow when the // append creates a previously absent key, not on every successful append. @@ -515,8 +514,7 @@ where /// Enumerate all keys in the counted map. /// /// If you alter the map while doing this, you'll get undefined results. - pub fn iter_keys( - ) -> crate::storage::KeyPrefixIterator> { + pub fn iter_keys() -> crate::storage::KeyPrefixIterator> { ::Map::iter_keys().convert_on_removal() } } diff --git a/frame/support/src/storage/types/counted_nmap.rs b/frame/support/src/storage/types/counted_nmap.rs index 1e1ee589..2db1486b 100644 --- a/frame/support/src/storage/types/counted_nmap.rs +++ b/frame/support/src/storage/types/counted_nmap.rs @@ -613,8 +613,8 @@ where /// Enumerate all keys in the map in no particular order. /// /// If you add or remove values to the map while doing this, you'll get undefined results. - pub fn iter_keys( - ) -> crate::storage::KeyPrefixIterator> { + pub fn iter_keys() -> crate::storage::KeyPrefixIterator> + { ::Map::iter_keys().convert_on_removal() } diff --git a/frame/support/src/tests/mod.rs b/frame/support/src/tests/mod.rs index 87dda338..151f71f0 100644 --- a/frame/support/src/tests/mod.rs +++ b/frame/support/src/tests/mod.rs @@ -332,8 +332,8 @@ fn new_pallet_storage_version_initialized_after_migrations_not_before() { assert!(!StorageVersion::exists::>()); // Mirrors `Executive::execute_on_runtime_upgrade` ordering: - // 1. `before_all_runtime_migrations` must not pre-seed the storage version, - // otherwise the version-gated migration below is silently skipped. + // 1. `before_all_runtime_migrations` must not pre-seed the storage version, otherwise the + // version-gated migration below is silently skipped. as BeforeAllRuntimeMigrations>::before_all_runtime_migrations(); assert!(!StorageVersion::exists::>()); @@ -385,9 +385,7 @@ fn runtime_task_enumeration_yields_pallet_tasks() { let tasks = ::iter().collect::>(); assert_eq!( tasks, - vec![RuntimeTask::System(frame_system::Task::::AddNumberIntoTotal { - i: 7 - })] + vec![RuntimeTask::System(frame_system::Task::::AddNumberIntoTotal { i: 7 })] ); }); } diff --git a/frame/support/src/traits/misc.rs b/frame/support/src/traits/misc.rs index 221f2652..fc87956c 100644 --- a/frame/support/src/traits/misc.rs +++ b/frame/support/src/traits/misc.rs @@ -1137,10 +1137,8 @@ impl WrapperKeepOpaque { /// Returns `None` if `data` is longer than the maximum encoded length of `T`, since such a /// value would exceed the `max_size` reported for bounded storage and PoV accounting. pub fn try_from_encoded(data: Vec) -> Option { - (data.len() <= T::max_encoded_len()).then(|| Self { - data, - _phantom: core::marker::PhantomData, - }) + (data.len() <= T::max_encoded_len()) + .then(|| Self { data, _phantom: core::marker::PhantomData }) } } diff --git a/frame/support/src/traits/tokens/fungibles/hold.rs b/frame/support/src/traits/tokens/fungibles/hold.rs index ce977c01..18ef5567 100644 --- a/frame/support/src/traits/tokens/fungibles/hold.rs +++ b/frame/support/src/traits/tokens/fungibles/hold.rs @@ -408,10 +408,7 @@ pub trait Mutate: // without this check a held balance could be credited to a non-existent account, leaving // hold state with no underlying asset account. if mode == OnHold { - ensure!( - !Self::total_balance(asset.clone(), dest).is_zero(), - TokenError::CannotCreate - ); + ensure!(!Self::total_balance(asset.clone(), dest).is_zero(), TokenError::CannotCreate); } // We want to make sure we can deposit the amount in advance. If we can't then something is diff --git a/frame/support/src/view_functions.rs b/frame/support/src/view_functions.rs index 259ccdcf..c08583b7 100644 --- a/frame/support/src/view_functions.rs +++ b/frame/support/src/view_functions.rs @@ -119,8 +119,7 @@ pub trait ViewFunction: DecodeWithMemLimit { ) -> Result<(), ViewFunctionDispatchError> { // Use the mem-tracked decode (bounded by `MAX_VIEW_FUNCTION_DECODE_MEM`) instead of a // plain `DecodeAll`, and still require the whole input to be consumed. - let view_function = - Self::decode_with_mem_limit(input, MAX_VIEW_FUNCTION_DECODE_MEM)?; + let view_function = Self::decode_with_mem_limit(input, MAX_VIEW_FUNCTION_DECODE_MEM)?; if !input.is_empty() { return Err(ViewFunctionDispatchError::Codec) } diff --git a/pallets/balances/src/tests/fungible_tests.rs b/pallets/balances/src/tests/fungible_tests.rs index bd46f908..b7aa3018 100644 --- a/pallets/balances/src/tests/fungible_tests.rs +++ b/pallets/balances/src/tests/fungible_tests.rs @@ -300,15 +300,7 @@ fn transfer_on_hold_onhold_requires_existing_dest() { assert_eq!(Balances::total_balance(&dest), 0); // `Free` mode may create the destination, so it still succeeds. - assert_ok!(Balances::transfer_on_hold( - &TestId::Foo, - &1, - &dest, - 5, - Exact, - Free, - Force - )); + assert_ok!(Balances::transfer_on_hold(&TestId::Foo, &1, &dest, 5, Exact, Free, Force)); assert_eq!(Balances::balance_on_hold(&TestId::Foo, &1), 2); assert_eq!(Balances::free_balance(dest), 5); }); diff --git a/pallets/frame-system/src/tests.rs b/pallets/frame-system/src/tests.rs index c7c7d50a..c05a9c42 100644 --- a/pallets/frame-system/src/tests.rs +++ b/pallets/frame-system/src/tests.rs @@ -673,7 +673,8 @@ fn set_code_drains_remaining_block_weight() { } } - let version = RuntimeVersion { spec_name: "test".into(), spec_version: 2, ..Default::default() }; + let version = + RuntimeVersion { spec_name: "test".into(), spec_version: 2, ..Default::default() }; let read_runtime_version = ReadRuntimeVersion(version.encode()); let mut ext = new_test_ext(); diff --git a/pallets/scheduler/src/tests.rs b/pallets/scheduler/src/tests.rs index 7a966604..4ccc10fb 100644 --- a/pallets/scheduler/src/tests.rs +++ b/pallets/scheduler/src/tests.rs @@ -758,8 +758,7 @@ fn failed_schedule_drops_noted_preimage() { run_to_block(1); // A call large enough to be stored as a `Lookup` preimage rather than inline. - let call = - RuntimeCall::System(frame_system::Call::remark { remark: vec![0u8; 1024] }); + let call = RuntimeCall::System(frame_system::Call::remark { remark: vec![0u8; 1024] }); let bounded = Preimage::bound(call).unwrap(); assert!(bounded.lookup_needed(), "the call must be noted as a preimage"); // `bound` has already noted the preimage. diff --git a/pallets/wormhole/src/lib.rs b/pallets/wormhole/src/lib.rs index ecb7f7d1..c8560cce 100644 --- a/pallets/wormhole/src/lib.rs +++ b/pallets/wormhole/src/lib.rs @@ -809,9 +809,7 @@ pub mod pallet { // Narrow reveal-only handle for pallets without balance/asset types (e.g. `pallet-utility` // revealing `as_derivative` pseudonyms). - impl qp_wormhole::AddressRevealer<::AccountId> - for Pallet - { + impl qp_wormhole::AddressRevealer<::AccountId> for Pallet { fn reveal_address(account: ::AccountId) { Self::reveal_account(&account); } diff --git a/runtime/src/transaction_extensions.rs b/runtime/src/transaction_extensions.rs index 23db7fbd..96a8a61c 100644 --- a/runtime/src/transaction_extensions.rs +++ b/runtime/src/transaction_extensions.rs @@ -241,11 +241,8 @@ impl TransactionEx fn weight(&self, call: &RuntimeCall) -> Weight { let n = Self::count_transfers(call); - let transfer_weight = if n > 0 { - Self::per_transfer_weight().saturating_mul(n) - } else { - Weight::zero() - }; + let transfer_weight = + if n > 0 { Self::per_transfer_weight().saturating_mul(n) } else { Weight::zero() }; // Soundness reveal bookkeeping for the signer. `validate` runs `is_ambiguous_account` on // the signer — nonce (1) + `Multisigs` (1) + `TreasuryAccount` (1) — and, when ambiguous, @@ -771,10 +768,7 @@ mod tests { let wrapped = RuntimeCall::Utility(pallet_utility::Call::as_derivative { index: 0, call: boxed(RuntimeCall::Utility(pallet_utility::Call::batch { - calls: vec![ - non_whitelisted_transfer(), - non_whitelisted_transfer(), - ], + calls: vec![non_whitelisted_transfer(), non_whitelisted_transfer()], })), }); let weight = as TransactionExtension< @@ -798,12 +792,8 @@ mod tests { // The presented call is opaque to the static matcher (like `Multisig::execute` or // `ReversibleTransfers::recover_funds`, whose inner call lives on-chain), but the // dispatch emits a real transfer event that post_dispatch must record. - let opaque_call = - RuntimeCall::System(frame_system::Call::remark { remark: vec![1] }); - assert_eq!( - WormholeProofRecorderExtension::::count_transfers(&opaque_call), - 0 - ); + let opaque_call = RuntimeCall::System(frame_system::Call::remark { remark: vec![1] }); + assert_eq!(WormholeProofRecorderExtension::::count_transfers(&opaque_call), 0); let weight_before = frame_system::Pallet::::block_weight().total(); From f5179c48fcc8dbe7772d08d114d2e549538ed699 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 3 Jul 2026 14:42:46 +0800 Subject: [PATCH 43/47] Bound the overlay-attribution key walks in child::clear_storage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre/post key counts used to attribute overlay-only removals walked the entire child trie twice, so a call with a small limit still paid O(total keys) next_key host calls — the same user-growable-state pattern the limit exists to prevent. Cap both walks at limit + 1024; when the cap saturates, overlay attribution degrades to zero (documented under-reporting) while the backend count stays exact. Unbounded clears keep exact counting since the host call is already O(total keys). Co-authored-by: Cursor --- frame/support/src/storage/child.rs | 69 ++++++++++++++++++++++++++---- 1 file changed, 61 insertions(+), 8 deletions(-) diff --git a/frame/support/src/storage/child.rs b/frame/support/src/storage/child.rs index 4d09f628..ce1cca73 100644 --- a/frame/support/src/storage/child.rs +++ b/frame/support/src/storage/child.rs @@ -181,6 +181,12 @@ pub fn kill_storage(child_info: &ChildInfo, limit: Option) -> KillStorageRe /// /// Please note that keys which are residing in the overlay for the child are deleted without /// counting towards the `limit`. +/// +/// When a `limit` is given, the pre/post key walks used to attribute overlay-only removals are +/// themselves bounded (to `limit` plus a fixed allowance), so a child trie holding many more +/// keys than the limit cannot force unbounded counting work. If the trie holds more keys than +/// the bound, overlay-only removals may be under-reported (never over-reported) in +/// `unique`/`loops`; the `backend` count is always exact. pub fn clear_storage( child_info: &ChildInfo, maybe_limit: Option, @@ -191,19 +197,37 @@ pub fn clear_storage( // not count them. Count the visible keys before and after so those overlay-only removals // are still reflected in `unique`/`loops`, which callers may use for weight or cleanup // accounting. - fn key_count(child_info: &ChildInfo) -> u32 { + // + // Both walks stop after `up_to` keys: without a bound, a limited clear of a large + // (potentially user-growable) child trie would perform O(total keys) `next_key` host calls + // just for accounting — the very unbounded-work pattern the `limit` exists to prevent. + fn key_count(child_info: &ChildInfo, up_to: u32) -> u32 { let mut count: u32 = if exists(child_info, &[]) { 1 } else { 0 }; let mut previous_key = Vec::new(); - while let Some(next_key) = - sp_io::default_child_storage::next_key(child_info.storage_key(), &previous_key) - { - count = count.saturating_add(1); - previous_key = next_key; + while count < up_to { + match sp_io::default_child_storage::next_key(child_info.storage_key(), &previous_key) + { + Some(next_key) => { + count = count.saturating_add(1); + previous_key = next_key; + }, + None => break, + } } count } - let keys_before = key_count(child_info); + // Extra headroom on top of `limit` for the counting walks. Overlay-resident keys (written + // earlier in the current block) do not count towards `limit`, so allow enumerating this many + // keys beyond it before giving up on exact overlay attribution. When both walks saturate at + // the cap the computed overlay removals collapse to zero rather than going negative. + const OVERLAY_ATTRIBUTION_CAP: u32 = 1024; + // With no limit the host call itself is already O(total keys), so exact counting does not + // change the asymptotic cost. + let count_cap = + maybe_limit.map_or(u32::MAX, |limit| limit.saturating_add(OVERLAY_ATTRIBUTION_CAP)); + + let keys_before = key_count(child_info, count_cap); // TODO: Once the network has upgraded to include the new host functions, this code can be // enabled. @@ -220,7 +244,7 @@ pub fn clear_storage( // Overlay-only removals = total keys deleted (before - after) minus those the host call // already accounted for in `backend`. - let keys_after = key_count(child_info); + let keys_after = key_count(child_info, count_cap); let overlay = keys_before.saturating_sub(keys_after).saturating_sub(backend); let total = backend.saturating_add(overlay); @@ -326,6 +350,35 @@ mod tests { }); } + #[test] + fn clear_storage_counting_is_bounded_for_large_tries() { + let child_info = ChildInfo::new_default(b"big-child"); + let mut ext = TestExternalities::new_empty(); + // Commit more backend keys than the counting cap (limit + 1024) can enumerate. + ext.execute_with(|| { + for i in 0u32..1_500 { + put_raw(&child_info, &i.to_le_bytes(), &[1]); + } + }); + ext.commit_all().unwrap(); + + ext.execute_with(|| { + // Stage a few overlay-only keys on top. + for i in 0u8..5 { + put_raw(&child_info, &[0xff, i], &[i]); + } + + // The counting walks saturate at the cap, so overlay attribution degrades to zero + // (documented under-reporting) instead of going negative or walking the whole trie. + let res = clear_storage(&child_info, Some(10), None); + assert_eq!(res.backend, 10, "backend removals are reported exactly"); + assert_eq!(res.unique, 10, "saturated counting must not inflate `unique`"); + for i in 0u8..5 { + assert!(!exists(&child_info, &[0xff, i]), "overlay keys are still deleted"); + } + }); + } + #[test] fn take_still_returns_and_clears_valid_entry() { TestExternalities::new_empty().execute_with(|| { From 1d611e031e383051e9e9f9422775cdc337f9c233 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 3 Jul 2026 14:45:08 +0800 Subject: [PATCH 44/47] Account for the KnownDerivatives read in wormhole weight model NonWormholeAccounts::contains now also checks Utility::KnownDerivatives (via is_derivative), so is_ambiguous_account costs one more storage read than the model assumed. Bump per_transfer_weight and the signer reveal weight from 4 to 5 reads and update the read breakdowns in the comments. Co-authored-by: Cursor --- runtime/src/transaction_extensions.rs | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/runtime/src/transaction_extensions.rs b/runtime/src/transaction_extensions.rs index 96a8a61c..8cc4955d 100644 --- a/runtime/src/transaction_extensions.rs +++ b/runtime/src/transaction_extensions.rs @@ -111,12 +111,13 @@ impl WormholeProofRecorderExtension /// Per recorded transfer, `record_transfer` touches (worst case): /// reads: TransferCount (1) + the `is_ambiguous_account` lookup on the recipient, /// which reads the recipient nonce (1) and, when nonce == 0, the - /// `NonWormholeAccounts` membership checks `Multisigs` (1) and the configured - /// `TreasuryAccount` (1) => 4 reads. + /// `NonWormholeAccounts` membership checks `Multisigs` (1), the utility + /// pallet's `KnownDerivatives` (1) and the configured `TreasuryAccount` (1) + /// => 5 reads. /// writes: TransferProof / ZK-tree leaf (1) + TransferCount (1) + the conditional /// `PotentialWormholeBalance` deposit add (1) => 3 writes. fn per_transfer_weight() -> Weight { - T::DbWeight::get().reads_writes(4, 3) + T::DbWeight::get().reads_writes(5, 3) } fn count_transfers(call: &RuntimeCall) -> u64 { @@ -245,10 +246,10 @@ impl TransactionEx if n > 0 { Self::per_transfer_weight().saturating_mul(n) } else { Weight::zero() }; // Soundness reveal bookkeeping for the signer. `validate` runs `is_ambiguous_account` on - // the signer — nonce (1) + `Multisigs` (1) + `TreasuryAccount` (1) — and, when ambiguous, - // reads the signer's balance (1): 4 reads worst case. `prepare` then writes - // `PotentialWormholeBalance` once. - let reveal_weight = T::DbWeight::get().reads_writes(4, 1); + // the signer — nonce (1) + `Multisigs` (1) + `KnownDerivatives` (1) + `TreasuryAccount` + // (1) — and, when ambiguous, reads the signer's balance (1): 5 reads worst case. + // `prepare` then writes `PotentialWormholeBalance` once. + let reveal_weight = T::DbWeight::get().reads_writes(5, 1); transfer_weight.saturating_add(reveal_weight) } @@ -716,10 +717,10 @@ mod tests { let ext = WormholeProofRecorderExtension::::new(); // Even non-transfer calls carry the constant soundness reveal-bookkeeping overhead - // (read signer nonce + multisig/treasury membership + balance, possibly write the - // pool), so the base weight is non-zero. + // (read signer nonce + multisig/derivative/treasury membership + balance, possibly + // write the pool), so the base weight is non-zero. let reveal_weight = - ::DbWeight::get().reads_writes(4, 1); + ::DbWeight::get().reads_writes(5, 1); let non_transfer = RuntimeCall::System(frame_system::Call::remark { remark: vec![1, 2, 3] }); let base_weight = as TransactionExtension< @@ -761,7 +762,7 @@ mod tests { new_test_ext().execute_with(|| { let ext = WormholeProofRecorderExtension::::new(); let reveal_weight = - ::DbWeight::get().reads_writes(4, 1); + ::DbWeight::get().reads_writes(5, 1); // A transfer hidden behind `as_derivative` (possibly batched) must be charged the // same per-transfer weight as a direct transfer. From 5a2981d36e0982459025e13627287cd6663114fa Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 3 Jul 2026 14:47:35 +0800 Subject: [PATCH 45/47] Skip the per-key value read in KeyPrefixIterator drains when unused The OnRemoval hook forced every drain to read the removed value even for hooks that ignore it (the no-op default and the counted-map counter update), costing a get_raw host call per key on plain iter_keys drains. Add a NEEDS_VALUE associated const so such hooks opt out of the read. Co-authored-by: Cursor --- frame/support/src/storage/mod.rs | 40 ++++++++++++------- .../support/src/storage/types/counted_map.rs | 2 + .../support/src/storage/types/counted_nmap.rs | 2 + 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 34ea4d61..85e77c55 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -1030,12 +1030,17 @@ impl PrefixIterator { /// Trait for specialising on removal logic of [`PrefixIterator`]. pub trait PrefixIteratorOnRemoval { + /// Whether [`Self::on_removal`] needs the removed value. When `false`, key-only drain + /// iterators skip the per-key value read and pass an empty slice instead, avoiding a host + /// call that would exist only to feed the hook. + const NEEDS_VALUE: bool = true; /// This function is called whenever a key/value is removed. fn on_removal(key: &[u8], value: &[u8]); } /// No-op implementation. impl PrefixIteratorOnRemoval for () { + const NEEDS_VALUE: bool = false; fn on_removal(_key: &[u8], _value: &[u8]) {} } @@ -1218,20 +1223,27 @@ impl Iterator for KeyPrefixIterator raw_value, - None => { - log::error!( - "next_key returned a key with no value at {:?}", - self.previous_key, - ); - continue - }, - }; - unhashed::kill(&self.previous_key); - OnRemoval::on_removal(&self.previous_key, &raw_value); + if OnRemoval::NEEDS_VALUE { + // The raw value is read before removal so the `OnRemoval` hook can + // observe what was deleted. + let raw_value = match unhashed::get_raw(&self.previous_key) { + Some(raw_value) => raw_value, + None => { + log::error!( + "next_key returned a key with no value at {:?}", + self.previous_key, + ); + continue + }, + }; + unhashed::kill(&self.previous_key); + OnRemoval::on_removal(&self.previous_key, &raw_value); + } else { + // The hook does not inspect the value (e.g. the counted map counter + // update), so skip the read that would exist only to feed it. + unhashed::kill(&self.previous_key); + OnRemoval::on_removal(&self.previous_key, &[]); + } } let raw_key_without_prefix = &self.previous_key[self.prefix.len()..]; diff --git a/frame/support/src/storage/types/counted_map.rs b/frame/support/src/storage/types/counted_map.rs index fc8974dc..deac72e8 100644 --- a/frame/support/src/storage/types/counted_map.rs +++ b/frame/support/src/storage/types/counted_map.rs @@ -132,6 +132,8 @@ pub struct OnRemovalCounterUpdate(core::marker::PhantomData); impl crate::storage::PrefixIteratorOnRemoval for OnRemovalCounterUpdate { + // Only the counter is touched; key-only drains need not read the removed value. + const NEEDS_VALUE: bool = false; fn on_removal(_key: &[u8], _value: &[u8]) { CounterFor::::mutate(|value| value.saturating_dec()); } diff --git a/frame/support/src/storage/types/counted_nmap.rs b/frame/support/src/storage/types/counted_nmap.rs index 2db1486b..2ff67e9c 100644 --- a/frame/support/src/storage/types/counted_nmap.rs +++ b/frame/support/src/storage/types/counted_nmap.rs @@ -126,6 +126,8 @@ pub struct OnRemovalCounterUpdate(core::marker::PhantomData); impl crate::storage::PrefixIteratorOnRemoval for OnRemovalCounterUpdate { + // Only the counter is touched; key-only drains need not read the removed value. + const NEEDS_VALUE: bool = false; fn on_removal(_key: &[u8], _value: &[u8]) { CounterFor::::mutate(|value| value.saturating_dec()); } From 3255146de183adc5d69804c9508ea838dfeaee66 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 3 Jul 2026 16:57:39 +0800 Subject: [PATCH 46/47] fmt --- frame/support/src/storage/child.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/support/src/storage/child.rs b/frame/support/src/storage/child.rs index ce1cca73..4dd868cb 100644 --- a/frame/support/src/storage/child.rs +++ b/frame/support/src/storage/child.rs @@ -205,8 +205,7 @@ pub fn clear_storage( let mut count: u32 = if exists(child_info, &[]) { 1 } else { 0 }; let mut previous_key = Vec::new(); while count < up_to { - match sp_io::default_child_storage::next_key(child_info.storage_key(), &previous_key) - { + match sp_io::default_child_storage::next_key(child_info.storage_key(), &previous_key) { Some(next_key) => { count = count.saturating_add(1); previous_key = next_key; From 893b1507efccf01f4590e7be443b9758af58e063 Mon Sep 17 00:00:00 2001 From: illuzen Date: Fri, 3 Jul 2026 18:57:00 +0800 Subject: [PATCH 47/47] Remove duplicated bad-ZK-tree-root test introduced by the merge Both main and this branch added block_import_of_bad_zk_tree_root_fails, and the merge auto-resolution kept both copies. Keep the variant that pins the expected panic message. Co-authored-by: Cursor --- frame/executive/src/tests.rs | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/frame/executive/src/tests.rs b/frame/executive/src/tests.rs index 1cd774a1..ca7b0b6b 100644 --- a/frame/executive/src/tests.rs +++ b/frame/executive/src/tests.rs @@ -730,28 +730,6 @@ fn block_import_of_bad_zk_tree_root_fails() { }); } -#[test] -#[should_panic] -fn block_import_of_bad_zk_tree_root_fails() { - new_test_ext(1).execute_with(|| { - let mut header = Header::new( - 1, - array_bytes::hex_n_into_unchecked( - "03170a2e7597b7b7e3d84c05391d139a62b157e78786d8c082f29dcf4c111314", - ), - array_bytes::hex_n_into_unchecked( - "d6b465f5a50c9f8d5a6edc0f01d285a6b19030f097d3aaf1649b7be81649f118", - ), - [69u8; 32].into(), - Digest { logs: vec![] }, - ); - // Forge a ZK tree root that does not match on-chain state (which is the default zero - // root). Before the `final_checks` fix this block would import successfully. - header.set_zk_tree_root([42u8; 32].into()); - Executive::execute_block(TestBlock { header, extrinsics: vec![] }.into()); - }); -} - #[test] #[should_panic] fn block_import_of_bad_extrinsic_root_fails() {