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

test: Check that stats are not recomputed #4616

Merged
merged 19 commits into from
Apr 3, 2024
Merged

test: Check that stats are not recomputed #4616

merged 19 commits into from
Apr 3, 2024

Conversation

bitdivine
Copy link
Member

@bitdivine bitdivine commented Mar 13, 2024

Motivation

Until the least release, account stats such as the total number of hardware wallets was computed as part of every upgrade in the post_upgrade hook. Iterating over all accounts on the heap was acceptably fast. This is absolutely not the case when accounts are stored in stable memory. Using the same code path when stable memory is in use would cause the upgrade to fail. We therefore wish to:

  • Test that stats are not recomputed on upgrade any more, during normal clean upgrades.
  • Delete the code for recomputing stats once we are sure that the new state of affairs is stable.

Changes

  • Make it visible whether stats were recomputed during an upgrade by recording this in the public stats.
  • Add a test that, with normal downgrades and upgrades, stats are never recomputed.

Tests

  • A test is included.

Todos

  • Add entry to changelog (if necessary).

@bitdivine bitdivine marked this pull request as ready for review March 13, 2024 12:19
@bitdivine bitdivine changed the title Migration slice 2 test: Don't recompute stats Mar 14, 2024
@bitdivine bitdivine changed the title test: Don't recompute stats test: Check that stats are not recomputed Mar 18, 2024
rs/backend/src/accounts_store.rs Show resolved Hide resolved
rs/backend/src/accounts_store.rs Outdated Show resolved Hide resolved
rs/backend/src/accounts_store.rs Show resolved Hide resolved
rs/backend/src/accounts_store.rs Outdated Show resolved Hide resolved
rs/backend/src/accounts_store.rs Show resolved Hide resolved
scripts/nns-dapp/migration-test.canister Outdated Show resolved Hide resolved
@bitdivine bitdivine added this pull request to the merge queue Apr 3, 2024
Merged via the queue into main with commit 2fa9c25 Apr 3, 2024
50 checks passed
@bitdivine bitdivine deleted the migration-slice-2 branch April 3, 2024 19:53
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.

2 participants