Skip to content

Tidies unused snapshot fields#10852

Merged
brooksprumo merged 1 commit into
anza-xyz:masterfrom
brooksprumo:snap/unused
Feb 26, 2026
Merged

Tidies unused snapshot fields#10852
brooksprumo merged 1 commit into
anza-xyz:masterfrom
brooksprumo:snap/unused

Conversation

@brooksprumo
Copy link
Copy Markdown

@brooksprumo brooksprumo commented Feb 26, 2026

Problem

There are unused fields in the snapshot. Some are prepended with "unused". Some are prepended with "obsolete". One, this is inconsistent. Two, there is now a concept of obsolete accounts, which are fully used in the snapshot. So now the term "obsolete" is ambiguous.

Additionally:

  • collected_rent is unused, but isn't prepended with 'unused'
  • unused_accounts is annotated with [allow(dead_code)], whereas all the other unused fields have a leading underscore. IMO the field is not dead, as we very much need it for binary compatibility. We just never read it.

Summary of Changes

Standardize on "unused" as the name for the snapshot when a field is no longer used.

@brooksprumo brooksprumo self-assigned this Feb 26, 2026
Comment thread runtime/src/bank/serde_snapshot.rs Outdated
feature = "frozen-abi",
derive(AbiExample),
frozen_abi(digest = "FHeeuJgYyE1rqdLRycbJWC8PoTRZqYsRWZZCaP5A7DWb")
frozen_abi(digest = "3V24skiuCjTRXDRLKwCwB3PDuk4Npo6whg5hefMZkQqu")
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.

Frozen abi digest changed due to renaming types/fields.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Frozen abi digest changed due to renaming types/fields.

I know renaming types triggers a changes, but I thought you could rename fields without altering the digest

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.

Yah, even the field names are in the output that frozen abi uses/generates. So even a rename triggers a change in the digest 🫠

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ah, today I learned - thanks!

@brooksprumo brooksprumo changed the title Renames snapshot 'obsolete' to 'unused' Tidies unused snapshot fields Feb 26, 2026
@brooksprumo brooksprumo marked this pull request as ready for review February 26, 2026 17:36
steviez
steviez previously approved these changes Feb 26, 2026
@roryharr
Copy link
Copy Markdown

roryharr commented Feb 26, 2026

These digest changes are annoying. How do we actually verify we haven't broken anything?

@brooksprumo
Copy link
Copy Markdown
Author

These digest changes are annoying. How do we actually verify we haven't broken anything?

Gotta validate the FrozenAbi output. Usually I put that info up, like here: #10835 (comment). I didn't here just because it was only renames and simple to confirm by eye. Want me to add that info anyway though?

roryharr
roryharr previously approved these changes Feb 26, 2026
feature = "frozen-abi",
derive(AbiExample),
frozen_abi(digest = "EEwojKqUDSpYNeutW56K9bsJmsjCMZn8i4n3qgvJYRxf")
frozen_abi(digest = "FCDuswxZnGvvJURSkrQPshX45a25CR6UAmPgzGeUfMFv")
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 frozen abi diff, showing the renames:

--- ./bank__serde_snapshot__tests__test_bank_serialize__BankAbiTestWrapper_frozen_abi__test_api_digest_EEwojKqUDSpYNeutW56K9bsJmsjCMZn8i4n3qgvJYRxf	2026-02-26 12:10:12
+++ ./bank__serde_snapshot__tests__test_bank_serialize__BankAbiTestWrapper_frozen_abi__test_api_digest_FCDuswxZnGvvJURSkrQPshX45a25CR6UAmPgzGeUfMFv	2026-02-26 15:05:03
@@ -424,7 +424,7 @@
                                 primitive u64
                             field burnPercent: u8
                                 primitive u8
-                    field collected_rent: u64
+                    field unused_collected_rent: u64
                         primitive u64
                     field rent_collector: solana_runtime::rent_collector::RentCollector
                         struct RentCollector (fields = 4)
@@ -1037,7 +1037,7 @@
                         primitive u64
                     element solana_runtime::serde_snapshot::BankHashInfo
                         struct BankHashInfo (fields = 3)
-                            field obsolete_accounts_delta_hash: [u8; 32]
+                            field unused_accounts_delta_hash: [u8; 32]
                                 tuple (elements = 32)
                                     element u8
                                         primitive u8
@@ -1103,7 +1103,7 @@
                                         primitive u8
                                     element u8
                                         primitive u8
-                            field obsolete_accounts_hash: [u8; 32]
+                            field unused_accounts_hash: [u8; 32]
                                 tuple (elements = 32)
                                     element u8
                                         primitive u8
@@ -1262,11 +1262,11 @@
                 struct ExtraFieldsToSerialize (fields = 5)
                     field lamports_per_signature: u64
                         primitive u64
-                    field obsolete_incremental_snapshot_persistence: core::option::Option<solana_runtime::serde_snapshot::ObsoleteIncrementalSnapshotPersistence>
+                    field unused_incremental_snapshot_persistence: core::option::Option<solana_runtime::serde_snapshot::UnusedIncrementalSnapshotPersistence>
                         enum Option (variants = 2)
                             variant(0) None (unit)
-                            variant(1) Some(solana_runtime::serde_snapshot::ObsoleteIncrementalSnapshotPersistence) (newtype)
-                                struct ObsoleteIncrementalSnapshotPersistence (fields = 5)
+                            variant(1) Some(solana_runtime::serde_snapshot::UnusedIncrementalSnapshotPersistence) (newtype)
+                                struct UnusedIncrementalSnapshotPersistence (fields = 5)
                                     field full_slot: u64
                                         primitive u64
                                     field full_hash: [u8; 32]
@@ -1405,7 +1405,7 @@
                                                 primitive u8
                                     field incremental_capitalization: u64
                                         primitive u64
-                    field obsolete_epoch_accounts_hash: core::option::Option<solana_hash::Hash>
+                    field unused_epoch_accounts_hash: core::option::Option<solana_hash::Hash>
                         enum Option (variants = 2)
                             variant(0) None (unit)
                             variant(1) Some(solana_hash::Hash) (newtype)

@brooksprumo
Copy link
Copy Markdown
Author

Had to rebase on top of #10851, since they conflict on the abi digest.

@brooksprumo brooksprumo added this pull request to the merge queue Feb 26, 2026
Merged via the queue into anza-xyz:master with commit 07ee01b Feb 26, 2026
51 checks passed
@brooksprumo brooksprumo deleted the snap/unused branch February 26, 2026 20:40
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.0%. Comparing base (4be1eb1) to head (747762d).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #10852   +/-   ##
=======================================
  Coverage    83.0%    83.0%           
=======================================
  Files         836      836           
  Lines      316307   316307           
=======================================
+ Hits       262644   262677   +33     
+ Misses      53663    53630   -33     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@brooksprumo brooksprumo added the v4.0 Backport to v4.0 branch label Mar 4, 2026
@mergify
Copy link
Copy Markdown

mergify Bot commented Mar 4, 2026

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.

mergify Bot pushed a commit that referenced this pull request Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v4.0 Backport to v4.0 branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants