feat: delegation pallet storage refactor by ntn-x2 · Pull Request #226 · KILTprotocol/kilt-node

ntn-x2 · 2021-07-13T10:19:31Z

fixes KILTProtocol/ticket#1451

fixes KILTProtocol/ticket#1476

fixes KILTProtocol/ticket#1478

This PR improves the efficiency of the delegation pallet's storage so that for most of the operations less DB reads and writes are performed. The base concept is the following:

All nodes (root and not) are considered the same, as opposite to before where a root node was not really a node. This means that they are all under the same DelegationNodes storage entry.
A second storage entry, DelegationHierarchies, which maps from a root node ID to the details of a hierarchy, which at the moment only includes the CTYPE hash.
The extrinsics create_root has been renamed to create_hierarchy.
Unit tests and benchmarks have been updated accordingly. Same goes for the parts of the attestation's pallet that needed to be updated.
Added storage migrations and a potentially new approach to better handle consecutive and sequential storage migrations.

21.07.2021: A bug was discovered that revocation status of the parent node is not checked upon delegation creation. This PR is updated to mitigate such bug -> https://github.com/KILTprotocol/ticket/issues/1476 and fixed in 1e6b436.
21.07.2021: After a more careful analysis, we found out that by now revoke_root and revoke_delegation are basically equivalent. So we have decided to get rid of revoke_root -> https://github.com/KILTprotocol/ticket/issues/1478 and addressed in 11bcc2d.

How to test:

Run cargo test --all --all-features --all-targets. Weights for the delegation pallets can also be compared with the current develop version to get an idea of the changed efficiency of the pallet.

For a dry run of migration on the live peregrine parachain, run

cargo run --release -p kilt-parachain --features try-runtime -- try-runtime --url wss://kilt-peregrine-k8s.kilt.io --block-at 0xe8be760e8b01b585c5aa858ec3a0a406d152c961d70a1b5c99b2ba21237cadda --dev --wasm-execution compiled --execution wasm on-runtime-upgrade live

Note

We decided to halt merging this PR until we are sure we can merge the updated SDK as well, so we avoid getting the new node version in the security audit but the SDK does not include the changes.

Custom types

KILTprotocol/type-definitions#10.

Checklist:

I have verified that the code works
- No panics! (checked arithmetic ops, no indexing array[3] use get(3), ...)
I have verified that the code is easy to understand
- If not, I have left a well-balanced amount of inline comments
[] This PR does not introduce new custom types
- If not, I have opened a companion PR with the type changes in the KILT types-definitions repository
I have left the code in a better state
I have documented the changes (where applicable)

…rily remove benchmarks from peregrine runtime

weichweich · 2021-07-13T11:51:14Z

pls merge develop! 😁

…a-delegation-refactoring

weichweich

didn't look at migrations.rs yet

pallets/delegation/src/delegation_hierarchy.rs

weichweich · 2021-07-15T08:14:59Z

pallets/delegation/src/delegation_hierarchy.rs

