Introduces TypeWithDefault<T, D: Get<T>> (#4034)

Needed for: polkadot-fellows/runtimes#248 This PR introduces a new type `TypeWithDefault<T, D: Get<T>>` to be able to provide a custom default for any type. This can, then, be used to provide the nonce type that returns the current block number as the default, to avoid replay of immortal transactions. --------- Co-authored-by: Oliver Tale-Yazdi <[email protected]>
paritytech · May 6, 2024 · 73c89d3 · 73c89d3
1 parent c0234be
commit 73c89d3
Show file tree

Hide file tree

Showing 8 changed files with 587 additions and 31 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/prdoc/pr_4034.prdoc b/prdoc/pr_4034.prdoc
@@ -0,0 +1,14 @@
+title: "Introduces `TypeWithDefault<T, D: Get<T>>`"
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      This PR introduces a new type `TypeWithDefault<T, D: Get<T>>` to be able to provide a 
+      custom default for any type. This can, then, be used to provide the nonce type that returns 
+      the current block number as the default, to avoid replay of immortal transactions.
+
+crates:
+- name: sp-runtime
+  bump: minor
+- name: frame-system
+  bump: patch
diff --git a/substrate/frame/system/src/extensions/check_nonce.rs b/substrate/frame/system/src/extensions/check_nonce.rs
@@ -142,7 +142,7 @@ mod tests {
 			crate::Account::<Test>::insert(
 				1,
 				crate::AccountInfo {
-					nonce: 1,
+					nonce: 1u64.into(),
 					consumers: 0,
 					providers: 1,
 					sufficients: 0,
@@ -153,20 +153,20 @@ mod tests {
 			let len = 0_usize;
 			// stale
 			assert_noop!(
-				CheckNonce::<Test>(0).validate(&1, CALL, &info, len),
+				CheckNonce::<Test>(0u64.into()).validate(&1, CALL, &info, len),
 				InvalidTransaction::Stale
 			);
 			assert_noop!(
-				CheckNonce::<Test>(0).pre_dispatch(&1, CALL, &info, len),
+				CheckNonce::<Test>(0u64.into()).pre_dispatch(&1, CALL, &info, len),
 				InvalidTransaction::Stale
 			);
 			// correct
-			assert_ok!(CheckNonce::<Test>(1).validate(&1, CALL, &info, len));
-			assert_ok!(CheckNonce::<Test>(1).pre_dispatch(&1, CALL, &info, len));
+			assert_ok!(CheckNonce::<Test>(1u64.into()).validate(&1, CALL, &info, len));
+			assert_ok!(CheckNonce::<Test>(1u64.into()).pre_dispatch(&1, CALL, &info, len));
 			// future
-			assert_ok!(CheckNonce::<Test>(5).validate(&1, CALL, &info, len));
+			assert_ok!(CheckNonce::<Test>(5u64.into()).validate(&1, CALL, &info, len));
 			assert_noop!(
-				CheckNonce::<Test>(5).pre_dispatch(&1, CALL, &info, len),
+				CheckNonce::<Test>(5u64.into()).pre_dispatch(&1, CALL, &info, len),
 				InvalidTransaction::Future
 			);
 		})
@@ -178,7 +178,7 @@ mod tests {
 			crate::Account::<Test>::insert(
 				2,
 				crate::AccountInfo {
-					nonce: 1,
+					nonce: 1u64.into(),
 					consumers: 0,
 					providers: 1,
 					sufficients: 0,
@@ -188,7 +188,7 @@ mod tests {
 			crate::Account::<Test>::insert(
 				3,
 				crate::AccountInfo {
-					nonce: 1,
+					nonce: 1u64.into(),
 					consumers: 0,
 					providers: 0,
 					sufficients: 1,
@@ -199,19 +199,19 @@ mod tests {
 			let len = 0_usize;
 			// Both providers and sufficients zero
 			assert_noop!(
-				CheckNonce::<Test>(1).validate(&1, CALL, &info, len),
+				CheckNonce::<Test>(1u64.into()).validate(&1, CALL, &info, len),
 				InvalidTransaction::Payment
 			);
 			assert_noop!(
-				CheckNonce::<Test>(1).pre_dispatch(&1, CALL, &info, len),
+				CheckNonce::<Test>(1u64.into()).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));
+			assert_ok!(CheckNonce::<Test>(1u64.into()).validate(&2, CALL, &info, len));
+			assert_ok!(CheckNonce::<Test>(1u64.into()).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));
+			assert_ok!(CheckNonce::<Test>(1u64.into()).validate(&3, CALL, &info, len));
+			assert_ok!(CheckNonce::<Test>(1u64.into()).pre_dispatch(&3, CALL, &info, len));
 		})
 	}
 }
diff --git a/substrate/frame/system/src/mock.rs b/substrate/frame/system/src/mock.rs
@@ -17,7 +17,7 @@
 
 use crate::{self as frame_system, *};
 use frame_support::{derive_impl, parameter_types};
-use sp_runtime::{BuildStorage, Perbill};
+use sp_runtime::{type_with_default::TypeWithDefault, BuildStorage, Perbill};
 
 type Block = mocking::MockBlock<Test>;
 
@@ -78,6 +78,14 @@ impl OnKilledAccount<u64> for RecordKilled {
 	}
 }
 
+#[derive(Debug, TypeInfo)]
+pub struct DefaultNonceProvider;
+impl Get<u64> for DefaultNonceProvider {
+	fn get() -> u64 {
+		System::block_number()
+	}
+}
+
 #[derive_impl(frame_system::config_preludes::TestDefaultConfig)]
 impl Config for Test {
 	type BlockWeights = RuntimeBlockWeights;
@@ -87,6 +95,7 @@ impl Config for Test {
 	type AccountData = u32;
 	type OnKilledAccount = RecordKilled;
 	type MultiBlockMigrator = MockedMigrator;
+	type Nonce = TypeWithDefault<u64, DefaultNonceProvider>;
 }
 
 parameter_types! {

diff --git a/substrate/frame/system/src/tests.rs b/substrate/frame/system/src/tests.rs
@@ -102,7 +102,13 @@ fn stored_map_works() {
 
 		assert_eq!(
 			Account::<Test>::get(0),
-			AccountInfo { nonce: 0, providers: 1, consumers: 0, sufficients: 0, data: 42 }
+			AccountInfo {
+				nonce: 0u64.into(),
+				providers: 1,
+				consumers: 0,
+				sufficients: 0,
+				data: 42
+			}
 		);
 
 		assert_ok!(System::inc_consumers(&0));
@@ -126,26 +132,26 @@ fn provider_ref_handover_to_self_sufficient_ref_works() {
 	new_test_ext().execute_with(|| {
 		assert_eq!(System::inc_providers(&0), IncRefStatus::Created);
 		System::inc_account_nonce(&0);
-		assert_eq!(System::account_nonce(&0), 1);
+		assert_eq!(System::account_nonce(&0), 1u64.into());
 
 		// a second reference coming and going doesn't change anything.
 		assert_eq!(System::inc_sufficients(&0), IncRefStatus::Existed);
 		assert_eq!(System::dec_sufficients(&0), DecRefStatus::Exists);
-		assert_eq!(System::account_nonce(&0), 1);
+		assert_eq!(System::account_nonce(&0), 1u64.into());
 
 		// a provider reference coming and going doesn't change anything.
 		assert_eq!(System::inc_providers(&0), IncRefStatus::Existed);
 		assert_eq!(System::dec_providers(&0).unwrap(), DecRefStatus::Exists);
-		assert_eq!(System::account_nonce(&0), 1);
+		assert_eq!(System::account_nonce(&0), 1u64.into());
 
 		// decreasing the providers with a self-sufficient present should not delete the account
 		assert_eq!(System::inc_sufficients(&0), IncRefStatus::Existed);
 		assert_eq!(System::dec_providers(&0).unwrap(), DecRefStatus::Exists);
-		assert_eq!(System::account_nonce(&0), 1);
+		assert_eq!(System::account_nonce(&0), 1u64.into());
 
 		// decreasing the sufficients should delete the account
 		assert_eq!(System::dec_sufficients(&0), DecRefStatus::Reaped);
-		assert_eq!(System::account_nonce(&0), 0);
+		assert_eq!(System::account_nonce(&0), 0u64.into());
 	});
 }
 
@@ -154,26 +160,26 @@ fn self_sufficient_ref_handover_to_provider_ref_works() {
 	new_test_ext().execute_with(|| {
 		assert_eq!(System::inc_sufficients(&0), IncRefStatus::Created);
 		System::inc_account_nonce(&0);
-		assert_eq!(System::account_nonce(&0), 1);
+		assert_eq!(System::account_nonce(&0), 1u64.into());
 
 		// a second reference coming and going doesn't change anything.
 		assert_eq!(System::inc_providers(&0), IncRefStatus::Existed);
 		assert_eq!(System::dec_providers(&0).unwrap(), DecRefStatus::Exists);
-		assert_eq!(System::account_nonce(&0), 1);
+		assert_eq!(System::account_nonce(&0), 1u64.into());
 
 		// a sufficient reference coming and going doesn't change anything.
 		assert_eq!(System::inc_sufficients(&0), IncRefStatus::Existed);
 		assert_eq!(System::dec_sufficients(&0), DecRefStatus::Exists);
-		assert_eq!(System::account_nonce(&0), 1);
+		assert_eq!(System::account_nonce(&0), 1u64.into());
 
 		// decreasing the sufficients with a provider present should not delete the account
 		assert_eq!(System::inc_providers(&0), IncRefStatus::Existed);
 		assert_eq!(System::dec_sufficients(&0), DecRefStatus::Exists);
-		assert_eq!(System::account_nonce(&0), 1);
+		assert_eq!(System::account_nonce(&0), 1u64.into());
 
 		// decreasing the providers should delete the account
 		assert_eq!(System::dec_providers(&0).unwrap(), DecRefStatus::Reaped);
-		assert_eq!(System::account_nonce(&0), 0);
+		assert_eq!(System::account_nonce(&0), 0u64.into());
 	});
 }
 
@@ -182,7 +188,7 @@ fn sufficient_cannot_support_consumer() {
 	new_test_ext().execute_with(|| {
 		assert_eq!(System::inc_sufficients(&0), IncRefStatus::Created);
 		System::inc_account_nonce(&0);
-		assert_eq!(System::account_nonce(&0), 1);
+		assert_eq!(System::account_nonce(&0), 1u64.into());
 		assert_noop!(System::inc_consumers(&0), DispatchError::NoProviders);
 
 		assert_eq!(System::inc_providers(&0), IncRefStatus::Existed);
@@ -198,18 +204,18 @@ fn provider_required_to_support_consumer() {
 
 		assert_eq!(System::inc_providers(&0), IncRefStatus::Created);
 		System::inc_account_nonce(&0);
-		assert_eq!(System::account_nonce(&0), 1);
+		assert_eq!(System::account_nonce(&0), 1u64.into());
 
 		assert_eq!(System::inc_providers(&0), IncRefStatus::Existed);
 		assert_eq!(System::dec_providers(&0).unwrap(), DecRefStatus::Exists);
-		assert_eq!(System::account_nonce(&0), 1);
+		assert_eq!(System::account_nonce(&0), 1u64.into());
 
 		assert_ok!(System::inc_consumers(&0));
 		assert_noop!(System::dec_providers(&0), DispatchError::ConsumerRemaining);
 
 		System::dec_consumers(&0);
 		assert_eq!(System::dec_providers(&0).unwrap(), DecRefStatus::Reaped);
-		assert_eq!(System::account_nonce(&0), 0);
+		assert_eq!(System::account_nonce(&0), 0u64.into());
 	});
 }
 
@@ -858,3 +864,20 @@ fn last_runtime_upgrade_spec_version_usage() {
 		}
 	}
 }
+
+#[test]
+fn test_default_account_nonce() {
+	new_test_ext().execute_with(|| {
+		System::set_block_number(2);
+		assert_eq!(System::account_nonce(&1), 2u64.into());
+
+		System::inc_account_nonce(&1);
+		assert_eq!(System::account_nonce(&1), 3u64.into());
+
+		System::set_block_number(5);
+		assert_eq!(System::account_nonce(&1), 3u64.into());
+
+		Account::<Test>::remove(&1);
+		assert_eq!(System::account_nonce(&1), 5u64.into());
+	});
+}
diff --git a/substrate/primitives/runtime/Cargo.toml b/substrate/primitives/runtime/Cargo.toml
@@ -22,6 +22,7 @@ either = { version = "1.5", default-features = false }
 hash256-std-hasher = { version = "0.15.2", default-features = false }
 impl-trait-for-tuples = "0.2.2"
 log = { workspace = true }
+num-traits = { version = "0.2.17", default-features = false }
 paste = "1.0"
 rand = { version = "0.8.5", optional = true }
 scale-info = { version = "2.11.1", default-features = false, features = ["derive"] }
@@ -54,6 +55,7 @@ std = [
 	"either/use_std",
 	"hash256-std-hasher/std",
 	"log/std",
+	"num-traits/std",
 	"rand",
 	"scale-info/std",
 	"serde/std",

diff --git a/substrate/primitives/runtime/src/lib.rs b/substrate/primitives/runtime/src/lib.rs
@@ -91,6 +91,7 @@ mod runtime_string;
 pub mod testing;
 pub mod traits;
 pub mod transaction_validity;
+pub mod type_with_default;
 
 pub use crate::runtime_string::*;