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

feat: compute pallet/storage prefix hash at compile time #1539

Merged
merged 26 commits into from
Oct 3, 2023

Conversation

yjhmelody
Copy link
Contributor

@yjhmelody yjhmelody commented Sep 13, 2023

Since the hash rules of this part of the pallet_prefix/storage_prefix are always fixed, we can put the runtime calculation into compile time.


polkadot address: 15ouFh2SHpGbHtDPsJ6cXQfes9Cx1gEFnJJsJVqPGzBSTudr

@yjhmelody yjhmelody requested review from a team September 13, 2023 12:51
@bkchr
Copy link
Member

bkchr commented Sep 13, 2023

Nice! This will close: #372

@paritytech-ci paritytech-ci requested a review from a team September 13, 2023 14:01
@paritytech-ci paritytech-ci requested a review from a team September 13, 2023 15:07
@juangirini
Copy link
Contributor

bot fmt

@command-bot
Copy link

command-bot bot commented Sep 13, 2023

@juangirini https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3677452 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". 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-22de9f65-b8e6-467f-95c7-78b7f8dcd262 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Sep 13, 2023

@juangirini Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3677452 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3677452/artifacts/download.

@yjhmelody
Copy link
Contributor Author

@bkchr hi ,it is done

@yjhmelody
Copy link
Contributor Author

Any other problem?

@yjhmelody
Copy link
Contributor Author

@juangirini @bkchr

@yjhmelody yjhmelody changed the title feat: compute pallet/storage prefix hash in compile time feat: compute pallet/storage prefix hash at compile time Sep 30, 2023
@yjhmelody
Copy link
Contributor Author

@bkchr

@bkchr bkchr added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Oct 1, 2023
@bkchr
Copy link
Member

bkchr commented Oct 1, 2023

@yjhmelody I'm fine issuing a medium tip for this pr when it is merged

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.

Looks good! Just some nitpicks!

Ty!

@paritytech-ci paritytech-ci requested a review from a team October 1, 2023 22:05
@kianenigma
Copy link
Contributor

/tip medium

@substrate-tip-bot
Copy link

@kianenigma A medium (80 DOT) tip was successfully submitted for @yjhmelody (15ouFh2SHpGbHtDPsJ6cXQfes9Cx1gEFnJJsJVqPGzBSTudr on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/referenda tip

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Pretty cool, thanks!

Are there some tests to check that the computed storage hashes are identical to the old ones?

<T as #frame_system::Config>::PalletInfo as #frame_support::traits::PalletInfo
>::name_hash::<Self>()
.expect("Pallet is part of the runtime because pallet `Config` trait is \
implemented by the runtime")
Copy link
Contributor

Choose a reason for hiding this comment

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

So, this might not be true under a misconfiguration, i.e. someone implements the pallet Config trait for a runtime, but forgets to include the actual pallet in construct_runtime.

Hopefully we do now have lints or warnings to catch this, but on the off-chance that we don't, I'd want to make sure that this expect doesn't trigger and cause a panic in runtime.

Copy link
Member

Choose a reason for hiding this comment

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

This assumption is already used there multiple times.


/// Storage prefix. Used for generating final key.
fn storage_prefix() -> &'static [u8];

/// The full prefix; just the hash of `module_prefix` concatenated to the hash of
/// The full prefix; just the hash of `pallet_prefix` concatenated to the hash of
/// `storage_prefix`.
Copy link
Contributor

@KiChjang KiChjang Oct 3, 2023

Choose a reason for hiding this comment

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

By removing the default implementation, we can no longer guarantee that the full prefix is "just the hash of pallet_prefix concatenated to the hash of storage_prefix".

Copy link
Contributor

Choose a reason for hiding this comment

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

Why was it removed in the first place? I see no explanation and commit message is unhelpful "minor improve" 😕

@bkchr bkchr merged commit aad80cc into paritytech:master Oct 3, 2023
14 of 22 checks passed
@shawntabrizi
Copy link
Member

are there any benchmarks for this and the improvements?

bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
…1539)

Since the hash rules of this part of the `pallet_prefix/storage_prefix`
are always fixed, we can put the runtime calculation into compile time.

---
polkadot address: 15ouFh2SHpGbHtDPsJ6cXQfes9Cx1gEFnJJsJVqPGzBSTudr

---------

Co-authored-by: Juan <[email protected]>
Co-authored-by: command-bot <>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
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.

8 participants