Skip to content

Commit

Permalink
FRAME: Revamp Preimage pallet to use Consideration (paritytech#1363)
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: command-bot <>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Kian Paimani <[email protected]>
  • Loading branch information
@gavofyork @ggwpez
@franciscoaguirre @kianenigma
4 people authored Sep 17, 2023
1 parent 5e5651e commit bc16714
Show file tree
Hide file tree
Showing 15 changed files with 654 additions and 317 deletions.
15 changes: 10 additions & 5 deletions substrate/bin/node/runtime/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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! {
Expand Down
14 changes: 13 additions & 1 deletion substrate/frame/balances/src/tests/currency_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand All @@ -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()
Expand Down
3 changes: 1 addition & 2 deletions substrate/frame/democracy/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
77 changes: 50 additions & 27 deletions substrate/frame/preimage/src/benchmarking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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 {
Expand All @@ -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()
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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.
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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
}
Loading

0 comments on commit bc16714

Please sign in to comment.