Switch programs activation to whole-set based gating#11736
Switch programs activation to whole-set based gating#11736ryoqun wants to merge 6 commits intosolana-labs:masterfrom
Conversation
| Some(vec
There was a problem hiding this comment.
Yeah, @mvines indicated that it was enabled on a branch at some point for mainnet and therefore now part of the ledger
| if !new.fix_recent_blockhashes_sysvar_delay() { | ||
| new.update_recent_blockhashes(); | ||
| } | ||
| dbg!(&new.message_processor); |
|
|
||
| // This is called from snapshot restore and for each epoch boundary | ||
| // The entire code path herein must be idempotent | ||
| pub fn refresh_programs_and_inflation(&mut self) { |
There was a problem hiding this comment.
NIT: I have a feeling this name won't age well. Maybe something like apply_feature_activations() ?
|
out of scope of this pr todos:
|
| Some(vec![Program::BuiltinLoader(solana_bpf_loader_program!())]) | ||
| } else { | ||
| None | ||
| programs.extend(vec![Program::BuiltinLoader(solana_bpf_loader_program!())]); |
There was a problem hiding this comment.
Is this added to the list twice?
| OperatingMode::Preview => { | ||
| if epoch == std::u64::MAX { | ||
| OperatingMode::Stable => { | ||
| // at which epoch, bpf_loader_program is enabled?? |
There was a problem hiding this comment.
Looks like the original loader was enabled in epoch 34
solana/genesis-programs/src/lib.rs
Line 74 in 86419df
@mvines Does this jive with what you would expect?
There was a problem hiding this comment.
@ryoqun I pushed changes to this to solana/fragile-programs-gating
| native_programs | ||
| } | ||
|
|
||
| fn recheck_cross_program_support(bank: &mut Bank) { |
There was a problem hiding this comment.
Why not leave this logic in the bank?
|
@ryoqun I'm not too familiar with the snapshot test coverage, can we add tests to recreate the scenario we ran into last night? |
Specifically, newer snapshot restoration code is now using |
| } | ||
|
|
||
| for program in get_builtins(self.operating_mode(), self.epoch()) { | ||
| self.add_builtin(&program.name, program.id, program.entrypoint); |
There was a problem hiding this comment.
Adding the same programs at every epoch is going to fail because the account already exists. Looks like the bank hash ci failure is related, looking into it.
| if OperatingMode::Stable == operating_mode { | ||
| bank.set_cross_program_support(bank.epoch() >= 63); | ||
| } else { | ||
| bank.set_cross_program_support(true); | ||
| } |
3 participants
Problem
get_entered_epoch_callbackisn't called when restoring from snapshots, which is too confusing and error-prone.Also, we can't simply call it immediately after snapshot restoration because it expects to be called exactly once at each epoch boundary
get_programsandget_builtinsreturns delta set. i.e. add these new additions of programs to the current available set at the given epoch. This works nicely in the ideal world, where we're running the validator since genesis without ever restarting a perfect bug-free validator.In reality, we must rely on snapshots 99.999% of time. When restoring from snapshots, the delta set doesn't work quite: we don't persist the current available set (namely
bank.message_processoris effectivelyserde(skip)).Summary of Changes
So, just reflect the reality by making these functions snapshot-friendly by returning whole-set of available programs at the given epoch. And make it callable from
finish_init(), which is called after snapshot restoration.Also, fix a bunch of other dangerous code along the way.
Also, this is intended to be back-port friendly; so the fix is intentionally not exhaustive. Still,
get_entered_epoch_callbackis a bit error-prone. Specifically, it must be idempotent. (We could solve this by artificially introducing some intermediatestructlikeScheduledBankFeaturesor the like instead of mind-opening way of passing&mut Bank).Fixes #