Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[pallet_balances] Align FreezeIdentifier, RuntimeFreezeReason and MaxFreezes #2997

Open
bkontur opened this issue Jan 19, 2024 · 0 comments
Open

[pallet_balances] Align FreezeIdentifier, RuntimeFreezeReason and MaxFreezes #2997

bkontur opened this issue Jan 19, 2024 · 0 comments
Assignees
@bkontur

Comments

@bkontur
Copy link
Contributor

bkontur commented Jan 19, 2024

Continuation of #2657 for RuntimeHoldReason where MaxHolds are removed.

Summary

The pallet_balances has storage item Freezes. They use T::FreezeIdentifier as identifier, which is not meant to be a composite_enum. There is one usage of FreezeIdentifier for implementing InspectFreeze here. However, we also have RuntimeFreezeReason here, which is not used anywhere.

For example, I see one usage of freezes in pallet_nomination_pools here, where the composite_enum FreezeReason (evaluated as RuntimeFreezeReason) is used as a FreezeIdentifier.

In the Westend runtime, it is configured as:

type RuntimeFreezeReason = RuntimeFreezeReason`
type FreezeIdentifier = RuntimeFreezeReason`

There is also an integrity_test that verifies RuntimeFreezeReason::VARIANT_COUNT against T::MaxFreezes.

And it seems that RuntimeFreezeReason::VARIANT_COUNT is kind of disconnected from FreezeIdentifier.

According to the original implementation:

Both Holds and Freezes require an identifier which is configurable and is expected to be an enum aggregated across all pallet instances of the runtime.

Questions:

  1. What is the reason for having both RuntimeFreezeReason and FreezeIdentifier?
  2. Why is FreezeIdentifier used as an identifier for Freezes instead of RuntimeFreezeReason?

Proposed solution

We could simplify it as follows:

  • remove MaxFreezes from pallet_balances
  • remove FreezeIdentifier from pallet_balances (add a migration to the RuntimeFreezeReason with some try-state check)
  • just use RuntimeFreezeReason as an id and also bound vec with its VARIANT_COUNT (as we did for RuntimeHoldReason)
	pub type Freezes<T: Config<I>, I: 'static = ()> = StorageMap<
		_,
		Blake2_128Concat,
		T::AccountId,
		BoundedVec<IdAmount<T::RuntimeFreezeReason, T::Balance>, VariantCountOf<T::RuntimeFreezeReason>>,
		ValueQuery,
	>;

I found out, that @bkchr did the same thing for HoldIdentifier here: paritytech/substrate#13869
@bkontur bkontur mentioned this issue Jan 19, 2024
Merged
@bkontur bkontur self-assigned this Jan 19, 2024
github-merge-queue bot pushed a commit that referenced this issue Jan 31, 2024
@bkontur @bkchr
@seadanda @codekitz
[frame] #[pallet::composite_enum] improved variant count handling +…
bb8ddc4 
… removed `pallet_balances`'s `MaxHolds` config (#2657)

I started this investigation/issue based on @liamaharon question
[here](#1801 (comment)).

## Problem

The `pallet_balances` integrity test should correctly detect that the
runtime has correct distinct `HoldReasons` variant count. I assume the
same situation exists for RuntimeFreezeReason.

It is not a critical problem, if we set `MaxHolds` with a sufficiently
large value, everything should be ok. However, in this case, the
integrity_test check becomes less useful.

**Situation for "any" runtime:**
- `HoldReason` enums from different pallets:
```rust
        /// from pallet_nis
        #[pallet::composite_enum]
	pub enum HoldReason {
		NftReceipt,
	}

        /// from pallet_preimage
        #[pallet::composite_enum]
	pub enum HoldReason {
		Preimage,
	}

        // from pallet_state-trie-migration
        #[pallet::composite_enum]
	pub enum HoldReason {
		SlashForContinueMigrate,
		SlashForMigrateCustomTop,
		SlashForMigrateCustomChild,
	}
```

- generated `RuntimeHoldReason` enum looks like:
```rust
pub enum RuntimeHoldReason {

    #[codec(index = 32u8)]
    Preimage(pallet_preimage::HoldReason),

    #[codec(index = 38u8)]
    Nis(pallet_nis::HoldReason),

    #[codec(index = 42u8)]
    StateTrieMigration(pallet_state_trie_migration::HoldReason),
}
```

- composite enum `RuntimeHoldReason` variant count is detected as `3`
- we set `type MaxHolds = ConstU32<3>`
- `pallet_balances::integrity_test` is ok with `3`(at least 3)

However, the real problem can occur in a live runtime where some
functionality might stop working. This is due to a total of 5 distinct
hold reasons (for pallets with multi-instance support, it is even more),
and not all of them can be used because of an incorrect `MaxHolds`,
which is deemed acceptable according to the `integrity_test`:
  ```
  // pseudo-code - if we try to call all of these:

T::Currency::hold(&pallet_nis::HoldReason::NftReceipt.into(),
&nft_owner, deposit)?;
T::Currency::hold(&pallet_preimage::HoldReason::Preimage.into(),
&nft_owner, deposit)?;

T::Currency::hold(&pallet_state_trie_migration::HoldReason::SlashForContinueMigrate.into(),
&nft_owner, deposit)?;

  // With `type MaxHolds = ConstU32<3>` these two will fail

T::Currency::hold(&pallet_state_trie_migration::HoldReason::SlashForMigrateCustomTop.into(),
&nft_owner, deposit)?;

T::Currency::hold(&pallet_state_trie_migration::HoldReason::SlashForMigrateCustomChild.into(),
&nft_owner, deposit)?;
  ```  


## Solutions

A macro `#[pallet::*]` expansion is extended of `VariantCount`
implementation for the `#[pallet::composite_enum]` enum type. This
expansion generates the `VariantCount` implementation for pallets'
`HoldReason`, `FreezeReason`, `LockId`, and `SlashReason`. Enum variants
must be plain enum values without fields to ensure a deterministic
count.

The composite runtime enum, `RuntimeHoldReason` and
`RuntimeFreezeReason`, now sets `VariantCount::VARIANT_COUNT` as the sum
of pallets' enum `VariantCount::VARIANT_COUNT`:
```rust
#[frame_support::pallet(dev_mode)]
mod module_single_instance {

	#[pallet::composite_enum]
	pub enum HoldReason {
		ModuleSingleInstanceReason1,
		ModuleSingleInstanceReason2,
	}
...
}

#[frame_support::pallet(dev_mode)]
mod module_multi_instance {

	#[pallet::composite_enum]
	pub enum HoldReason<I: 'static = ()> {
		ModuleMultiInstanceReason1,
		ModuleMultiInstanceReason2,
		ModuleMultiInstanceReason3,
	}
...
}


impl self::sp_api_hidden_includes_construct_runtime::hidden_include::traits::VariantCount
    for RuntimeHoldReason
{
    const VARIANT_COUNT: u32 = 0
        + module_single_instance::HoldReason::VARIANT_COUNT
        + module_multi_instance::HoldReason::<module_multi_instance::Instance1>::VARIANT_COUNT
        + module_multi_instance::HoldReason::<module_multi_instance::Instance2>::VARIANT_COUNT
        + module_multi_instance::HoldReason::<module_multi_instance::Instance3>::VARIANT_COUNT;
}
```

In addition, `MaxHolds` is removed (as suggested
[here](#2657 (comment)))
from `pallet_balances`, and its `Holds` are now bounded to
`RuntimeHoldReason::VARIANT_COUNT`. Therefore, there is no need to let
the runtime specify `MaxHolds`.


## For reviewers

Relevant changes can be found here:
- `substrate/frame/support/procedural/src/lib.rs` 
-  `substrate/frame/support/procedural/src/pallet/parse/composite.rs`
-  `substrate/frame/support/procedural/src/pallet/expand/composite.rs`
-
`substrate/frame/support/procedural/src/construct_runtime/expand/composite_helper.rs`
-
`substrate/frame/support/procedural/src/construct_runtime/expand/hold_reason.rs`
-
`substrate/frame/support/procedural/src/construct_runtime/expand/freeze_reason.rs`
- `substrate/frame/support/src/traits/misc.rs`

And the rest of the files is just about removed `MaxHolds` from
`pallet_balances`

## Next steps

Do the same for `MaxFreezes`
#2997.

---------

Co-authored-by: command-bot <>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Dónal Murray <donal.murray@parity.io>
Co-authored-by: gupnik <nikhilgupta.iitk@gmail.com>
This was referenced Mar 11, 2024
Open
Closed
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this issue Mar 25, 2024
@bkontur @bkchr
@seadanda @codekitz
[frame] #[pallet::composite_enum] improved variant count handling +…
5a7532a 
… removed `pallet_balances`'s `MaxHolds` config (paritytech#2657)

I started this investigation/issue based on @liamaharon question
[here](paritytech#1801 (comment)).

## Problem

The `pallet_balances` integrity test should correctly detect that the
runtime has correct distinct `HoldReasons` variant count. I assume the
same situation exists for RuntimeFreezeReason.

It is not a critical problem, if we set `MaxHolds` with a sufficiently
large value, everything should be ok. However, in this case, the
integrity_test check becomes less useful.

**Situation for "any" runtime:**
- `HoldReason` enums from different pallets:
```rust
        /// from pallet_nis
        #[pallet::composite_enum]
	pub enum HoldReason {
		NftReceipt,
	}

        /// from pallet_preimage
        #[pallet::composite_enum]
	pub enum HoldReason {
		Preimage,
	}

        // from pallet_state-trie-migration
        #[pallet::composite_enum]
	pub enum HoldReason {
		SlashForContinueMigrate,
		SlashForMigrateCustomTop,
		SlashForMigrateCustomChild,
	}
```

- generated `RuntimeHoldReason` enum looks like:
```rust
pub enum RuntimeHoldReason {

    #[codec(index = 32u8)]
    Preimage(pallet_preimage::HoldReason),

    #[codec(index = 38u8)]
    Nis(pallet_nis::HoldReason),

    #[codec(index = 42u8)]
    StateTrieMigration(pallet_state_trie_migration::HoldReason),
}
```

- composite enum `RuntimeHoldReason` variant count is detected as `3`
- we set `type MaxHolds = ConstU32<3>`
- `pallet_balances::integrity_test` is ok with `3`(at least 3)

However, the real problem can occur in a live runtime where some
functionality might stop working. This is due to a total of 5 distinct
hold reasons (for pallets with multi-instance support, it is even more),
and not all of them can be used because of an incorrect `MaxHolds`,
which is deemed acceptable according to the `integrity_test`:
  ```
  // pseudo-code - if we try to call all of these:

T::Currency::hold(&pallet_nis::HoldReason::NftReceipt.into(),
&nft_owner, deposit)?;
T::Currency::hold(&pallet_preimage::HoldReason::Preimage.into(),
&nft_owner, deposit)?;

T::Currency::hold(&pallet_state_trie_migration::HoldReason::SlashForContinueMigrate.into(),
&nft_owner, deposit)?;

  // With `type MaxHolds = ConstU32<3>` these two will fail

T::Currency::hold(&pallet_state_trie_migration::HoldReason::SlashForMigrateCustomTop.into(),
&nft_owner, deposit)?;

T::Currency::hold(&pallet_state_trie_migration::HoldReason::SlashForMigrateCustomChild.into(),
&nft_owner, deposit)?;
  ```  


## Solutions

A macro `#[pallet::*]` expansion is extended of `VariantCount`
implementation for the `#[pallet::composite_enum]` enum type. This
expansion generates the `VariantCount` implementation for pallets'
`HoldReason`, `FreezeReason`, `LockId`, and `SlashReason`. Enum variants
must be plain enum values without fields to ensure a deterministic
count.

The composite runtime enum, `RuntimeHoldReason` and
`RuntimeFreezeReason`, now sets `VariantCount::VARIANT_COUNT` as the sum
of pallets' enum `VariantCount::VARIANT_COUNT`:
```rust
#[frame_support::pallet(dev_mode)]
mod module_single_instance {

	#[pallet::composite_enum]
	pub enum HoldReason {
		ModuleSingleInstanceReason1,
		ModuleSingleInstanceReason2,
	}
...
}

#[frame_support::pallet(dev_mode)]
mod module_multi_instance {

	#[pallet::composite_enum]
	pub enum HoldReason<I: 'static = ()> {
		ModuleMultiInstanceReason1,
		ModuleMultiInstanceReason2,
		ModuleMultiInstanceReason3,
	}
...
}


impl self::sp_api_hidden_includes_construct_runtime::hidden_include::traits::VariantCount
    for RuntimeHoldReason
{
    const VARIANT_COUNT: u32 = 0
        + module_single_instance::HoldReason::VARIANT_COUNT
        + module_multi_instance::HoldReason::<module_multi_instance::Instance1>::VARIANT_COUNT
        + module_multi_instance::HoldReason::<module_multi_instance::Instance2>::VARIANT_COUNT
        + module_multi_instance::HoldReason::<module_multi_instance::Instance3>::VARIANT_COUNT;
}
```

In addition, `MaxHolds` is removed (as suggested
[here](paritytech#2657 (comment)))
from `pallet_balances`, and its `Holds` are now bounded to
`RuntimeHoldReason::VARIANT_COUNT`. Therefore, there is no need to let
the runtime specify `MaxHolds`.


## For reviewers

Relevant changes can be found here:
- `substrate/frame/support/procedural/src/lib.rs` 
-  `substrate/frame/support/procedural/src/pallet/parse/composite.rs`
-  `substrate/frame/support/procedural/src/pallet/expand/composite.rs`
-
`substrate/frame/support/procedural/src/construct_runtime/expand/composite_helper.rs`
-
`substrate/frame/support/procedural/src/construct_runtime/expand/hold_reason.rs`
-
`substrate/frame/support/procedural/src/construct_runtime/expand/freeze_reason.rs`
- `substrate/frame/support/src/traits/misc.rs`

And the rest of the files is just about removed `MaxHolds` from
`pallet_balances`

## Next steps

Do the same for `MaxFreezes`
paritytech#2997.

---------

Co-authored-by: command-bot <>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Dónal Murray <donal.murray@parity.io>
Co-authored-by: gupnik <nikhilgupta.iitk@gmail.com>
@bkontur bkontur mentioned this issue May 17, 2024
Merged
@bkontur bkontur mentioned this issue May 28, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Jun 13, 2024
@bkontur
Use aggregated types for RuntimeFreezeReason and better examples of…
988103d 
… `MaxFreezes` (#4615)

This PR aligns the settings for `MaxFreezes`, `RuntimeFreezeReason`, and
`FreezeIdentifier`.

#### Future work and improvements
#2997 (remove
`MaxFreezes` and `FreezeIdentifier`)
Ank4n pushed a commit that referenced this issue Jun 14, 2024
@bkontur @Ank4n
Use aggregated types for RuntimeFreezeReason and better examples of…
6c113c8 
… `MaxFreezes` (#4615)

This PR aligns the settings for `MaxFreezes`, `RuntimeFreezeReason`, and
`FreezeIdentifier`.

#### Future work and improvements
#2997 (remove
`MaxFreezes` and `FreezeIdentifier`)
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this issue Aug 2, 2024
@bkontur @TarekkMA
Use aggregated types for RuntimeFreezeReason and better examples of…
807322b 
… `MaxFreezes` (paritytech#4615)

This PR aligns the settings for `MaxFreezes`, `RuntimeFreezeReason`, and
`FreezeIdentifier`.

#### Future work and improvements
paritytech#2997 (remove
`MaxFreezes` and `FreezeIdentifier`)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bkontur bkontur

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@bkontur