feat: benchmarks #585

Ad96el · 2023-11-22T09:06:33Z

fixes KILTProtocol/ticket#2551

This pull request introduces benchmark logic for the DIP templates. To calculate default weights, a dummy implementation of the pallets in the Peregrine runtime is defined. After the PR is approved, I will remove the dummy implementation.

Leftover tasks

Make the provider and consumer binaries compiles for benchmarking. Currently the VersionedSiblingParachainDipStateProof type does not implement the WorstCase trait, which is a blocker to use it in benchmarks
Adjust the DipProofVerifier logic to allow any calls to be dispatched when being benchmarked
Make sure everything compiles
Make sure benchmarks can actually be run for both provider and consumer runtimes

pallets/pallet-dip-provider/src/benchmarking.rs

pallets/pallet-deposit-storage/Cargo.toml

Ad96el

LGTM. I have one concern, which may not be related to this PR. I believe the benchmarks might not currently work for the template. The dip-consumer template is currently using KiltVersionedSiblingProviderVerifier as ProofVerifier. The derive_verification_key_relationship function would return None for the call defined in the benchmark (frame_support::call::remark). As a result, the check_call_origin_info returns an error.

I would argue that the template should demonstrate how to benchmark the consumer pallet. In the pallet-dip-consumer pallet, the call is currently excluded based on feature guidance. So maybe adding a feature guided match arm in derive_verification_key_relationship is for the moment a good solution? If it is not important, ignore this comment.

Ad96el · 2023-12-04T11:11:08Z

dip-template/runtimes/dip-provider/src/lib.rs

+#[cfg(feature = "runtime-benchmarks")]
+mod benches {
+	frame_benchmarking::define_benchmarks!(
+		[frame_system, SystemBench::<Runtime>]
+		[cumulus_pallet_parachain_system, ParachainSystem]
+		[pallet_timestamp, Timestamp]
+		[pallet_sudo, Sudo]
+		[pallet_utility, Utility]
+		[pallet_balances, Balances]
+		[pallet_transaction_payment, TransactionPayment]
+		[pallet_authorship, Authorship]
+		[pallet_collator_selection, CollatorSelection]
+		[pallet_session, Session]
+		[pallet_aura, Aura]
+		[cumulus_pallet_aura_ext, AuraExt]
+		[did, Did]
+		[pallet_did_lookup, DidLookup]
+		[pallet_web3_names, Web3Names]
+		[pallet_deposit_storage, DepositStorage]
+		[pallet_dip_provider, DipProvider]
+		[frame_benchmarking::baseline, Baseline::<Runtime>]
+	);
+}
+


Perhaps I made a mistake. I compiled the branch using: cargo build --features runtime-benchmarks.

When I run: ./target/debug/dip-provider-node-template -h, the benchmark command is not appearing.

Mhh, I might have missed some dependency when the benchmark feature is enabled. I will investigate that

ntn-x2 · 2023-12-04T13:26:02Z

I would argue that the template should demonstrate how to benchmark the consumer pallet. In the pallet-dip-consumer pallet, the call is currently excluded based on feature guidance. So maybe adding a feature guided match arm in derive_verification_key_relationship is for the moment a good solution? If it is not important, ignore this comment.

Yes, you are correct. I feel this PR needs some further polishing. The feature in the pallet is for now good enough, since the call never needs to be included in the computed weight. As for the benchmarks, I will probably add a feature flag that returns true if the verifier is being called upon benchmarks. We would have caught this issue if the benchmarking command would actually work for the templates. But since that is not the case, the tests are not run for that. Fixing the benchmarking command first will highlight this issue, which can then be fixed accordingly.