-			permissions,
-			revoked: false,
+	#[derive(Clone, Debug, Encode, Decode, PartialEq)]
+	pub struct DelegationDetails<T: Config> {


why do we move the details to a separate struct?

Because I think it's easier to self-contain whatever does not belong to a node as such, such as a parent, an ID, and the children.

So you would like to have an abstract tree and then just some content. I wouldn't call it DelegationTree then but Tree<Delegation> maybe there is already a tree implementation that we could use. 🤔

But i think it makes it not clearer and just adds one unnecessary step. 🤷🏻 But that is subjective.

I don't think we need to have such a general implementation of a tree, but on the contrary I think that not having everything on the same level is a good idea.

I think I couldn't convey my point so let me rephrase this.

In my opinion it's always a good idea to structure data when there is a category to structure them by. Eg. don't have x and y fields, but have a nested position field that has y and x. It makes sense to have position because it is a meaningful group for x and y.

Looking at the DelegationDetails it's not clear what this category is. What goes into details and what doesn't? details is not a real category. It's just a way of saying the number of fields in this struct was to high and i move it some where else.

But I think you actually split the fields by things that are relevant for the tree structure and things that are not relevant for the tree structure. So for me a meaningful category would be tree_content which makes only sense if you have a generic tree data structure.

For me it looks like the structure does more harm than it helps at the moment (especially DelegationHierarchyDetails which only has one field).

That said, this is my opinion and it's a style thing. So if i couldn't convince you go ahead and merge.

tl;dr; I fucking hate the word details! could be anything! What even are details?!

I see your point, and I probably agree that details is not the best word. Plus, you got the point that my intention here is twofold:

group everything that is tree-related and everything else that is not tree related. How could that be achieved in a different way that perhaps satisfies both preferences?

for the hierarchy, it is true that for now it is just the CTYPE, but could be in future something else as well. And we would have to make it a struct anyway eventually, right? In order to avoid having tuples around. What would be your suggestion here?

imHo i wouldn't have a nested structure here. 😅 If you prefer it nested, I don't see a better solution.

pallets/delegation/src/lib.rs

weichweich · 2021-07-15T08:27:02Z

pallets/delegation/src/lib.rs

+			root_node_id: DelegationNodeIdOf<T>,
+			parent_id: DelegationNodeIdOf<T>,


would it be possible to pass a different root_node_id than the one that is stored in the parent? Since we always query the parent, no need to have the root node as a parameter?

Addressed in cbc29eb.

pallets/delegation/src/lib.rs

wischli

Very minor things on the nitpick level. Otherwise, LGTM! Really like the updated approach. Looks much cleaner.

pallets/delegation/src/migrations.rs

wischli

LGTM! I suppose Albi should still take a look once he's back.

weichweich

LGTM

pallets/delegation/src/lib.rs

pallets/delegation/src/migrations.rs

weichweich

everything has been said!

…a-delegation-refactoring

…a-delegation-refactoring # Conflicts: # pallets/attestation/src/lib.rs # pallets/delegation/src/delegation_hierarchy.rs # pallets/delegation/src/lib.rs

* feat: initial commit * feat: pallet compiling * wip: fixing test cases * wip: refactoring test cases * test: unit tests passing again * benchmark tests passing * test: attestation unit tests passing * chore: clippy fixes * chore: fmt fixes * bench: re-execute benchmarks for delegation and attestation + temporarily remove benchmarks from peregrine runtime * fix: adjust pre-condition for an attestation unit test * wip: add initial support for delegation storage migrations * delegation storage migrator compiling * wip: storage migrator working, now writing unit tests for it * test: unit tests passing * chore: clippy hints * chore: further clippy fixes * chore: fmt * wip: adjusting last things before possible to review * chore: clippy + fmt * fix: address part of PR review comments * fix: more comments from PR review * wip: change vector of versions to enum * test: unit tests refactor for the new storage migrator version * chore: refactor * test: unit tests passing again * chore: move deprecated type to deprecated.rs * chore: refactor import/export * doc: refine documentation * chore: fmt + clippy * test: add TODO to improve post_migration tests * feat: remove test feature from traits * test: add checks during migration in try-runtime * bench: add spiritnet benchmarks for delegation and attestation * feat: additional logs for count of migrated nodes * chore: fmt * chore: removed useless file * chore: bump runtime version * fix: PR comments * fix: re-introduce root_id in delegation creation hash * chore: capitalize storage version enum * chore: move migrations in their own module * chore: move migrations.rs out of the migrations folder * fix: add check for parent revocation status when creating a delegation * chore: fmt * feat: remove revoke_hierarchy extrinsic * bench: add peregrine benchmarks * bench: re-run delegation and attestation default benchmarks * chore: fmt

* test: first unit tests passing after refactor * feat: refactor complete * feat: revert DID creation to old version and update test cases * fix: update test cases and benchmarks * wip: comment out current weights * feat: delegation pallet storage refactor (#226) * feat: initial commit * feat: pallet compiling * wip: fixing test cases * wip: refactoring test cases * test: unit tests passing again * benchmark tests passing * test: attestation unit tests passing * chore: clippy fixes * chore: fmt fixes * bench: re-execute benchmarks for delegation and attestation + temporarily remove benchmarks from peregrine runtime * fix: adjust pre-condition for an attestation unit test * wip: add initial support for delegation storage migrations * delegation storage migrator compiling * wip: storage migrator working, now writing unit tests for it * test: unit tests passing * chore: clippy hints * chore: further clippy fixes * chore: fmt * wip: adjusting last things before possible to review * chore: clippy + fmt * fix: address part of PR review comments * fix: more comments from PR review * wip: change vector of versions to enum * test: unit tests refactor for the new storage migrator version * chore: refactor * test: unit tests passing again * chore: move deprecated type to deprecated.rs * chore: refactor import/export * doc: refine documentation * chore: fmt + clippy * test: add TODO to improve post_migration tests * feat: remove test feature from traits * test: add checks during migration in try-runtime * bench: add spiritnet benchmarks for delegation and attestation * feat: additional logs for count of migrated nodes * chore: fmt * chore: removed useless file * chore: bump runtime version * fix: PR comments * fix: re-introduce root_id in delegation creation hash * chore: capitalize storage version enum * chore: move migrations in their own module * chore: move migrations.rs out of the migrations folder * fix: add check for parent revocation status when creating a delegation * chore: fmt * feat: remove revoke_hierarchy extrinsic * bench: add peregrine benchmarks * bench: re-run delegation and attestation default benchmarks * chore: fmt * fix: refine code after checking the changes * chore: update comments to reflect latest changes * bench: update benchmarks * chore: fmt * bench: add peregrine benchmarks * fix: add new DID weights to peregrine runtime * chore: bump runtime version * chore: fmt + clippy * chore: clippy * fix: test errors * fix: typo in docs * chore: remove useless elements in unit tests * feat: remove TryFrom for DidDetails * chore: fmt * fix: replace tuple with two parameters in from_creation_details

…unded versions (#228) * wip: use bounded vec in staking * fix: add try_insert_replace * fix: staking benchmarks * fix: migrations by adding deprecated * fix: clippy fix: clippy * refactor: apply migration structure from #226 * chore: add staking migration to v5 * chore: migrate to BoundedBTreeMap in staking * fix: remove v5 migration * chore: migrate UnlockingAt to BoundedVec * chore: use BoundedVec in DelegatedAttestations * refactor: align exceeded errors * feat: delegation pallet storage refactor (#226) * feat: initial commit * feat: pallet compiling * wip: fixing test cases * wip: refactoring test cases * test: unit tests passing again * benchmark tests passing * test: attestation unit tests passing * chore: clippy fixes * chore: fmt fixes * bench: re-execute benchmarks for delegation and attestation + temporarily remove benchmarks from peregrine runtime * fix: adjust pre-condition for an attestation unit test * wip: add initial support for delegation storage migrations * delegation storage migrator compiling * wip: storage migrator working, now writing unit tests for it * test: unit tests passing * chore: clippy hints * chore: further clippy fixes * chore: fmt * wip: adjusting last things before possible to review * chore: clippy + fmt * fix: address part of PR review comments * fix: more comments from PR review * wip: change vector of versions to enum * test: unit tests refactor for the new storage migrator version * chore: refactor * test: unit tests passing again * chore: move deprecated type to deprecated.rs * chore: refactor import/export * doc: refine documentation * chore: fmt + clippy * test: add TODO to improve post_migration tests * feat: remove test feature from traits * test: add checks during migration in try-runtime * bench: add spiritnet benchmarks for delegation and attestation * feat: additional logs for count of migrated nodes * chore: fmt * chore: removed useless file * chore: bump runtime version * fix: PR comments * fix: re-introduce root_id in delegation creation hash * chore: capitalize storage version enum * chore: move migrations in their own module * chore: move migrations.rs out of the migrations folder * fix: add check for parent revocation status when creating a delegation * chore: fmt * feat: remove revoke_hierarchy extrinsic * bench: add peregrine benchmarks * bench: re-run delegation and attestation default benchmarks * chore: fmt * chore: convert BTreeSet to bounded version in delegations * refactor: add missing const declarations * wip: migrate did pallet to bounded * fix: did tests * refactor: rm redundancy, improve mock by splitting * fix: clippy * docs: improve insertion attempt comments * fix: incorrect names in Debug * refactor: remove redundant isCandidate check * fix: migrate Url to BoundedVec * refactor: MaxPublicKeysPerDid * fix: bounds for did structs * refactor: apply shorter unlocking mutation * fix: remove unused pub * refactor: try_insert, try_upsert * fix: rm expect in delegator constructor * fix: allow delegator constructor to throw * refactor: apply suggestion for sort_greatest_to_lowest * refactor: remove deprecated in staking * fix: clippy * fix: split did::mock to make benchmarks work again * refactor: DidKeyAgreementKeys * chore: update benchmarks * chore: bump spec version to 20 Co-authored-by: Antonio <antonio@kilt.io>

ntn-x2 added 10 commits July 8, 2021 16:50

feat: initial commit

2c2d8b5

feat: pallet compiling

b07bf96

wip: fixing test cases

fde4a99

wip: refactoring test cases

7249e29

test: unit tests passing again

7c9e8e8

benchmark tests passing

5c8dc90

test: attestation unit tests passing

080bb41

chore: clippy fixes

626c23b

chore: fmt fixes

16c8e01

bench: re-execute benchmarks for delegation and attestation + tempora…

b292231

…rily remove benchmarks from peregrine runtime

ntn-x2 requested review from weichweich and wischli July 13, 2021 10:19

ntn-x2 self-assigned this Jul 13, 2021

ntn-x2 added the Breaking JS Types This PR will break the JS types. label Jul 13, 2021

fix: adjust pre-condition for an attestation unit test

9f11e2d

Merge branch 'develop' of github.com:KILTprotocol/mashnet-node into a…

cf43f9e

…a-delegation-refactoring

weichweich marked this pull request as ready for review July 14, 2021 06:16

ntn-x2 added 10 commits July 14, 2021 12:22

wip: add initial support for delegation storage migrations

9bf1119

delegation storage migrator compiling

badcb0f

wip: storage migrator working, now writing unit tests for it

2dd672f

test: unit tests passing

ce7c8c7

chore: clippy hints

18955fb

chore: further clippy fixes

82b2c13

chore: fmt

3122b26

wip: adjusting last things before possible to review

9730fc4

Merge branch 'develop' of github.com:KILTprotocol/mashnet-node into a…

86fa9f2

…a-delegation-refactoring

chore: clippy + fmt

e51b478

weichweich reviewed Jul 15, 2021

View reviewed changes

weichweich marked this pull request as draft July 15, 2021 08:33

wischli reviewed Jul 19, 2021

View reviewed changes

pallets/delegation/src/migrations.rs Outdated Show resolved Hide resolved

pallets/delegation/src/migrations.rs Outdated Show resolved Hide resolved

pallets/delegation/src/migrations.rs Outdated Show resolved Hide resolved

pallets/delegation/src/migrations.rs Outdated Show resolved Hide resolved

ntn-x2 changed the title ~~feat: delegation pallet storage refactor~~ [DO NOT MERGE] feat: delegation pallet storage refactor Jul 19, 2021

fix: PR comments

78ea8b5

wischli approved these changes Jul 19, 2021

View reviewed changes

weichweich suggested changes Jul 20, 2021

View reviewed changes

ntn-x2 added 4 commits July 21, 2021 08:46

fix: re-introduce root_id in delegation creation hash

310cad8

chore: capitalize storage version enum

ef941d3

chore: move migrations in their own module

e96bc31

chore: move migrations.rs out of the migrations folder

234571c

ntn-x2 added the 🐛 bug priority: bug label Jul 21, 2021

ntn-x2 added 5 commits July 21, 2021 13:34

fix: add check for parent revocation status when creating a delegation

1e6b436

chore: fmt

364c7a8

feat: remove revoke_hierarchy extrinsic

11bcc2d

bench: add peregrine benchmarks

cf550eb

bench: re-run delegation and attestation default benchmarks

05fac68

weichweich approved these changes Jul 21, 2021

View reviewed changes

ntn-x2 added 3 commits July 22, 2021 08:54

Merge branch 'develop' of github.com:KILTprotocol/mashnet-node into a…

dc25b86

…a-delegation-refactoring

Merge branch 'develop' of github.com:KILTprotocol/mashnet-node into a…

78d2bcf

…a-delegation-refactoring # Conflicts: # pallets/attestation/src/lib.rs # pallets/delegation/src/delegation_hierarchy.rs # pallets/delegation/src/lib.rs

chore: fmt

879e194

wischli added a commit that referenced this pull request Jul 29, 2021

refactor: apply migration structure from #226

61e99a2

ntn-x2 mentioned this pull request Jul 30, 2021

feat: update SDK to work with refactored delegation pallet KILTprotocol/sdk-js#406

Merged

4 tasks

ntn-x2 changed the title ~~[DO NOT MERGE] feat: delegation pallet storage refactor~~ feat: delegation pallet storage refactor Aug 3, 2021

ntn-x2 merged commit 974ba36 into develop Aug 3, 2021

ntn-x2 deleted the aa-delegation-refactoring branch August 3, 2021 14:37

wischli added a commit that referenced this pull request Aug 4, 2021

refactor: apply migration structure from #226

8b1d5b5

wischli mentioned this pull request Aug 5, 2021

refactor: migrate runtime storage Vec, BTreeSet, BTreeMap to their bounded versions #228

Merged

15 tasks

		root_node_id: DelegationNodeIdOf<T>,
		parent_id: DelegationNodeIdOf<T>,

Conversation

ntn-x2 commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

fixes KILTProtocol/ticket#1451

fixes KILTProtocol/ticket#1476

fixes KILTProtocol/ticket#1478

How to test:

Note

Custom types

Checklist:

Uh oh!

weichweich commented Jul 13, 2021

Uh oh!

weichweich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wischli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wischli left a comment

Choose a reason for hiding this comment

Uh oh!

weichweich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

weichweich left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ntn-x2 commented Jul 13, 2021 •

edited

Loading