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

Remove redundant check during loading upgradeable program for writing #30561

Merged
merged 2 commits into from
Mar 28, 2023

Conversation

pgarg66
Copy link
Contributor

@pgarg66 pgarg66 commented Feb 28, 2023

Problem

The accounts.rs code currently checks if the message includes upgradeable loader key, if an upgradeable program is being loaded for writing. The check looks necessary and harmless at quick glance. But its unnecessary, as the transaction will fail due to other checks in the execution. The check is making it harder to refactor the code for the runtime V2/Cache replacement (#29803).

Summary of Changes

  • Remove the check under a feature gate.
  • Update the unit tests.
  • Add feature gate.

Feature Gate Issue #30559

@pgarg66 pgarg66 requested a review from Lichtso February 28, 2023 23:05
@pgarg66 pgarg66 marked this pull request as ready for review March 1, 2023 02:49
@CriesofCarrots
Copy link
Contributor

But its unnecessary, as the transaction will fail due to other checks in the execution.

Can you please link to these checks?

@pgarg66
Copy link
Contributor Author

pgarg66 commented Mar 2, 2023

But its unnecessary, as the transaction will fail due to other checks in the execution.

Can you please link to these checks?

@Lichtso could you please point to the code?

@Lichtso
Copy link
Contributor

Lichtso commented Mar 2, 2023

When an account has the is_executable flag set, it essentially becomes immutable, regardless of what is_writable says:

@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Mar 17, 2023
@pgarg66 pgarg66 removed the stale [bot only] Added to stale content; results in auto-close after a week. label Mar 18, 2023
@codecov
Copy link

codecov bot commented Mar 18, 2023

Codecov Report

Merging #30561 (bd360fa) into master (aaac046) will increase coverage by 0.0%.
The diff coverage is 97.2%.

@@           Coverage Diff           @@
##           master   #30561   +/-   ##
=======================================
  Coverage    81.5%    81.5%           
=======================================
  Files         727      727           
  Lines      205250   205282   +32     
=======================================
+ Hits       167378   167428   +50     
+ Misses      37872    37854   -18     

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.

Thanks for the program-cache context we discussed directly. This now looks okay to me

@pgarg66 pgarg66 merged commit 035c974 into solana-labs:master Mar 28, 2023
@pgarg66 pgarg66 deleted the remove-upgradable-check branch March 28, 2023 18:50
@pgarg66
Copy link
Contributor Author

pgarg66 commented Sep 21, 2023

Activating on testnet in epoch 530.

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