Skip to content

Stop writing TransactionError into snapshots from status cache#6460

Closed
steveluscher wants to merge 1 commit intoanza-xyz:masterfrom
steveluscher:stop-writing-transaction-error-into-snapshots
Closed

Stop writing TransactionError into snapshots from status cache#6460
steveluscher wants to merge 1 commit intoanza-xyz:masterfrom
steveluscher:stop-writing-transaction-error-into-snapshots

Conversation

@steveluscher
Copy link
Copy Markdown

@steveluscher steveluscher commented Jun 6, 2025

Problem

It is very difficult to change the format of TransactionError because it gets written to disk when the validator snapshots BankStatusCache. The thing is, BankStatusCache has no need of TransactionErrors or indeed TransactionResults at all; all it needs to track is whether a given transaction message has been seen or not.

Summary of Changes

In this PR we shim the code that serializes/deserializes the status_cache snapshot to unconditionally write Ok(()) into the snapshot, even if the actual Result is Err(TransactionError).

This ensures that:

  1. the snapshot file format is backward compatible with old validators (we're only ever writing Ok(()) to disk)
  2. one validator version from now the snapshots will be forward compatible (it doesn't matter if TransactionError changes, the snapshot file will only ever contain Ok(()))

FAQ

Q: Doesn't this require a SIMD for the snapshot format?
A: No! This doesn't actually change the format of the snapshot, it just makes it so that conveniently the snapshot will never contain TransactionError. This is one step along the path to making it so that a change to TransactionError like this will not break snapshots, nor require a SIMD at all.

Q: Doesn't this bust the ABI hash?
A: No! As far as the snapshotter is concerned, it's still serializing TransactionResult, even though this change makes every single value a non-error.

Furthers #6457.

@mergify
Copy link
Copy Markdown

mergify Bot commented Jun 6, 2025

If this PR represents a change to the public RPC API:

  1. Make sure it includes a complementary update to rpc-client/ (example)
  2. Open a follow-up PR to update the JavaScript client @solana/kit (example)

Thank you for keeping the RPC clients in sync with the server API @steveluscher.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.8%. Comparing base (1ff0bbf) to head (d7b04a5).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #6460    +/-   ##
========================================
  Coverage    82.8%    82.8%            
========================================
  Files         849      850     +1     
  Lines      379161   379200    +39     
========================================
+ Hits       314064   314171   +107     
+ Misses      65097    65029    -68     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Looks reasonable so far! Please ensure that a snapshot created with this PR can be loaded by v2.3 (or v2.2 if the plan is to backport).

Also, also please re-request a review once the other PRs that this one depends on are merged.

@steveluscher steveluscher force-pushed the stop-writing-transaction-error-into-snapshots branch from d71eb28 to d7b04a5 Compare June 13, 2025 00:05
@steveluscher steveluscher changed the title Stop writing TransactionError into snapshots from seen transaction cache Stop writing TransactionError into snapshots from status cache Jun 13, 2025
@steveluscher
Copy link
Copy Markdown
Author

I unstacked this from the abandoned #6459, and changed it so that it unconditionally writes Ok(()) into the snapshot.

I'm going to abandon this now, but @jstarry if you want to pick this up, here's a starting point!

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