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

Feature/plmc 412 on initialize benchmarks #159

Merged
merged 228 commits into from Feb 14, 2024

Conversation

JuaniRios
Copy link
Contributor

@JuaniRios JuaniRios commented Feb 5, 2024

What?

Add the benchmarks for all the functions called inside on_initialize. Make them as accurate as possible as in #148

Why?

on_initialize needs to know the exact weight consumed to let the rest of the extrinsics execute.
If it's inaccurate, we risk never producing blocks in the worst case, or consuming all the block space and having no space for extrinsics.

How?

Same as #148

Testing?

Same as #148

Screenshots (optional)

Anything Else?

  • We're currently discussing if it makes sense to remove the on_initialize in favor of extrinsics that any user can call, and add some incentive for them to do it.
  • If we don't do that, then we should make it possible to call the project transitions from root origin in case one of these functions fails.

automatic hrmp connection working

Genesis instantiator usage and first draft of HRMP connection

formatting

feature propagation

cleanup

new node functioning. genesis not yet sure if working

new node functioning. genesis not yet sure if working

somehow compiling and test passing

save

save

save

feat(287): para_id setting extrinsic implemented and tested

save

same log crate across workspace

same log crate across workspace

save

save

save

feat(287): changed tight couple of pallet_xcm by extracting sender trait

feat(287): first commit

feat(285): POC Hrmp automatic acceptance
# Conflicts:
#	Cargo.lock
#	Cargo.toml
#	integration-tests/src/tests/mod.rs
#	pallets/funding/Cargo.toml
#	pallets/funding/src/functions.rs
#	pallets/funding/src/instantiator.rs
#	pallets/funding/src/lib.rs
#	pallets/funding/src/mock.rs
#	pallets/funding/src/types.rs
#	pallets/xcm-executor/src/config.rs
#	pallets/xcm-executor/src/lib.rs
#	runtimes/standalone/src/lib.rs
#	runtimes/testnet/src/xcm_config.rs
…s-polimecs-balance-and-pallet

# Conflicts:
#	Cargo.toml
… new map of query id -> unconfirmed transactions
Signed-off-by: Juan Ignacio Rios <juani.rios.99@gmail.com>
Signed-off-by: Juan Ignacio Rios <juani.rios.99@gmail.com>
Signed-off-by: Juan Ignacio Rios <juani.rios.99@gmail.com>
Base automatically changed from feature/plmc-267-add-complexity-parameters-to-extrinsic-benchmarks to main February 9, 2024 15:25
@JuaniRios JuaniRios force-pushed the feature/plmc-412-on_initialize-benchmarks branch from 30bd050 to 37e7848 Compare February 9, 2024 15:45
# Conflicts:
#	Cargo.lock
#	pallets/funding/src/benchmarking.rs
#	pallets/funding/src/functions.rs
#	pallets/funding/src/instantiator.rs
#	pallets/funding/src/lib.rs
#	pallets/funding/src/mock.rs
#	pallets/funding/src/tests.rs
#	pallets/funding/src/weights.rs
#	runtimes/testnet/src/lib.rs
Signed-off-by: Juan Ignacio Rios <juani.rios.99@gmail.com>
# Conflicts:
#	pallets/funding/src/benchmarking.rs
#	pallets/funding/src/functions.rs
#	pallets/funding/src/tests.rs
#	runtimes/base/src/lib.rs
#	runtimes/testnet/src/xcm_config.rs
Signed-off-by: Juan Ignacio Rios <juani.rios.99@gmail.com>
Signed-off-by: Juan Ignacio Rios <juani.rios.99@gmail.com>
Signed-off-by: Juan Ignacio Rios <juani.rios.99@gmail.com>
Signed-off-by: Juan Ignacio Rios <juani.rios.99@gmail.com>
Copy link
Collaborator

@vstam1 vstam1 left a comment

Choose a reason for hiding this comment

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

Logic looks good to me with a couple of comments. One more general comment would be around all the unwraps in instantiator/benchmarks/tests. Might be good to handle them (where making sense) more gracefully. Had some tests failing in previous PR with big panic around unwrap on error. Better to handle it properly.

debug_weight_gen.rs Outdated Show resolved Hide resolved
pallets/funding/src/instantiator.rs Outdated Show resolved Hide resolved
pallets/funding/src/instantiator.rs Show resolved Hide resolved
pallets/funding/src/lib.rs Show resolved Hide resolved
pallets/funding/src/mock.rs Show resolved Hide resolved
pallets/funding/src/mock.rs Show resolved Hide resolved
pallets/funding/src/benchmarking.rs Show resolved Hide resolved
pallets/funding/src/benchmarking.rs Outdated Show resolved Hide resolved
pallets/funding/src/functions.rs Outdated Show resolved Hide resolved
pallets/funding/src/functions.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@lrazovic lrazovic left a comment

Choose a reason for hiding this comment

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

Great job, we're getting closer and closer to having accurate weights for every possible branch of the pallet!

In addition to Just's comments, I would like clarification on the use of unwrap_or_default in the hook. Once that's clear, we can merge.

pallets/funding/src/lib.rs Outdated Show resolved Hide resolved
Signed-off-by: Juan Ignacio Rios <juani.rios.99@gmail.com>
…ight functions

Signed-off-by: Juan Ignacio Rios <juani.rios.99@gmail.com>
@JuaniRios
Copy link
Contributor Author

Logic looks good to me with a couple of comments. One more general comment would be around all the unwraps in instantiator/benchmarks/tests. Might be good to handle them (where making sense) more gracefully. Had some tests failing in previous PR with big panic around unwrap on error. Better to handle it properly.

What would you suggest is a better way to handle it?
IMO we want the code to panic with the error if its compiled only when testing.
Otherwise chasing the error might be harder

@vstam1
Copy link
Collaborator

vstam1 commented Feb 14, 2024

Logic looks good to me with a couple of comments. One more general comment would be around all the unwraps in instantiator/benchmarks/tests. Might be good to handle them (where making sense) more gracefully. Had some tests failing in previous PR with big panic around unwrap on error. Better to handle it properly.

What would you suggest is a better way to handle it? IMO we want the code to panic with the error if its compiled only when testing. Otherwise chasing the error might be harder

Could do "expect("some useful error message")" instead of unwrap? Could also do assert!(result.is_ok()).

@JuaniRios
Copy link
Contributor Author

Logic looks good to me with a couple of comments. One more general comment would be around all the unwraps in instantiator/benchmarks/tests. Might be good to handle them (where making sense) more gracefully. Had some tests failing in previous PR with big panic around unwrap on error. Better to handle it properly.

What would you suggest is a better way to handle it? IMO we want the code to panic with the error if its compiled only when testing. Otherwise chasing the error might be harder

Could do "expect("some useful error message")" instead of unwrap? Could also do assert!(result.is_ok()).

assert_ok would do the same as unwrap, in that it will panic with the error type. We could do a new PR where we replace the unwraps with expects explaining the possible causes for that panic. But lets not do it here

@lrazovic lrazovic self-requested a review February 14, 2024 14:51
@JuaniRios JuaniRios merged commit 31e30f3 into main Feb 14, 2024
@JuaniRios JuaniRios deleted the feature/plmc-412-on_initialize-benchmarks branch February 14, 2024 14:58
@JuaniRios JuaniRios restored the feature/plmc-412-on_initialize-benchmarks branch February 14, 2024 16:49
@JuaniRios JuaniRios deleted the feature/plmc-412-on_initialize-benchmarks branch February 14, 2024 16:51
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.

None yet

3 participants