Skip to content

v2.2: Fixes storage alive size with secondary indexes (backport of #5778)#5793

Merged
brooksprumo merged 2 commits intov2.2from
mergify/bp/v2.2/pr-5778
Apr 14, 2025
Merged

v2.2: Fixes storage alive size with secondary indexes (backport of #5778)#5793
brooksprumo merged 2 commits intov2.2from
mergify/bp/v2.2/pr-5778

Conversation

@mergify
Copy link
Copy Markdown

@mergify mergify Bot commented Apr 14, 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.

Justification to Backport

The alive bytes is used to determine which storages to shrink, and when. Having the wrong value is a bug and makes debugging/reasoning about behavior harder.


This is an automatic backport of pull request #5778 done by [Mergify](https://mergify.com).

(cherry picked from commit 679ad80)

# Conflicts:
#	accounts-db/src/accounts_db.rs
@mergify mergify Bot requested a review from a team as a code owner April 14, 2025 13:24
@mergify mergify Bot added the conflicts label Apr 14, 2025
@mergify
Copy link
Copy Markdown
Author

mergify Bot commented Apr 14, 2025

Cherry-pick of 679ad80 has failed:

On branch mergify/bp/v2.2/pr-5778
Your branch is up to date with 'origin/v2.2'.

You are currently cherry-picking commit 679ad8034.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   accounts-db/src/accounts_db/tests.rs

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   accounts-db/src/accounts_db.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

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

@t-nelson
Copy link
Copy Markdown

this won't have much bake time on testnet before 2.2 hits mb. we're confident?

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.4%. Comparing base (52fbd17) to head (f13d78e).
⚠️ Report is 72 commits behind head on v2.2.

Additional details and impacted files
@@           Coverage Diff           @@
##             v2.2    #5793   +/-   ##
=======================================
  Coverage    83.4%    83.4%           
=======================================
  Files         806      806           
  Lines      373256   373276   +20     
=======================================
+ Hits       311314   311381   +67     
+ Misses      61942    61895   -47     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@brooksprumo
Copy link
Copy Markdown

this won't have much bake time on testnet before 2.2 hits mb. we're confident?

I feel good, yes. I also spun up a v2.2 node with secondary indexes to confirm.

There's three sections in this graph: Left, Middle, and Right.

  • Left: v2.2.8, no secondary indexes.
  • Middle: v2.2.8, yes secondary indexes.
  • Right: This PR, yes secondary indexes.

We can see the percentage alive in the storages is wrong (200%) in the middle with secondary indexes, and then correct again with this PR.

percent alive

Copy link
Copy Markdown

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm

@mircea-c
Copy link
Copy Markdown

@mergify rebase

@mergify
Copy link
Copy Markdown
Author

mergify Bot commented Apr 14, 2025

rebase

❌ Unable to rebase: user mircea-c is unknown.

Details

Please make sure mircea-c has logged in Mergify dashboard.

@brooksprumo
Copy link
Copy Markdown

@mircea-c Why the rebase?

@mircea-c
Copy link
Copy Markdown

@brooksprumo don't mind me. Just getting the hang of this. I thought it needed a rebase based on the github message, but it has no conflicts so can just be merged.

@brooksprumo
Copy link
Copy Markdown

@brooksprumo don't mind me. Just getting the hang of this. I thought it needed a rebase based on the github message, but it has no conflicts so can just be merged.

Gotcha. Yeah, please avoid rebasing PRs. I think it's best to let the owner/assignee be responsible for any rebasing; so as to avoid going through a long CI cycle unnecessarily.

@brooksprumo brooksprumo merged commit b45a258 into v2.2 Apr 14, 2025
46 checks passed
@brooksprumo brooksprumo deleted the mergify/bp/v2.2/pr-5778 branch April 14, 2025 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants