FRAME: Revamp Preimage pallet to use Consideration (paritytech#1363)

Make Preimage pallet use Consideration instead of handling deposits
directly.

Other half of paritytech/substrate#13666.
Depends/based on paritytech#1361.

Script for the lazy migration that should be run manually once:
[migrate-preimage-lazy.py](https://github.com/ggwpez/substrate-scripts/blob/master/migrate-preimage-lazy.py).

## TODO

- [x] Migration code.

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: command-bot <>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

substrate/bin/node/runtime/src/lib.rs

@@ -35,11 +35,12 @@ use frame_support::{
 	pallet_prelude::Get,
 	parameter_types,
 	traits::{
-		fungible::{Balanced, Credit, ItemOf},
+		fungible::{Balanced, Credit, HoldConsideration, ItemOf},
 		tokens::{nonfungibles_v2::Inspect, GetSalary, PayFromAccount},
 		AsEnsureOriginWithArg, ConstBool, ConstU128, ConstU16, ConstU32, Contains, Currency,
 		EitherOfDiverse, EqualPrivilegeOnly, Imbalance, InsideBoth, InstanceFilter,
-		KeyOwnerProofSystem, LockIdentifier, Nothing, OnUnbalanced, WithdrawReasons,
+		KeyOwnerProofSystem, LinearStoragePrice, LockIdentifier, Nothing, OnUnbalanced,
+		WithdrawReasons,
 	},
 	weights::{
 		constants::{
@@ -447,19 +448,23 @@ impl pallet_glutton::Config for Runtime {
 }
 
 parameter_types! {
-	pub const PreimageMaxSize: u32 = 4096 * 1024;
 	pub const PreimageBaseDeposit: Balance = 1 * DOLLARS;
 	// One cent: $10,000 / MB
 	pub const PreimageByteDeposit: Balance = 1 * CENTS;
+	pub const PreimageHoldReason: RuntimeHoldReason = RuntimeHoldReason::Preimage(pallet_preimage::HoldReason::Preimage);
 }
 
 impl pallet_preimage::Config for Runtime {
 	type WeightInfo = pallet_preimage::weights::SubstrateWeight<Runtime>;
 	type RuntimeEvent = RuntimeEvent;
 	type Currency = Balances;
 	type ManagerOrigin = EnsureRoot<AccountId>;
-	type BaseDeposit = PreimageBaseDeposit;
-	type ByteDeposit = PreimageByteDeposit;
+	type Consideration = HoldConsideration<
+		AccountId,
+		Balances,
+		PreimageHoldReason,
+		LinearStoragePrice<PreimageBaseDeposit, PreimageByteDeposit, Balance>,
+	>;
 }
 
 parameter_types! {

substrate/frame/balances/src/tests/currency_tests.rs

@@ -22,7 +22,7 @@ use crate::NegativeImbalance;
 use frame_support::traits::{
 	BalanceStatus::{Free, Reserved},
 	Currency,
-	ExistenceRequirement::{self, AllowDeath},
+	ExistenceRequirement::{self, AllowDeath, KeepAlive},
 	Hooks, LockIdentifier, LockableCurrency, NamedReservableCurrency, ReservableCurrency,
 	WithdrawReasons,
 };
@@ -33,6 +33,18 @@ const ID_2: LockIdentifier = *b"2       ";
 pub const CALL: &<Test as frame_system::Config>::RuntimeCall =
 	&RuntimeCall::Balances(crate::Call::transfer_allow_death { dest: 0, value: 0 });
 
+#[test]
+fn ed_should_work() {
+	ExtBuilder::default().existential_deposit(1).build_and_execute_with(|| {
+		assert_ok!(Balances::force_set_balance(RuntimeOrigin::root(), 1, 1000));
+		assert_noop!(
+			<Balances as Currency<_>>::transfer(&1, &10, 1000, KeepAlive),
+			TokenError::NotExpendable
+		);
+		assert_ok!(<Balances as Currency<_>>::transfer(&1, &10, 1000, AllowDeath));
+	});
+}
+
 #[test]
 fn set_lock_with_amount_zero_removes_lock() {
 	ExtBuilder::default()

substrate/frame/democracy/src/tests.rs

@@ -111,8 +111,7 @@ impl pallet_preimage::Config for Test {
 	type WeightInfo = ();
 	type Currency = Balances;
 	type ManagerOrigin = EnsureRoot<u64>;
-	type BaseDeposit = ConstU64<0>;
-	type ByteDeposit = ConstU64<0>;
+	type Consideration = ();
 }
 
 impl pallet_scheduler::Config for Test {

substrate/frame/preimage/src/benchmarking.rs

@@ -18,19 +18,17 @@
 //! Preimage pallet benchmarking.
 
 use super::*;
-use frame_benchmarking::v1::{account, benchmarks, whitelist_account, BenchmarkError};
+use frame_benchmarking::v1::{account, benchmarks, whitelisted_caller, BenchmarkError};
 use frame_support::assert_ok;
 use frame_system::RawOrigin;
 use sp_runtime::traits::Bounded;
 use sp_std::{prelude::*, vec};
 
 use crate::Pallet as Preimage;
 
-const SEED: u32 = 0;
-
-fn funded_account<T: Config>(name: &'static str, index: u32) -> T::AccountId {
-	let caller: T::AccountId = account(name, index, SEED);
-	T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
+fn funded_account<T: Config>() -> T::AccountId {
+	let caller: T::AccountId = whitelisted_caller();
+	T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value() / 2u32.into());
 	caller
 }
 
@@ -49,8 +47,7 @@ benchmarks! {
 	// Expensive note - will reserve.
 	note_preimage {
 		let s in 0 .. MAX_SIZE;
-		let caller = funded_account::<T>("caller", 0);
-		whitelist_account!(caller);
+		let caller = funded_account::<T>();
 		let (preimage, hash) = sized_preimage_and_hash::<T>(s);
 	}: _(RawOrigin::Signed(caller), preimage)
 	verify {
@@ -59,8 +56,7 @@ benchmarks! {
 	// Cheap note - will not reserve since it was requested.
 	note_requested_preimage {
 		let s in 0 .. MAX_SIZE;
-		let caller = funded_account::<T>("caller", 0);
-		whitelist_account!(caller);
+		let caller = funded_account::<T>();
 		let (preimage, hash) = sized_preimage_and_hash::<T>(s);
 		assert_ok!(Preimage::<T>::request_preimage(
 			T::ManagerOrigin::try_successful_origin()
@@ -89,8 +85,7 @@ benchmarks! {
 
 	// Expensive unnote - will unreserve.
 	unnote_preimage {
-		let caller = funded_account::<T>("caller", 0);
-		whitelist_account!(caller);
+		let caller = funded_account::<T>();
 		let (preimage, hash) = preimage_and_hash::<T>();
 		assert_ok!(Preimage::<T>::note_preimage(RawOrigin::Signed(caller.clone()).into(), preimage));
 	}: _(RawOrigin::Signed(caller), hash)
@@ -115,16 +110,15 @@ benchmarks! {
 	// Expensive request - will unreserve the noter's deposit.
 	request_preimage {
 		let (preimage, hash) = preimage_and_hash::<T>();
-		let noter = funded_account::<T>("noter", 0);
-		whitelist_account!(noter);
+		let noter = funded_account::<T>();
 		assert_ok!(Preimage::<T>::note_preimage(RawOrigin::Signed(noter.clone()).into(), preimage));
 	}: _<T::RuntimeOrigin>(
 		T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
 		hash
 	) verify {
-		let deposit = T::BaseDeposit::get() + T::ByteDeposit::get() * MAX_SIZE.into();
-		let s = RequestStatus::Requested { deposit: Some((noter, deposit)), count: 1, len: Some(MAX_SIZE) };
-		assert_eq!(StatusFor::<T>::get(&hash), Some(s));
+		let ticket = TicketOf::<T>::new(&noter, Footprint { count: 1, size: MAX_SIZE as u64 }).unwrap();
+		let s = RequestStatus::Requested { maybe_ticket: Some((noter, ticket)), count: 1, maybe_len: Some(MAX_SIZE) };
+		assert_eq!(RequestStatusFor::<T>::get(&hash), Some(s));
 	}
 	// Cheap request - would unreserve the deposit but none was held.
 	request_no_deposit_preimage {
@@ -138,8 +132,8 @@ benchmarks! {
 		T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
 		hash
 	) verify {
-		let s = RequestStatus::Requested { deposit: None, count: 2, len: Some(MAX_SIZE) };
-		assert_eq!(StatusFor::<T>::get(&hash), Some(s));
+		let s = RequestStatus::Requested { maybe_ticket: None, count: 2, maybe_len: Some(MAX_SIZE) };
+		assert_eq!(RequestStatusFor::<T>::get(&hash), Some(s));
 	}
 	// Cheap request - the preimage is not yet noted, so deposit to unreserve.
 	request_unnoted_preimage {
@@ -148,8 +142,8 @@ benchmarks! {
 		T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
 		hash
 	) verify {
-		let s = RequestStatus::Requested { deposit: None, count: 1, len: None };
-		assert_eq!(StatusFor::<T>::get(&hash), Some(s));
+		let s = RequestStatus::Requested { maybe_ticket: None, count: 1, maybe_len: None };
+		assert_eq!(RequestStatusFor::<T>::get(&hash), Some(s));
 	}
 	// Cheap request - the preimage is already requested, so just a counter bump.
 	request_requested_preimage {
@@ -163,8 +157,8 @@ benchmarks! {
 		T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
 		hash
 	) verify {
-		let s = RequestStatus::Requested { deposit: None, count: 2, len: None };
-		assert_eq!(StatusFor::<T>::get(&hash), Some(s));
+		let s = RequestStatus::Requested { maybe_ticket: None, count: 2, maybe_len: None };
+		assert_eq!(RequestStatusFor::<T>::get(&hash), Some(s));
 	}
 
 	// Expensive unrequest - last reference and it's noted, so will destroy the preimage.
@@ -184,7 +178,7 @@ benchmarks! {
 		T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
 		hash
 	) verify {
-		assert_eq!(StatusFor::<T>::get(&hash), None);
+		assert_eq!(RequestStatusFor::<T>::get(&hash), None);
 	}
 	// Cheap unrequest - last reference, but it's not noted.
 	unrequest_unnoted_preimage {
@@ -198,7 +192,7 @@ benchmarks! {
 		T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
 		hash
 	) verify {
-		assert_eq!(StatusFor::<T>::get(&hash), None);
+		assert_eq!(RequestStatusFor::<T>::get(&hash), None);
 	}
 	// Cheap unrequest - not the last reference.
 	unrequest_multi_referenced_preimage {
@@ -217,9 +211,38 @@ benchmarks! {
 		T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
 		hash
 	) verify {
-		let s = RequestStatus::Requested { deposit: None, count: 1, len: None };
-		assert_eq!(StatusFor::<T>::get(&hash), Some(s));
+		let s = RequestStatus::Requested { maybe_ticket: None, count: 1, maybe_len: None };
+		assert_eq!(RequestStatusFor::<T>::get(&hash), Some(s));
+	}
+
+	ensure_updated {
+		let n in 0..MAX_HASH_UPGRADE_BULK_COUNT;
+
+		let caller = funded_account::<T>();
+		let hashes = (0..n).map(|i| insert_old_unrequested::<T>(i)).collect::<Vec<_>>();
+	}: _(RawOrigin::Signed(caller), hashes)
+	verify {
+		assert_eq!(RequestStatusFor::<T>::iter_keys().count(), n as usize);
+		#[allow(deprecated)]
+		let c = StatusFor::<T>::iter_keys().count();
+		assert_eq!(c, 0);
 	}
 
 	impl_benchmark_test_suite!(Preimage, crate::mock::new_test_ext(), crate::mock::Test);
 }
+
+fn insert_old_unrequested<T: Config>(s: u32) -> <T as frame_system::Config>::Hash {
+	let acc = account("old", s, 0);
+	T::Currency::make_free_balance_be(&acc, BalanceOf::<T>::max_value() / 2u32.into());
+
+	// The preimage size does not matter here as it is not touched.
+	let preimage = s.to_le_bytes();
+	let hash = <T as frame_system::Config>::Hashing::hash(&preimage[..]);
+
+	#[allow(deprecated)]
+	StatusFor::<T>::insert(
+		&hash,
+		OldRequestStatus::Unrequested { deposit: (acc, 123u32.into()), len: preimage.len() as u32 },
+	);
+	hash
+}