Inc provider ref count of authorized accounts

And decrement when their authorization is removed, so the account in
frame_system gets cleaned up.

Author: zdave-parity

Cargo.lock

@@ -15,6 +15,7 @@ targets = ["x86_64-unknown-linux-gnu"]
 [dependencies]
 array-bytes = { version = "6.1", optional = true }
 codec = { package = "parity-scale-codec", version = "3.6.1", default-features = false }
+log = "0.4.17"
 scale-info = { version = "2.5.0", default-features = false, features = ["derive"] }
 frame-benchmarking = { version = "4.0.0-dev", default-features = false, optional = true, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v1.0.0" }
 frame-support = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v1.0.0" }

pallets/transaction-storage/Cargo.toml

@@ -48,6 +48,8 @@ use sp_transaction_storage_proof::{
 pub use pallet::*;
 pub use weights::WeightInfo;
 
+const LOG_TARGET: &str = "runtime::transaction-storage";
+
 /// Maximum bytes that can be stored in one transaction.
 // Setting higher limit also requires raising the allocator limit.
 pub const DEFAULT_MAX_TRANSACTION_SIZE: u32 = 8 * 1024 * 1024;
@@ -621,6 +623,35 @@ pub mod pallet {
 			now >= expiration
 		}
 
+		fn authorization_added(scope: &AuthorizationScopeFor<T>) {
+			match scope {
+				AuthorizationScope::Account(who) => {
+					// Allow nonce storage for transaction replay protection
+					frame_system::Pallet::<T>::inc_providers(who);
+				},
+				AuthorizationScope::Preimage(_) => (),
+			}
+		}
+
+		fn authorization_removed(scope: &AuthorizationScopeFor<T>) {
+			match scope {
+				AuthorizationScope::Account(who) => {
+					// Cleanup nonce storage. Authorized accounts should be careful to use a short
+					// enough lifetime for their store/renew transactions that they aren't at risk
+					// of replay when the account is next authorized.
+					if let Err(err) = frame_system::Pallet::<T>::dec_providers(who) {
+						log::warn!(
+							target: LOG_TARGET,
+							"Failed to decrement provider reference count for authorized account {:?}, \
+							leaking reference: {:?}",
+							who, err
+						);
+					}
+				},
+				AuthorizationScope::Preimage(_) => (),
+			}
+		}
+
 		/// Authorize data storage.
 		fn authorize(scope: AuthorizationScopeFor<T>, transactions: u32, bytes: u64) {
 			let expiration = frame_system::Pallet::<T>::block_number()
@@ -629,34 +660,37 @@ pub mod pallet {
 			Authorizations::<T>::mutate(&scope, |maybe_authorization| {
 				if let Some(authorization) = maybe_authorization {
 					if Self::expired(authorization.expiration) {
-						*maybe_authorization = None;
-					}
-				}
-
-				if let Some(authorization) = maybe_authorization {
-					// An unexpired authorization already exists. Extend it.
-					match scope {
-						AuthorizationScope::Account(_) => {
-							// Add
-							authorization.extent.transactions =
-								authorization.extent.transactions.saturating_add(transactions);
-							authorization.extent.bytes =
-								authorization.extent.bytes.saturating_add(bytes);
-						},
-						AuthorizationScope::Preimage(_) => {
-							// Max
-							authorization.extent.transactions =
-								authorization.extent.transactions.max(transactions);
-							authorization.extent.bytes = authorization.extent.bytes.max(bytes);
-						},
+						// Previous authorization expired. Overwrite it.
+						*authorization = Authorization {
+							extent: AuthorizationExtent { transactions, bytes },
+							expiration,
+						};
+					} else {
+						// An unexpired authorization already exists. Extend it.
+						match scope {
+							AuthorizationScope::Account(_) => {
+								// Add
+								authorization.extent.transactions =
+									authorization.extent.transactions.saturating_add(transactions);
+								authorization.extent.bytes =
+									authorization.extent.bytes.saturating_add(bytes);
+							},
+							AuthorizationScope::Preimage(_) => {
+								// Max
+								authorization.extent.transactions =
+									authorization.extent.transactions.max(transactions);
+								authorization.extent.bytes = authorization.extent.bytes.max(bytes);
+							},
+						}
+						authorization.expiration = expiration;
 					}
-					authorization.expiration = expiration;
 				} else {
-					// No previous authorization, or it expired. Create a fresh one.
+					// No previous authorization. Create a fresh one.
 					*maybe_authorization = Some(Authorization {
 						extent: AuthorizationExtent { transactions, bytes },
 						expiration,
 					});
+					Self::authorization_added(&scope);
 				}
 			});
 		}
@@ -669,6 +703,7 @@ pub mod pallet {
 				return Err(Error::<T>::AuthorizationNotFound.into())
 			};
 			ensure!(Self::expired(authorization.expiration), Error::<T>::AuthorizationNotExpired);
+			Self::authorization_removed(&scope);
 			Ok(())
 		}
 
@@ -741,7 +776,8 @@ pub mod pallet {
 			size: u32,
 			consume: bool,
 		) -> Result<(), TransactionValidityError> {
-			let consume_authorization = |maybe_authorization: &mut Option<Authorization<_>>| -> Result<(), TransactionValidityError> {
+			// Returns true if authorization was removed
+			let consume_authorization = |maybe_authorization: &mut Option<Authorization<_>>| -> Result<bool, TransactionValidityError> {
 				let Some(authorization) = maybe_authorization else {
 					return Err(InvalidTransaction::Payment.into())
 				};
@@ -764,21 +800,26 @@ pub mod pallet {
 				// left.
 				if transactions == 0 || bytes == 0 {
 					*maybe_authorization = None;
+					Ok(true)
 				} else {
 					authorization.extent.transactions = transactions;
 					authorization.extent.bytes = bytes;
+					Ok(false)
 				}
-				Ok(())
 			};
 
 			if consume {
-				Authorizations::<T>::mutate(&scope, consume_authorization)
+				if Authorizations::<T>::mutate(&scope, consume_authorization)? {
+					Self::authorization_removed(&scope);
+				}
 			} else {
 				// Note we call consume_authorization on a temporary; the authorization in storage
 				// is untouched and doesn't actually get consumed
 				let mut authorization = Authorizations::<T>::get(&scope);
-				consume_authorization(&mut authorization)
+				consume_authorization(&mut authorization)?;
 			}
+
+			Ok(())
 		}
 
 		/// Check that authorization with the given scope exists in storage but has expired.

pallets/transaction-storage/src/lib.rs

@@ -28,7 +28,7 @@ use super::{
 use frame_support::{assert_noop, assert_ok};
 use sp_core::blake2_256;
 use sp_runtime::{
-	traits::{Dispatchable, ValidateUnsigned},
+	traits::{Dispatchable, ValidateUnsigned, Zero},
 	transaction_validity::InvalidTransaction,
 };
 use sp_transaction_storage_proof::registration::build_proof;
@@ -224,11 +224,13 @@ fn expired_authorization_clears() {
 	new_test_ext().execute_with(|| {
 		run_to_block(1, || None);
 		let who = 1;
+		assert!(System::providers(&who).is_zero());
 		assert_ok!(TransactionStorage::authorize_account(RuntimeOrigin::root(), who, 2, 2000));
 		assert_eq!(
 			TransactionStorage::account_authorization_extent(who),
 			AuthorizationExtent { transactions: 2, bytes: 2000 },
 		);
+		assert!(!System::providers(&who).is_zero());
 
 		// User uses some of the authorization, and the remaining amount gets updated appropriately
 		run_to_block(2, || None);
@@ -251,6 +253,7 @@ fn expired_authorization_clears() {
 		// User has sufficient storage authorization, but it has expired
 		run_to_block(11, || None);
 		assert!(Authorizations::contains_key(AuthorizationScope::Account(who)));
+		assert!(!System::providers(&who).is_zero());
 		// User cannot use authorization
 		assert_noop!(
 			TransactionStorage::pre_dispatch_signed(&who, &store_call),
@@ -264,6 +267,7 @@ fn expired_authorization_clears() {
 		));
 		// No longer in storage
 		assert!(!Authorizations::contains_key(AuthorizationScope::Account(who)));
+		assert!(System::providers(&who).is_zero());
 	});
 }
 
@@ -272,11 +276,13 @@ fn consumed_authorization_clears() {
 	new_test_ext().execute_with(|| {
 		run_to_block(1, || None);
 		let who = 1;
+		assert!(System::providers(&who).is_zero());
 		assert_ok!(TransactionStorage::authorize_account(RuntimeOrigin::root(), who, 2, 2000));
 		assert_eq!(
 			TransactionStorage::account_authorization_extent(who),
 			AuthorizationExtent { transactions: 2, bytes: 2000 },
 		);
+		assert!(!System::providers(&who).is_zero());
 
 		// User uses some of the authorization, and the remaining amount gets updated appropriately
 		let call = Call::store { data: vec![0; 1000] };
@@ -286,9 +292,11 @@ fn consumed_authorization_clears() {
 			TransactionStorage::account_authorization_extent(who),
 			AuthorizationExtent { transactions: 1, bytes: 1000 },
 		);
+		assert!(!System::providers(&who).is_zero());
 		// Consume the remaining amount
 		assert_ok!(TransactionStorage::pre_dispatch_signed(&who, &call));
 		// Key should be cleared from Authorizations
 		assert!(!Authorizations::contains_key(AuthorizationScope::Account(who)));
+		assert!(System::providers(&who).is_zero());
 	});
 }