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

Enable rolling update of "Permit paying oneself" / "No longer allow create-account to add funds to an existing account" #10375

Merged
merged 3 commits into from
Jun 3, 2020

Conversation

mvines
Copy link
Member

@mvines mvines commented Jun 3, 2020

Two recent system program changes will cause a hard fork if deployed without feature gating:

The system_instruction_processor without these two features is now found in legacy_system_instruction_processor.rs. legacy_system_instruction_processor will be used for testnet/mainnet-beta for now, while system_instruction_processor is used for devnet.

Once this PR is backported to v1.2 and v1.1, separate PRs will be created at the appropriate time to activate these two new features on testnet/mainnet-beta

BuiltinProgram::new(
"system_program",
system_program::id(),
system_instruction_processor::process_instruction, // TODO: This is the OLD system instruction processor
Copy link
Member Author

Choose a reason for hiding this comment

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

Big advantage to this approach IMO is that system_instruction_processor doesn't need to be peppered with if checks. We make a copy of system_instruction_processor.rs into legacy_system_instruction_processor.rs with the old code, then system_instruction_processor.rs only contains the new code. Now devs don't need to dance around ifs as they make future modifications to system_instruction_processor.rs

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of "legacy" how about starting off with a version in case there are more of these kinds of changes later?

Copy link
Member Author

Choose a reason for hiding this comment

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

Once the new system_instruction_processor is deployed to all clusters I was thinking we delete legacy_system_instruction_processor from master and then we're back to just system_instruction_processor. But yes in that time it means we could ends up with a legacy_legacy_instruction_processor.

I like the idea of keeping the latest version just named system_instruction_processor. maybe legacy_system_instruction_processor0 instead of legacy_system_instruction_processor so there's room to add legacy_system_instruction_processor1 if needed?

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 went with legacy_system_instruction_processor0

Copy link
Contributor

@garious garious left a comment

Choose a reason for hiding this comment

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

Yeah, okay. Legacy copies will add to compile times, but I don't see any other drawbacks.

@mvines
Copy link
Member Author

mvines commented Jun 3, 2020

Yeah, okay. Legacy copies will add to compile times, but I don't see any other drawbacks.

Slightly yes. But we can/should also purge the legacy copies once all three clusters have moved beyond their respective activation epoch

@mvines mvines changed the title Permit builtin programs to be swapped out at epoch boundaries Enable rolling update of "Permit paying oneself" / "No longer allow create-account to add funds to an existing account" Jun 3, 2020
@mvines mvines marked this pull request as ready for review June 3, 2020 03:19
@mvines
Copy link
Member Author

mvines commented Jun 3, 2020

Need to deal with

let builtin_programs = get_builtin_programs();
still...

@mvines mvines added the blocked Unable to proceed label Jun 3, 2020
@mvines
Copy link
Member Author

mvines commented Jun 3, 2020

Blocked on #10383

@mvines mvines removed blocked Unable to proceed noCI Suppress CI on this Pull Request labels Jun 3, 2020
@mvines
Copy link
Member Author

mvines commented Jun 3, 2020

unblocked!

@mvines mvines added the v1.2 label Jun 3, 2020
@codecov
Copy link

codecov bot commented Jun 3, 2020

Codecov Report

Merging #10375 into master will increase coverage by 0.1%.
The diff coverage is 96.5%.

@@            Coverage Diff            @@
##           master   #10375     +/-   ##
=========================================
+ Coverage    81.4%    81.5%   +0.1%     
=========================================
  Files         294      295      +1     
  Lines       68258    69132    +874     
=========================================
+ Hits        55582    56366    +784     
- Misses      12676    12766     +90     

@mvines mvines merged commit 21d6249 into solana-labs:master Jun 3, 2020
@mvines mvines deleted the swap branch June 3, 2020 23:08
solana-grimes pushed a commit that referenced this pull request Jun 4, 2020
…reate-account to add funds to an existing account" (bp #10375) (#10404)

automerge
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