Fix: deploy program on last slot of epoch during environment change #101

pgarg66 · 2024-03-06T01:05:49Z

Problem

On runtime environment change, the program deployed in the last slot of the prior epoch will use incorrect environment. This will also prevent the loading of the program in future epochs with the correct runtime environment.

Summary of Changes

The problem happens because the effective slot for the deployed program is in the next epoch, where the new environment becomes effective.
This change updates the program deployment procedure, to use the environment of the epoch where the program actually becomes effective.

Fixes #

alessandrod

A few minor nits but otherwise looks good to me.

The logic in this PR hinges on the fact that LoadedPrograms::upcoming_environments is always set correctly. That field is set here https://github.com/pgarg66/solana/blob/9044bb8f08a0ade2d1e42ca1ebd9fabf2757506c/runtime/src/bank.rs#L1379. Whenever a bank for any slot within slots_in_recompilation_phase from the epoch end is created, if upcoming_environments isn't already set, it will be set. So even in the case of skipped slots, by the time we get to tx execution for a given bank, upcoming_environments is guaranteed to be set correctly.

program-runtime/src/invoke_context.rs

program-runtime/src/loaded_programs.rs

alessandrod · 2024-03-06T05:48:50Z

program-runtime/src/loaded_programs.rs

            hit_max_limit: false,
        }
    }

