Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

feat: rocksdb (persistent) transaction status cache #6958

Merged

Conversation

sunnygleason
Copy link
Contributor

@sunnygleason sunnygleason commented Nov 14, 2019

Problem

Summary of Changes

  • add a transaction status column family to rocksdb keyed by (slot, hash) 🏦

Open Issues

  • We can use the int equivalent of the TransactionError enum; however, that is vulnerable to future code changes that change enum value indices. Should we use a more change-resistant value or technique for the status code?

Fixes # N/A (potentially multiple)

@sunnygleason
Copy link
Contributor Author

(@CriesofCarrots FYI)

@mvines
Copy link
Contributor

mvines commented Nov 14, 2019

We can use the int equivalent of the TransactionError enum; however, that is vulnerable to future code changes that change enum value indices. Should we use a more change-resistant value or technique for the status code?

Using TransactionError is ok IMO. This is just another facet of our binary ABI that will cause a hard fork if ever changed in a way that is not backwards compatible after mainnet launch

@CriesofCarrots
Copy link
Contributor

Using TransactionError is ok IMO

Yeah, my thinking was to use transaction::Result<()>

@codecov
Copy link

codecov bot commented Nov 14, 2019

Codecov Report

Merging #6958 into master will increase coverage by 4.1%.
The diff coverage is 81.2%.

@@           Coverage Diff            @@
##           master   #6958     +/-   ##
========================================
+ Coverage    74.7%   78.8%   +4.1%     
========================================
  Files         223     223             
  Lines       45245   42924   -2321     
========================================
+ Hits        33815   33845     +30     
+ Misses      11430    9079   -2351

@sunnygleason sunnygleason force-pushed the transaction-status-cache branch from 9c0e0d6 to ae04a7c Compare November 14, 2019 20:29
@sunnygleason sunnygleason force-pushed the transaction-status-cache branch from ae04a7c to d1b6153 Compare November 16, 2019 12:48
@sunnygleason sunnygleason marked this pull request as ready for review November 16, 2019 12:48
@sunnygleason
Copy link
Contributor Author

@CriesofCarrots ok, I think this is worth a look -- looking forward to your feedback!

@sunnygleason
Copy link
Contributor Author

🎉

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

One nit, one suggestion, then let's :shipit:

@sunnygleason sunnygleason merged commit 086e5da into solana-labs:master Nov 17, 2019
@sunnygleason sunnygleason deleted the transaction-status-cache branch November 17, 2019 16:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants