Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Improve inactive fund tracking #13009

Merged
merged 4 commits into from
Dec 27, 2022
Merged

Improve inactive fund tracking #13009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2f8ebc3
Improve inactive fund tracking
gavofyork Dec 22, 2022
9113474
Resetting migration
gavofyork Dec 23, 2022
9d096f0
Fix
gavofyork Dec 23, 2022
38cf923
Update frame/balances/src/migration.rs
gavofyork Dec 23, 2022
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
11 changes: 10 additions & 1 deletion bin/node/executor/tests/basic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use sp_runtime::{
use kitchensink_runtime::{
constants::{currency::*, time::SLOT_DURATION},
Balances, CheckedExtrinsic, Header, Runtime, RuntimeCall, RuntimeEvent, System,
TransactionPayment, UncheckedExtrinsic,
TransactionPayment, Treasury, UncheckedExtrinsic,
};
use node_primitives::{Balance, Hash};
use node_testing::keyring::*;
Expand Down Expand Up @@ -398,6 +398,7 @@ fn full_native_block_import_works() {
});

fees = t.execute_with(|| transfer_fee(&xt()));
let pot = t.execute_with(|| Treasury::pot());

executor_call(&mut t, "Core_execute_block", &block2.0, true).0.unwrap();

Expand All @@ -408,6 +409,14 @@ fn full_native_block_import_works() {
);
assert_eq!(Balances::total_balance(&bob()), 179 * DOLLARS - fees);
let events = vec![
EventRecord {
phase: Phase::Initialization,
event: RuntimeEvent::Treasury(pallet_treasury::Event::UpdatedInactive {
reactivated: 0,
deactivated: pot,
}),
topics: vec![],
},
EventRecord {
phase: Phase::ApplyExtrinsic(0),
event: RuntimeEvent::System(frame_system::Event::ExtrinsicSuccess {
Expand Down
26 changes: 26 additions & 0 deletions frame/balances/src/migration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,29 @@ impl<T: Config<I>, A: Get<Vec<T::AccountId>>, I: 'static> OnRuntimeUpgrade
migrate_v0_to_v1::<T, I>(&A::get())
}
}

pub struct ResetInactive<T, I = ()>(PhantomData<(T, I)>);
impl<T: Config<I>, I: 'static> OnRuntimeUpgrade for ResetInactive<T, I> {
fn on_runtime_upgrade() -> Weight {
let onchain_version = Pallet::<T, I>::on_chain_storage_version();

if onchain_version == 1 {
// Remove the old `StorageVersion` type.
frame_support::storage::unhashed::kill(&frame_support::storage::storage_prefix(
Pallet::<T, I>::name().as_bytes(),
"StorageVersion".as_bytes(),
));
Comment on lines +79 to +83
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Remove the old `StorageVersion` type.
frame_support::storage::unhashed::kill(&frame_support::storage::storage_prefix(
Pallet::<T, I>::name().as_bytes(),
"StorageVersion".as_bytes(),
));

Should have a proper storage version now since it is at version 1.

Copy link
Member

Choose a reason for hiding this comment

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

This is also wrong. The key is not StorageVersion

Copy link
Member Author

Choose a reason for hiding this comment

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

This was old code. AFAIK it should do no harm.


InactiveIssuance::<T, I>::kill();

// Set storage version to `0`.
StorageVersion::new(0).put::<Pallet<T, I>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

@ggwpez If we're at version 1 now, then it doesn't quite make sense to regress and go back to version 0? Or are we really treating this all as a revert?

Copy link
Member

@ggwpez ggwpez Dec 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It is a trick so that the MigrateToTrackInactive v0 -> v1 migration can run again.

Copy link
Member

Choose a reason for hiding this comment

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

Can we not just write a proper migration for this? And introduce storage Version 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

0 is defined as "not tracking InactiveIssuance", 1 is defined as "tracking InactiveIssuance". In Polkadot/Kusama mainnet (which are already at version 1), we are migrating back to 0 and thus no longer tracking InactiveIssuance, and then migrating back to 1 (and thus tracking it, this time correctly). There are no new alterations to storage formats or indeed state transition semantics, and thus it makes no sense to introduce new StorageVersions which would in all manner be exactly equivalent to the existing StorageVersions. The only thing we are doing is mending state.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, from the PoV of Polkadot/Kusama this makes sense. However, we should write these migrations in a generic way so that other downstream users (e.g. parachain teams) can use them as well.

There could at least have been some docs on what this migration does and that it should be executed before the other migration.


log::info!(target: "runtime::balances", "Storage to version 0");
T::DbWeight::get().reads_writes(1, 2)
} else {
log::info!(target: "runtime::balances", "Migration did not execute. This probably should be removed");
T::DbWeight::get().reads(1)
}
}
}
19 changes: 12 additions & 7 deletions frame/treasury/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,8 @@ pub mod pallet {

/// The amount which has been reported as inactive to Currency.
#[pallet::storage]
pub type Inactive<T: Config<I>, I: 'static = ()> = StorageValue<_, BalanceOf<T, I>, ValueQuery>;
pub type Deactivated<T: Config<I>, I: 'static = ()> =
StorageValue<_, BalanceOf<T, I>, ValueQuery>;

/// Proposal indices that have been approved but not yet awarded.
#[pallet::storage]
Expand Down Expand Up @@ -292,6 +293,8 @@ pub mod pallet {
amount: BalanceOf<T, I>,
beneficiary: T::AccountId,
},
/// The inactive funds of the pallet have been updated.
UpdatedInactive { reactivated: BalanceOf<T, I>, deactivated: BalanceOf<T, I> },
}

/// Error for the treasury pallet.
Expand Down Expand Up @@ -321,13 +324,15 @@ pub mod pallet {
/// # </weight>
fn on_initialize(n: T::BlockNumber) -> Weight {
let pot = Self::pot();
let deactivated = Inactive::<T, I>::get();
let deactivated = Deactivated::<T, I>::get();
if pot != deactivated {
match (pot > deactivated, pot.max(deactivated) - pot.min(deactivated)) {
(true, delta) => T::Currency::deactivate(delta),
(false, delta) => T::Currency::reactivate(delta),
}
Inactive::<T, I>::put(&pot);
T::Currency::reactivate(deactivated);
T::Currency::deactivate(pot);
Deactivated::<T, I>::put(&pot);
Copy link
Member

Choose a reason for hiding this comment

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

Also kill the Inactive here as cleanup?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually unsure what the implications are for changing the logic here. Is it simply just reverting code?

Copy link
Member

Choose a reason for hiding this comment

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

Its undoing and fixing the fund-deactivation since it was faultily implemented the first time.
IIUC on the next run the Deactivated will be zero, so the whole pot will be deactivated, which is fine since the foregoing balances migration will reset all deactivated funds.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also kill the Inactive here as cleanup?

Let's delete it in a migration so that it happens once, not in on_init.

Copy link
Contributor

Choose a reason for hiding this comment

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

since it was faultily implemented the first time.

What's the case case where the old logic was wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

Incorrect logic was in crowdloan where a datatype was being misused. See paritytech/polkadot#6471

Self::deposit_event(Event::<T, I>::UpdatedInactive {
reactivated: deactivated,
deactivated: pot,
});
}

// Check to see if we should spend some funds!
Expand Down