Feature branch for everything DIP. It will collect other PRs until we are happy with the features, and will add the DIP to some of our runtimes and merge this into `develop`. ## WIP Checklist for the open tasks for v1 - [x] Basic structure -> #489 - [x] Merkleization of DID Documents -> #492 - [x] `RuntimeCall` verification logic -> #502 - [x] DID signature verification -> #516 - [x] Add support for linked accounts and web3name -> #525 - [x] Configurable origin for `commit_identity` -> #526 - [x] Proper fee management -> #528 - [x] Update to Polkadot 0.9.43 -> c18a6ce - [x] Replace XCM with state proofs -> #543 - [x] Add support for relaychain consumer -> #553 (part of #543) - [x] Proper error handling -> #572 - [x] Add support for versioning -> #573 - [x] Take deposits for identity commitments -> #574 - [x] Expose common definitions usable by consumers -> #577 - [x] Change ensure_signed! to configurable origin also for the `dispatch_as` function -> #577 - [x] Proper benchmarking and weights -> #585 - [x] Comments and docs -> #584 - [x] Revert Dockerfile changes in #587 - [x] [OPTIONAL] Add support for Zombienet -> #587 - [x] [OPTIONAL] Add chain spec loading from file for template runtimes -> #587 - [x] Big, final review -> #494 (review) - [x] Improvements n.1 PR -> #591 - [x] Improvements n.2 PR -> #592 - [x] Add to Peregrine runtime -> #594 - [ ] Deploy on Peregrine - [ ] Unit tests - [ ] Add to Spiritnet runtime - [ ] Deploy on Spiritnet - [ ] [OPTIONAL] Move DIP-related stuff into its own repo --------- Co-authored-by: Adel Golghalyani <48685760+Ad96el@users.noreply.github.com> Co-authored-by: Chris Chinchilla <chris@chrischinchilla.com> Co-authored-by: Albrecht <albrecht@kilt.io>

Feature branch for everything DIP. It will collect other PRs until we are happy with the features, and will add the DIP to some of our runtimes and merge this into `develop`. ## WIP Checklist for the open tasks for v1 - [x] Basic structure -> KILTprotocol/kilt-node#489 - [x] Merkleization of DID Documents -> KILTprotocol/kilt-node#492 - [x] `RuntimeCall` verification logic -> KILTprotocol/kilt-node#502 - [x] DID signature verification -> KILTprotocol/kilt-node#516 - [x] Add support for linked accounts and web3name -> KILTprotocol/kilt-node#525 - [x] Configurable origin for `commit_identity` -> KILTprotocol/kilt-node#526 - [x] Proper fee management -> KILTprotocol/kilt-node#528 - [x] Update to Polkadot 0.9.43 -> KILTprotocol/kilt-node@c18a6ce - [x] Replace XCM with state proofs -> KILTprotocol/kilt-node#543 - [x] Add support for relaychain consumer -> KILTprotocol/kilt-node#553 (part of KILTprotocol/kilt-node#543) - [x] Proper error handling -> KILTprotocol/kilt-node#572 - [x] Add support for versioning -> KILTprotocol/kilt-node#573 - [x] Take deposits for identity commitments -> KILTprotocol/kilt-node#574 - [x] Expose common definitions usable by consumers -> KILTprotocol/kilt-node#577 - [x] Change ensure_signed! to configurable origin also for the `dispatch_as` function -> KILTprotocol/kilt-node#577 - [x] Proper benchmarking and weights -> KILTprotocol/kilt-node#585 - [x] Comments and docs -> KILTprotocol/kilt-node#584 - [x] Revert Dockerfile changes in KILTprotocol/kilt-node#587 - [x] [OPTIONAL] Add support for Zombienet -> KILTprotocol/kilt-node#587 - [x] [OPTIONAL] Add chain spec loading from file for template runtimes -> KILTprotocol/kilt-node#587 - [x] Big, final review -> KILTprotocol/kilt-node#494 (review) - [x] Improvements n.1 PR -> KILTprotocol/kilt-node#591 - [x] Improvements n.2 PR -> KILTprotocol/kilt-node#592 - [x] Add to Peregrine runtime -> KILTprotocol/kilt-node#594 - [ ] Deploy on Peregrine - [ ] Unit tests - [ ] Add to Spiritnet runtime - [ ] Deploy on Spiritnet - [ ] [OPTIONAL] Move DIP-related stuff into its own repo --------- Co-authored-by: Adel Golghalyani <48685760+Ad96el@users.noreply.github.com> Co-authored-by: Chris Chinchilla <chris@chrischinchilla.com> Co-authored-by: Albrecht <albrecht@kilt.io>

