Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fetch sysvars from invoke context for vote program #22444

Merged
merged 2 commits into from Jan 13, 2022

Conversation

jstarry
Copy link
Member

@jstarry jstarry commented Jan 12, 2022

Problem

Vote program cannot take advantage of optimizations in the sysvar cache because it loads sysvars from keyed accounts

Summary of Changes

Transition to fetching sysvars from the invoke context without breaking consensus

Fixes #

@jstarry jstarry force-pushed the perf/vote-sysvars branch 3 times, most recently from 1e0c602 to 9f977d0 Compare January 12, 2022 02:39
@codecov
Copy link

codecov bot commented Jan 12, 2022

Codecov Report

Merging #22444 (ee552f5) into master (eaae2f3) will decrease coverage by 0.0%.
The diff coverage is 90.0%.

❗ Current head ee552f5 differs from pull request most recent head f1d530f. Consider uploading reports for the commit f1d530f to get more accurate results

@@            Coverage Diff            @@
##           master   #22444     +/-   ##
=========================================
- Coverage    81.1%    81.1%   -0.1%     
=========================================
  Files         555      553      -2     
  Lines      150563   150503     -60     
=========================================
- Hits       122198   122112     -86     
- Misses      28365    28391     +26     

pub fn from_keyed_account<S: Sysvar>(
keyed_account: &crate::keyed_account::KeyedAccount,
) -> Result<S, InstructionError> {
pub fn check_sysvar_keyed_account<'a, S: Sysvar>(
Copy link
Contributor

@Lichtso Lichtso Jan 12, 2022

Choose a reason for hiding this comment

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

Can't this be inlined at its only two use sites?
Because otherwise this will be one more public interface that needs to be removed again,
when KeyedAccounts in general are removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see any problem with removing it then. I'd like to reuse the code here because consensus depends on the checks being the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I thought you wanted to share it for the error messages to stay the same. However, you also intend to replace one of the call sites by the sysvar cache right? So, that would remove the potential error case there.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR already changes the call site to use the sysvar cache, I'm not sure what you mean

jackcmay
jackcmay previously approved these changes Jan 12, 2022
@mergify mergify bot dismissed jackcmay’s stale review January 12, 2022 22:22

Pull request has been modified.

@jstarry jstarry merged commit b211f83 into solana-labs:master Jan 13, 2022
mergify bot pushed a commit that referenced this pull request Jan 13, 2022
(cherry picked from commit b211f83)

# Conflicts:
#	programs/vote/src/vote_instruction.rs
mergify bot pushed a commit that referenced this pull request Jan 13, 2022
(cherry picked from commit b211f83)

# Conflicts:
#	programs/vote/src/vote_instruction.rs
@jstarry jstarry deleted the perf/vote-sysvars branch January 13, 2022 00:59
mergify bot added a commit that referenced this pull request Jan 15, 2022
…22469)

* Fetch sysvars from invoke context for vote program (#22444)

* resolve conflicts

Co-authored-by: Justin Starry <justin@solana.com>
jstarry added a commit to jstarry/solana that referenced this pull request Jan 21, 2022
mergify bot pushed a commit that referenced this pull request Jan 21, 2022
…22621)

* Fetch sysvars from invoke context for vote program (#22444)

* resolve conflicts
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.

None yet

4 participants