Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make CheckNonce refuse transactions signed by accounts with no providers #1578

Merged
merged 2 commits into from
Oct 10, 2023
Merged

Make CheckNonce refuse transactions signed by accounts with no providers #1578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7589baa
Make CheckNonce refuse transactions signed by accounts with no providers
zdave-parity Sep 14, 2023
c509e17
Merge branch 'master' into nonce-require-provider
bkchr Oct 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion substrate/bin/node/executor/tests/submit_transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ fn submitted_transaction_should_be_valid() {
let author = extrinsic.signature.clone().unwrap().0;
let address = Indices::lookup(author).unwrap();
let data = pallet_balances::AccountData { free: 5_000_000_000_000, ..Default::default() };
let account = frame_system::AccountInfo { data, ..Default::default() };
let account = frame_system::AccountInfo { providers: 1, data, ..Default::default() };
<frame_system::Account<Runtime>>::insert(&address, account);

// check validity
Expand Down
25 changes: 8 additions & 17 deletions substrate/client/service/test/src/client/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ use sp_runtime::{
};
use sp_state_machine::{backend::Backend as _, InMemoryBackend, OverlayedChanges, StateMachine};
use sp_storage::{ChildInfo, StorageKey};
use sp_trie::{LayoutV0, TrieConfiguration};
use std::{collections::HashSet, sync::Arc};
use substrate_test_runtime::TestAPI;
use substrate_test_runtime_client::{
Expand All @@ -62,22 +61,17 @@ fn construct_block(
backend: &InMemoryBackend<BlakeTwo256>,
number: BlockNumber,
parent_hash: Hash,
state_root: Hash,
txs: Vec<Transfer>,
) -> (Vec<u8>, Hash) {
) -> Vec<u8> {
let transactions = txs.into_iter().map(|tx| tx.into_unchecked_extrinsic()).collect::<Vec<_>>();

let iter = transactions.iter().map(Encode::encode);
let extrinsics_root = LayoutV0::<BlakeTwo256>::ordered_trie_root(iter).into();

let mut header = Header {
parent_hash,
number,
state_root,
extrinsics_root,
state_root: Default::default(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AIUI state_root and extrinsics_root are not used by Core_initialize_block and should just be set to 0 here.

extrinsics_root: Default::default(),
digest: Digest { logs: vec![] },
};
let hash = header.hash();
let mut overlay = OverlayedChanges::default();
let backend_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(backend);
let runtime_code = backend_runtime_code.runtime_code().expect("Code is part of the backend");
Expand Down Expand Up @@ -124,19 +118,16 @@ fn construct_block(
.unwrap();
header = Header::decode(&mut &ret_data[..]).unwrap();

(vec![].and(&Block { header, extrinsics: transactions }), hash)
vec![].and(&Block { header, extrinsics: transactions })
}

fn block1(genesis_hash: Hash, backend: &InMemoryBackend<BlakeTwo256>) -> (Vec<u8>, Hash) {
fn block1(genesis_hash: Hash, backend: &InMemoryBackend<BlakeTwo256>) -> Vec<u8> {
construct_block(
backend,
1,
genesis_hash,
array_bytes::hex_n_into_unchecked(
"25e5b37074063ab75c889326246640729b40d0c86932edc527bc80db0e04fe5c",
),
vec![Transfer {
from: AccountKeyring::Alice.into(),
from: AccountKeyring::One.into(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alice has no balance in the genesis state. As this test runtime has no transaction fees I think this was just resulting in the transaction failing. With the CheckNonce change the transaction was invalid and causing the runtime to panic.

to: AccountKeyring::Two.into(),
amount: 69 * DOLLARS,
nonce: 0,
Expand Down Expand Up @@ -175,7 +166,7 @@ fn construct_genesis_should_work_with_native() {
let genesis_hash = insert_genesis_block(&mut storage);

let backend = InMemoryBackend::from((storage, StateVersion::default()));
let (b1data, _b1hash) = block1(genesis_hash, &backend);
let b1data = block1(genesis_hash, &backend);
let backend_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&backend);
let runtime_code = backend_runtime_code.runtime_code().expect("Code is part of the backend");

Expand Down Expand Up @@ -206,7 +197,7 @@ fn construct_genesis_should_work_with_wasm() {
let genesis_hash = insert_genesis_block(&mut storage);

let backend = InMemoryBackend::from((storage, StateVersion::default()));
let (b1data, _b1hash) = block1(genesis_hash, &backend);
let b1data = block1(genesis_hash, &backend);
let backend_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&backend);
let runtime_code = backend_runtime_code.runtime_code().expect("Code is part of the backend");

Expand Down
56 changes: 53 additions & 3 deletions substrate/frame/system/src/extensions/check_nonce.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use codec::{Decode, Encode};
use frame_support::dispatch::DispatchInfo;
use scale_info::TypeInfo;
use sp_runtime::{
traits::{DispatchInfoOf, Dispatchable, One, SignedExtension},
traits::{DispatchInfoOf, Dispatchable, One, SignedExtension, Zero},
transaction_validity::{
InvalidTransaction, TransactionLongevity, TransactionValidity, TransactionValidityError,
ValidTransaction,
Expand Down Expand Up @@ -80,6 +80,10 @@ where
_len: usize,
) -> Result<(), TransactionValidityError> {
let mut account = crate::Account::<T>::get(who);
if account.providers.is_zero() && account.sufficients.is_zero() {
// Nonce storage not paid for
return Err(InvalidTransaction::Payment.into())
}
if self.0 != account.nonce {
return Err(if self.0 < account.nonce {
InvalidTransaction::Stale
Expand All @@ -100,8 +104,11 @@ where
_info: &DispatchInfoOf<Self::Call>,
_len: usize,
) -> TransactionValidity {
// check index
let account = crate::Account::<T>::get(who);
if account.providers.is_zero() && account.sufficients.is_zero() {
// Nonce storage not paid for
return InvalidTransaction::Payment.into()
}
if self.0 < account.nonce {
return InvalidTransaction::Stale.into()
}
Expand Down Expand Up @@ -137,7 +144,7 @@ mod tests {
crate::AccountInfo {
nonce: 1,
consumers: 0,
providers: 0,
providers: 1,
sufficients: 0,
data: 0,
},
Expand All @@ -164,4 +171,47 @@ mod tests {
);
})
}

#[test]
fn signed_ext_check_nonce_requires_provider() {
new_test_ext().execute_with(|| {
crate::Account::<Test>::insert(
2,
crate::AccountInfo {
nonce: 1,
consumers: 0,
providers: 1,
sufficients: 0,
data: 0,
},
);
crate::Account::<Test>::insert(
3,
crate::AccountInfo {
nonce: 1,
consumers: 0,
providers: 0,
sufficients: 1,
data: 0,
},
);
let info = DispatchInfo::default();
let len = 0_usize;
// Both providers and sufficients zero
assert_noop!(
CheckNonce::<Test>(1).validate(&1, CALL, &info, len),
InvalidTransaction::Payment
);
assert_noop!(
CheckNonce::<Test>(1).pre_dispatch(&1, CALL, &info, len),
InvalidTransaction::Payment
);
// Non-zero providers
assert_ok!(CheckNonce::<Test>(1).validate(&2, CALL, &info, len));
assert_ok!(CheckNonce::<Test>(1).pre_dispatch(&2, CALL, &info, len));
// Non-zero sufficients
assert_ok!(CheckNonce::<Test>(1).validate(&3, CALL, &info, len));
assert_ok!(CheckNonce::<Test>(1).pre_dispatch(&3, CALL, &info, len));
})
}
}
Loading