+    /// Returns the current environments depending on the given epoch
+    pub fn get_environments_for_epoch(&self, epoch: Epoch) -> &ProgramRuntimeEnvironments {
+        if epoch != self.latest_root_epoch {


This works, but I think > instead of != would make the logic clearer (although I know that in other places we use !=, so feel free to leave != if you prefer to be consistent)

I would prefer to leave it as is for now. I am not 100% sure why we have != check in the other function. Just want to make sure we don't oversee some actual condition.

I think I chose == and != over <= and > to avoid off-by-one-errors such as writing < instead of <=. That is all, don't see a semantic reason why we need to stick with ==.

alessandrod · 2024-03-06T05:51:29Z

program-runtime/src/loaded_programs.rs

+        &self,
+        epoch: Epoch,
+    ) -> Option<ProgramRuntimeEnvironments> {
+        if epoch == self.latest_root_epoch {


Same here, I'd prefer <=

I would prefer to leave it as is for now. I am not 100% sure why we have != check in the other function. Just want to make sure we don't oversee some actual condition.

programs/bpf_loader/src/lib.rs

programs/bpf_loader/src/syscalls/mod.rs

codecov-commenter · 2024-03-06T18:52:23Z

Codecov Report

Attention: Patch coverage is 90.10989% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (4a67cd4) to head (42bb424).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master     #101     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         838      838             
  Lines      226929   227066    +137     
=========================================
+ Hits       185924   186024    +100     
- Misses      41005    41042     +37

pgarg66 · 2024-03-06T19:55:55Z

@Lichtso FYI.

program-runtime/src/invoke_context.rs

pgarg66 · 2024-03-19T23:31:48Z

@Lichtso, @alessandrod could you help review this?

Lichtso · 2024-03-20T16:05:55Z

programs/bpf_loader/src/syscalls/mod.rs

@@ -239,6 +239,24 @@ macro_rules! register_feature_gated_function {
    };
 }

+pub fn morph_into_deployment_environment_v1(
+    from: Arc<BuiltinProgram<InvokeContext>>,
+) -> Result<BuiltinProgram<InvokeContext>, Error> {


We should probably do this in RBPF instead and have a set_config or a copy constructor there.

Do you want to push an update to rBPF crate for this?
We also need to backport it to v1.18. So maybe leave this here for time being, and remove it from master once rBPF change is available?

We can remove it from master in the next RBPF release.

runtime/src/bank.rs

mergify · 2024-03-22T14:43:03Z

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

…101) * Fix: deploy program on last slot of epoch during environment change * solana-runtime: deploy at last epoch slot test * disable deployment of sol_alloc_free * Move tx-batch-constructor to its own function * use new_from_cache --------- Co-authored-by: Alessandro Decina <[email protected]> (cherry picked from commit 91b1ee3) # Conflicts: # ledger-tool/src/program.rs # runtime/src/bank/tests.rs # svm/src/transaction_processor.rs

…101) * Fix: deploy program on last slot of epoch during environment change * solana-runtime: deploy at last epoch slot test * disable deployment of sol_alloc_free * Move tx-batch-constructor to its own function * use new_from_cache --------- Co-authored-by: Alessandro Decina <[email protected]> (cherry picked from commit 91b1ee3)

…hange (backport of #101) (#387)

…hange (backport of anza-xyz#101) (anza-xyz#387)

alessandrod reviewed Mar 6, 2024

View reviewed changes

programs/bpf_loader/src/syscalls/mod.rs Outdated Show resolved Hide resolved

pgarg66 force-pushed the last-slot-deploy branch 4 times, most recently from 6150a73 to 10549ea Compare March 6, 2024 17:21

pgarg66 marked this pull request as ready for review March 6, 2024 19:55

pgarg66 requested a review from alessandrod March 6, 2024 19:55

pgarg66 force-pushed the last-slot-deploy branch from 239f348 to ff5995a Compare March 9, 2024 00:29

t-nelson reviewed Mar 9, 2024

View reviewed changes

program-runtime/src/invoke_context.rs Show resolved Hide resolved

pgarg66 force-pushed the last-slot-deploy branch from 155cb95 to 1e53616 Compare March 12, 2024 19:53

pgarg66 requested a review from Lichtso March 15, 2024 14:56

pgarg66 force-pushed the last-slot-deploy branch from 1e53616 to 3f994fe Compare March 18, 2024 13:35

Lichtso reviewed Mar 20, 2024

View reviewed changes

runtime/src/bank.rs Outdated Show resolved Hide resolved

pgarg66 force-pushed the last-slot-deploy branch from 3f994fe to 7de6428 Compare March 20, 2024 16:47

pgarg66 requested a review from Lichtso March 20, 2024 16:49

pgarg66 force-pushed the last-slot-deploy branch from 448b611 to 2655981 Compare March 20, 2024 20:19

pgarg66 and others added 9 commits March 21, 2024 05:42

Fix: deploy program on last slot of epoch during environment change

17f94d7

solana-runtime: deploy at last epoch slot test

bf97ccd

fix tests

2db2892

disable deployment of sol_alloc_free

e438f8d

address review comments

edc773e

address review comments

0744297

Move tx-batch-constructor to its own function

2796df2

fix imports

04768d0

use new_from_cache

6b09337

pgarg66 force-pushed the last-slot-deploy branch from 42bb424 to 6b09337 Compare March 21, 2024 12:46

Lichtso reviewed Mar 22, 2024

View reviewed changes

runtime/src/bank.rs Show resolved Hide resolved

Lichtso approved these changes Mar 22, 2024

View reviewed changes

pgarg66 added the v1.18 label Mar 22, 2024

pgarg66 merged commit 91b1ee3 into anza-xyz:master Mar 22, 2024
38 checks passed

pgarg66 deleted the last-slot-deploy branch March 22, 2024 14:43

mergify bot mentioned this pull request Mar 22, 2024

v1.18: Fix: deploy program on last slot of epoch during environment change (backport of #101) #387

Merged

pgarg66 pushed a commit that referenced this pull request Mar 22, 2024

v1.18: Fix: deploy program on last slot of epoch during environment c…

85f6c36

…hange (backport of #101) (#387)

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore #475

Closed

anwayde pushed a commit to firedancer-io/agave that referenced this pull request Jul 23, 2024

v1.18: Fix: deploy program on last slot of epoch during environment c…

d754789

…hange (backport of anza-xyz#101) (anza-xyz#387)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: deploy program on last slot of epoch during environment change #101

Fix: deploy program on last slot of epoch during environment change #101

pgarg66 commented Mar 6, 2024

alessandrod left a comment

alessandrod Mar 6, 2024

pgarg66 Mar 6, 2024

Lichtso Mar 20, 2024

alessandrod Mar 6, 2024

pgarg66 Mar 6, 2024

codecov-commenter commented Mar 6, 2024 •

edited

Loading

pgarg66 commented Mar 6, 2024

pgarg66 commented Mar 19, 2024

Lichtso Mar 20, 2024

pgarg66 Mar 20, 2024

Lichtso Mar 20, 2024

mergify bot commented Mar 22, 2024

Fix: deploy program on last slot of epoch during environment change #101

Fix: deploy program on last slot of epoch during environment change #101

Conversation

pgarg66 commented Mar 6, 2024

Problem

Summary of Changes

alessandrod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 6, 2024 • edited Loading

Codecov Report

pgarg66 commented Mar 6, 2024

pgarg66 commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Mar 22, 2024

codecov-commenter commented Mar 6, 2024 •

edited

Loading