chore: improve benchmark logic #612
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntn-x2
commented
Mar 6, 2024
| proof: Self::Proof, | ||
| ) -> Result<Self::VerificationResult, Self::Error> { | ||
| <ProviderTemplateProofVerifier as IdentityProofVerifier<Runtime>>::verify_proof_for_call_against_details( | ||
| call, |
Contributor
Author
There was a problem hiding this comment.
Reviewers can skip this altogether, as it's been generated in a test setup and copy-pasted here. The unit test below is more meaningful to review.
ntn-x2
commented
Mar 6, 2024
| T::AccountId: Instanciate, | ||
| T::Identifier: Instanciate, | ||
| <<T as Config>::IdentityProvider as IdentityProvider<T>>::Success: GetWorstCase<IdentityContext<T::Identifier, T::AccountId>> | ||
| <<T as Config>::IdentityProvider as IdentityProvider<T>>::Success: GetWorstCase<IdentityContext<T::Identifier, T::AccountId>, Output = <<T as Config>::IdentityProvider as IdentityProvider<T>>::Success> |
Contributor
Author
There was a problem hiding this comment.
This is a quick fix to ensure nothing breaks with the new definition of the GetWorstCase trait. The logic can be refined in future PRs.
This was referenced Mar 6, 2024
ntn-x2
force-pushed
the
aa/dip-improved-benchmarks
branch
from
f49354e to
bfc7ae5
Compare
Ad96el
approved these changes
Mar 28, 2024
Contributor
Ad96el
left a comment
Ad96el
left a comment
There was a problem hiding this comment.
LGTM. Elegant solution. 3 Minor comments.
| # External dependencies | ||
| hash-db.workspace = true | ||
| log.workspace = true | ||
| cfg-if.workspace = true |
Contributor
There was a problem hiding this comment.
I guess we can also remove it from the root cargo toml
Contributor
Author
There was a problem hiding this comment.
No it's still used in the pallet-dip-consumer crate:
// TODO: Maybe find a nicer way to exclude the call dispatched from the
// benchmarks while making sure the call is actually dispatched and passes any
// filters the consumer proof verifier has set.
cfg_if::cfg_if! {
if #[cfg(not(feature = "runtime-benchmark"))] {
call.dispatch(did_origin.into())
} else {
().into()
}
}| const MAX_DID_MERKLE_PROOF_LEAVE_COUNT: u32 = 64, | ||
| const MAX_DID_MERKLE_PROOF_LEAVE_SIZE: u32 = 1024, | ||
| const MAX_DID_MERKLE_LEAVES_REVEALED: u32 = 64, | ||
| const MAX_PROVIDER_HEAD_PROOF_LEAVE_COUNT: u32 = DEFAULT_MAX_PROVIDER_HEAD_PROOF_LEAVE_COUNT, |
Contributor
There was a problem hiding this comment.
Is it intended to increase the MAX_PROVIDER_HEAD_PROOF_LEAVE_COUNT from 64 to 128? Same for the others.
Contributor
Author
There was a problem hiding this comment.
Yes I noticed every proof I generated was larger than 64, so I bumped it up to the next power of 2. These are anyway default values, can be overridden by specific consumers.
| ConsumerBlockNumber, | ||
| >, | ||
| ) -> Self { | ||
| Self::V0(value) |
Contributor
There was a problem hiding this comment.
What if a new version is introduced? Is there a way to distinguish them?
Contributor
Author
There was a problem hiding this comment.
If a new version is introduced, it will be because of a breaking change, so they will be different from each other. Otherwise if they cannot be distinguished, why would they be different in their versions?
Changing the storage hasher before we deploy on production, otherwise we would then need a migration strategy. I think we can disregard any migrations on our testnets, as the effort is not justified. I also lowered the limits of the DIP provider template, which were higher than Peregrine, so that it is faster to generate the benchmark worst case. Related to #601, based on top of #612.
ntn-x2
added a commit
that referenced
this pull request
Mar 28, 2024
Partially fixes https://github.com/KILTprotocol/ticket/issues/2562. Adds unit tests for the verifier components (components tested are shown in the checklist below). It also splits up the proof verification logic into multiple files, split by when those checks are performed. Check the `crates/kilt-dip-primitives/src/merkle/v0/` folder for more details. It addresses an open comment in the DIP refactoring PR: #602 (comment). ## Elements to add tests for - [x] `pallet-dip-consumer` - [x] `pallet-relay-store` - [x] `ProofVerifier` - [x] Merge #612 - [x] Merge #613
Ad96el
pushed a commit
that referenced
this pull request
Apr 2, 2024
Partially fixes https://github.com/KILTprotocol/ticket/issues/2562. Adds unit tests for the verifier components (components tested are shown in the checklist below). It also splits up the proof verification logic into multiple files, split by when those checks are performed. Check the `crates/kilt-dip-primitives/src/merkle/v0/` folder for more details. It addresses an open comment in the DIP refactoring PR: #602 (comment). ## Elements to add tests for - [x] `pallet-dip-consumer` - [x] `pallet-relay-store` - [x] `ProofVerifier` - [x] Merge #612 - [x] Merge #613
ntn-x2
added a commit
that referenced
this pull request
Apr 4, 2024
Partially fixes https://github.com/KILTprotocol/ticket/issues/2562. Adds unit tests for the verifier components (components tested are shown in the checklist below). It also splits up the proof verification logic into multiple files, split by when those checks are performed. Check the `crates/kilt-dip-primitives/src/merkle/v0/` folder for more details. It addresses an open comment in the DIP refactoring PR: #602 (comment). ## Elements to add tests for - [x] `pallet-dip-consumer` - [x] `pallet-relay-store` - [x] `ProofVerifier` - [x] Merge #612 - [x] Merge #613
hacker-nova7431
added a commit
to hacker-nova7431/kilt-node
that referenced
this pull request
Sep 29, 2025
Partially fixes https://github.com/KILTprotocol/ticket/issues/2562. Adds unit tests for the verifier components (components tested are shown in the checklist below). It also splits up the proof verification logic into multiple files, split by when those checks are performed. Check the `crates/kilt-dip-primitives/src/merkle/v0/` folder for more details. It addresses an open comment in the DIP refactoring PR: KILTprotocol/kilt-node#602 (comment). ## Elements to add tests for - [x] `pallet-dip-consumer` - [x] `pallet-relay-store` - [x] `ProofVerifier` - [x] Merge KILTprotocol/kilt-node#612 - [x] Merge KILTprotocol/kilt-node#613
infra-riderju74
added a commit
to infra-riderju74/kilt-node
that referenced
this pull request
Oct 27, 2025
Partially fixes https://github.com/KILTprotocol/ticket/issues/2562. Adds unit tests for the verifier components (components tested are shown in the checklist below). It also splits up the proof verification logic into multiple files, split by when those checks are performed. Check the `crates/kilt-dip-primitives/src/merkle/v0/` folder for more details. It addresses an open comment in the DIP refactoring PR: KILTprotocol/kilt-node#602 (comment). ## Elements to add tests for - [x] `pallet-dip-consumer` - [x] `pallet-relay-store` - [x] `ProofVerifier` - [x] Merge KILTprotocol/kilt-node#612 - [x] Merge KILTprotocol/kilt-node#613
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes https://github.com/KILTprotocol/ticket/issues/3104, based on top of #611.
It fixes the logic for the
dip-consumerpallet, by delegating the generation of a proof worst case to the proof verifier, which must make sure the proof is indeed the one that requires the most weight to verify, and that it verifies successfully. This also means that each consumer runtime is responsible to implement this method, as there cannot be a "universal" worst proof, as that depends on the use case. The pallet benchmarking logic is now generic enough to make this possible and flexible ✨✨✨