Skip to content

Fixes storage alive size with secondary indexes#5778

Merged
brooksprumo merged 1 commit intoanza-xyz:masterfrom
brooksprumo:secondary-index/stored-size-alive
Apr 14, 2025
Merged

Fixes storage alive size with secondary indexes#5778
brooksprumo merged 1 commit intoanza-xyz:masterfrom
brooksprumo:secondary-index/stored-size-alive

Conversation

@brooksprumo
Copy link
Copy Markdown

@brooksprumo brooksprumo commented Apr 11, 2025

Problem

When generating the index with secondary indexes, the AccountStorageEntry's alive bytes is incorrectly set (approximately double). This likely causes issues later, e.g. not shrinking when they should.

Summary of Changes

Fix it.

Note, I intend to backport this fix to v2.2.

@brooksprumo brooksprumo self-assigned this Apr 11, 2025
storage
.accounts
.scan_accounts_stored_meta(|stored_account| {
stored_size_alive += stored_account.stored_size();
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Here's the bug. We've already incremented the stored_size_alive for this storage (above). So doing it again here is wrong.

@mergify
Copy link
Copy Markdown

mergify Bot commented Apr 11, 2025

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.0%. Comparing base (09f105e) to head (afd7ab6).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5778   +/-   ##
=======================================
  Coverage    82.9%    83.0%           
=======================================
  Files         826      826           
  Lines      375758   375778   +20     
=======================================
+ Hits       311829   311914   +85     
+ Misses      63929    63864   -65     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

lgtm.

Maybe it is worth to add an assert of alive_bytes <= account_file.len().

@brooksprumo
Copy link
Copy Markdown
Author

Maybe it is worth to add an assert of alive_bytes <= account_file.len().

Yeah, seems like a reasonable thing to add.

@brooksprumo brooksprumo merged commit 679ad80 into anza-xyz:master Apr 14, 2025
31 checks passed
@brooksprumo brooksprumo deleted the secondary-index/stored-size-alive branch April 14, 2025 13:23
mergify Bot pushed a commit that referenced this pull request Apr 14, 2025
(cherry picked from commit 679ad80)

# Conflicts:
#	accounts-db/src/accounts_db.rs
brooksprumo added a commit that referenced this pull request Apr 14, 2025
) (#5793)

* Fixes storage alive size with secondary indexes (#5778)

(cherry picked from commit 679ad80)

# Conflicts:
#	accounts-db/src/accounts_db.rs

* fix merge conflicts

---------

Co-authored-by: Brooks <brooks@anza.xyz>
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.

3 participants