Feature branch for everything DIP. It will collect other PRs until we are happy with the features, and will add the DIP to some of our runtimes and merge this into `develop`. ## WIP Checklist for the open tasks for v1 - [x] Basic structure -> #489 - [x] Merkleization of DID Documents -> #492 - [x] `RuntimeCall` verification logic -> #502 - [x] DID signature verification -> #516 - [x] Add support for linked accounts and web3name -> #525 - [x] Configurable origin for `commit_identity` -> #526 - [x] Proper fee management -> #528 - [x] Update to Polkadot 0.9.43 -> c18a6ce - [x] Replace XCM with state proofs -> #543 - [x] Add support for relaychain consumer -> #553 (part of #543) - [x] Proper error handling -> #572 - [x] Add support for versioning -> #573 - [x] Take deposits for identity commitments -> #574 - [x] Expose common definitions usable by consumers -> #577 - [x] Change ensure_signed! to configurable origin also for the `dispatch_as` function -> #577 - [x] Proper benchmarking and weights -> #585 - [x] Comments and docs -> #584 - [x] Revert Dockerfile changes in #587 - [x] [OPTIONAL] Add support for Zombienet -> #587 - [x] [OPTIONAL] Add chain spec loading from file for template runtimes -> #587 - [x] Big, final review -> #494 (review) - [x] Improvements n.1 PR -> #591 - [x] Improvements n.2 PR -> #592 - [x] Add to Peregrine runtime -> #594 - [ ] Deploy on Peregrine - [ ] Unit tests - [ ] Add to Spiritnet runtime - [ ] Deploy on Spiritnet - [ ] [OPTIONAL] Move DIP-related stuff into its own repo --------- Co-authored-by: Adel Golghalyani <48685760+Ad96el@users.noreply.github.com> Co-authored-by: Chris Chinchilla <chris@chrischinchilla.com> Co-authored-by: Albrecht <albrecht@kilt.io>

This reverts commit b4158f9.

