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

Do not unnecessarily re-verify unloaded program #32722

Merged
merged 7 commits into from
Sep 13, 2023

Conversation

pgarg66
Copy link
Contributor

@pgarg66 pgarg66 commented Aug 4, 2023

Problem

The programs might get unloaded from the cache due to cache overflow. The current code re-verifies these programs when reloaded. This is not needed if the runtime environment hasn't changed.

Summary of Changes

Do not re-verify the unloaded programs if the environment hasn't changed.

Fixes #

@pgarg66 pgarg66 force-pushed the optimize-unloaded-program-loading branch from 380e8e2 to bb0b285 Compare August 4, 2023 23:07
@pgarg66 pgarg66 marked this pull request as ready for review August 5, 2023 00:23
@codecov
Copy link

codecov bot commented Aug 5, 2023

Codecov Report

Merging #32722 (16f0309) into master (f4816dc) will decrease coverage by 0.1%.
Report is 34 commits behind head on master.
The diff coverage is 86.0%.

@@            Coverage Diff            @@
##           master   #32722     +/-   ##
=========================================
- Coverage    82.1%    82.1%   -0.1%     
=========================================
  Files         785      785             
  Lines      211205   211381    +176     
=========================================
+ Hits       173432   173559    +127     
- Misses      37773    37822     +49     

@pgarg66 pgarg66 force-pushed the optimize-unloaded-program-loading branch 2 times, most recently from ddf2e50 to 9de8fc5 Compare August 16, 2023 18:58
@pgarg66 pgarg66 requested a review from dmakarov August 17, 2023 16:33
@dmakarov
Copy link
Contributor

LGTM

@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Sep 1, 2023
@Lichtso
Copy link
Contributor

Lichtso commented Sep 6, 2023

@pgarg66 Can you rebase this? #33104 has landed so the Verifier generic parameter on Executable is gone.

@pgarg66
Copy link
Contributor Author

pgarg66 commented Sep 6, 2023

@pgarg66 Can you rebase this? #33104 has landed so the Verifier generic parameter on Executable is gone.

Sounds good. Will do.

@pgarg66 pgarg66 force-pushed the optimize-unloaded-program-loading branch from 9de8fc5 to 45b1f74 Compare September 7, 2023 00:23
@pgarg66 pgarg66 requested a review from Lichtso September 7, 2023 04:02
@github-actions github-actions bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Sep 7, 2023
@pgarg66 pgarg66 requested a review from alessandrod September 11, 2023 17:44
@@ -228,16 +228,22 @@ impl LoadedProgram {
elf_bytes: &[u8],
account_size: usize,
metrics: &mut LoadProgramMetrics,
reloading: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not very familiar with this code, but this PR lgtm.

That said, the API now exposes a potential footgun: you get the
reloading argument wrong and we end up executing untrusted code, which
potentially has catastrophic consequences.

We don't do what I'm about to suggest a lot (sadly), so I'm fine if you want to
merge this as is. However I think we should get into the habit of not having
footguns at the type system level. In this case, I think a safer and rustier API
could be:

fn new(
	...leave it as is, no reloading argument
)

/// Reloads an user program, *without* running the verifier.
///
/// This method is unsafe since it assumes that the program has already been verified. Should
/// only be called when ...
unsafe fn reloaded(
	...
)

The methods could call a new private new_internal constructor which does
what new() does now in this PR.

With this API, all but one call sites wouldn't have to worry about potentially
executing unverified code. The (future) call site that will reload, will have a
safety comment like:

// Safety: this is safe because ...
unsafe { LoadedProgram::reload(...) };

I realize that in this context this might seem like somewhat of a pedantic/paranoid
remark! But I do wish we did this everywhere in the validator, and we gotta
start somewhere :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of keeping the reload as a separate API that requires an explicit call to it. I'll update the PR.

@pgarg66 pgarg66 requested a review from alessandrod September 12, 2023 17:27
@pgarg66 pgarg66 merged commit 5562f79 into solana-labs:master Sep 13, 2023
16 checks passed
@pgarg66 pgarg66 deleted the optimize-unloaded-program-loading branch September 13, 2023 13:26
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.

4 participants