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.

Sign up for GitHub

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

Jump to bottom

Futurepass storage mapping - update Holders hashing algo #682

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Futurepass storage mapping - update Holders hashing algo #682

wants to merge 9 commits into from

Conversation

zees-dev
Copy link
Contributor

@zees-dev zees-dev commented Sep 4, 2023 •
edited
Loading

Changelog

Addresses https://futureverse.atlassian.net/browse/TRN-159

Core changes

  • Updated Holders and DefaultProxy storage map hashing algorithms from Twox64Concat to Blake2_128Concat
    • The DefaultProxy should not need to get migrated because it has no entries
    • Added a migration for the Holders mapping - which migrates all Holders underlying storage keys(~200,000 keys for Root)
      • This can be done by simply draining the storage map and re-populating entries using updated hashing algo using lower level storage APIs (direct storage access)
      • Added pre and post migration tests to validate migration (testing single account which already has Futurepass on Root)

@zees-dev zees-dev force-pushed the audit/159-futurepass-holders-hashing-algo branch from a93f5ba to 64c0b72 Compare September 6, 2023 02:29
@zees-dev zees-dev marked this pull request as ready for review September 12, 2023 04:02
@zees-dev zees-dev force-pushed the audit/159-futurepass-holders-hashing-algo branch from be3c28a to 2804ca9 Compare September 13, 2023 00:36
@justinfrevert
Copy link
Contributor

How does the migration do on the production data? I was curious if this is subject to similar issues to the one we saw on the last fpass migration

@zees-dev
Copy link
Contributor Author

How does the migration do on the production data? I was curious if this is subject to similar issues to the one we saw on the last fpass migration

I didnt do a production test on the previous fpass migration.
This one should definetly be less work as we are traversing less data (single pass i believe).
With current tests on my machine it takes ~1min for the migration.

JasonTulp
JasonTulp approved these changes Sep 14, 2023
View reviewed changes
Copy link
Contributor

@JasonTulp JasonTulp left a comment

Choose a reason for hiding this comment

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

Nice work! Few small things but looks good overall

runtime/src/migrations/futurepass.rs Show resolved Hide resolved
runtime/src/migrations/futurepass.rs
if onchain == 1 {
return Ok(crate::Vec::new())
}
assert_eq!(onchain, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this assert not do essentially the same as onchain == 1 above?

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 sure, i copied this part from a previous migration

runtime/src/migrations/futurepass.rs Show resolved Hide resolved
runtime/src/migrations/mod.rs Outdated Show resolved Hide resolved
runtime/src/migrations/mod.rs Outdated Show resolved Hide resolved
surangap
surangap reviewed Sep 14, 2023
View reviewed changes
Copy link
Contributor

@surangap surangap left a comment

Choose a reason for hiding this comment

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

The code looks good. need to run some tests and get the numbers.

hanverse
hanverse approved these changes Sep 14, 2023
View reviewed changes
Copy link
Contributor

@hanverse hanverse left a comment

Choose a reason for hiding this comment

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

Great implementation! Very clever way to check and verify the low level storages before and after the migration. It looks good to me.

@zees-dev zees-dev force-pushed the audit/159-futurepass-holders-hashing-algo branch from 2804ca9 to 65a3c29 Compare September 14, 2023 22:50
@zees-dev
Copy link
Contributor Author

changes are due to rebase from main branch (and resolution of merge conflicts) - which now also includes NFT migration

@zees-dev zees-dev force-pushed the audit/159-futurepass-holders-hashing-algo branch from 65a3c29 to d4e4ec4 Compare September 27, 2023 00:00
@zees-dev zees-dev force-pushed the audit/159-futurepass-holders-hashing-algo branch from d4e4ec4 to 4996f74 Compare August 29, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@surangap surangap surangap left review comments

@hanverse hanverse hanverse approved these changes

@justinfrevert justinfrevert justinfrevert approved these changes

@JasonTulp JasonTulp JasonTulp approved these changes

Assignees
No one assigned
Labels
ops:do-not-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zees-dev @justinfrevert @surangap @JasonTulp @hanverse