* moved the `deposit_event(Event::AttestationRemoved)` to the `remove_attestation` so that the event shall never be forgotten to be deposited. * restructured Attestation event * added authorized_by to revoked and removed, added ctype * test attestation events * removed unused `DepositReclaimed` events * deposit `W3nReleased` event when governance bans an already used w3n <details> <summary>Peregrine Diff</summary> ``` !!! THE SUBWASM REDUCED DIFFER IS EXPERIMENTAL, DOUBLE CHECK THE RESULTS !!! [≠] pallet 62: Attestation -> 4 change(s) - events changes: [≠] 0: AttestationCreated ( : AttesterOf<T>, : ClaimHashOf<T>, : CtypeHashOf<T>, : Option<AuthorizationIdOf<T>>, ) ) [Signature(SignatureChange { args: [Changed(0, [Name(StringChange("", "attester"))]), Changed(1, [Name(StringChange("", "claim_hash"))]), Changed(2, [Name(StringChange("", "ctype_hash"))]), Changed(3, [Name(StringChange("", "authorization"))])] })] [≠] 1: AttestationRevoked ( : AttesterOf<T>, : ClaimHashOf<T>, ) ) [Signature(SignatureChange { args: [Changed(0, [Name(StringChange("", "authorized_by")), Ty(StringChange("AttesterOf<T>", "AuthorizedByOf<T>"))]), Changed(1, [Name(StringChange("", "attester")), Ty(StringChange("ClaimHashOf<T>", "AttesterOf<T>"))]), Added(2, ArgDesc { name: "claim_hash", ty: "ClaimHashOf<T>" })] })] [≠] 2: AttestationRemoved ( : AttesterOf<T>, : ClaimHashOf<T>, ) ) [Signature(SignatureChange { args: [Changed(0, [Name(StringChange("", "authorized_by")), Ty(StringChange("AttesterOf<T>", "AuthorizedByOf<T>"))]), Changed(1, [Name(StringChange("", "attester")), Ty(StringChange("ClaimHashOf<T>", "AttesterOf<T>"))]), Added(2, ArgDesc { name: "claim_hash", ty: "ClaimHashOf<T>" })] })] [-] "DepositReclaimed" [≠] pallet 63: Delegation -> 1 change(s) - events changes: [-] "DepositReclaimed" SUMMARY: - Compatible.......................: true - Require transaction_version bump.: false !!! THE SUBWASM REDUCED DIFFER IS EXPERIMENTAL, DOUBLE CHECK THE RESULTS !!! ``` </details> <details> <summary>Spiritnet Diff</summary> ``` !!! THE SUBWASM REDUCED DIFFER IS EXPERIMENTAL, DOUBLE CHECK THE RESULTS !!! [≠] pallet 62: Attestation -> 4 change(s) - events changes: [≠] 0: AttestationCreated ( : AttesterOf<T>, : ClaimHashOf<T>, : CtypeHashOf<T>, : Option<AuthorizationIdOf<T>>, ) ) [Signature(SignatureChange { args: [Changed(0, [Name(StringChange("", "attester"))]), Changed(1, [Name(StringChange("", "claim_hash"))]), Changed(2, [Name(StringChange("", "ctype_hash"))]), Changed(3, [Name(StringChange("", "authorization"))])] })] [≠] 1: AttestationRevoked ( : AttesterOf<T>, : ClaimHashOf<T>, ) ) [Signature(SignatureChange { args: [Changed(0, [Name(StringChange("", "authorized_by")), Ty(StringChange("AttesterOf<T>", "AuthorizedByOf<T>"))]), Changed(1, [Name(StringChange("", "attester")), Ty(StringChange("ClaimHashOf<T>", "AttesterOf<T>"))]), Added(2, ArgDesc { name: "claim_hash", ty: "ClaimHashOf<T>" })] })] [≠] 2: AttestationRemoved ( : AttesterOf<T>, : ClaimHashOf<T>, ) ) [Signature(SignatureChange { args: [Changed(0, [Name(StringChange("", "authorized_by")), Ty(StringChange("AttesterOf<T>", "AuthorizedByOf<T>"))]), Changed(1, [Name(StringChange("", "attester")), Ty(StringChange("ClaimHashOf<T>", "AttesterOf<T>"))]), Added(2, ArgDesc { name: "claim_hash", ty: "ClaimHashOf<T>" })] })] [-] "DepositReclaimed" [≠] pallet 63: Delegation -> 1 change(s) - events changes: [-] "DepositReclaimed" SUMMARY: - Compatible.......................: true - Require transaction_version bump.: false !!! THE SUBWASM REDUCED DIFFER IS EXPERIMENTAL, DOUBLE CHECK THE RESULTS !!! ``` </details> - [x] I have verified that the code works - [ ] No panics! (checked arithmetic ops, no indexing `array[3]` use `get(3)`, ...) - [x] I have verified that the code is easy to understand - [ ] If not, I have left a well-balanced amount of inline comments - [x] I have [left the code in a better state](https://deviq.com/principles/boy-scout-rule) - [x] I have documented the changes (where applicable) * Either PR or Ticket to update [the Docs](https://github.com/KILTprotocol/docs) * Link the PR/Ticket here

Ad96el and others added 12 commits November 14, 2023 10:44

change trait for retrieve

0c56e4c

identity provider

2df9776

alter traits

e1f186d

compiling

f20beba

Merge branch 'aa/dip' into ag-dip-benchmarks

bc2bdc2

runtime missing

fddc666

Clean up runtime-common Cargo.toml

681dc77

DIP provider pallet benchmarks WIP

279d241

intermediate state

7fe3593

compiling

2ccadf5

std error with features

70431ef

compiling

538a234

Ad96el changed the title ~~Ag dip benchmarks~~ feat: benchmarks Nov 22, 2023

Ad96el added 7 commits November 22, 2023 11:10

remove stuff

cacf04c

remove whitespaces

870f424

remove generic

22ccaad

feature error

c8aed6f

feature error

da6314d

with working features

5706638

linked accounts missing

14748ef

ntn-x2 mentioned this pull request Nov 23, 2023

feat: DIP v1 features #494

Merged

30 tasks

Ad96el added 5 commits November 23, 2023 13:30

finish worst case for LinkedDidInfoOf

0cba1d1

give money

90f5fe4

add default weights

cc00b0b

has to be reverted. Add provider and cosumer to peregrine

d276f0e

lockfile

489806f

Ad96el commented Nov 23, 2023

View reviewed changes

pallets/pallet-dip-provider/src/benchmarking.rs Outdated Show resolved Hide resolved

Ad96el added 3 commits November 23, 2023 17:21

adjust cargo toml

b493baf

compiling

8b97889

comment

048b72d

ntn-x2 added 2 commits December 1, 2023 17:21

Final changes

459ab82

Remove some TODOs and FIXMEs

69ad8e4

ntn-x2 force-pushed the ag-dip-benchmarks branch from d0e5609 to 69ad8e4 Compare December 1, 2023 16:37

Ad96el commented Dec 4, 2023

View reviewed changes

pallets/pallet-deposit-storage/Cargo.toml Outdated Show resolved Hide resolved

Ad96el commented Dec 4, 2023

View reviewed changes

ntn-x2 added 7 commits December 4, 2023 17:02

Fix features in node binaries

e64d943

Fix features

82ac447

Compiling

b7add42

Benchmarks working

8a44925

Compiling

1ccc2fe

Weights

20b0d16

Last fixes

3c5e579

ntn-x2 merged commit 5afb750 into aa/dip Dec 6, 2023
5 checks passed

ntn-x2 deleted the ag-dip-benchmarks branch December 6, 2023 10:38

ntn-x2 assigned ntn-x2 and Ad96el Dec 6, 2023

Ad96el added a commit that referenced this pull request Aug 21, 2024

Revert "fix: check events (#585)"

32a8c5a

This reverts commit b4158f9.

Ad96el added a commit that referenced this pull request Aug 29, 2024

Revert "fix: check events (#585)"

602f2c5

This reverts commit b4158f9.

Ad96el added a commit that referenced this pull request Aug 30, 2024

Revert "fix: check events (#585)"

c9da1a7

This reverts commit b4158f9.

Ad96el added a commit that referenced this pull request Aug 30, 2024

Revert "fix: check events (#585)"

62e26c5

This reverts commit b4158f9.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: benchmarks #585

feat: benchmarks #585

Ad96el commented Nov 22, 2023 •

edited by ntn-x2

Loading

Ad96el left a comment •

edited

Loading

Ad96el Dec 4, 2023

ntn-x2 Dec 4, 2023

ntn-x2 commented Dec 4, 2023

feat: benchmarks #585

feat: benchmarks #585

Conversation

Ad96el commented Nov 22, 2023 • edited by ntn-x2 Loading

fixes KILTProtocol/ticket#2551

Leftover tasks

Ad96el left a comment • edited Loading

Choose a reason for hiding this comment

Ad96el Dec 4, 2023

Choose a reason for hiding this comment

ntn-x2 Dec 4, 2023

Choose a reason for hiding this comment

ntn-x2 commented Dec 4, 2023

Ad96el commented Nov 22, 2023 •

edited by ntn-x2

Loading

Ad96el left a comment •

edited

Loading