Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Introduce CheckNonZeroSender #10413

Merged
merged 14 commits into from
Dec 8, 2021
6 changes: 5 additions & 1 deletion bin/node/cli/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ pub fn create_extrinsic(
.unwrap_or(2) as u64;
let tip = 0;
let extra: node_runtime::SignedExtra = (
frame_system::CheckNonZeroSender::<node_runtime::Runtime>::new(),
frame_system::CheckSpecVersion::<node_runtime::Runtime>::new(),
frame_system::CheckTxVersion::<node_runtime::Runtime>::new(),
frame_system::CheckGenesis::<node_runtime::Runtime>::new(),
Expand All @@ -99,6 +100,7 @@ pub fn create_extrinsic(
function.clone(),
extra.clone(),
(
(),
node_runtime::VERSION.spec_version,
node_runtime::VERSION.transaction_version,
genesis_hash,
Expand Down Expand Up @@ -719,6 +721,7 @@ mod tests {
let function =
Call::Balances(BalancesCall::transfer { dest: to.into(), value: amount });

let check_non_zero_sender = frame_system::CheckNonZeroSender::new();
let check_spec_version = frame_system::CheckSpecVersion::new();
let check_tx_version = frame_system::CheckTxVersion::new();
let check_genesis = frame_system::CheckGenesis::new();
Expand All @@ -727,6 +730,7 @@ mod tests {
let check_weight = frame_system::CheckWeight::new();
let tx_payment = pallet_asset_tx_payment::ChargeAssetTxPayment::from(0, None);
let extra = (
check_non_zero_sender,
check_spec_version,
check_tx_version,
check_genesis,
Expand All @@ -738,7 +742,7 @@ mod tests {
let raw_payload = SignedPayload::from_raw(
function,
extra,
(spec_version, transaction_version, genesis_hash, genesis_hash, (), (), ()),
((), spec_version, transaction_version, genesis_hash, genesis_hash, (), (), ()),
);
let signature = raw_payload.using_encoded(|payload| signer.sign(payload));
let (function, extra, _) = raw_payload.deconstruct();
Expand Down
4 changes: 2 additions & 2 deletions bin/node/executor/tests/submit_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ fn should_submit_signed_twice_from_the_same_account() {
let s = state.read();
fn nonce(tx: UncheckedExtrinsic) -> frame_system::CheckNonce<Runtime> {
let extra = tx.signature.unwrap().2;
extra.4
extra.5
}
let nonce1 = nonce(UncheckedExtrinsic::decode(&mut &*s.transactions[0]).unwrap());
let nonce2 = nonce(UncheckedExtrinsic::decode(&mut &*s.transactions[1]).unwrap());
Expand Down Expand Up @@ -195,7 +195,7 @@ fn should_submit_signed_twice_from_all_accounts() {
let s = state.read();
fn nonce(tx: UncheckedExtrinsic) -> frame_system::CheckNonce<Runtime> {
let extra = tx.signature.unwrap().2;
extra.4
extra.5
}
let nonce1 = nonce(UncheckedExtrinsic::decode(&mut &*s.transactions[0]).unwrap());
let nonce2 = nonce(UncheckedExtrinsic::decode(&mut &*s.transactions[1]).unwrap());
Expand Down
2 changes: 2 additions & 0 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -973,6 +973,7 @@ where
.saturating_sub(1);
let era = Era::mortal(period, current_block);
let extra = (
frame_system::CheckNonZeroSender::<Runtime>::new(),
frame_system::CheckSpecVersion::<Runtime>::new(),
frame_system::CheckTxVersion::<Runtime>::new(),
frame_system::CheckGenesis::<Runtime>::new(),
Expand Down Expand Up @@ -1322,6 +1323,7 @@ pub type BlockId = generic::BlockId<Block>;
///
/// [`sign`]: <../../testing/src/keyring.rs.html>
pub type SignedExtra = (
frame_system::CheckNonZeroSender<Runtime>,
frame_system::CheckSpecVersion<Runtime>,
frame_system::CheckTxVersion<Runtime>,
frame_system::CheckGenesis<Runtime>,
Expand Down
1 change: 1 addition & 0 deletions bin/node/test-runner-example/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ impl ChainInfo for NodeTemplateChainInfo {
from: <Self::Runtime as frame_system::Config>::AccountId,
) -> Self::SignedExtras {
(
frame_system::CheckNonZeroSender::<Self::Runtime>::new(),
frame_system::CheckSpecVersion::<Self::Runtime>::new(),
frame_system::CheckTxVersion::<Self::Runtime>::new(),
frame_system::CheckGenesis::<Self::Runtime>::new(),
Expand Down
1 change: 1 addition & 0 deletions bin/node/testing/src/keyring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ pub fn to_session_keys(
/// Returns transaction extra.
pub fn signed_extra(nonce: Index, extra_fee: Balance) -> SignedExtra {
(
frame_system::CheckNonZeroSender::new(),
frame_system::CheckSpecVersion::new(),
frame_system::CheckTxVersion::new(),
frame_system::CheckGenesis::new(),
Expand Down
100 changes: 100 additions & 0 deletions frame/system/src/extensions/check_non_zero_sender.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
// This file is part of Substrate.

// Copyright (C) 2017-2021 Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

use crate::Config;
use codec::{Decode, Encode};
use frame_support::weights::DispatchInfo;
use scale_info::TypeInfo;
use sp_runtime::{
traits::{DispatchInfoOf, Dispatchable, SignedExtension},
transaction_validity::{
InvalidTransaction, TransactionValidity, TransactionValidityError, ValidTransaction,
},
};
use sp_std::{marker::PhantomData, prelude::*};

/// Check to ensure that the sender is not the zero address.
#[derive(Encode, Decode, Clone, Eq, PartialEq, TypeInfo)]
#[scale_info(skip_type_params(T))]
pub struct CheckNonZeroSender<T: Config + Send + Sync>(PhantomData<T>);

impl<T: Config + Send + Sync> sp_std::fmt::Debug for CheckNonZeroSender<T> {
#[cfg(feature = "std")]
fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result {
write!(f, "CheckNonZeroSender")
}

#[cfg(not(feature = "std"))]
fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result {
Ok(())
}
}

impl<T: Config + Send + Sync> CheckNonZeroSender<T> {
/// Create new `SignedExtension` to check runtime version.
pub fn new() -> Self {
Self(sp_std::marker::PhantomData)
}
}

impl<T: Config + Send + Sync> SignedExtension for CheckNonZeroSender<T>
where
T::Call: Dispatchable<Info = DispatchInfo>,
{
type AccountId = T::AccountId;
type Call = T::Call;
type AdditionalSigned = ();
type Pre = ();
const IDENTIFIER: &'static str = "CheckNonZeroSender";

fn additional_signed(&self) -> sp_std::result::Result<(), TransactionValidityError> {
Ok(())
}

fn validate(
&self,
who: &Self::AccountId,
_call: &Self::Call,
_info: &DispatchInfoOf<Self::Call>,
_len: usize,
) -> TransactionValidity {
if who.using_encoded(|d| d.into_iter().all(|x| *x == 0)) {
return Err(TransactionValidityError::Invalid(InvalidTransaction::BadSigner))
}
Ok(ValidTransaction::default())
}
}

#[cfg(test)]
mod tests {
use super::*;
use crate::mock::{new_test_ext, Test, CALL};
use frame_support::{assert_noop, assert_ok};

#[test]
fn zero_account_ban_works() {
new_test_ext().execute_with(|| {
let info = DispatchInfo::default();
let len = 0_usize;
assert_noop!(
CheckNonZeroSender::<Test>::new().validate(&0, CALL, &info, len),
InvalidTransaction::BadSigner
);
assert_ok!(CheckNonZeroSender::<Test>::new().validate(&1, CALL, &info, len));
})
}
}
1 change: 1 addition & 0 deletions frame/system/src/extensions/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

pub mod check_genesis;
pub mod check_mortality;
pub mod check_non_zero_sender;
pub mod check_nonce;
pub mod check_spec_version;
pub mod check_tx_version;
Expand Down
3 changes: 2 additions & 1 deletion frame/system/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,8 @@ mod tests;
pub mod weights;

pub use extensions::{
check_genesis::CheckGenesis, check_mortality::CheckMortality, check_nonce::CheckNonce,
check_genesis::CheckGenesis, check_mortality::CheckMortality,
check_non_zero_sender::CheckNonZeroSender, check_nonce::CheckNonce,
check_spec_version::CheckSpecVersion, check_tx_version::CheckTxVersion,
check_weight::CheckWeight,
};
Expand Down
3 changes: 3 additions & 0 deletions primitives/runtime/src/transaction_validity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ pub enum InvalidTransaction {
/// A transaction with a mandatory dispatch. This is invalid; only inherent extrinsics are
/// allowed to have mandatory dispatches.
MandatoryDispatch,
/// The sending address is disabled or known to be invalid.
BadSigner,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will require that clients update before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean it is not a hard requirement, aka we don't break authoring. We will just get a failure on validate_block when trying to decode the return value.

}

impl InvalidTransaction {
Expand Down Expand Up @@ -109,6 +111,7 @@ impl From<InvalidTransaction> for &'static str {
InvalidTransaction::MandatoryDispatch =>
"Transaction dispatch is mandatory; transactions may not have mandatory dispatches.",
InvalidTransaction::Custom(_) => "InvalidTransaction custom error",
InvalidTransaction::BadSigner => "Invalid signing address",
}
}
}
Expand Down
1 change: 1 addition & 0 deletions test-utils/test-runner/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@
//! let nonce = frame_system::Pallet::<Self::Runtime>::account_nonce(from);
//!
//! (
//! frame_system::CheckNonZeroSender::<Self::Runtime>::new(),
//! frame_system::CheckSpecVersion::<Self::Runtime>::new(),
//! frame_system::CheckTxVersion::<Self::Runtime>::new(),
//! frame_system::CheckGenesis::<Self::Runtime>::new(),
Expand Down