diff --git a/polkadot/runtime/common/src/assigned_slots/mod.rs b/polkadot/runtime/common/src/assigned_slots/mod.rs index 65942c127b1c..7ff7031de989 100644 --- a/polkadot/runtime/common/src/assigned_slots/mod.rs +++ b/polkadot/runtime/common/src/assigned_slots/mod.rs @@ -682,6 +682,28 @@ mod tests { } } + impl frame_system::offchain::CreateTransaction for Test + where + RuntimeCall: From, + { + type Extension = (); + fn create_transaction( + call: >::RuntimeCall, + extension: Self::Extension, + ) -> Self::Extrinsic { + UncheckedExtrinsic::new_transaction(call, extension) + } + } + + impl frame_system::offchain::CreateAuthorizedTransaction for Test + where + RuntimeCall: From, + { + fn create_extension() -> Self::Extension { + () + } + } + #[derive_impl(frame_system::config_preludes::TestDefaultConfig)] impl frame_system::Config for Test { type BaseCallFilter = frame_support::traits::Everything; diff --git a/polkadot/runtime/common/src/claims.rs b/polkadot/runtime/common/src/claims.rs index 1ee80dd76e2d..fade84779bb8 100644 --- a/polkadot/runtime/common/src/claims.rs +++ b/polkadot/runtime/common/src/claims.rs @@ -51,9 +51,13 @@ type CurrencyOf = <::VestingSchedule as VestingSchedule< type BalanceOf = as Currency<::AccountId>>::Balance; pub trait WeightInfo { - fn claim() -> Weight; + fn claim_with_validate_unsigned() -> Weight; + fn claim_without_validate_unsigned() -> Weight; + fn authorize_claim() -> Weight; fn mint_claim() -> Weight; - fn claim_attest() -> Weight; + fn claim_attest_with_validate_unsigned() -> Weight; + fn claim_attest_without_validate_unsigned() -> Weight; + fn authorize_claim_attest() -> Weight; fn attest() -> Weight; fn move_claim() -> Weight; fn prevalidate_attests() -> Weight; @@ -61,13 +65,25 @@ pub trait WeightInfo { pub struct TestWeightInfo; impl WeightInfo for TestWeightInfo { - fn claim() -> Weight { + fn claim_with_validate_unsigned() -> Weight { + Weight::zero() + } + fn claim_without_validate_unsigned() -> Weight { + Weight::zero() + } + fn authorize_claim() -> Weight { Weight::zero() } fn mint_claim() -> Weight { Weight::zero() } - fn claim_attest() -> Weight { + fn claim_attest_with_validate_unsigned() -> Weight { + Weight::zero() + } + fn claim_attest_without_validate_unsigned() -> Weight { + Weight::zero() + } + fn authorize_claim_attest() -> Weight { Weight::zero() } fn attest() -> Weight { @@ -289,13 +305,14 @@ pub mod pallet { #[pallet::hooks] impl Hooks> for Pallet {} - #[pallet::call] + #[pallet::call(weight = T::WeightInfo)] impl Pallet { /// Make a claim to collect your DOTs. /// - /// The dispatch origin for this call must be _None_. + /// The dispatch origin for this call must be _None_ or _Authorized_. + /// It can be dispatched with a general transaction or an unsigned transaction. /// - /// Unsigned Validation: + /// Validation/Authorization: /// A call to claim is deemed valid if the signature provided matches /// the expected signed message of: /// @@ -316,21 +333,27 @@ pub mod pallet { /// Total Complexity: O(1) /// #[pallet::call_index(0)] - #[pallet::weight(T::WeightInfo::claim())] + #[pallet::authorize(|_source, dest, ethereum_sig| + Pallet::::validate_claim(dest, ethereum_sig, None).map(|v| (v, Weight::zero())) + )] + #[pallet::weight(T::WeightInfo::claim_with_validate_unsigned())] pub fn claim( origin: OriginFor, dest: T::AccountId, ethereum_signature: EcdsaSignature, - ) -> DispatchResult { - ensure_none(origin)?; + ) -> DispatchResultWithPostInfo { + let is_authorized = ensure_authorized(origin.clone()).is_ok(); + let is_none = ensure_none(origin).is_ok(); - let data = dest.using_encoded(to_ascii_hex); - let signer = Self::eth_recover(ðereum_signature, &data, &[][..]) - .ok_or(Error::::InvalidEthereumSignature)?; - ensure!(Signing::::get(&signer).is_none(), Error::::InvalidStatement); + ensure!(is_authorized || is_none, DispatchError::BadOrigin); - Self::process_claim(signer, dest)?; - Ok(()) + Self::do_claim(dest, ethereum_signature)?; + + if is_authorized { + Ok(Some(T::WeightInfo::claim_without_validate_unsigned()).into()) + } else { + Ok(().into()) + } } /// Mint a new claim to collect DOTs. @@ -372,9 +395,10 @@ pub mod pallet { /// Make a claim to collect your DOTs by signing a statement. /// - /// The dispatch origin for this call must be _None_. + /// The dispatch origin for this call must be _None_ or _Authorized_. + /// It can be dispatched with a general transaction or an unsigned transaction. /// - /// Unsigned Validation: + /// Validation/Authorization: /// A call to `claim_attest` is deemed valid if the signature provided matches /// the expected signed message of: /// @@ -398,23 +422,28 @@ pub mod pallet { /// Total Complexity: O(1) /// #[pallet::call_index(2)] - #[pallet::weight(T::WeightInfo::claim_attest())] + #[pallet::authorize(|_source, dest, ethereum_sig, stmt| + Pallet::::validate_claim(dest, ethereum_sig, Some(stmt)).map(|v| (v, Weight::zero())) + )] + #[pallet::weight(T::WeightInfo::claim_attest_with_validate_unsigned())] pub fn claim_attest( origin: OriginFor, dest: T::AccountId, ethereum_signature: EcdsaSignature, statement: Vec, - ) -> DispatchResult { - ensure_none(origin)?; + ) -> DispatchResultWithPostInfo { + let is_authorized = ensure_authorized(origin.clone()).is_ok(); + let is_none = ensure_none(origin).is_ok(); - let data = dest.using_encoded(to_ascii_hex); - let signer = Self::eth_recover(ðereum_signature, &data, &statement) - .ok_or(Error::::InvalidEthereumSignature)?; - if let Some(s) = Signing::::get(signer) { - ensure!(s.to_text() == &statement[..], Error::::InvalidStatement); + ensure!(is_authorized || is_none, DispatchError::BadOrigin); + + Self::do_claim_attest(dest, ethereum_signature, statement)?; + + if is_authorized { + Ok(Some(T::WeightInfo::claim_attest_without_validate_unsigned()).into()) + } else { + Ok(().into()) } - Self::process_claim(signer, dest)?; - Ok(()) } /// Attest to a statement, needed to finalize the claims process. @@ -482,27 +511,61 @@ pub mod pallet { type Call = Call; fn validate_unsigned(_source: TransactionSource, call: &Self::Call) -> TransactionValidity { - const PRIORITY: u64 = 100; - - let (maybe_signer, maybe_statement) = match call { + match call { // // The weight of this logic is included in the `claim` dispatchable. // - Call::claim { dest: account, ethereum_signature } => { - let data = account.using_encoded(to_ascii_hex); - (Self::eth_recover(ðereum_signature, &data, &[][..]), None) - }, + Call::claim { dest: account, ethereum_signature } => + Self::validate_claim(account, ethereum_signature, None), // // The weight of this logic is included in the `claim_attest` dispatchable. // - Call::claim_attest { dest: account, ethereum_signature, statement } => { - let data = account.using_encoded(to_ascii_hex); - ( - Self::eth_recover(ðereum_signature, &data, &statement), - Some(statement.as_slice()), - ) - }, + Call::claim_attest { dest: account, ethereum_signature, statement } => + Self::validate_claim(account, ethereum_signature, Some(statement)), _ => return Err(InvalidTransaction::Call.into()), + } + } + } + + impl Pallet { + fn do_claim(dest: T::AccountId, ethereum_signature: EcdsaSignature) -> DispatchResult { + let data = dest.using_encoded(to_ascii_hex); + let signer = Self::eth_recover(ðereum_signature, &data, &[][..]) + .ok_or(Error::::InvalidEthereumSignature)?; + ensure!(Signing::::get(&signer).is_none(), Error::::InvalidStatement); + + Self::process_claim(signer, dest)?; + Ok(()) + } + + fn do_claim_attest( + dest: T::AccountId, + ethereum_signature: EcdsaSignature, + statement: Vec, + ) -> DispatchResult { + let data = dest.using_encoded(to_ascii_hex); + let signer = Self::eth_recover(ðereum_signature, &data, &statement) + .ok_or(Error::::InvalidEthereumSignature)?; + if let Some(s) = Signing::::get(signer) { + ensure!(s.to_text() == &statement[..], Error::::InvalidStatement); + } + Self::process_claim(signer, dest)?; + Ok(()) + } + + fn validate_claim( + account: &T::AccountId, + ethereum_signature: &EcdsaSignature, + maybe_statement: Option<&Vec>, + ) -> TransactionValidity { + const PRIORITY: u64 = 100; + + let maybe_signer = if let Some(statement) = &maybe_statement { + let data = account.using_encoded(to_ascii_hex); + Self::eth_recover(ðereum_signature, &data, statement) + } else { + let data = account.using_encoded(to_ascii_hex); + Self::eth_recover(ðereum_signature, &data, &[][..]) }; let signer = maybe_signer.ok_or(InvalidTransaction::Custom( @@ -515,7 +578,7 @@ pub mod pallet { let e = InvalidTransaction::Custom(ValidityError::InvalidStatement.into()); match Signing::::get(signer) { None => ensure!(maybe_statement.is_none(), e), - Some(s) => ensure!(Some(s.to_text()) == maybe_statement, e), + Some(s) => ensure!(Some(s.to_text()) == maybe_statement.map(|s| s.as_slice()), e), } Ok(ValidTransaction { @@ -718,8 +781,8 @@ mod tests { use super::*; use hex_literal::hex; use secp_utils::*; - use sp_runtime::transaction_validity::TransactionSource::External; + use frame_support::pallet_prelude::TransactionSource::External; use codec::Encode; // The testing primitives are very useful for avoiding having to work with signatures // or public keys. `u64` is used as the `AccountId` and no `Signature`s are required. @@ -733,14 +796,24 @@ mod tests { }; use pallet_balances; use sp_runtime::{ - traits::{DispatchTransaction, Identity}, - transaction_validity::TransactionLongevity, + testing::UintAuthorityId, + traits::{Applyable, Checkable, DispatchTransaction, Identity}, + transaction_validity::{TransactionLongevity, TransactionSource}, BuildStorage, DispatchError::BadOrigin, TokenError, }; - type Block = frame_system::mocking::MockBlock; + pub type TransactionExtension = frame_system::AuthorizeCall; + + pub type Header = sp_runtime::generic::Header; + pub type Block = sp_runtime::generic::Block; + pub type UncheckedExtrinsic = sp_runtime::generic::UncheckedExtrinsic< + u64, + RuntimeCall, + UintAuthorityId, + TransactionExtension, + >; frame_support::construct_runtime!( pub enum Test @@ -754,12 +827,8 @@ mod tests { #[derive_impl(frame_system::config_preludes::TestDefaultConfig)] impl frame_system::Config for Test { - type RuntimeOrigin = RuntimeOrigin; - type RuntimeCall = RuntimeCall; type Block = Block; - type RuntimeEvent = RuntimeEvent; type AccountData = pallet_balances::AccountData; - type MaxConsumers = frame_support::traits::ConstU32<16>; } #[derive_impl(pallet_balances::config_preludes::TestDefaultConfig)] @@ -864,7 +933,8 @@ mod tests { } #[test] - fn claiming_works() { + #[allow(deprecated)] + fn legacy_claiming_works() { new_test_ext().execute_with(|| { assert_eq!(Balances::free_balance(42), 0); assert_ok!(Claims::claim( @@ -878,6 +948,21 @@ mod tests { }); } + #[test] + fn claiming_works() { + new_test_ext().execute_with(|| { + assert_eq!(Balances::free_balance(42), 0); + assert_ok!(Claims::claim( + frame_system::RawOrigin::Authorized.into(), + 42, + sig::(&alice(), &42u64.encode(), &[][..]) + )); + assert_eq!(Balances::free_balance(&42), 100); + assert_eq!(Vesting::vesting_balance(&42), Some(50)); + assert_eq!(claims::Total::::get(), total_claims() - 100); + }); + } + #[test] fn basic_claim_moving_works() { new_test_ext().execute_with(|| { @@ -894,14 +979,14 @@ mod tests { )); assert_noop!( Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 42, sig::(&alice(), &42u64.encode(), &[][..]) ), Error::::SignerHasNoClaim ); assert_ok!(Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 42, sig::(&bob(), &42u64.encode(), &[][..]) )); @@ -912,7 +997,8 @@ mod tests { } #[test] - fn claim_attest_moving_works() { + #[allow(deprecated)] + fn legacy_claim_attest_moving_works() { new_test_ext().execute_with(|| { assert_ok!(Claims::move_claim( RuntimeOrigin::signed(6), @@ -931,6 +1017,26 @@ mod tests { }); } + #[test] + fn claim_attest_moving_works() { + new_test_ext().execute_with(|| { + assert_ok!(Claims::move_claim( + RuntimeOrigin::signed(6), + eth(&dave()), + eth(&bob()), + None + )); + let s = sig::(&bob(), &42u64.encode(), StatementKind::Regular.to_text()); + assert_ok!(Claims::claim_attest( + frame_system::RawOrigin::Authorized.into(), + 42, + s, + StatementKind::Regular.to_text().to_vec() + )); + assert_eq!(Balances::free_balance(&42), 200); + }); + } + #[test] fn attest_moving_works() { new_test_ext().execute_with(|| { @@ -952,13 +1058,13 @@ mod tests { fn claiming_does_not_bypass_signing() { new_test_ext().execute_with(|| { assert_ok!(Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 42, sig::(&alice(), &42u64.encode(), &[][..]) )); assert_noop!( Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 42, sig::(&dave(), &42u64.encode(), &[][..]) ), @@ -966,14 +1072,14 @@ mod tests { ); assert_noop!( Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 42, sig::(&eve(), &42u64.encode(), &[][..]) ), Error::::InvalidStatement, ); assert_ok!(Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 42, sig::(&frank(), &42u64.encode(), &[][..]) )); @@ -986,7 +1092,7 @@ mod tests { assert_eq!(Balances::free_balance(42), 0); let s = sig::(&dave(), &42u64.encode(), StatementKind::Saft.to_text()); let r = Claims::claim_attest( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 42, s.clone(), StatementKind::Saft.to_text().to_vec(), @@ -994,7 +1100,7 @@ mod tests { assert_noop!(r, Error::::InvalidStatement); let r = Claims::claim_attest( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 42, s, StatementKind::Regular.to_text().to_vec(), @@ -1005,7 +1111,7 @@ mod tests { let s = sig::(&dave(), &42u64.encode(), StatementKind::Regular.to_text()); assert_ok!(Claims::claim_attest( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 42, s, StatementKind::Regular.to_text().to_vec() @@ -1015,7 +1121,7 @@ mod tests { let s = sig::(&dave(), &42u64.encode(), StatementKind::Regular.to_text()); let r = Claims::claim_attest( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 42, s, StatementKind::Regular.to_text().to_vec(), @@ -1054,7 +1160,7 @@ mod tests { assert_eq!(Balances::free_balance(42), 0); // Alice's claim is 100 assert_ok!(Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 42, sig::(&alice(), &42u64.encode(), &[][..]) )); @@ -1107,7 +1213,7 @@ mod tests { new_test_ext().execute_with(|| { assert_eq!(Balances::free_balance(42), 0); let s = sig::(&dave(), &42u64.encode(), &[]); - let r = Claims::claim(RuntimeOrigin::none(), 42, s.clone()); + let r = Claims::claim(frame_system::RawOrigin::Authorized.into(), 42, s.clone()); assert_noop!(r, Error::::InvalidStatement); }); } @@ -1122,7 +1228,7 @@ mod tests { assert_eq!(Balances::free_balance(42), 0); assert_noop!( Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 69, sig::(&bob(), &69u64.encode(), &[][..]) ), @@ -1131,7 +1237,7 @@ mod tests { assert_ok!(Claims::mint_claim(RuntimeOrigin::root(), eth(&bob()), 200, None, None)); assert_eq!(claims::Total::::get(), total_claims() + 200); assert_ok!(Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 69, sig::(&bob(), &69u64.encode(), &[][..]) )); @@ -1157,7 +1263,7 @@ mod tests { assert_eq!(Balances::free_balance(42), 0); assert_noop!( Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 69, sig::(&bob(), &69u64.encode(), &[][..]) ), @@ -1171,7 +1277,7 @@ mod tests { None )); assert_ok!(Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 69, sig::(&bob(), &69u64.encode(), &[][..]) )); @@ -1208,7 +1314,7 @@ mod tests { let signature = sig::(&bob(), &69u64.encode(), StatementKind::Regular.to_text()); assert_noop!( Claims::claim_attest( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 69, signature.clone(), StatementKind::Regular.to_text().to_vec() @@ -1223,11 +1329,16 @@ mod tests { Some(StatementKind::Regular) )); assert_noop!( - Claims::claim_attest(RuntimeOrigin::none(), 69, signature.clone(), vec![],), + Claims::claim_attest( + frame_system::RawOrigin::Authorized.into(), + 69, + signature.clone(), + vec![], + ), Error::::SignerHasNoClaim ); assert_ok!(Claims::claim_attest( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 69, signature.clone(), StatementKind::Regular.to_text().to_vec() @@ -1256,13 +1367,13 @@ mod tests { new_test_ext().execute_with(|| { assert_eq!(Balances::free_balance(42), 0); assert_ok!(Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 42, sig::(&alice(), &42u64.encode(), &[][..]) )); assert_noop!( Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 42, sig::(&alice(), &42u64.encode(), &[][..]) ), @@ -1296,7 +1407,7 @@ mod tests { // They should not be able to claim assert_noop!( Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 69, sig::(&bob(), &69u64.encode(), &[][..]) ), @@ -1311,7 +1422,7 @@ mod tests { assert_eq!(Balances::free_balance(42), 0); assert_noop!( Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 42, sig::(&alice(), &69u64.encode(), &[][..]) ), @@ -1326,7 +1437,7 @@ mod tests { assert_eq!(Balances::free_balance(42), 0); assert_noop!( Claims::claim( - RuntimeOrigin::none(), + frame_system::RawOrigin::Authorized.into(), 42, sig::(&bob(), &69u64.encode(), &[][..]) ), @@ -1448,6 +1559,85 @@ mod tests { ); }); } + + #[test] + fn test_legacy_valid_tx() { + new_test_ext().execute_with(|| { + let call = RuntimeCall::Claims(ClaimsCall::claim { + dest: 42, + ethereum_signature: sig::(&alice(), &42u64.encode(), &[][..]), + }); + let tx = UncheckedExtrinsic::new_bare(call); + + let info = tx.get_dispatch_info(); + let len = tx.using_encoded(|e| e.len()); + + let checked = Checkable::check(tx, &frame_system::ChainContext::::default()) + .expect("Signature is good"); + + checked + .validate::(TransactionSource::External, &info, len) + .expect("Transaction is valid"); + + checked + .apply::(&info, len) + .expect("Transaction is valid") + .expect("Transaction is successful"); + }); + } + + #[test] + fn test_valid_tx() { + new_test_ext().execute_with(|| { + let call = RuntimeCall::Claims(ClaimsCall::claim { + dest: 42, + ethereum_signature: sig::(&alice(), &42u64.encode(), &[][..]), + }); + let tx = UncheckedExtrinsic::new_bare(call); + + let info = tx.get_dispatch_info(); + let len = tx.using_encoded(|e| e.len()); + + let checked = Checkable::check(tx, &frame_system::ChainContext::::default()) + .expect("Signature is good"); + + checked + .validate::(TransactionSource::External, &info, len) + .expect("Transaction is valid"); + + checked + .apply::(&info, len) + .expect("Transaction is valid") + .expect("Transaction is successful"); + }); + } + + #[test] + fn test_invalid_signed_tx() { + new_test_ext().execute_with(|| { + let call = RuntimeCall::Claims(ClaimsCall::claim { + dest: 42, + ethereum_signature: sig::(&alice(), &42u64.encode(), &[][..]), + }); + let tx_ext = frame_system::AuthorizeCall::new(); + let tx = UncheckedExtrinsic::new_signed(call, 4u64, 4.into(), tx_ext); + + let info = tx.get_dispatch_info(); + let len = tx.using_encoded(|e| e.len()); + + let checked = Checkable::check(tx, &frame_system::ChainContext::::default()) + .expect("Signature is good"); + + checked + .validate::(TransactionSource::External, &info, len) + .expect("Transaction is valid"); + + checked + .apply::(&info, len) + .expect("Transaction is valid") + .expect_err("Transaction is failing"); + }); + } } #[cfg(feature = "runtime-benchmarks")] @@ -1511,7 +1701,7 @@ mod benchmarking { // Benchmark `claim` including `validate_unsigned` logic. #[benchmark] - fn claim() -> Result<(), BenchmarkError> { + fn claim_with_validate_unsigned() -> Result<(), BenchmarkError> { let c = MAX_CLAIMS; for _ in 0..c / 2 { create_claim::(c)?; @@ -1548,6 +1738,39 @@ mod benchmarking { Ok(()) } + // Benchmark `claim` without `validate_unsigned` logic. + #[benchmark] + fn claim_without_validate_unsigned() -> Result<(), BenchmarkError> { + let c = MAX_CLAIMS; + for _ in 0..c / 2 { + create_claim::(c)?; + create_claim_attest::(u32::MAX - c)?; + } + let secret_key = libsecp256k1::SecretKey::parse(&keccak_256(&c.encode())).unwrap(); + let eth_address = eth(&secret_key); + let account: T::AccountId = account("user", c, SEED); + let vesting = Some((100_000u32.into(), 1_000u32.into(), 100u32.into())); + let signature = sig::(&secret_key, &account.encode(), &[][..]); + super::Pallet::::mint_claim( + RawOrigin::Root.into(), + eth_address, + VALUE.into(), + vesting, + None, + )?; + assert_eq!(Claims::::get(eth_address), Some(VALUE.into())); + let call_enc = + Call::::claim { dest: account.clone(), ethereum_signature: signature.clone() }; + + #[block] + { + call.dispatch_bypass_filter(RawOrigin::None.into())?; + } + + assert_eq!(Claims::::get(eth_address), None); + Ok(()) + } + // Benchmark `mint_claim` when there already exists `c` claims in storage. #[benchmark] fn mint_claim() -> Result<(), BenchmarkError> { @@ -1569,7 +1792,7 @@ mod benchmarking { // Benchmark `claim_attest` including `validate_unsigned` logic. #[benchmark] - fn claim_attest() -> Result<(), BenchmarkError> { + fn claim_attest_with_validate_unsigned() -> Result<(), BenchmarkError> { let c = MAX_CLAIMS; for _ in 0..c / 2 { create_claim::(c)?; @@ -1613,6 +1836,46 @@ mod benchmarking { Ok(()) } + // Benchmark `claim_attest` without `validate_unsigned` logic. + #[benchmark] + fn claim_attest_without_validate_unsigned() -> Result<(), BenchmarkError> { + let c = MAX_CLAIMS; + for _ in 0..c / 2 { + create_claim::(c)?; + create_claim_attest::(u32::MAX - c)?; + } + // Crate signature + let attest_c = u32::MAX - c; + let secret_key = + libsecp256k1::SecretKey::parse(&keccak_256(&attest_c.encode())).unwrap(); + let eth_address = eth(&secret_key); + let account: T::AccountId = account("user", c, SEED); + let vesting = Some((100_000u32.into(), 1_000u32.into(), 100u32.into())); + let statement = StatementKind::Regular; + let signature = sig::(&secret_key, &account.encode(), statement.to_text()); + super::Pallet::::mint_claim( + RawOrigin::Root.into(), + eth_address, + VALUE.into(), + vesting, + Some(statement), + )?; + assert_eq!(Claims::::get(eth_address), Some(VALUE.into())); + let call = Call::::claim_attest { + dest: account.clone(), + ethereum_signature: signature.clone(), + statement: StatementKind::Regular.to_text().to_vec(), + }; + + #[block] + { + call.dispatch_bypass_filter(RawOrigin::None.into())?; + } + + assert_eq!(Claims::::get(eth_address), None); + Ok(()) + } + // Benchmark `attest` including prevalidate logic. #[benchmark] fn attest() -> Result<(), BenchmarkError> { @@ -1749,6 +2012,69 @@ mod benchmarking { Ok(()) } + #[benchmark] + fn authorize_claim() -> Result<(), BenchmarkError> { + let c = MAX_CLAIMS; + + for i in 0 .. c / 2 { + create_claim::(c)?; + create_claim_attest::(u32::MAX - c)?; + } + + let secret_key = libsecp256k1::SecretKey::parse(&keccak_256(&c.encode())).unwrap(); + let eth_address = eth(&secret_key); + let account: T::AccountId = account("user", c, SEED); + let vesting = Some((100_000u32.into(), 1_000u32.into(), 100u32.into())); + let signature = sig::(&secret_key, &account.encode(), &[][..]); + super::Pallet::::mint_claim(RawOrigin::Root.into(), eth_address, VALUE.into(), vesting, None)?; + assert_eq!(Claims::::get(eth_address), Some(VALUE.into())); + let call = Call::::claim { + dest: account.clone(), ethereum_signature: signature.clone() + }; + + #[block] + { + use frame_support::pallet_prelude::Authorize; + call.authorize(TransactionSource::External) + .expect("Call give some authorization") + .expect("Authorization is valid"); + } + } + + #[benchmark] + fn authorize_claim_attest() -> Result<(), BenchmarkError> { + let c = MAX_CLAIMS; + + for i in 0 .. c / 2 { + create_claim::(c)?; + create_claim_attest::(u32::MAX - c)?; + } + + // Crate signature + let attest_c = u32::MAX - c; + let secret_key = libsecp256k1::SecretKey::parse(&keccak_256(&attest_c.encode())).unwrap(); + let eth_address = eth(&secret_key); + let account: T::AccountId = account("user", c, SEED); + let vesting = Some((100_000u32.into(), 1_000u32.into(), 100u32.into())); + let statement = StatementKind::Regular; + let signature = sig::(&secret_key, &account.encode(), statement.to_text()); + super::Pallet::::mint_claim(RawOrigin::Root.into(), eth_address, VALUE.into(), vesting, Some(statement))?; + assert_eq!(Claims::::get(eth_address), Some(VALUE.into())); + let call = Call::::claim_attest { + dest: account.clone(), + ethereum_signature: signature.clone(), + statement: StatementKind::Regular.to_text().to_vec() + }; + + #[block] + { + use frame_support::pallet_prelude::Authorize; + call.authorize(TransactionSource::External) + .expect("Call give some authorization") + .expect("Authorization is valid"); + } + } + impl_benchmark_test_suite!( Pallet, crate::claims::tests::new_test_ext(), diff --git a/polkadot/runtime/common/src/integration_tests.rs b/polkadot/runtime/common/src/integration_tests.rs index 8a76a138305e..91435a41078d 100644 --- a/polkadot/runtime/common/src/integration_tests.rs +++ b/polkadot/runtime/common/src/integration_tests.rs @@ -106,6 +106,28 @@ where type RuntimeCall = RuntimeCall; } +impl frame_system::offchain::CreateTransaction for Test +where + RuntimeCall: From, +{ + type Extension = (); + fn create_transaction( + call: >::RuntimeCall, + extension: Self::Extension, + ) -> Self::Extrinsic { + UncheckedExtrinsic::new_transaction(call, extension) + } +} + +impl frame_system::offchain::CreateAuthorizedTransaction for Test +where + RuntimeCall: From, +{ + fn create_extension() -> Self::Extension { + () + } +} + impl frame_system::offchain::CreateInherent for Test where RuntimeCall: From, diff --git a/polkadot/runtime/common/src/mock.rs b/polkadot/runtime/common/src/mock.rs index 54170b07fa62..9f9e90c534ae 100644 --- a/polkadot/runtime/common/src/mock.rs +++ b/polkadot/runtime/common/src/mock.rs @@ -262,8 +262,8 @@ pub fn conclude_pvf_checking( validator_index: validator_index.into(), }; let signature = key.sign(&statement.signing_payload()); - let _ = paras::Pallet::::include_pvf_check_statement( - frame_system::Origin::::None.into(), + let _ = paras::Pallet::::include_pvf_check_statement_general( + frame_system::Origin::::Authorized.into(), statement, signature.into(), ); diff --git a/polkadot/runtime/common/src/paras_registrar/mod.rs b/polkadot/runtime/common/src/paras_registrar/mod.rs index 2ead621dedf0..57e584d88022 100644 --- a/polkadot/runtime/common/src/paras_registrar/mod.rs +++ b/polkadot/runtime/common/src/paras_registrar/mod.rs @@ -760,6 +760,28 @@ mod tests { type RuntimeCall = RuntimeCall; } + impl frame_system::offchain::CreateTransaction for Test + where + RuntimeCall: From, + { + type Extension = (); + fn create_transaction( + call: >::RuntimeCall, + extension: Self::Extension, + ) -> Self::Extrinsic { + UncheckedExtrinsic::new_transaction(call, extension) + } + } + + impl frame_system::offchain::CreateAuthorizedTransaction for Test + where + RuntimeCall: From, + { + fn create_extension() -> Self::Extension { + () + } + } + impl frame_system::offchain::CreateInherent for Test where RuntimeCall: From, diff --git a/polkadot/runtime/parachains/src/disputes/slashing.rs b/polkadot/runtime/parachains/src/disputes/slashing.rs index 2e09ea667f74..92be509dc000 100644 --- a/polkadot/runtime/parachains/src/disputes/slashing.rs +++ b/polkadot/runtime/parachains/src/disputes/slashing.rs @@ -45,6 +45,7 @@ use crate::{disputes, initializer::ValidatorSetCount, session_info::IdentificationTuple}; use frame_support::{ dispatch::Pays, + pallet_prelude::{DispatchError, DispatchResultWithPostInfo}, traits::{Defensive, Get, KeyOwnerProofSystem, ValidatorSet, ValidatorSetWithIdentification}, weights::Weight, }; @@ -356,6 +357,7 @@ impl HandleReports for () { pub trait WeightInfo { fn report_dispute_lost(validator_count: ValidatorSetCount) -> Weight; + fn authorize_report_dispute_lost() -> Weight; } pub struct TestWeightInfo; @@ -363,6 +365,9 @@ impl WeightInfo for TestWeightInfo { fn report_dispute_lost(_validator_count: ValidatorSetCount) -> Weight { Weight::zero() } + fn authorize_report_dispute_lost() -> Weight { + Weight::zero() + } } pub use pallet::*; @@ -393,9 +398,10 @@ pub mod pallet { /// The slashing report handling subsystem, defines methods to report an /// offence (after the slashing report has been validated) and for /// submitting a transaction to report a slash (from an offchain - /// context). NOTE: when enabling slashing report handling (i.e. this - /// type isn't set to `()`) you must use this pallet's - /// `ValidateUnsigned` in the runtime definition. + /// context). + /// NOTE: when enabling slashing report handling (i.e. this + /// type isn't set to `()`) you must use frame system authorize transaction + /// extension in the transaction extension pipeline. type HandleReports: HandleReports; /// Weight information for extrinsics in this pallet. @@ -442,70 +448,41 @@ pub mod pallet { DuplicateSlashingReport, } - #[pallet::call] + #[pallet::call(weight = ::WeightInfo)] impl Pallet { #[pallet::call_index(0)] - #[pallet::weight(::WeightInfo::report_dispute_lost( - key_owner_proof.validator_count() - ))] + #[pallet::weight( + ::WeightInfo::report_dispute_lost( + key_owner_proof.validator_count() + ) + // We need to include the weight for the `validate_unsigned` logic. + // The validation logic weight is same as the authorization logic. + .saturating_add(::WeightInfo::authorize_report_dispute_lost()) + )] pub fn report_dispute_lost_unsigned( origin: OriginFor, // box to decrease the size of the call dispute_proof: Box, key_owner_proof: T::KeyOwnerProof, ) -> DispatchResultWithPostInfo { - ensure_none(origin)?; - - let validator_set_count = key_owner_proof.validator_count() as ValidatorSetCount; - // check the membership proof to extract the offender's id - let key = - (polkadot_primitives::PARACHAIN_KEY_TYPE_ID, dispute_proof.validator_id.clone()); - let offender = T::KeyOwnerProofSystem::check_proof(key, key_owner_proof) - .ok_or(Error::::InvalidKeyOwnershipProof)?; - - let session_index = dispute_proof.time_slot.session_index; - - // check that there is a pending slash for the given - // validator index and candidate hash - let candidate_hash = dispute_proof.time_slot.candidate_hash; - let try_remove = |v: &mut Option| -> Result<(), DispatchError> { - let pending = v.as_mut().ok_or(Error::::InvalidCandidateHash)?; - if pending.kind != dispute_proof.kind { - return Err(Error::::InvalidCandidateHash.into()) - } - - match pending.keys.entry(dispute_proof.validator_index) { - Entry::Vacant(_) => return Err(Error::::InvalidValidatorIndex.into()), - // check that `validator_index` matches `validator_id` - Entry::Occupied(e) if e.get() != &dispute_proof.validator_id => - return Err(Error::::ValidatorIndexIdMismatch.into()), - Entry::Occupied(e) => { - e.remove(); // the report is correct - }, - } - - // if the last validator is slashed for this dispute, clean up the storage - if pending.keys.is_empty() { - *v = None; - } + // TODO TODO: maybe have some ensure_none_ref and ensure_authorized_ref??? + let is_none = ensure_none(origin.clone()).is_ok(); + let is_authorized = ensure_authorized(origin).is_ok(); - Ok(()) - }; + ensure!(is_none || is_authorized, DispatchError::BadOrigin); - >::try_mutate_exists(&session_index, &candidate_hash, try_remove)?; + let key_owner_proof_validator_count = key_owner_proof.validator_count(); + let mut post_info = Self::do_report_dispute_lost(dispute_proof, key_owner_proof)?; - let offence = SlashingOffence::new( - session_index, - candidate_hash, - validator_set_count, - vec![offender], - dispute_proof.kind, - ); - - >::report_offence(offence) - .map_err(|_| Error::::DuplicateSlashingReport)?; + if is_authorized { + // The call comes from authorization, we no longer include the weight for the + // `validate_unsigned` logic. + post_info.actual_weight = Some(::WeightInfo::report_dispute_lost( + key_owner_proof_validator_count, + )); + } - Ok(Pays::No.into()) + Ok(post_info) } } @@ -557,9 +534,106 @@ impl Pallet { ) -> Option<()> { T::HandleReports::submit_unsigned_slashing_report(dispute_proof, key_ownership_proof).ok() } + + /// The storage must be reverted on error. + fn do_report_dispute_lost( + dispute_proof: Box, + key_owner_proof: T::KeyOwnerProof, + ) -> DispatchResultWithPostInfo { + let validator_set_count = key_owner_proof.validator_count() as ValidatorSetCount; + // check the membership proof to extract the offender's id + let key = (polkadot_primitives::PARACHAIN_KEY_TYPE_ID, dispute_proof.validator_id.clone()); + let offender = T::KeyOwnerProofSystem::check_proof(key, key_owner_proof) + .ok_or(Error::::InvalidKeyOwnershipProof)?; + + let session_index = dispute_proof.time_slot.session_index; + + // check that there is a pending slash for the given + // validator index and candidate hash + let candidate_hash = dispute_proof.time_slot.candidate_hash; + let try_remove = |v: &mut Option| -> Result<(), DispatchError> { + let pending = v.as_mut().ok_or(Error::::InvalidCandidateHash)?; + if pending.kind != dispute_proof.kind { + return Err(Error::::InvalidCandidateHash.into()) + } + + match pending.keys.entry(dispute_proof.validator_index) { + Entry::Vacant(_) => return Err(Error::::InvalidValidatorIndex.into()), + // check that `validator_index` matches `validator_id` + Entry::Occupied(e) if e.get() != &dispute_proof.validator_id => + return Err(Error::::ValidatorIndexIdMismatch.into()), + Entry::Occupied(e) => { + e.remove(); // the report is correct + }, + } + + // if the last validator is slashed for this dispute, clean up the storage + if pending.keys.is_empty() { + *v = None; + } + + Ok(()) + }; + + >::try_mutate_exists(&session_index, &candidate_hash, try_remove)?; + + let offence = SlashingOffence::new( + session_index, + candidate_hash, + validator_set_count, + vec![offender], + dispute_proof.kind, + ); + + >::report_offence(offence) + .map_err(|_| Error::::DuplicateSlashingReport)?; + + Ok(Pays::No.into()) + } + + fn validate_report_dispute_lost_call( + source: TransactionSource, + dispute_proof: &Box, + key_owner_proof: &T::KeyOwnerProof, + ) -> Result<(ValidTransaction, Weight), TransactionValidityError> { + // discard slashing report not coming from the local node + match source { + TransactionSource::Local | TransactionSource::InBlock => { /* allowed */ }, + _ => { + log::warn!( + target: LOG_TARGET, + "rejecting unsigned transaction because it is not local/in-block." + ); + + return Err(InvalidTransaction::Call.into()) + }, + } + + // check report staleness + is_known_offence::(dispute_proof, key_owner_proof)?; + + let longevity = >::ReportLongevity::get(); + + let tag_prefix = match dispute_proof.kind { + SlashingOffenceKind::ForInvalid => "DisputeForInvalid", + SlashingOffenceKind::AgainstValid => "DisputeAgainstValid", + }; + + let valid_transaction = ValidTransaction::with_tag_prefix(tag_prefix) + // We assign the maximum priority for any report. + .priority(TransactionPriority::max_value()) + // Only one report for the same offender at the same slot. + .and_provides((dispute_proof.time_slot.clone(), dispute_proof.validator_id.clone())) + .longevity(longevity) + // We don't propagate this. This can never be included on a remote node. + .propagate(false) + .into(); + + Ok((valid_transaction, Weight::zero())) + } } -/// Methods for the `ValidateUnsigned` implementation: +/// Methods for the `ValidateUnsigned` or authorize implementation: /// /// It restricts calls to `report_dispute_lost_unsigned` to local calls (i.e. /// extrinsics generated on this node) or that already in a block. This @@ -567,38 +641,8 @@ impl Pallet { impl Pallet { pub fn validate_unsigned(source: TransactionSource, call: &Call) -> TransactionValidity { if let Call::report_dispute_lost_unsigned { dispute_proof, key_owner_proof } = call { - // discard slashing report not coming from the local node - match source { - TransactionSource::Local | TransactionSource::InBlock => { /* allowed */ }, - _ => { - log::warn!( - target: LOG_TARGET, - "rejecting unsigned transaction because it is not local/in-block." - ); - - return InvalidTransaction::Call.into() - }, - } - - // check report staleness - is_known_offence::(dispute_proof, key_owner_proof)?; - - let longevity = >::ReportLongevity::get(); - - let tag_prefix = match dispute_proof.kind { - SlashingOffenceKind::ForInvalid => "DisputeForInvalid", - SlashingOffenceKind::AgainstValid => "DisputeAgainstValid", - }; - - ValidTransaction::with_tag_prefix(tag_prefix) - // We assign the maximum priority for any report. - .priority(TransactionPriority::max_value()) - // Only one report for the same offender at the same slot. - .and_provides((dispute_proof.time_slot.clone(), dispute_proof.validator_id.clone())) - .longevity(longevity) - // We don't propagate this. This can never be included on a remote node. - .propagate(false) - .build() + Self::validate_report_dispute_lost_call(source, dispute_proof, key_owner_proof) + .map(|(valid_transaction, _)| valid_transaction) } else { InvalidTransaction::Call.into() } diff --git a/polkadot/runtime/parachains/src/disputes/slashing/benchmarking.rs b/polkadot/runtime/parachains/src/disputes/slashing/benchmarking.rs index b53f98caeea3..1a0de1094e8e 100644 --- a/polkadot/runtime/parachains/src/disputes/slashing/benchmarking.rs +++ b/polkadot/runtime/parachains/src/disputes/slashing/benchmarking.rs @@ -160,4 +160,23 @@ benchmarks! { let unapplied = >::get(session_index, CANDIDATE_HASH); assert!(unapplied.is_none()); } + + // in this setup we have a single `ForInvalid` dispute + // submitted for a past session + authorize_report_dispute_lost { + let n in 4..<::BenchmarkingConfig as BenchmarkingConfiguration>::MAX_VALIDATORS; + + let (session_index, key_owner_proof, validator_id) = setup_validator_set::(n); + let dispute_proof = setup_dispute::(session_index, validator_id); + let call = Call::::report_dispute_lost { + dispute_proof: Box::new(dispute_proof), + key_owner_proof, + }; + }: { + use frame_support::traits::Authorize; + call.authorize(TransactionSource::Local) + .expect("Some authorization") + .expect("Authorization is successful"); + } + } diff --git a/polkadot/runtime/parachains/src/mock.rs b/polkadot/runtime/parachains/src/mock.rs index ee1990a7b618..599d6d28eb45 100644 --- a/polkadot/runtime/parachains/src/mock.rs +++ b/polkadot/runtime/parachains/src/mock.rs @@ -108,6 +108,28 @@ where } } +impl frame_system::offchain::CreateTransaction for Test +where + RuntimeCall: From, +{ + type Extension = (); + fn create_transaction( + call: >::RuntimeCall, + extension: Self::Extension, + ) -> Self::Extrinsic { + UncheckedExtrinsic::new_transaction(call, extension) + } +} + +impl frame_system::offchain::CreateAuthorizedTransaction for Test +where + RuntimeCall: From, +{ + fn create_extension() -> Self::Extension { + () + } +} + parameter_types! { pub static BlockWeights: frame_system::limits::BlockWeights = frame_system::limits::BlockWeights::simple_max( diff --git a/polkadot/runtime/parachains/src/paras/benchmarking.rs b/polkadot/runtime/parachains/src/paras/benchmarking.rs index 7bf8b833ed91..182601aeec9d 100644 --- a/polkadot/runtime/parachains/src/paras/benchmarking.rs +++ b/polkadot/runtime/parachains/src/paras/benchmarking.rs @@ -162,6 +162,7 @@ benchmarks! { include_pvf_check_statement { let (stmt, signature) = pvf_check::prepare_inclusion_bench::(); }: { + #[allow(deprecated)] let _ = Pallet::::include_pvf_check_statement(RawOrigin::None.into(), stmt, signature); } @@ -171,6 +172,7 @@ benchmarks! { VoteOutcome::Accept, ); }: { + #[allow(deprecated)] let _ = Pallet::::include_pvf_check_statement(RawOrigin::None.into(), stmt, signature); } @@ -180,6 +182,7 @@ benchmarks! { VoteOutcome::Reject, ); }: { + #[allow(deprecated)] let _ = Pallet::::include_pvf_check_statement(RawOrigin::None.into(), stmt, signature); } @@ -189,6 +192,7 @@ benchmarks! { VoteOutcome::Accept, ); }: { + #[allow(deprecated)] let _ = Pallet::::include_pvf_check_statement(RawOrigin::None.into(), stmt, signature); } @@ -198,9 +202,62 @@ benchmarks! { VoteOutcome::Reject, ); }: { + #[allow(deprecated)] let _ = Pallet::::include_pvf_check_statement(RawOrigin::None.into(), stmt, signature); } + include_pvf_check_statement_finalize_general_upgrade_accept { + let (stmt, signature) = pvf_check::prepare_finalization_bench::( + VoteCause::Upgrade, + VoteOutcome::Accept, + ); + }: { + let _ = Pallet::::include_pvf_check_statement_general(RawOrigin::Authorized.into(), stmt, signature); + } + + include_pvf_check_statement_finalize_general_upgrade_reject { + let (stmt, signature) = pvf_check::prepare_finalization_bench::( + VoteCause::Upgrade, + VoteOutcome::Reject, + ); + }: { + let _ = Pallet::::include_pvf_check_statement_general(RawOrigin::Authorized.into(), stmt, signature); + } + + include_pvf_check_statement_finalize_general_onboarding_accept { + let (stmt, signature) = pvf_check::prepare_finalization_bench::( + VoteCause::Onboarding, + VoteOutcome::Accept, + ); + }: { + let _ = Pallet::::include_pvf_check_statement_general(RawOrigin::Authorized.into(), stmt, signature); + } + + include_pvf_check_statement_finalize_general_onboarding_reject { + let (stmt, signature) = pvf_check::prepare_finalization_bench::( + VoteCause::Onboarding, + VoteOutcome::Reject, + ); + }: { + let _ = Pallet::::include_pvf_check_statement_general(RawOrigin::Authorized.into(), stmt, signature); + } + + include_pvf_check_statement_general { + let (stmt, signature) = pvf_check::prepare_inclusion_bench::(); + }: { + let _ = Pallet::::include_pvf_check_statement_general(RawOrigin::Authorized.into(), stmt, signature); + } + + authorize_include_pvf_check_statement_general { + let (stmt, signature) = pvf_check::prepare_inclusion_bench::(); + }: { + use frame_support::pallet_prelude::Authorize; + Call::::include_pvf_check_statement_general { stmt, signature } + .authorize(TransactionSource::Local) + .expect("Call give some authorization") + .expect("Authorization is valid"); + } + impl_benchmark_test_suite!( Pallet, crate::mock::new_test_ext(Default::default()), diff --git a/polkadot/runtime/parachains/src/paras/benchmarking/pvf_check.rs b/polkadot/runtime/parachains/src/paras/benchmarking/pvf_check.rs index 80443c7626e2..0fa6f7c1389f 100644 --- a/polkadot/runtime/parachains/src/paras/benchmarking/pvf_check.rs +++ b/polkadot/runtime/parachains/src/paras/benchmarking/pvf_check.rs @@ -80,7 +80,7 @@ where let stmt_n_sig = stmts.pop().unwrap(); for (stmt, sig) in stmts { - let r = Pallet::::include_pvf_check_statement(RawOrigin::None.into(), stmt, sig); + let r = Pallet::::include_pvf_check_statement_general(RawOrigin::Authorized.into(), stmt, sig); assert!(r.is_ok()); } diff --git a/polkadot/runtime/parachains/src/paras/mod.rs b/polkadot/runtime/parachains/src/paras/mod.rs index e0f244dbd863..b670df8c719e 100644 --- a/polkadot/runtime/parachains/src/paras/mod.rs +++ b/polkadot/runtime/parachains/src/paras/mod.rs @@ -174,6 +174,8 @@ pub struct ParaPastCodeMeta { last_pruned: Option, } +// TODO TODO: refactor the code a bit + /// The possible states of a para, to take into account delayed lifecycle changes. /// /// If the para is in a "transition state", it is expected that the parachain is @@ -551,6 +553,12 @@ pub trait WeightInfo { fn include_pvf_check_statement_finalize_onboarding_accept() -> Weight; fn include_pvf_check_statement_finalize_onboarding_reject() -> Weight; fn include_pvf_check_statement() -> Weight; + fn include_pvf_check_statement_general_finalize_upgrade_accept() -> Weight; + fn include_pvf_check_statement_general_finalize_upgrade_reject() -> Weight; + fn include_pvf_check_statement_general_finalize_onboarding_accept() -> Weight; + fn include_pvf_check_statement_general_finalize_onboarding_reject() -> Weight; + fn include_pvf_check_statement_general() -> Weight; + fn authorize_include_pvf_check_statement_general() -> Weight; } pub struct TestWeightInfo; @@ -596,6 +604,24 @@ impl WeightInfo for TestWeightInfo { // This special value is to distinguish from the finalizing variants above in tests. Weight::MAX - Weight::from_parts(1, 1) } + fn include_pvf_check_statement_general_finalize_upgrade_accept() -> Weight { + Weight::MAX + } + fn include_pvf_check_statement_general_finalize_upgrade_reject() -> Weight { + Weight::MAX + } + fn include_pvf_check_statement_general_finalize_onboarding_accept() -> Weight { + Weight::MAX + } + fn include_pvf_check_statement_general_finalize_onboarding_reject() -> Weight { + Weight::MAX + } + fn include_pvf_check_statement_general() -> Weight { + Weight::MAX + } + fn authorize_include_pvf_check_statement_general() -> Weight { + Weight::MAX + } } #[frame_support::pallet] @@ -603,7 +629,6 @@ pub mod pallet { use super::*; use sp_runtime::transaction_validity::{ InvalidTransaction, TransactionPriority, TransactionSource, TransactionValidity, - ValidTransaction, }; #[pallet::pallet] @@ -615,7 +640,7 @@ pub mod pallet { frame_system::Config + configuration::Config + shared::Config - + frame_system::offchain::CreateInherent> + + frame_system::offchain::CreateAuthorizedTransaction> { type RuntimeEvent: From + IsType<::RuntimeEvent>; @@ -884,7 +909,7 @@ pub mod pallet { } } - #[pallet::call] + #[pallet::call(weight = ::WeightInfo)] impl Pallet { /// Set the storage for the parachain validation code immediately. #[pallet::call_index(0)] @@ -1045,6 +1070,8 @@ pub mod pallet { Ok(()) } + /// Deprecated in favor of [`include_pvf_check_statement_general`]. + /// /// Includes a statement for a PVF pre-checking vote. Potentially, finalizes the vote and /// enacts the results if that was the last vote before achieving the supermajority. #[pallet::call_index(7)] @@ -1055,6 +1082,7 @@ pub mod pallet { .max(::WeightInfo::include_pvf_check_statement_finalize_onboarding_reject()) ) )] + #[deprecated(note = "Use `include_pvf_check_statement_general` instead.")] pub fn include_pvf_check_statement( origin: OriginFor, stmt: PvfCheckStatement, @@ -1149,6 +1177,83 @@ pub mod pallet { MostRecentContext::::insert(¶, context); Ok(()) } + + /// Includes a statement for a PVF pre-checking vote. Potentially, finalizes the vote and + /// enacts the results if that was the last vote before achieving the supermajority. + /// + /// Transaction must be general. + #[pallet::call_index(9)] + #[pallet::weight( + ::WeightInfo::include_pvf_check_statement_general_finalize_upgrade_accept() + .max(::WeightInfo::include_pvf_check_statement_general_finalize_upgrade_reject()) + .max(::WeightInfo::include_pvf_check_statement_general_finalize_onboarding_accept()) + .max(::WeightInfo::include_pvf_check_statement_general_finalize_onboarding_reject()) + )] + #[pallet::authorize(|_source, stmt, sig| Pallet::::validate_include_pvf_check_statement(stmt, sig).map(|v| (v, Weight::zero())))] + pub fn include_pvf_check_statement_general( + origin: OriginFor, + stmt: PvfCheckStatement, + _signature: ValidatorSignature, + ) -> DispatchResultWithPostInfo { + ensure_authorized(origin)?; + + // Transaction validation checks: + // * statement.session_index is not stale nor future. + // * signature is correct. + // * no double vote. + + let validators = shared::ActiveValidatorKeys::::get(); + let validator_index = stmt.validator_index.0 as usize; + + let mut active_vote = PvfActiveVoteMap::::get(&stmt.subject) + // Defensive, it must have been checked by transaction validation. + .ok_or(Error::::PvfCheckSubjectInvalid)?; + + // Finally, cast the vote and persist. + if stmt.accept { + active_vote.votes_accept.set(validator_index, true); + } else { + active_vote.votes_reject.set(validator_index, true); + } + + if let Some(outcome) = active_vote.quorum(validators.len()) { + // The quorum has been achieved. + // + // Remove the PVF vote from the active map and finalize the PVF checking according + // to the outcome. + PvfActiveVoteMap::::remove(&stmt.subject); + PvfActiveVoteList::::mutate(|l| { + if let Ok(i) = l.binary_search(&stmt.subject) { + l.remove(i); + } + }); + match outcome { + PvfCheckOutcome::Accepted => { + let cfg = configuration::ActiveConfig::::get(); + Self::enact_pvf_accepted( + frame_system::Pallet::::block_number(), + &stmt.subject, + &active_vote.causes, + active_vote.age, + &cfg, + ); + }, + PvfCheckOutcome::Rejected => { + Self::enact_pvf_rejected(&stmt.subject, active_vote.causes); + }, + } + + // No weight refund since this statement was the last one and lead to finalization. + Ok(().into()) + } else { + // No quorum has been achieved. + // + // - So just store the updated state back into the storage. + // - Only charge weight for simple vote inclusion. + PvfActiveVoteMap::::insert(&stmt.subject, active_vote); + Ok(Some(::WeightInfo::include_pvf_check_statement_general()).into()) + } + } } #[pallet::validate_unsigned] @@ -1161,47 +1266,7 @@ pub mod pallet { _ => return InvalidTransaction::Call.into(), }; - let current_session = shared::CurrentSessionIndex::::get(); - if stmt.session_index < current_session { - return InvalidTransaction::Stale.into() - } else if stmt.session_index > current_session { - return InvalidTransaction::Future.into() - } - - let validator_index = stmt.validator_index.0 as usize; - let validators = shared::ActiveValidatorKeys::::get(); - let validator_public = match validators.get(validator_index) { - Some(pk) => pk, - None => return InvalidTransaction::Custom(INVALID_TX_BAD_VALIDATOR_IDX).into(), - }; - - let signing_payload = stmt.signing_payload(); - if !signature.verify(&signing_payload[..], &validator_public) { - return InvalidTransaction::BadProof.into() - } - - let active_vote = match PvfActiveVoteMap::::get(&stmt.subject) { - Some(v) => v, - None => return InvalidTransaction::Custom(INVALID_TX_BAD_SUBJECT).into(), - }; - - match active_vote.has_vote(validator_index) { - Some(false) => (), - Some(true) => return InvalidTransaction::Custom(INVALID_TX_DOUBLE_VOTE).into(), - None => return InvalidTransaction::Custom(INVALID_TX_BAD_VALIDATOR_IDX).into(), - } - - ValidTransaction::with_tag_prefix("PvfPreCheckingVote") - .priority(T::UnsignedPriority::get()) - .longevity( - TryInto::::try_into( - T::NextSessionRotation::average_session_length() / 2u32.into(), - ) - .unwrap_or(64_u64), - ) - .and_provides((stmt.session_index, stmt.validator_index, stmt.subject)) - .propagate(true) - .build() + Self::validate_include_pvf_check_statement(stmt, signature) } fn pre_dispatch(_call: &Self::Call) -> Result<(), TransactionValidityError> { @@ -2177,7 +2242,9 @@ impl Pallet { ) { use frame_system::offchain::SubmitTransaction; - let xt = T::create_inherent(Call::include_pvf_check_statement { stmt, signature }.into()); + let xt = T::create_authorized_transaction( + Call::include_pvf_check_statement_general { stmt, signature }.into(), + ); if let Err(e) = SubmitTransaction::>::submit_transaction(xt) { log::error!(target: LOG_TARGET, "Error submitting pvf check statement: {:?}", e,); } @@ -2320,6 +2387,59 @@ impl Pallet { MostRecentContext::::insert(&id, BlockNumberFor::::from(0u32)); } + /// Validate the transaction `include_pvf_check_statement`. + /// + /// Checks: + /// * statement.session_index is not stale nor future. + /// * signature is correct. + /// * no double vote. + fn validate_include_pvf_check_statement( + stmt: &PvfCheckStatement, + signature: &ValidatorSignature, + ) -> TransactionValidity { + let current_session = shared::CurrentSessionIndex::::get(); + if stmt.session_index < current_session { + return InvalidTransaction::Stale.into() + } else if stmt.session_index > current_session { + return InvalidTransaction::Future.into() + } + + let validator_index = stmt.validator_index.0 as usize; + let validators = shared::ActiveValidatorKeys::::get(); + let validator_public = match validators.get(validator_index) { + Some(pk) => pk, + None => return InvalidTransaction::Custom(INVALID_TX_BAD_VALIDATOR_IDX).into(), + }; + + let signing_payload = stmt.signing_payload(); + if !signature.verify(&signing_payload[..], &validator_public) { + return InvalidTransaction::BadProof.into() + } + + let active_vote = match PvfActiveVoteMap::::get(&stmt.subject) { + Some(v) => v, + None => return InvalidTransaction::Custom(INVALID_TX_BAD_SUBJECT).into(), + }; + + match active_vote.has_vote(validator_index) { + Some(false) => (), + Some(true) => return InvalidTransaction::Custom(INVALID_TX_DOUBLE_VOTE).into(), + None => return InvalidTransaction::Custom(INVALID_TX_BAD_VALIDATOR_IDX).into(), + } + + ValidTransaction::with_tag_prefix("PvfPreCheckingVote") + .priority(T::UnsignedPriority::get()) + .longevity( + TryInto::::try_into( + T::NextSessionRotation::average_session_length() / 2u32.into(), + ) + .unwrap_or(64_u64), + ) + .and_provides((stmt.session_index, stmt.validator_index, stmt.subject)) + .propagate(true) + .build() + } + #[cfg(test)] pub(crate) fn active_vote_state( code_hash: &ValidationCodeHash, diff --git a/polkadot/runtime/parachains/src/paras/tests.rs b/polkadot/runtime/parachains/src/paras/tests.rs index 6e4f99aa3d84..5153125db217 100644 --- a/polkadot/runtime/parachains/src/paras/tests.rs +++ b/polkadot/runtime/parachains/src/paras/tests.rs @@ -22,6 +22,8 @@ use sc_keystore::LocalKeystore; use sp_keyring::Sr25519Keyring; use sp_keystore::{Keystore, KeystorePtr}; use std::sync::Arc; +use frame_system::RawOrigin; +use frame_support::storage::{transactional::with_transaction_unchecked, TransactionOutcome}; use crate::{ configuration::HostConfiguration, @@ -46,7 +48,7 @@ fn sign_and_include_pvf_check_statement(stmt: PvfCheckStatement) { Sr25519Keyring::Ferdie, ]; let signature = validators[stmt.validator_index.0 as usize].sign(&stmt.signing_payload()); - Paras::include_pvf_check_statement(None.into(), stmt, signature.into()).unwrap(); + Paras::include_pvf_check_statement_general(RawOrigin::Authorized.into(), stmt, signature.into()).unwrap(); } fn submit_super_majority_pvf_votes( @@ -1427,12 +1429,14 @@ fn pvf_check_upgrade_reject() { }); } +// TODO TODO: add some legacy tests + #[test] fn pvf_check_submit_vote() { let code_a = test_validation_code_1(); let code_b = test_validation_code_2(); - let check = |stmt: PvfCheckStatement| -> (Result<_, _>, Result<_, _>) { + let check = |stmt: PvfCheckStatement| -> Result, TransactionValidityError> { let validators = &[ Sr25519Keyring::Alice, Sr25519Keyring::Bob, @@ -1445,15 +1449,26 @@ fn pvf_check_submit_vote() { validators[stmt.validator_index.0 as usize].sign(&stmt.signing_payload()).into(); let call = - Call::include_pvf_check_statement { stmt: stmt.clone(), signature: signature.clone() }; - let validate_unsigned = - ::validate_unsigned(TransactionSource::InBlock, &call) + Call::::include_pvf_check_statement_general { stmt: stmt.clone(), signature: signature.clone() }; + + with_transaction_unchecked(|| { + let authorized = call.authorize(TransactionSource::InBlock) + .expect("Some authorization are set for the call") .map(|_| ()); - let dispatch_result = - Paras::include_pvf_check_statement(None.into(), stmt.clone(), signature.clone()) + + if let Err(err) = authorized { + return TransactionOutcome::Rollback(Err(err)); + } + + let res = Paras::include_pvf_check_statement_general( + RawOrigin::Authorized.into(), + stmt.clone(), + signature.clone() + ) .map(|_| ()); - (validate_unsigned, dispatch_result) + TransactionOutcome::Commit(Ok(res)) + }) }; let genesis_config = MockGenesisConfig::default(); @@ -1478,68 +1493,62 @@ fn pvf_check_submit_vote() { session_index: 1, validator_index: 1.into(), }), - (Ok(()), Ok(())), + Ok(Ok(())), ); // A vote in the same direction. - let (unsigned, dispatch) = check(PvfCheckStatement { + let res = check(PvfCheckStatement { accept: false, subject: code_a.hash(), session_index: 1, validator_index: 1.into(), }); - assert_eq!(unsigned, Err(InvalidTransaction::Custom(INVALID_TX_DOUBLE_VOTE).into())); - assert_err!(dispatch, Error::::PvfCheckDoubleVote); + assert_eq!(res, Err(InvalidTransaction::Custom(INVALID_TX_DOUBLE_VOTE).into())); // Equivocation - let (unsigned, dispatch) = check(PvfCheckStatement { + let res = check(PvfCheckStatement { accept: true, subject: code_a.hash(), session_index: 1, validator_index: 1.into(), }); - assert_eq!(unsigned, Err(InvalidTransaction::Custom(INVALID_TX_DOUBLE_VOTE).into())); - assert_err!(dispatch, Error::::PvfCheckDoubleVote); + assert_eq!(res, Err(InvalidTransaction::Custom(INVALID_TX_DOUBLE_VOTE).into())); // Vote for an earlier session. - let (unsigned, dispatch) = check(PvfCheckStatement { + let res = check(PvfCheckStatement { accept: false, subject: code_a.hash(), session_index: 0, validator_index: 1.into(), }); - assert_eq!(unsigned, Err(InvalidTransaction::Stale.into())); - assert_err!(dispatch, Error::::PvfCheckStatementStale); + assert_eq!(res, Err(InvalidTransaction::Stale.into())); // Vote for an later session. - let (unsigned, dispatch) = check(PvfCheckStatement { + let res = check(PvfCheckStatement { accept: false, subject: code_a.hash(), session_index: 2, validator_index: 1.into(), }); - assert_eq!(unsigned, Err(InvalidTransaction::Future.into())); - assert_err!(dispatch, Error::::PvfCheckStatementFuture); + assert_eq!(res, Err(InvalidTransaction::Future.into())); // Validator not in the set. - let (unsigned, dispatch) = check(PvfCheckStatement { + let res = check(PvfCheckStatement { accept: false, subject: code_a.hash(), session_index: 1, validator_index: 5.into(), }); - assert_eq!(unsigned, Err(InvalidTransaction::Custom(INVALID_TX_BAD_VALIDATOR_IDX).into())); - assert_err!(dispatch, Error::::PvfCheckValidatorIndexOutOfBounds); + assert_eq!(res, Err(InvalidTransaction::Custom(INVALID_TX_BAD_VALIDATOR_IDX).into())); // Bad subject (code_b) - let (unsigned, dispatch) = check(PvfCheckStatement { + let res = check(PvfCheckStatement { accept: false, subject: code_b.hash(), session_index: 1, validator_index: 1.into(), }); - assert_eq!(unsigned, Err(InvalidTransaction::Custom(INVALID_TX_BAD_SUBJECT).into())); - assert_err!(dispatch, Error::::PvfCheckSubjectInvalid); + assert_eq!(res, Err(InvalidTransaction::Custom(INVALID_TX_BAD_SUBJECT).into())); }); } @@ -1597,13 +1606,13 @@ fn include_pvf_check_statement_refunds_weight() { // Verify that just vote submission is priced accordingly. for (stmt, sig) in stmts { - let r = Paras::include_pvf_check_statement(None.into(), stmt, sig.into()).unwrap(); - assert_eq!(r.actual_weight, Some(TestWeightInfo::include_pvf_check_statement())); + let r = Paras::include_pvf_check_statement_general(RawOrigin::Authorized.into(), stmt, sig.into()).unwrap(); + assert_eq!(r.actual_weight, Some(TestWeightInfo::include_pvf_check_statement_general())); } // Verify that the last statement is priced maximally. let (stmt, sig) = last_one; - let r = Paras::include_pvf_check_statement(None.into(), stmt, sig.into()).unwrap(); + let r = Paras::include_pvf_check_statement_general(RawOrigin::Authorized.into(), stmt, sig.into()).unwrap(); assert_eq!(r.actual_weight, None); }); } diff --git a/polkadot/runtime/rococo/src/weights/polkadot_runtime_common_claims.rs b/polkadot/runtime/rococo/src/weights/polkadot_runtime_common_claims.rs index 3871310678ef..5716231be7ed 100644 --- a/polkadot/runtime/rococo/src/weights/polkadot_runtime_common_claims.rs +++ b/polkadot/runtime/rococo/src/weights/polkadot_runtime_common_claims.rs @@ -179,4 +179,16 @@ impl polkadot_runtime_common::claims::WeightInfo for We .saturating_add(Weight::from_parts(0, 3761)) .saturating_add(T::DbWeight::get().reads(2)) } + fn claim_general() -> Weight { + Weight::zero() + } + fn claim_attest_general() -> Weight { + Weight::zero() + } + fn authorize_claim_general() -> Weight { + Weight::zero() + } + fn authorize_claim_attest_general() -> Weight { + Weight::zero() + } } diff --git a/polkadot/runtime/rococo/src/weights/polkadot_runtime_parachains_paras.rs b/polkadot/runtime/rococo/src/weights/polkadot_runtime_parachains_paras.rs index c463552b6ad4..b5094e61a005 100644 --- a/polkadot/runtime/rococo/src/weights/polkadot_runtime_parachains_paras.rs +++ b/polkadot/runtime/rococo/src/weights/polkadot_runtime_parachains_paras.rs @@ -295,4 +295,10 @@ impl polkadot_runtime_parachains::paras::WeightInfo for .saturating_add(T::DbWeight::get().reads(3)) .saturating_add(T::DbWeight::get().writes(1)) } + fn include_pvf_check_statement_general_finalize_upgrade_accept() -> Weight { Weight::zero() } + fn include_pvf_check_statement_general_finalize_upgrade_reject() -> Weight { Weight::zero() } + fn include_pvf_check_statement_general_finalize_onboarding_accept() -> Weight { Weight::zero() } + fn include_pvf_check_statement_general_finalize_onboarding_reject() -> Weight { Weight::zero() } + fn include_pvf_check_statement_general() -> Weight { Weight::zero() } + fn authorize_include_pvf_check_statement_general() -> Weight { Weight::zero() } } diff --git a/polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_disputes_slashing.rs b/polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_disputes_slashing.rs index a035ea2b0b5e..ec72a8f4710f 100644 --- a/polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_disputes_slashing.rs +++ b/polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_disputes_slashing.rs @@ -98,4 +98,7 @@ impl polkadot_runtime_parachains::disputes::slashing::W .saturating_add(T::DbWeight::get().writes(9)) .saturating_add(Weight::from_parts(0, 192).saturating_mul(n.into())) } + fn authorize_report_dispute_lost_general() -> Weight { + Weight::zero() + } } diff --git a/polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_paras.rs b/polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_paras.rs index d96964e69c11..914ba7e2bf85 100644 --- a/polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_paras.rs +++ b/polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_paras.rs @@ -286,4 +286,10 @@ impl polkadot_runtime_parachains::paras::WeightInfo for .saturating_add(T::DbWeight::get().reads(3)) .saturating_add(T::DbWeight::get().writes(1)) } + fn include_pvf_check_statement_general_finalize_upgrade_accept() -> Weight { Weight::zero() } + fn include_pvf_check_statement_general_finalize_upgrade_reject() -> Weight { Weight::zero() } + fn include_pvf_check_statement_general_finalize_onboarding_accept() -> Weight { Weight::zero() } + fn include_pvf_check_statement_general_finalize_onboarding_reject() -> Weight { Weight::zero() } + fn include_pvf_check_statement_general() -> Weight { Weight::zero() } + fn authorize_include_pvf_check_statement_general() -> Weight { Weight::zero() } } diff --git a/substrate/frame/im-online/src/benchmarking.rs b/substrate/frame/im-online/src/benchmarking.rs index 439720bcab38..b052d315282f 100644 --- a/substrate/frame/im-online/src/benchmarking.rs +++ b/substrate/frame/im-online/src/benchmarking.rs @@ -108,6 +108,22 @@ mod benchmarks { Ok(()) } + #[benchmark] + fn authorize_hearbeat(k: Linear<1, MAX_KEYS>) -> Result<(), BenchmarkError> { + let (input_heartbeat, signature) = create_heartbeat::(k)?; + let call = Call::::heartbeat { heartbeat: input_heartbeat, signature }; + + #[block] + { + use frame_support::traits::Authorize; + call.authorize(TransactionSource::External) + .expect("Call give some authorization") + .expect("Call is authorized"); + } + + Ok(()) + } + impl_benchmark_test_suite! { Pallet, mock::new_test_ext(), diff --git a/substrate/frame/im-online/src/lib.rs b/substrate/frame/im-online/src/lib.rs index 74d3bc6484dd..8110ac1d8000 100644 --- a/substrate/frame/im-online/src/lib.rs +++ b/substrate/frame/im-online/src/lib.rs @@ -95,7 +95,7 @@ use frame_support::{ BoundedSlice, WeakBoundedVec, }; use frame_system::{ - offchain::{CreateInherent, SubmitTransaction}, + offchain::{CreateAuthorizedTransaction, SubmitTransaction}, pallet_prelude::*, }; pub use pallet::*; @@ -261,7 +261,7 @@ pub mod pallet { pub struct Pallet(_); #[pallet::config] - pub trait Config: CreateInherent> + frame_system::Config { + pub trait Config: CreateAuthorizedTransaction> + frame_system::Config { /// The identifier type for an authority. type AuthorityId: Member + Parameter @@ -378,7 +378,7 @@ pub mod pallet { } } - #[pallet::call] + #[pallet::call(weight = T::WeightInfo)] impl Pallet { /// ## Complexity: /// - `O(K)` where K is length of `Keys` (heartbeat.validators_len) @@ -389,30 +389,28 @@ pub mod pallet { #[pallet::weight(::WeightInfo::validate_unsigned_and_then_heartbeat( heartbeat.validators_len, ))] + #[pallet::authorize(|_source, heartbeat, sig| + Pallet::::validate_heartbeat(heartbeat, sig).map(|v| (v, Weight::zero())) + )] pub fn heartbeat( origin: OriginFor, heartbeat: Heartbeat>, // since signature verification is done in `validate_unsigned` // we can skip doing it here again. _signature: ::Signature, - ) -> DispatchResult { - ensure_none(origin)?; - - let current_session = T::ValidatorSet::session_index(); - let exists = - ReceivedHeartbeats::::contains_key(current_session, heartbeat.authority_index); - let keys = Keys::::get(); - let public = keys.get(heartbeat.authority_index as usize); - if let (false, Some(public)) = (exists, public) { - Self::deposit_event(Event::::HeartbeatReceived { authority_id: public.clone() }); - - ReceivedHeartbeats::::insert(current_session, heartbeat.authority_index, true); - - Ok(()) - } else if exists { - Err(Error::::DuplicatedHeartbeat.into()) + ) -> DispatchResultWithPostInfo { + let is_none = ensure_none(origin.clone()).is_ok(); + let is_authorized = ensure_authorized(origin).is_ok(); + + ensure!(is_none || is_authorized, DispatchError::BadOrigin); + + let heartbeat_validators_len = heartbeat.validators_len; + Self::do_verified_heartbeat(heartbeat)?; + + if is_authorized { + Ok(Some(T::WeightInfo::heartbeat(heartbeat_validators_len)).into()) } else { - Err(Error::::InvalidKey.into()) + Ok(().into()) } } } @@ -452,47 +450,7 @@ pub mod pallet { fn validate_unsigned(_source: TransactionSource, call: &Self::Call) -> TransactionValidity { if let Call::heartbeat { heartbeat, signature } = call { - if >::is_online(heartbeat.authority_index) { - // we already received a heartbeat for this authority - return InvalidTransaction::Stale.into() - } - - // check if session index from heartbeat is recent - let current_session = T::ValidatorSet::session_index(); - if heartbeat.session_index != current_session { - return InvalidTransaction::Stale.into() - } - - // verify that the incoming (unverified) pubkey is actually an authority id - let keys = Keys::::get(); - if keys.len() as u32 != heartbeat.validators_len { - return InvalidTransaction::Custom(INVALID_VALIDATORS_LEN).into() - } - let authority_id = match keys.get(heartbeat.authority_index as usize) { - Some(id) => id, - None => return InvalidTransaction::BadProof.into(), - }; - - // check signature (this is expensive so we do it last). - let signature_valid = heartbeat.using_encoded(|encoded_heartbeat| { - authority_id.verify(&encoded_heartbeat, signature) - }); - - if !signature_valid { - return InvalidTransaction::BadProof.into() - } - - ValidTransaction::with_tag_prefix("ImOnline") - .priority(T::UnsignedPriority::get()) - .and_provides((current_session, authority_id)) - .longevity( - TryInto::::try_into( - T::NextSessionRotation::average_session_length() / 2u32.into(), - ) - .unwrap_or(64_u64), - ) - .propagate(true) - .build() + Self::validate_heartbeat(heartbeat, signature) } else { InvalidTransaction::Call.into() } @@ -511,6 +469,25 @@ impl } impl Pallet { + fn do_verified_heartbeat(heartbeat: Heartbeat>) -> DispatchResult { + let current_session = T::ValidatorSet::session_index(); + let exists = + ReceivedHeartbeats::::contains_key(current_session, heartbeat.authority_index); + let keys = Keys::::get(); + let public = keys.get(heartbeat.authority_index as usize); + if let (false, Some(public)) = (exists, public) { + Self::deposit_event(Event::::HeartbeatReceived { authority_id: public.clone() }); + + ReceivedHeartbeats::::insert(current_session, heartbeat.authority_index, true); + + Ok(()) + } else if exists { + Err(Error::::DuplicatedHeartbeat.into()) + } else { + Err(Error::::InvalidKey.into()) + } + } + /// Returns `true` if a heartbeat has been received for the authority at /// `authority_index` in the authorities series or if the authority has /// authored at least one block, during the current session. Otherwise @@ -642,7 +619,7 @@ impl Pallet { call, ); - let xt = T::create_inherent(call.into()); + let xt = T::create_authorized_transaction(call.into()); SubmitTransaction::>::submit_transaction(xt) .map_err(|_| OffchainErr::SubmitTransaction)?; @@ -733,6 +710,52 @@ impl Pallet { .expect("More than the maximum number of keys provided"); Keys::::put(bounded_keys); } + + fn validate_heartbeat( + heartbeat: &Heartbeat>, + signature: &::Signature, + ) -> TransactionValidity { + if >::is_online(heartbeat.authority_index) { + // we already received a heartbeat for this authority + return InvalidTransaction::Stale.into() + } + + // check if session index from heartbeat is recent + let current_session = T::ValidatorSet::session_index(); + if heartbeat.session_index != current_session { + return InvalidTransaction::Stale.into() + } + + // verify that the incoming (unverified) pubkey is actually an authority id + let keys = Keys::::get(); + if keys.len() as u32 != heartbeat.validators_len { + return InvalidTransaction::Custom(INVALID_VALIDATORS_LEN).into() + } + let authority_id = match keys.get(heartbeat.authority_index as usize) { + Some(id) => id, + None => return InvalidTransaction::BadProof.into(), + }; + + // check signature (this is expensive so we do it last). + let signature_valid = heartbeat + .using_encoded(|encoded_heartbeat| authority_id.verify(&encoded_heartbeat, signature)); + + if !signature_valid { + return InvalidTransaction::BadProof.into() + } + + ValidTransaction::with_tag_prefix("ImOnline") + .priority(T::UnsignedPriority::get()) + .and_provides((current_session, authority_id)) + .longevity( + TryInto::::try_into( + T::NextSessionRotation::average_session_length() / 2u32.into(), + ) + .unwrap_or(64_u64), + ) + .propagate(true) + .build() + } } impl sp_runtime::BoundToRuntimeAppPublic for Pallet { diff --git a/substrate/frame/im-online/src/mock.rs b/substrate/frame/im-online/src/mock.rs index a5d9a6e20e61..dc07dfdbb593 100644 --- a/substrate/frame/im-online/src/mock.rs +++ b/substrate/frame/im-online/src/mock.rs @@ -34,7 +34,11 @@ use sp_staking::{ use crate as imonline; use crate::Config; -type Block = frame_system::mocking::MockBlock; +pub type Extension = frame_system::AuthorizeCall; +pub type Header = sp_runtime::generic::Header; +pub type Block = sp_runtime::generic::Block; +pub type UncheckedExtrinsic = + sp_runtime::generic::UncheckedExtrinsic; frame_support::construct_runtime!( pub enum Runtime { @@ -72,8 +76,6 @@ impl pallet_session::historical::SessionManager for TestSessionManager fn start_session(_: SessionIndex) {} } -/// An extrinsic type used for tests. -pub type Extrinsic = sp_runtime::testing::TestXt; type IdentificationTuple = (u64, u64); type Offence = crate::UnresponsivenessOffence; @@ -192,15 +194,25 @@ where RuntimeCall: From, { type RuntimeCall = RuntimeCall; - type Extrinsic = Extrinsic; + type Extrinsic = UncheckedExtrinsic; } -impl frame_system::offchain::CreateInherent for Runtime +impl frame_system::offchain::CreateTransaction for Runtime where RuntimeCall: From, { - fn create_inherent(call: Self::RuntimeCall) -> Self::Extrinsic { - Extrinsic::new_bare(call) + type Extension = Extension; + fn create_transaction(call: Self::RuntimeCall, extension: Self::Extension) -> Self::Extrinsic { + UncheckedExtrinsic::new_transaction(call, extension) + } +} + +impl frame_system::offchain::CreateAuthorizedTransaction for Runtime +where + RuntimeCall: From, +{ + fn create_extension() -> Self::Extension { + Extension::new() } } diff --git a/substrate/frame/im-online/src/tests.rs b/substrate/frame/im-online/src/tests.rs index b9a2772da689..c55c05a2709e 100644 --- a/substrate/frame/im-online/src/tests.rs +++ b/substrate/frame/im-online/src/tests.rs @@ -21,12 +21,18 @@ use super::*; use crate::mock::*; -use frame_support::{assert_noop, dispatch}; +use frame_support::{assert_noop, dispatch::GetDispatchInfo}; +use frame_system::offchain::CreateAuthorizedTransaction; use sp_core::offchain::{ testing::{TestOffchainExt, TestTransactionPoolExt}, OffchainDbExt, OffchainWorkerExt, TransactionPoolExt, }; -use sp_runtime::testing::UintAuthorityId; +use sp_runtime::{ + testing::UintAuthorityId, + traits::{Applyable, Checkable}, + transaction_validity::{InvalidTransaction, TransactionValidityError}, + ApplyExtrinsicResult, +}; #[test] fn test_unresponsiveness_slash_fraction() { @@ -52,6 +58,59 @@ fn test_unresponsiveness_slash_fraction() { ); } +#[test] +fn should_report_offline_validators_legacy() { + new_test_ext().execute_with(|| { + // given + let block = 1; + System::set_block_number(block); + // buffer new validators + advance_session(); + // enact the change and buffer another one + let validators = vec![1, 2, 3, 4, 5, 6]; + Validators::mutate(|l| *l = Some(validators.clone())); + advance_session(); + + // when + // we end current session and start the next one + advance_session(); + + // then + let offences = Offences::take(); + assert_eq!( + offences, + vec![( + vec![], + UnresponsivenessOffence { + session_index: 2, + validator_set_count: 3, + offenders: vec![(1, 1), (2, 2), (3, 3),], + } + )] + ); + + // should not report when heartbeat is sent + for (idx, v) in validators.into_iter().take(4).enumerate() { + let _ = heartbeat(block, 3, idx as u32, v.into(), Session::validators(), true).unwrap(); + } + advance_session(); + + // then + let offences = Offences::take(); + assert_eq!( + offences, + vec![( + vec![], + UnresponsivenessOffence { + session_index: 3, + validator_set_count: 6, + offenders: vec![(5, 5), (6, 6),], + } + )] + ); + }); +} + #[test] fn should_report_offline_validators() { new_test_ext().execute_with(|| { @@ -85,7 +144,8 @@ fn should_report_offline_validators() { // should not report when heartbeat is sent for (idx, v) in validators.into_iter().take(4).enumerate() { - let _ = heartbeat(block, 3, idx as u32, v.into(), Session::validators()).unwrap(); + let _ = + heartbeat(block, 3, idx as u32, v.into(), Session::validators(), false).unwrap(); } advance_session(); @@ -111,7 +171,8 @@ fn heartbeat( authority_index: u32, id: UintAuthorityId, validators: Vec, -) -> dispatch::DispatchResult { + legacy: bool, +) -> ApplyExtrinsicResult { let heartbeat = Heartbeat { block_number, session_index, @@ -120,16 +181,57 @@ fn heartbeat( }; let signature = id.sign(&heartbeat.encode()).unwrap(); - ImOnline::pre_dispatch(&crate::Call::heartbeat { - heartbeat: heartbeat.clone(), - signature: signature.clone(), - }) - .map_err(|e| match e { - TransactionValidityError::Invalid(InvalidTransaction::Custom(INVALID_VALIDATORS_LEN)) => - "invalid validators len", - e @ _ => <&'static str>::from(e), - })?; - ImOnline::heartbeat(RuntimeOrigin::none(), heartbeat, signature) + let call = Call::heartbeat { heartbeat, signature }; + + let tx = if legacy { + UncheckedExtrinsic::new_bare(call.into()) + } else { + >>::create_authorized_transaction( + call.into(), + ) + }; + + let info = tx.get_dispatch_info(); + let len = tx.using_encoded(|e| e.len()); + + let checked = Checkable::check(tx, &frame_system::ChainContext::::default())?; + + checked.apply::(&info, len).map(|r| r.map(|_| ()).map_err(|e| e.error)) +} + +#[test] +fn should_mark_online_validator_when_heartbeat_is_received_legacy() { + new_test_ext().execute_with(|| { + advance_session(); + // given + Validators::mutate(|l| *l = Some(vec![1, 2, 3, 4, 5, 6])); + assert_eq!(Session::validators(), Vec::::new()); + // enact the change and buffer another one + advance_session(); + + assert_eq!(Session::current_index(), 2); + assert_eq!(Session::validators(), vec![1, 2, 3]); + + assert!(!ImOnline::is_online(0)); + assert!(!ImOnline::is_online(1)); + assert!(!ImOnline::is_online(2)); + + // when + let _ = heartbeat(1, 2, 0, 1.into(), Session::validators(), true).unwrap(); + + // then + assert!(ImOnline::is_online(0)); + assert!(!ImOnline::is_online(1)); + assert!(!ImOnline::is_online(2)); + + // and when + let _ = heartbeat(1, 2, 2, 3.into(), Session::validators(), true).unwrap(); + + // then + assert!(ImOnline::is_online(0)); + assert!(!ImOnline::is_online(1)); + assert!(ImOnline::is_online(2)); + }); } #[test] @@ -150,7 +252,7 @@ fn should_mark_online_validator_when_heartbeat_is_received() { assert!(!ImOnline::is_online(2)); // when - let _ = heartbeat(1, 2, 0, 1.into(), Session::validators()).unwrap(); + let _ = heartbeat(1, 2, 0, 1.into(), Session::validators(), false).unwrap(); // then assert!(ImOnline::is_online(0)); @@ -158,7 +260,7 @@ fn should_mark_online_validator_when_heartbeat_is_received() { assert!(!ImOnline::is_online(2)); // and when - let _ = heartbeat(1, 2, 2, 3.into(), Session::validators()).unwrap(); + let _ = heartbeat(1, 2, 2, 3.into(), Session::validators(), false).unwrap(); // then assert!(ImOnline::is_online(0)); @@ -167,6 +269,37 @@ fn should_mark_online_validator_when_heartbeat_is_received() { }); } +#[test] +fn late_heartbeat_and_invalid_keys_len_should_fail_legacy() { + new_test_ext().execute_with(|| { + advance_session(); + // given + Validators::mutate(|l| *l = Some(vec![1, 2, 3, 4, 5, 6])); + assert_eq!(Session::validators(), Vec::::new()); + // enact the change and buffer another one + advance_session(); + + assert_eq!(Session::current_index(), 2); + assert_eq!(Session::validators(), vec![1, 2, 3]); + + // when + assert_noop!( + heartbeat(1, 3, 0, 1.into(), Session::validators(), true), + TransactionValidityError::Invalid(InvalidTransaction::Stale) + ); + assert_noop!( + heartbeat(1, 1, 0, 1.into(), Session::validators(), true), + TransactionValidityError::Invalid(InvalidTransaction::Stale) + ); + + // invalid validators_len + assert_noop!( + heartbeat(1, 2, 0, 1.into(), vec![], false), + TransactionValidityError::Invalid(InvalidTransaction::Custom(INVALID_VALIDATORS_LEN)), + ); + }); +} + #[test] fn late_heartbeat_and_invalid_keys_len_should_fail() { new_test_ext().execute_with(|| { @@ -182,16 +315,19 @@ fn late_heartbeat_and_invalid_keys_len_should_fail() { // when assert_noop!( - heartbeat(1, 3, 0, 1.into(), Session::validators()), - "Transaction is outdated" + heartbeat(1, 3, 0, 1.into(), Session::validators(), false), + TransactionValidityError::Invalid(InvalidTransaction::Stale) ); assert_noop!( - heartbeat(1, 1, 0, 1.into(), Session::validators()), - "Transaction is outdated" + heartbeat(1, 1, 0, 1.into(), Session::validators(), false), + TransactionValidityError::Invalid(InvalidTransaction::Stale) ); // invalid validators_len - assert_noop!(heartbeat(1, 2, 0, 1.into(), vec![]), "invalid validators len"); + assert_noop!( + heartbeat(1, 2, 0, 1.into(), vec![], false), + TransactionValidityError::Invalid(InvalidTransaction::Custom(INVALID_VALIDATORS_LEN)), + ); }); } @@ -224,7 +360,7 @@ fn should_generate_heartbeats() { assert_eq!(state.read().transactions.len(), 2); // check stuff about the transaction. - let ex: Extrinsic = Decode::decode(&mut &*transaction).unwrap(); + let ex: UncheckedExtrinsic = Decode::decode(&mut &*transaction).unwrap(); let heartbeat = match ex.function { crate::mock::RuntimeCall::ImOnline(crate::Call::heartbeat { heartbeat, .. }) => heartbeat, @@ -258,7 +394,7 @@ fn should_cleanup_received_heartbeats_on_session_end() { assert_eq!(Session::validators(), vec![1, 2, 3]); // send an heartbeat from authority id 0 at session 2 - let _ = heartbeat(1, 2, 0, 1.into(), Session::validators()).unwrap(); + let _ = heartbeat(1, 2, 0, 1.into(), Session::validators(), false).unwrap(); // the heartbeat is stored assert!(!super::pallet::ReceivedHeartbeats::::get(2, 0).is_none()); @@ -338,7 +474,7 @@ fn should_not_send_a_report_if_already_online() { // All validators have `0` as their session key, but we should only produce 1 heartbeat. assert_eq!(pool_state.read().transactions.len(), 0); // check stuff about the transaction. - let ex: Extrinsic = Decode::decode(&mut &*transaction).unwrap(); + let ex: UncheckedExtrinsic = Decode::decode(&mut &*transaction).unwrap(); let heartbeat = match ex.function { crate::mock::RuntimeCall::ImOnline(crate::Call::heartbeat { heartbeat, .. }) => heartbeat, diff --git a/substrate/frame/im-online/src/weights.rs b/substrate/frame/im-online/src/weights.rs index 6fde451caf9e..0baeb1d4b9ee 100644 --- a/substrate/frame/im-online/src/weights.rs +++ b/substrate/frame/im-online/src/weights.rs @@ -52,6 +52,8 @@ use core::marker::PhantomData; /// Weight functions needed for `pallet_im_online`. pub trait WeightInfo { fn validate_unsigned_and_then_heartbeat(k: u32, ) -> Weight; + fn heartbeat(k: u32, ) -> Weight; + fn authorize_heartbeat() -> Weight; } /// Weights for `pallet_im_online` using the Substrate node and recommended hardware. @@ -80,6 +82,14 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().writes(1_u64)) .saturating_add(Weight::from_parts(0, 1761).saturating_mul(k.into())) } + fn heartbeat(_k: u32, ) -> Weight { + // TODO TODO + Weight::zero() + } + fn authorize_heartbeat() -> Weight { + // TODO TODO + Weight::zero() + } } // For backwards compatibility and tests. @@ -107,4 +117,12 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().writes(1_u64)) .saturating_add(Weight::from_parts(0, 1761).saturating_mul(k.into())) } + fn heartbeat(_k: u32, ) -> Weight { + // TODO TODO + Weight::zero() + } + fn authorize_heartbeat() -> Weight { + // TODO TODO + Weight::zero() + } } diff --git a/substrate/frame/mixnet/src/lib.rs b/substrate/frame/mixnet/src/lib.rs index 6579ed678ae7..10eb4b6d3e9d 100644 --- a/substrate/frame/mixnet/src/lib.rs +++ b/substrate/frame/mixnet/src/lib.rs @@ -44,7 +44,10 @@ use sp_mixnet::types::{ AuthorityId, AuthoritySignature, KxPublic, Mixnode, MixnodesErr, PeerId, SessionIndex, SessionPhase, SessionStatus, KX_PUBLIC_SIZE, }; -use sp_runtime::RuntimeDebug; +use sp_runtime::{ + transaction_validity::{InvalidTransaction, TransactionValidity, ValidTransaction}, + RuntimeDebug, +}; const LOG_TARGET: &str = "runtime::mixnet"; @@ -280,14 +283,15 @@ pub mod pallet { /// Register a mixnode for the following session. #[pallet::call_index(0)] #[pallet::weight(1)] // TODO + #[pallet::authorize(|_source, registration, sig| Pallet::::validate_register(registration, sig).map(|v| (v, Weight::zero())))] pub fn register( origin: OriginFor, registration: RegistrationFor, _signature: AuthoritySignature, ) -> DispatchResult { - ensure_none(origin)?; + ensure_authorized(origin)?; - // Checked by ValidateUnsigned + // Checked by authorize debug_assert_eq!(registration.session_index, CurrentSessionIndex::::get()); debug_assert!(registration.authority_index < T::MaxAuthorities::get()); @@ -301,64 +305,6 @@ pub mod pallet { Ok(()) } } - - #[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() - }; - - // Check session index matches - match registration.session_index.cmp(&CurrentSessionIndex::::get()) { - Ordering::Greater => return InvalidTransaction::Future.into(), - Ordering::Less => return InvalidTransaction::Stale.into(), - Ordering::Equal => (), - } - - // Check authority index is valid - if registration.authority_index >= T::MaxAuthorities::get() { - return InvalidTransaction::BadProof.into() - } - let Some(authority_id) = NextAuthorityIds::::get(registration.authority_index) - else { - return 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() - } - - // Check signature. Note that we don't use regular signed transactions for registration - // as we don't want validators to have to pay to register. Spam is prevented by only - // allowing one registration per session per validator (see above). - let signature_ok = registration.using_encoded(|encoded_registration| { - authority_id.verify(&encoded_registration, signature) - }); - if !signature_ok { - return InvalidTransaction::BadProof.into() - } - - ValidTransaction::with_tag_prefix("MixnetRegistration") - .priority(T::RegistrationPriority::get()) - // Include both authority index _and_ ID in tag in case of forks with different - // authority lists - .and_provides(( - registration.session_index, - registration.authority_index, - authority_id, - )) - .longevity( - (T::NextSessionRotation::average_session_length() / 2_u32.into()) - .try_into() - .unwrap_or(64_u64), - ) - .build() - } - } } impl Pallet { @@ -543,6 +489,53 @@ impl Pallet { }, } } + + fn validate_register( + registration: &RegistrationFor, + signature: &AuthoritySignature, + ) -> TransactionValidity { + // Check session index matches + match registration.session_index.cmp(&CurrentSessionIndex::::get()) { + Ordering::Greater => return InvalidTransaction::Future.into(), + Ordering::Less => return InvalidTransaction::Stale.into(), + Ordering::Equal => (), + } + + // Check authority index is valid + if registration.authority_index >= T::MaxAuthorities::get() { + return InvalidTransaction::BadProof.into() + } + let Some(authority_id) = NextAuthorityIds::::get(registration.authority_index) else { + return 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() + } + + // Check signature. Note that we don't use regular signed transactions for registration + // as we don't want validators to have to pay to register. Spam is prevented by only + // allowing one registration per session per validator (see above). + let signature_ok = registration.using_encoded(|encoded_registration| { + authority_id.verify(&encoded_registration, signature) + }); + if !signature_ok { + return InvalidTransaction::BadProof.into() + } + + ValidTransaction::with_tag_prefix("MixnetRegistration") + .priority(T::RegistrationPriority::get()) + // Include both authority index _and_ ID in tag in case of forks with different + // authority lists + .and_provides((registration.session_index, registration.authority_index, authority_id)) + .longevity( + (T::NextSessionRotation::average_session_length() / 2_u32.into()) + .try_into() + .unwrap_or(64_u64), + ) + .build() + } } impl sp_runtime::BoundToRuntimeAppPublic for Pallet { diff --git a/substrate/frame/offences/benchmarking/src/mock.rs b/substrate/frame/offences/benchmarking/src/mock.rs index c5c178aa4443..ef9538373b2a 100644 --- a/substrate/frame/offences/benchmarking/src/mock.rs +++ b/substrate/frame/offences/benchmarking/src/mock.rs @@ -173,6 +173,28 @@ where } } +impl frame_system::offchain::CreateTransaction for Test +where + RuntimeCall: From, +{ + type Extension = (); + fn create_transaction( + call: >::RuntimeCall, + extension: Self::Extension, + ) -> Self::Extrinsic { + UncheckedExtrinsic::new_transaction(call, extension) + } +} + +impl frame_system::offchain::CreateAuthorizedTransaction for Test +where + RuntimeCall: From, +{ + fn create_extension() -> Self::Extension { + () + } +} + impl crate::Config for Test {} pub type Block = sp_runtime::generic::Block; diff --git a/substrate/frame/support/procedural/src/construct_runtime/expand/call.rs b/substrate/frame/support/procedural/src/construct_runtime/expand/call.rs index f8a774e38554..b00448761d74 100644 --- a/substrate/frame/support/procedural/src/construct_runtime/expand/call.rs +++ b/substrate/frame/support/procedural/src/construct_runtime/expand/call.rs @@ -29,7 +29,6 @@ pub fn expand_outer_dispatch( ) -> TokenStream { let mut variant_defs = TokenStream::new(); let mut variant_patterns = Vec::new(); - let mut variant_usages = Vec::new(); let mut query_call_part_macros = Vec::new(); let mut pallet_names = Vec::new(); let mut pallet_attrs = Vec::new(); @@ -56,7 +55,6 @@ pub fn expand_outer_dispatch( #[codec(index = #index)] #name( #scrate::dispatch::CallableCallFor<#name, #runtime> ), }); - variant_usages.push(quote!( #scrate::dispatch::CallableCallFor<#name, #runtime> )); variant_patterns.push(quote!(RuntimeCall::#name(call))); pallet_names.push(name); pallet_attrs.push(attr); @@ -227,15 +225,8 @@ pub fn expand_outer_dispatch( fn authorize( &self, source: #scrate::pallet_prelude::TransactionSource, - ) -> ::core::option::Option< - ::core::result::Result< - ( - #scrate::pallet_prelude::ValidTransaction, - #scrate::pallet_prelude::Weight, - ), - #scrate::pallet_prelude::TransactionValidityError - > - > { + ) -> ::core::option::Option<#scrate::pallet_prelude::TransactionValidityWithRefund> + { match self { #( #pallet_attrs