Refactor - Makes loader-v4 the default #2796
Conversation
|
The Firedancer team maintains a line-for-line reimplementation of the |
Lichtso
force-pushed
the
rekey/enable_loader_v4
branch
5 times, most recently
from
43b7b38 to
221fd7d
Compare
Lichtso
changed the title
enable_loader_v4
Lichtso
force-pushed
the
rekey/enable_loader_v4
branch
5 times, most recently
from
a1303e6 to
f984bbc
Compare
Lichtso
force-pushed
the
rekey/enable_loader_v4
branch
2 times, most recently
from
eec5813 to
1cb4d44
Compare
Lichtso
force-pushed
the
rekey/enable_loader_v4
branch
6 times, most recently
from
8a2a279 to
19c1df9
Compare
Lichtso
force-pushed
the
rekey/enable_loader_v4
branch
from
19c1df9 to
30f3fcb
Compare
Lichtso
force-pushed
the
rekey/enable_loader_v4
branch
2 times, most recently
from
95261f9 to
64c09b6
Compare
Lichtso
force-pushed
the
rekey/enable_loader_v4
branch
from
64c09b6 to
2d4b884
Compare
Lichtso
force-pushed
the
rekey/enable_loader_v4
branch
from
2d4b884 to
489694d
Compare
Lichtso
force-pushed
the
rekey/enable_loader_v4
branch
from
489694d to
33f0270
Compare
Lichtso
force-pushed
the
rekey/enable_loader_v4
branch
from
33f0270 to
fc55020
Compare
Lichtso
force-pushed
the
rekey/enable_loader_v4
branch
6 times, most recently
from
8a2d4bc to
f7b5eb0
Compare
Lichtso
force-pushed
the
rekey/enable_loader_v4
branch
3 times, most recently
from
dbb8d90 to
47a487f
Compare
Lichtso
force-pushed
the
rekey/enable_loader_v4
branch
2 times, most recently
from
d2ab272 to
38dacf0
Compare
| @@ -13277,12 +13278,10 @@ fn test_deploy_last_epoch_slot() { | |||
| solana_logger::setup(); | |||
|
|
|||
| // Bank Setup | |||
| let (mut genesis_config, mint_keypair) = create_genesis_config(1_000_000 * LAMPORTS_PER_SOL); | |||
| genesis_config | |||
There was a problem hiding this comment.
Was this line removed on purpose? It doesn't seem related to loader-v4.
There was a problem hiding this comment.
I refactored the test to use loader-v4 and have remove_accounts_executable_flag_checks instead of disable_fees_sysvar as the dummy feature to trigger an environment change.
cli/tests/program.rs
Outdated
| @@ -82,8 +82,17 @@ fn test_cli_program_deploy_non_upgradeable() { | |||
| let mint_keypair = Keypair::new(); | |||
| let mint_pubkey = mint_keypair.pubkey(); | |||
| let faucet_addr = run_local_faucet(mint_keypair, None); | |||
| let test_validator = | |||
| TestValidator::with_no_fees(mint_pubkey, Some(faucet_addr), SocketAddrSpace::Unspecified); | |||
| let test_validator = TestValidatorGenesis::default() | |||
There was a problem hiding this comment.
Maybe it's worth adding a wrapper function, as this block of code is being repeated in multiple tests.
Lichtso
force-pushed
the
rekey/enable_loader_v4
branch
from
38dacf0 to
0c10a9f
Compare
| if invoke_context | ||
| .get_feature_set() | ||
| .is_active(&enable_loader_v4::id()) | ||
| { | ||
| ic_logger_msg!(log_collector, "Unsupported instruction"); | ||
| return Err(InstructionError::InvalidInstructionData); | ||
| } |
There was a problem hiding this comment.
Sorry for the late flyby review, but disabling new deployments on the old loader needs to be done through a separate feature in a future release.
We can't expect all tooling to immediately switch over to loader-v4 as soon as it's available. There needs to be some amount time where both loaders are available for new deployments while people upgrade, same as we did when the upgradeable loader was added. This could be at least 1 minor release, but 3 minor releases (~1 year) would likely be ideal.
cc @nickfrosty for his thoughts, since he's been looking at the associated SIMD solana-foundation/solana-improvement-documents#167
There was a problem hiding this comment.
Well, that is why we only disable new deployments. All existing programs can be still be redeployed etc. under loader-v3. There is no point in turning on loader-v4 if we do not deprecate loader-v3, because getting rid of loader-v3 is the entire point. Also, keep in mind that tooling can be tested against the new loader on testnet and devnet, if that is the concern.
There was a problem hiding this comment.
The main point is that we can't immediately break users, especially when they're typically slow to adopt new releases.
Take the anchor deploy code for example: https://github.com/coral-xyz/anchor/blob/2b0f3a7c999771f40e240b3ce421895ce6b066cf/cli/src/lib.rs#L3874 -- they're relying on calling solana program deploy.
If they wanted to avoid breaking immediately, they would need logic to check the feature gate status, conditionally call program-v4 instead of program, and force everybody to upgrade to the newest version of the CLI using that code, all before the feature gate is enabled. It isn't reasonable to expect people to make changes and force adoption of new tools so quickly.
We didn't break people immediately when loader-v3 was added, and we had a fraction of developers at the time. We can't immediately break them here. I'm happy to come up with an alternative roll out plan together.
There was a problem hiding this comment.
We could do the feature gate checking in our CLI and alias program deploy to program-v4 deploy.
What I am saying is that the day that people start deploying to loader-v4 will not be the day it becomes available, but the day the old one stops working. And that day will be breaking users independently of whether the new loader is available before or not.
There was a problem hiding this comment.
That plan still requires people to update to the newest tools immediately, which is neither reasonable nor a good developer experience.
And I'm not sure I agree with you -- for example, if we alias program to program-v4 as you suggest, then people will automatically start using loader-v4 as they adopt the new tools without realizing it.
Then, after some time, we can put out a big loud message that solana program deploy will start failing if you're using any version before 2.2, give people 3-6 months to upgrade, and then disable new deployments. That way, only people who haven't upgraded their tools in a long time will be broken, which should be a small amount of people, and we can point to the loud message that we put out months ago.
Either way, this change needs to come with a thoughtful rollout strategy.
There was a problem hiding this comment.
Alright, I will add another feature gate. This however would mean there are four phases:
- Now: loader-v3 is active, loader-v4 is inactive
- v2.2: loader-v4 becomes active, loader-v3 stays active
- v2.3: loader-v3 deployments become inactive
- v2.4: All loader-v3 programs are force migrated to loader-v4
There was a problem hiding this comment.
Beautiful, thanks!
We can save it for the future, but I'll need to understand more about what "force migration" looks like, since that could potentially break tools like DAOs which rely on using loader-v3 instructions for program management. And we should start signaling this whole process as soon as possible to devrel.
Problem
Loader-v4 was originally planned as a PRv2 loader, but will be used as default loader (superseding loader-v3) in PRv1 now.
Summary of Changes
enable_program_runtime_v2_and_loader_v4toenable_loader_v4.test_program_upgradeable_locks()(which was introduced in Prevent the invoke and upgrade of programs in the same tx batch solana-labs/solana#14653) and is superseded bytest_program_sbf_invoke_in_same_tx_as_redeployment().test_program_sbf_invoke_upgradeable()intotest_program_sbf_upgrade().test_program_sbf_invoke_upgradeable_via_cpi()andtest_program_sbf_set_upgrade_authority_via_cpi()intotest_program_sbf_upgrade_via_cpi().test_program_sbf_invoke_in_same_tx_as_redeployment()andtest_program_sbf_invoke_in_same_tx_as_undeployment()