diff --git a/Cargo.lock b/Cargo.lock index a4831c56d116a..58517edbb675a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13623,6 +13623,7 @@ dependencies = [ "scale-info", "serde", "sp-application-crypto 30.0.0", + "sp-keystore 0.34.0", "sp-mixnet", ] diff --git a/prdoc/pr_11010.prdoc b/prdoc/pr_11010.prdoc new file mode 100644 index 0000000000000..e726c27062707 --- /dev/null +++ b/prdoc/pr_11010.prdoc @@ -0,0 +1,33 @@ +title: Migrate `pallet-mixnet` to use `TransactionExtension` API + +doc: + - audience: Runtime Dev + description: |- + Migrates `pallet-mixnet` from the deprecated `ValidateUnsigned` trait to the modern + `TransactionExtension` API using the `#[pallet::authorize]` attribute. + + This change uses the `#[pallet::authorize]` attribute to validate mixnet registration + transactions, providing better composability and flexibility. + + The pallet now uses `#[pallet::authorize]` on the `register` call to validate: + - Session index matches the current session (not future/stale) + - Authority index is valid and within bounds + - Authority ID exists in the authority set + - Authority hasn't already registered for this session + - Signature is valid for the registration data + + Runtime integrators: the pallet's `Config` trait now requires + `CreateAuthorizedTransaction>` instead of `CreateBare>`. + If your runtime already has a blanket `impl CreateAuthorizedTransaction for Runtime` + (as kitchensink does), no extra changes are needed. + + This serves as a reference example for other consensus/authority-based pallets + migrating away from `ValidateUnsigned`. + +crates: + - name: pallet-mixnet + bump: major + - name: kitchensink-runtime + bump: patch + - name: polkadot-sdk + bump: patch diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index cc56538769902..7c5129e879258 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -2496,6 +2496,7 @@ parameter_types! { } impl pallet_mixnet::Config for Runtime { + type WeightInfo = pallet_mixnet::weights::SubstrateWeight; type MaxAuthorities = MaxAuthorities; type MaxExternalAddressSize = ConstU32<128>; type MaxExternalAddressesPerMixnode = ConstU32<16>; @@ -3330,7 +3331,11 @@ mod benches { [pallet_asset_conversion_ops, AssetConversionMigration] [pallet_verify_signature, VerifySignature] [pallet_meta_tx, MetaTx] +<<<<<<< romarq/migrate-pallet-mixnet-to-tx-extension + [pallet_mixnet, Mixnet] +======= [pallet_psm, Psm] +>>>>>>> master ); } diff --git a/substrate/frame/mixnet/Cargo.toml b/substrate/frame/mixnet/Cargo.toml index 33bf7146980d5..bec546cbf16bc 100644 --- a/substrate/frame/mixnet/Cargo.toml +++ b/substrate/frame/mixnet/Cargo.toml @@ -24,6 +24,9 @@ serde = { features = ["derive"], workspace = true } sp-application-crypto = { workspace = true } sp-mixnet = { workspace = true } +[dev-dependencies] +sp-keystore = { workspace = true, default-features = true } + [features] default = ["std"] std = [ @@ -35,6 +38,9 @@ std = [ "sp-application-crypto/std", "sp-mixnet/std", ] +runtime-benchmarks = [ + "frame/runtime-benchmarks", +] try-runtime = [ "frame/try-runtime", ] diff --git a/substrate/frame/mixnet/src/benchmarking.rs b/substrate/frame/mixnet/src/benchmarking.rs new file mode 100644 index 0000000000000..2c341ba634ccf --- /dev/null +++ b/substrate/frame/mixnet/src/benchmarking.rs @@ -0,0 +1,88 @@ +// 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. + +//! Benchmarking for `pallet-mixnet`. + +#![cfg(feature = "runtime-benchmarks")] + +use super::*; +use frame::{benchmarking::prelude::*, deps::frame_support::traits::Authorize}; +use sp_mixnet::types::{AuthorityId, AuthoritySignature}; + +fn setup_registration() -> (RegistrationFor, AuthoritySignature) { + let session_index = 0u32; + let authority_index = 0u32; + + // Use RuntimeAppPublic to generate a keypair in the keystore. + // This works in both std and no_std (via host functions), unlike + // sp_core::Pair which requires the `full_crypto` feature (std only). + let authority_id = AuthorityId::generate_pair(None); + + pallet::CurrentSessionIndex::::put(session_index); + pallet::NextAuthorityIds::::insert(authority_index, authority_id.clone()); + + let registration = Registration { + block_number: 1u32.into(), + session_index, + authority_index, + mixnode: BoundedMixnode { + kx_public: [1u8; 32], + peer_id: [2u8; 32], + external_addresses: Default::default(), + }, + }; + let signature = authority_id + .sign(®istration.encode()) + .expect("Key was just generated and is in keystore; qed"); + + (registration, signature) +} + +#[benchmarks] +mod benchmarks { + use super::*; + + /// Measures the execution time of `register` dispatch. + #[benchmark] + fn register() { + let (registration, signature) = setup_registration::(); + let session_index = registration.session_index; + let authority_index = registration.authority_index; + + #[extrinsic_call] + _(RawOrigin::Authorized, registration, signature); + + assert!(pallet::Mixnodes::::contains_key(session_index + 1, authority_index)); + } + + /// Measures the weight of the authorize closure for `register`. + #[benchmark] + fn authorize_register() { + let (registration, signature) = setup_registration::(); + let call = pallet::Call::::register { registration, signature }; + let source = TransactionSource::External; + + #[block] + { + call.authorize(source) + .expect("Call should have authorize logic") + .expect("Authorization should succeed"); + } + } + + impl_benchmark_test_suite!(Pallet, crate::tests::new_test_ext(), crate::tests::Test); +} diff --git a/substrate/frame/mixnet/src/lib.rs b/substrate/frame/mixnet/src/lib.rs index edb43bf6f1c99..20afd634efb7a 100644 --- a/substrate/frame/mixnet/src/lib.rs +++ b/substrate/frame/mixnet/src/lib.rs @@ -23,6 +23,13 @@ extern crate alloc; +#[cfg(feature = "runtime-benchmarks")] +mod benchmarking; +#[cfg(test)] +mod tests; +pub mod weights; +pub use weights::*; + pub use pallet::*; use alloc::vec::Vec; @@ -30,7 +37,7 @@ use core::cmp::Ordering; use frame::{ deps::{ sp_io::{self, MultiRemovalResults}, - sp_runtime, + sp_runtime::{self, transaction_validity::TransactionValidityWithRefund}, }, prelude::*, }; @@ -178,8 +185,15 @@ pub mod pallet { #[pallet::pallet] pub struct Pallet(_); + /// # Requirements + /// + /// This pallet requires `frame_system::AuthorizeCall` to be included in the runtime's + /// transaction extension pipeline. #[pallet::config] - pub trait Config: frame_system::Config + CreateBare> { + pub trait Config: frame_system::Config + CreateAuthorizedTransaction> { + /// Weight functions for this pallet. + type WeightInfo: WeightInfo; + /// The maximum number of authorities per session. #[pallet::constant] type MaxAuthorities: Get; @@ -221,7 +235,7 @@ pub mod pallet { #[pallet::constant] type NumRegisterEndSlackBlocks: Get>; - /// Priority of unsigned transactions used to register mixnodes. + /// Priority of authorized transactions used to register mixnodes. #[pallet::constant] type RegistrationPriority: Get; @@ -280,58 +294,32 @@ pub mod pallet { impl Pallet { /// Register a mixnode for the following session. #[pallet::call_index(0)] - #[pallet::weight(1)] // TODO - pub fn register( - origin: OriginFor, - registration: RegistrationFor, - _signature: AuthoritySignature, - ) -> DispatchResult { - ensure_none(origin)?; - - // Checked by ValidateUnsigned - debug_assert_eq!(registration.session_index, CurrentSessionIndex::::get()); - debug_assert!(registration.authority_index < T::MaxAuthorities::get()); - - Mixnodes::::insert( - // Registering for the _following_ session - registration.session_index + 1, - registration.authority_index, - registration.mixnode, - ); - - Ok(()) - } - } - - #[allow(deprecated)] - #[pallet::validate_unsigned] - impl ValidateUnsigned for Pallet { - type Call = Call; - - fn validate_unsigned(_source: TransactionSource, call: &Self::Call) -> TransactionValidity { - let Self::Call::register { registration, signature } = call else { - return InvalidTransaction::Call.into(); - }; - + #[pallet::weight(T::WeightInfo::register())] + #[pallet::weight_of_authorize(T::WeightInfo::authorize_register())] + #[pallet::authorize(| + _source: TransactionSource, + registration: &RegistrationFor, + signature: &AuthoritySignature, + | -> TransactionValidityWithRefund { // Check session index matches match registration.session_index.cmp(&CurrentSessionIndex::::get()) { - Ordering::Greater => return InvalidTransaction::Future.into(), - Ordering::Less => return InvalidTransaction::Stale.into(), + Ordering::Greater => return Err(InvalidTransaction::Future.into()), + Ordering::Less => return Err(InvalidTransaction::Stale.into()), Ordering::Equal => (), } // Check authority index is valid if registration.authority_index >= T::MaxAuthorities::get() { - return InvalidTransaction::BadProof.into(); + return Err(InvalidTransaction::BadProof.into()); } let Some(authority_id) = NextAuthorityIds::::get(registration.authority_index) else { - return InvalidTransaction::BadProof.into(); + return Err(InvalidTransaction::BadProof.into()); }; // Check the authority hasn't registered a mixnode yet - if Self::already_registered(registration.session_index, registration.authority_index) { - return InvalidTransaction::Stale.into(); + if Pallet::::already_registered(registration.session_index, registration.authority_index) { + return Err(InvalidTransaction::Stale.into()); } // Check signature. Note that we don't use regular signed transactions for registration @@ -341,7 +329,7 @@ pub mod pallet { authority_id.verify(&encoded_registration, signature) }); if !signature_ok { - return InvalidTransaction::BadProof.into(); + return Err(InvalidTransaction::BadProof.into()); } ValidTransaction::with_tag_prefix("MixnetRegistration") @@ -359,6 +347,27 @@ pub mod pallet { .unwrap_or(64_u64), ) .build() + .map(|v| (v, /* no refund */ Weight::zero())) + })] + pub fn register( + origin: OriginFor, + registration: RegistrationFor, + _signature: AuthoritySignature, + ) -> DispatchResult { + ensure_authorized(origin)?; + + // Checked by authorize + debug_assert_eq!(registration.session_index, CurrentSessionIndex::::get()); + debug_assert!(registration.authority_index < T::MaxAuthorities::get()); + + Mixnodes::::insert( + // Registering for the _following_ session + registration.session_index + 1, + registration.authority_index, + registration.mixnode, + ); + + Ok(()) } } } @@ -533,7 +542,7 @@ impl Pallet { return false; }; let call = Call::register { registration, signature }; - let xt = T::create_bare(call.into()); + let xt = T::create_authorized_transaction(call.into()); match SubmitTransaction::>::submit_transaction(xt) { Ok(()) => true, Err(()) => { diff --git a/substrate/frame/mixnet/src/tests.rs b/substrate/frame/mixnet/src/tests.rs new file mode 100644 index 0000000000000..c87ac73238e1a --- /dev/null +++ b/substrate/frame/mixnet/src/tests.rs @@ -0,0 +1,212 @@ +// 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. + +#![cfg(test)] + +use super::*; +use frame::{ + deps::{ + frame_support::traits::Authorize, + sp_core::{sr25519, Pair}, + sp_runtime::{ + self, + testing::TestXt, + transaction_validity::{ + InvalidTransaction, TransactionSource, TransactionValidityError, + }, + }, + }, + testing_prelude::*, +}; +use sp_mixnet::types::{AuthorityId, AuthoritySignature}; + +type TxExtension = frame_system::AuthorizeCall; +type Extrinsic = TestXt; +type Block = sp_runtime::generic::Block< + sp_runtime::generic::Header, + Extrinsic, +>; + +construct_runtime!( + pub enum Test { + System: frame_system, + Mixnet: crate, + } +); + +#[derive_impl(frame_system::config_preludes::TestDefaultConfig)] +impl frame_system::Config for Test { + type Block = Block; +} + +impl frame_system::offchain::SigningTypes for Test { + type Public = sp_runtime::testing::UintAuthorityId; + type Signature = sp_runtime::testing::TestSignature; +} + +impl frame_system::offchain::CreateTransactionBase for Test +where + RuntimeCall: From, +{ + type RuntimeCall = RuntimeCall; + type Extrinsic = Extrinsic; +} + +impl frame_system::offchain::CreateTransaction for Test +where + RuntimeCall: From, +{ + type Extension = TxExtension; + fn create_transaction(call: RuntimeCall, extension: Self::Extension) -> Extrinsic { + Extrinsic::new_transaction(call, extension) + } +} + +impl frame_system::offchain::CreateAuthorizedTransaction for Test +where + RuntimeCall: From, +{ + fn create_extension() -> Self::Extension { + TxExtension::new() + } +} + +parameter_types! { + pub const RegistrationPriority: TransactionPriority = 1 << 20; +} + +impl Config for Test { + type WeightInfo = (); + type MaxAuthorities = ConstU32<10>; + type MaxExternalAddressSize = ConstU32<64>; + type MaxExternalAddressesPerMixnode = ConstU32<4>; + type NextSessionRotation = (); + type NumCoverToCurrentBlocks = ConstU64<3>; + type NumRequestsToCurrentBlocks = ConstU64<3>; + type NumCoverToPrevBlocks = ConstU64<3>; + type NumRegisterStartSlackBlocks = ConstU64<3>; + type NumRegisterEndSlackBlocks = ConstU64<3>; + type RegistrationPriority = RegistrationPriority; + type MinMixnodes = ConstU32<1>; +} + +pub(crate) fn new_test_ext() -> TestState { + use sp_keystore::{testing::MemoryKeystore, KeystoreExt}; + + let t = frame_system::GenesisConfig::::default().build_storage().unwrap(); + let mut ext = TestState::new(t); + ext.register_extension(KeystoreExt::new(MemoryKeystore::new())); + ext +} + +fn generate_authority() -> (sr25519::Pair, AuthorityId) { + let (pair, _) = sr25519::Pair::generate(); + let authority_id = AuthorityId::from(pair.public()); + (pair, authority_id) +} + +fn test_mixnode() -> BoundedMixnodeFor { + BoundedMixnode { + kx_public: [1u8; 32], + peer_id: [2u8; 32], + external_addresses: Default::default(), + } +} + +fn create_registration( + session_index: u32, + authority_index: AuthorityIndex, +) -> RegistrationFor { + Registration { block_number: 1u64, session_index, authority_index, mixnode: test_mixnode() } +} + +fn sign_registration( + pair: &sr25519::Pair, + registration: &RegistrationFor, +) -> AuthoritySignature { + AuthoritySignature::from(pair.sign(®istration.encode())) +} + +fn setup_authority(authority_index: AuthorityIndex) -> (sr25519::Pair, AuthorityId) { + let (pair, authority_id) = generate_authority(); + pallet::NextAuthorityIds::::insert(authority_index, authority_id.clone()); + (pair, authority_id) +} + +fn make_register_call( + registration: RegistrationFor, + signature: AuthoritySignature, +) -> RuntimeCall { + RuntimeCall::Mixnet(pallet::Call::register { registration, signature }) +} + +#[test] +fn authorize_accepts_valid_registration() { + new_test_ext().execute_with(|| { + pallet::CurrentSessionIndex::::put(0u32); + let (pair, _) = setup_authority(0); + + let registration = create_registration(0, 0); + let signature = sign_registration(&pair, ®istration); + let call = make_register_call(registration, signature); + + let result = call.authorize(TransactionSource::External); + assert!(result.is_some(), "Call should have authorize logic"); + assert!(result.unwrap().is_ok(), "Valid registration should be accepted"); + }); +} + +#[test] +fn authorize_rejects_bad_signature() { + new_test_ext().execute_with(|| { + pallet::CurrentSessionIndex::::put(0u32); + let (_pair, _) = setup_authority(0); + + let registration = create_registration(0, 0); + // Sign with a different key + let (wrong_pair, _) = generate_authority(); + let signature = sign_registration(&wrong_pair, ®istration); + let call = make_register_call(registration, signature); + + let result = call.authorize(TransactionSource::External); + assert!(result.is_some()); + assert_eq!( + result.unwrap(), + Err(TransactionValidityError::Invalid(InvalidTransaction::BadProof)) + ); + }); +} + +#[test] +fn register_dispatches_with_authorized_origin() { + new_test_ext().execute_with(|| { + pallet::CurrentSessionIndex::::put(0u32); + let (pair, _) = setup_authority(0); + + let registration = create_registration(0, 0); + let signature = sign_registration(&pair, ®istration); + + assert_ok!(Mixnet::register( + RuntimeOrigin::from(frame_system::RawOrigin::Authorized), + registration, + signature, + )); + + // Verify the mixnode was stored for the next session + assert!(pallet::Mixnodes::::contains_key(1u32, 0u32)); + }); +} diff --git a/substrate/frame/mixnet/src/weights.rs b/substrate/frame/mixnet/src/weights.rs new file mode 100644 index 0000000000000..26acfb52e1fed --- /dev/null +++ b/substrate/frame/mixnet/src/weights.rs @@ -0,0 +1,79 @@ +// 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. + +//! Autogenerated weights for `pallet_mixnet` +//! +//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev +//! DATE: 2024-01-01, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! WORST CASE MAP SIZE: `1000000` +//! +//! These weights are placeholder values. Run the benchmarks to generate accurate weights: +//! ```text +//! cargo run --release --features runtime-benchmarks -- benchmark pallet \ +//! --pallet pallet_mixnet --extrinsic '*' --steps 50 --repeat 20 +//! ``` + +#![cfg_attr(rustfmt, rustfmt_skip)] +#![allow(unused_parens)] +#![allow(unused_imports)] +#![allow(missing_docs)] + +use frame::{ + deps::frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}, + prelude::*, +}; +use core::marker::PhantomData; + +/// Weight functions needed for `pallet_mixnet`. +pub trait WeightInfo { + fn register() -> Weight; + fn authorize_register() -> Weight; +} + +/// Weight functions for `pallet_mixnet`. +pub struct SubstrateWeight(PhantomData); +impl WeightInfo for SubstrateWeight { + /// Storage: `Mixnet::Mixnodes` (r:0 w:1) + fn register() -> Weight { + Weight::from_parts(10_000_000, 0) + .saturating_add(T::DbWeight::get().writes(1)) + } + + /// Storage: `Mixnet::CurrentSessionIndex` (r:1 w:0) + /// Storage: `Mixnet::NextAuthorityIds` (r:1 w:0) + /// Storage: `Mixnet::Mixnodes` (r:1 w:0) + fn authorize_register() -> Weight { + Weight::from_parts(50_000_000, 0) + .saturating_add(T::DbWeight::get().reads(3)) + } +} + +impl WeightInfo for () { + /// Storage: `Mixnet::Mixnodes` (r:0 w:1) + fn register() -> Weight { + Weight::from_parts(10_000_000, 0) + .saturating_add(RocksDbWeight::get().writes(1)) + } + + /// Storage: `Mixnet::CurrentSessionIndex` (r:1 w:0) + /// Storage: `Mixnet::NextAuthorityIds` (r:1 w:0) + /// Storage: `Mixnet::Mixnodes` (r:1 w:0) + fn authorize_register() -> Weight { + Weight::from_parts(50_000_000, 0) + .saturating_add(RocksDbWeight::get().reads(3)) + } +} diff --git a/umbrella/Cargo.toml b/umbrella/Cargo.toml index b52c2a416a381..d44ff95e1cd9b 100644 --- a/umbrella/Cargo.toml +++ b/umbrella/Cargo.toml @@ -310,6 +310,7 @@ runtime-benchmarks = [ "pallet-message-queue?/runtime-benchmarks", "pallet-meta-tx?/runtime-benchmarks", "pallet-migrations?/runtime-benchmarks", + "pallet-mixnet?/runtime-benchmarks", "pallet-mmr?/runtime-benchmarks", "pallet-multi-asset-bounties?/runtime-benchmarks", "pallet-multisig?/runtime-benchmarks",