Split Status Cache into one cache that holds ‘transaction seenedness’ for the validator, and one that holds transaction results for the RPC#6459
Closed
steveluscher wants to merge 2 commits intoanza-xyz:masterfrom
Conversation
|
If this PR represents a change to the public RPC API:
Thank you for keeping the RPC clients in sync with the server API @steveluscher. |
steveluscher
commented
Jun 6, 2025
| // can be queried by transaction signature over RPC. In the future, this should | ||
| // only be added for API nodes because voting validators don't need to do this. | ||
| seen_transaction_cache.insert( | ||
| status_cache.insert( |
Author
There was a problem hiding this comment.
Now that this cache is distinct, a follow up PR could contemplate disabling this write when the node is not in RPC mode.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6459 +/- ##
=========================================
- Coverage 82.8% 82.8% -0.1%
=========================================
Files 848 848
Lines 379479 379528 +49
=========================================
- Hits 314588 314536 -52
- Misses 64891 64992 +101 🚀 New features to boost your workflow:
|
Author
|
Abandoning this with the belief that #6546 is the way forward. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Today, the status cache does two things
SignaturestoTransactionResultsfor RPC lookupsNever the less, when we take a snapshot of the validator, we get both of these things. My claim is that we only need the set of seen messages, and not the transaction statuses. The reason being: the cache only holds the latest 300 slots of statuses, and there's no way that you can start up an RPC node from a snapshot in <300 slots. By the time the RPC is ready to respond to requests (ie. is ‘healthy’) the snapshotted transaction statuses will be too old to be useful and will have since been replaced.
Summary of Changes
Bank; one into which message hashes are written, and one into whichSignaturesare writtenA follow up PR could now make
status_cacheanOption<BankStatusCache>and disable it when the node is not running in RPC mode.How to review this PR
seen_transaction_cachestatus_cache, and makes it so that it only deals withSignaturesandTransactionResults.FAQ
Q: Aren't we doing double the writes now?
A: Yes and no. Only message
Hashesare being written toseen_transaction_cacheand onlySignaturesare being written tostatus_cache. Onlyseen_transaction_cacheparticipates in snapshots now. All in all we're not doubling the writes but rather splitting them (and splitting the locks too) but we are duplicating whatever work thatStatusCache::insert()does.Q: Why don't you stop writing
TransactionStatustoseen_transaction_cacheA: We do that in #6460 because it changes how snapshots are written and I didn't want to bother you with that code here.