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

Introduce config param for state version instead using a StateVersion::V0 for extrinsic root derivation #1691

Conversation

vedhavyas
Copy link
Contributor

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context,
including:

  • What does this PR do?
    Adds Extrinsinc root state version as config parameter so that users can pick V0/V1 depending on the use case.
  • Why are these changes needed?
    Currently extrinsic root is derived using StateVersion::V0. In order to generate the extrinsic root, we would need to have access to full extrinsic data. We have a use case where, domains extrinsic root need to be derived on consensus runtime and passing all the extrinsic data through Fraud proof will exceed block limit. We are planning to use StateVersion::V1 so that we can just use extrinsic hashes if the extrinsic data is more than 32 bytes.
  • How were these changes implemented and what do they affect?
    Config parameter for ExtrinsicRootStateVersion can be passed through runtime and I have set it to StateVersion::V0 so this should not change the extrinsic root derivation for existing projects.

Checklist

  • My PR includes a detailed description as outlined in the "Description" section above
  • My PR follows the labeling requirements of this project (at minimum one label for T
    required)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@vedhavyas vedhavyas requested review from a team September 25, 2023 09:06
@vedhavyas vedhavyas force-pushed the default_state_version_for_extrinsics_root branch 2 times, most recently from 88a1810 to 4ef2460 Compare September 25, 2023 17:34
@vedhavyas vedhavyas requested a review from a team September 25, 2023 17:34
@vedhavyas vedhavyas force-pushed the default_state_version_for_extrinsics_root branch from 4ef2460 to d54ae59 Compare September 26, 2023 10:47
@vedhavyas vedhavyas force-pushed the default_state_version_for_extrinsics_root branch 3 times, most recently from dd2d9ef to 9f9902e Compare September 26, 2023 16:31
@vedhavyas
Copy link
Contributor Author

Hey folks,

I could use some help in fixing the failing test and I'm not sure if this is related to my changes. Not to say the cliched thing but the following cargo build --locked --profile testnet -p polkadot-test-malus --bin malus --bin polkadot-prepare-worker --bin polkadot-execute-worker succefully builds on my mac and my rust version is rustc 1.71.0-nightly same. as the one used in the gitlab ci.

Thanks!

@cheme
Copy link
Contributor

cheme commented Sep 27, 2023

Hey folks,

I could use some help in fixing the failing test and I'm not sure if this is related to my changes. Not to say the cliched thing but the following cargo build --locked --profile testnet -p polkadot-test-malus --bin malus --bin polkadot-prepare-worker --bin polkadot-execute-worker succefully builds on my mac and my rust version is rustc 1.71.0-nightly same. as the one used in the gitlab ci.

Thanks!

Will be reviewing the pr next week, but at first glance the CI error does not look related (also not a required one), probably no need to check before reviews (not a 'required' test).

@paritytech-ci paritytech-ci requested review from a team September 27, 2023 12:53
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Yeah. Generally I think this is fine. It will break everybodys runtime, but sadly we can not do better right know. However, I would re-export StarteVersion from frame_system to not require people to pull in sp_core.

@paritytech-ci paritytech-ci requested review from a team September 27, 2023 13:12
@vedhavyas vedhavyas force-pushed the default_state_version_for_extrinsics_root branch 2 times, most recently from 40a5130 to 14c894e Compare September 27, 2023 16:20
@@ -401,6 +415,9 @@ pub mod pallet {

/// The maximum number of consumers allowed on a single account.
type MaxConsumers: ConsumerLimits;

/// State verison used to derive extrinsics root.
type ExtrinsicsRootStateVersion: Get<sp_core::storage::StateVersion>;
Copy link
Contributor

Choose a reason for hiding this comment

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

No idea yet if doable or fine, but would it be suitable to put this state version as part of RuntimeVersion (type Version) as a new field or a specific api version (as a specific api version, could default to 0 when missing so there would be no need to change existing pallets).

@vedhavyas vedhavyas force-pushed the default_state_version_for_extrinsics_root branch from 14c894e to 725068c Compare October 11, 2023 11:17
@vedhavyas vedhavyas requested a review from bkchr October 11, 2023 12:05
@vedhavyas
Copy link
Contributor Author

Apologies for the delay. The PR should be updated per review suggestions @bkchr

@vedhavyas vedhavyas requested a review from cheme October 11, 2023 12:41
@vedhavyas
Copy link
Contributor Author

@bkchr Bump

@vedhavyas vedhavyas force-pushed the default_state_version_for_extrinsics_root branch from 8ba38f7 to 6365073 Compare October 18, 2023 06:38
@vedhavyas
Copy link
Contributor Author

vedhavyas commented Oct 20, 2023

No idea yet if doable or fine, but would it be suitable to put this state version as part of RuntimeVersion (type Version) as a new field or a specific api version (as a specific api version, could default to 0 when missing so there would be no need to change existing pallets).

@cheme you are correct, adding an extrinsic_state_version field to RuntimeVersion was indeed a good idea and feels more correct to me. I have opened another PR with this approach. I can close this PR if team agrees with the changing RuntimeVersion approach instead of adding a config parameter.

@bkchr bkchr closed this Mar 20, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Mar 26, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Mar 27, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
bkchr pushed a commit that referenced this pull request Apr 10, 2024
* Replace const parameters

* fmt

* missed out Maxlocks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants