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.

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

[frame] #[pallet::composite_enum] improved variant count handling + removed pallet_balances's MaxHolds config #2657

Merged
merged 63 commits into from
Jan 31, 2024

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Dec 7, 2023

I started this investigation/issue based on @liamaharon question here.

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:
        /// 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:
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:

#[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) 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.

@bkontur
Copy link
Contributor Author

bkontur commented Dec 7, 2023

I think it went unnoticed before because the pallet_balances mock runtime has a TestId enum with three values and type MaxHolds = ConstU32<3>;. The Rococo runtime, for example, has just two pallets with HoldReason, and while each HoldReason has only one value, type MaxHolds = ConstU32<2>; is ok and sufficient.

The issue arises when a HoldReason has more than one value.

@bkchr
Copy link
Member

bkchr commented Dec 22, 2023

@bkontur any problem with this pr? Why not finish it?

@bkontur bkontur force-pushed the bko-max-holds-integrity-test branch from a8745ef to a2b272a Compare January 5, 2024 14:32
@bkontur bkontur changed the base branch from bko-pallet-nicks-refactor to master January 5, 2024 14:35
@bkontur
Copy link
Contributor Author

bkontur commented Jan 5, 2024

bot fmt

@bkontur bkontur added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Jan 5, 2024
@bkontur bkontur changed the title Test for MaxHolds vs integrity_test [frame] #[pallet::composite_enum] improved variant count handling Jan 5, 2024
@bkontur bkontur marked this pull request as ready for review January 5, 2024 14:45
@bkontur bkontur requested review from a team January 5, 2024 14:45
@@ -37,8 +37,6 @@ pub const DEFENSIVE_OP_PUBLIC_ERROR: &str = "a defensive failure has been trigge
pub const DEFENSIVE_OP_INTERNAL_ERROR: &str = "Defensive failure has been triggered!";

/// Trait to get the number of variants in any enum.
///
/// NOTE: can be removed once <https://doc.rust-lang.org/std/mem/fn.variant_count.html> is stable.
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, I think because that nightly feature won't for our result runtime enum RuntimeHoldReason / RuntimeFreezeReason, e.g.: check this gist: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=57f1091d103a488db9290f637d292e20 .

We need real composite count (sum of inner enum variants), e.g. if we want to remove MaxHolds / MaxFreezes as discussed bellow.

I was even considering renaming the trait VariantCount to `trait CompositeVariantCount."

Or maybe even better, leave VariantCount with that NOTE and use it only for inner enums with #[pallet::composite_enum], so when nighly becomes stable we could remove VariantCount,
and create trait CompositeVariantCount dedicated just to the result runtime enum like RuntimeHoldReason / RuntimeFreezeReason.

So at the end, we will have just CompositeVariantCount.

@bkchr wdyt?

substrate/frame/support/procedural/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/support/procedural/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/support/procedural/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/support/procedural/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/support/procedural/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Bastian Köcher <git@kchr.de>
@paritytech-review-bot paritytech-review-bot bot requested a review from a team January 8, 2024 08:38
bkontur and others added 7 commits January 8, 2024 09:44
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Bastian Köcher <git@kchr.de>
…d/hold_reason.rs

Co-authored-by: Bastian Köcher <git@kchr.de>
…d/hold_reason.rs

Co-authored-by: Bastian Köcher <git@kchr.de>
@command-bot
Copy link

command-bot bot commented Jan 30, 2024

@bkontur https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5081585 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --pallet=pallet_balances. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 2-09a0cd55-aa42-49c9-8400-60563e8ebf4a to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jan 31, 2024

@bkontur Command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --pallet=pallet_balances has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5081585 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5081585/artifacts/download.

@bkontur bkontur added this pull request to the merge queue Jan 31, 2024
Comment on lines +89 to +96
// ```nocompile
// pub struct Pallet<T, I = ()>{..}
//
// #[pallet::composite_enum]
// pub enum HoldReason<I: 'static = ()> {..}
//
// Pallet1: pallet_x, // <- default type parameter
// ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Is nocompile needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liamaharon The auto-merge happened faster than I could get to my PC. Thank you, I've created another PR to address your comments: #3150

@@ -1519,13 +1513,14 @@ pub fn origin(_: TokenStream, _: TokenStream) -> TokenStream {
///
/// ```ignore
/// Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Encode, Decode, MaxEncodedLen, TypeInfo,
/// RuntimeDebug
/// RuntimeDebug,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// RuntimeDebug,
/// RuntimeDebug

use crate::pallet::Def;
use proc_macro2::TokenStream;

/// Expands `composite_enum` and adds the `VariantCount` implementation for it."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Expands `composite_enum` and adds the `VariantCount` implementation for it."
/// Expands `composite_enum` and adds the `VariantCount` implementation for it.

Comment on lines +96 to +97
pub ident: syn::Ident,
/// Type parameters and where clause attached to a declaration of the pallet::composite_enum.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub ident: syn::Ident,
/// Type parameters and where clause attached to a declaration of the pallet::composite_enum.
pub ident: syn::Ident,
/// Type parameters and where clause attached to a declaration of the pallet::composite_enum.

Merged via the queue into master with commit bb8ddc4 Jan 31, 2024
124 checks passed
@bkontur bkontur deleted the bko-max-holds-integrity-test branch January 31, 2024 06:58
bkontur added a commit that referenced this pull request Jan 31, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jan 31, 2024
bkontur added a commit to bkontur/runtimes that referenced this pull request Feb 16, 2024
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
… 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>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
AurevoirXavier added a commit to darwinia-network/darwinia that referenced this pull request Jun 24, 2024
AurevoirXavier added a commit to darwinia-network/darwinia that referenced this pull request Jun 28, 2024
* Setup deps

* Remove Koi from account migration test

* paritytech/polkadot-sdk#1495

* Bump

* paritytech/polkadot-sdk#1524

* !! paritytech/polkadot-sdk#1363

* paritytech/polkadot-sdk#1492

* paritytech/polkadot-sdk#1911

* paritytech/polkadot-sdk#1900

Signed-off-by: Xavier Lau <xavier@inv.cafe>

* paritytech/polkadot-sdk#1661

* paritytech/polkadot-sdk#2144

* paritytech/polkadot-sdk#2048

* paritytech/polkadot-sdk#1672

* paritytech/polkadot-sdk#2303

* paritytech/polkadot-sdk#1256

* Remove identity and vesting

* Fixes

* paritytech/polkadot-sdk#2657

* paritytech/polkadot-sdk#1313

* paritytech/polkadot-sdk#2331

* paritytech/polkadot-sdk#2409 part.1

* paritytech/polkadot-sdk#2767

* paritytech/polkadot-sdk#2521

Signed-off-by: Xavier Lau <xavier@inv.cafe>

* paritytech/polkadot-sdk#1222

* paritytech/polkadot-sdk#1234 part.1

* Satisfy compiler

* XCM V4 part.1

* paritytech/polkadot-sdk#1246

* Remove pallet-democracy part.1

* paritytech/polkadot-sdk#2142

* paritytech/polkadot-sdk#2428

* paritytech/polkadot-sdk#3228

* XCM V4 part.2

* Bump

* Build all runtimes

* Build node

* Remove pallet-democracy

Signed-off-by: Xavier Lau <xavier@inv.cafe>

* Format

* Fix pallet tests

* Fix precompile tests

* Format

* Fixes

* Async, remove council, common pallet config

* Fix `ethtx-forward` test case (#1519)

* Fix ethtx-forward tests

* Format

* Fix following the review

* Fixes

* Fixes

* Use default impl

* Benchmark helper

* Bench part.1

* Bench part.2

* Bench part.3

* Fix all tests

* Typo

* Feat

* Fix EVM tracing build

* Reuse upstream `proof_size_base_cost()` (#1521)

* Format issue

* Fixes

* Fix CI

---------

Signed-off-by: Xavier Lau <xavier@inv.cafe>
Co-authored-by: Bear Wang <boundless.forest@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants