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

[After Transaction Extension PR] CheckMetadataHash transaction extension benchmark. #5277

Draft
wants to merge 13 commits into
base: george/restore-gav-tx-ext
Choose a base branch
from

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Aug 8, 2024

After #3685

The transaction extensions cost is:

  • 3 match. each match is a 2 variant match.
  • 1 call to array_bytes::hex2array_unchecked to convert a hexadecimal string into a 32 bytes array.
  • (a debug log which converts a 32 bytes array to hex string, but I think it shouldn't be part of benchmark no?

So I feel introducing a new pallet to have it benchmarked is overkill.

In this PR:

  • I removed the array_bytes::hex2array_unchecked call and changed to a const implementation.
  • I set the weight of the transaction extension to 0: it is basically 3 boolean condition.

Alternative 1:

  • put this transaction extension into frame-system and have its benchmark part of system extension benchmarks.

Alternative 2:

  • do the new pallet and benchmark stuff.

cc @georgepisaltu

Copy link
Contributor

@georgepisaltu georgepisaltu left a comment

Choose a reason for hiding this comment

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

I agree that this extension doesn't need the benchmarks and can remain weightless. I'm not completely sure whether this extra code in utils is worth the effort for the benefit of doing that check at compile time though.

substrate/frame/metadata-hash-extension/src/utils.rs Outdated Show resolved Hide resolved
substrate/frame/metadata-hash-extension/src/utils.rs Outdated Show resolved Hide resolved
Comment on lines 35 to 38
let high = from_hex_digit_panic(hex_str.as_bytes()[start + i * 2]);
let low = from_hex_digit_panic(hex_str.as_bytes()[start + i * 2 + 1]);

let (Some(high), Some(low)) = (high, low) else { return None };
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
let high = from_hex_digit_panic(hex_str.as_bytes()[start + i * 2]);
let low = from_hex_digit_panic(hex_str.as_bytes()[start + i * 2 + 1]);
let (Some(high), Some(low)) = (high, low) else { return None };
let high = from_hex_digit_panic(hex_str.as_bytes()[start + i * 2])?;
let low = from_hex_digit_panic(hex_str.as_bytes()[start + i * 2 + 1])?;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

? not allowed in const fn

substrate/frame/metadata-hash-extension/src/utils.rs Outdated Show resolved Hide resolved
substrate/frame/metadata-hash-extension/src/utils.rs Outdated Show resolved Hide resolved

/// Function to convert hex string to Option<[u8; 32]> in a const context.
/// Returns `None` if fails to decode.
pub const fn hex_str_to_32_bytes(hex_str: &str) -> Option<[u8; HASH_LEN]> {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please upstream 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.

I discovered const-hex, it seems popular. I will switch to it.

https://crates.io/crates/const-hex

gui1117 and others added 2 commits August 15, 2024 20:45
Cargo.toml Outdated
@@ -668,6 +668,7 @@ codec = { version = "3.6.12", default-features = false, package = "parity-scale-
collectives-westend-emulated-chain = { path = "cumulus/parachains/integration-tests/emulated/chains/parachains/collectives/collectives-westend" }
collectives-westend-runtime = { path = "cumulus/parachains/runtimes/collectives/collectives-westend" }
color-eyre = { version = "0.6.1", default-features = false }
const-hex = { version = "1.10.0", default-features = false }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a new dependency because some of our dependency already depend on it.

I picked up the version already used by some dependencies in the Cargo.lock

const RUNTIME_METADATA: Option<[u8; 32]> = if let Some(hex) = option_env!("RUNTIME_METADATA_HASH") {
match const_hex::const_decode_to_array(hex.as_bytes()) {
Ok(hex) => Some(hex),
Err(_) => None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot find a way to print some error in const fn, but this behavior is unchanged.

Copy link
Member

Choose a reason for hiding this comment

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

You can panic in const? That should be good enough 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.

Yes, done in 8ffd8a9

Note that I can't put the exact error in the panic because it only accepts a string literal.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable-int
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7100379

@gui1117 gui1117 changed the title [WIP] CheckMetadataHash transaction extension benchmark. [After Transaction Extension PR] CheckMetadataHash transaction extension benchmark. Oct 2, 2024
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.

4 participants