Skip adding builtins if they will be removed #23233

jstarry · 2022-02-18T11:17:22Z

Problem

When loading from a snapshot, a validator adds and removes builtins and precompiles according to which runtime features are activated. Also, it's important to note that builtin additions are always processed before removals. So after a builtin has been turned into a precompile account on a cluster, the validator will still try to re-add it whenever it starts up from a snapshot. When the builtin is re-added while starting from a snapshot, the builtin account is checked to see whether it is owned by the native loader (precompiles are system-owned) and if not, the validator will hard exit because it thinks it booted from an incompatible snapshot.

Summary of Changes

Add additional types for feature activated transitions to the list of active builtin programs
Ensure that any removed builtin references an existing builtin that is retained if the removal feature hasn't been activated yet

Fixes #23231

ryoqun · 2022-02-18T14:43:52Z

ci/test-checks.sh

@@ -44,6 +44,8 @@ echo --- build environment
 export RUST_BACKTRACE=1
 export RUSTFLAGS="-D warnings -A incomplete_features"

+_ "$cargo" stable clean


is this intended to be included in this pr? i think this increase the build time sometimes needlessly by removing the build cache too much?

It was my only way to get past a compiler issue on CI. I will remove before merging. I mentioned it in #devops on Discord as well

jstarry · 2022-02-18T14:47:10Z

Oops I still need to fix the Abi changes. But the logic of the PR should still be reviewable in its current form

ryoqun · 2022-02-18T14:51:01Z

runtime/src/builtins.rs

+    RemoveOrRetain {
+        previous_builtin: Builtin,
+        removal_feature_id: Pubkey,
+    },


nits: how about commenting about the lack of the obvious Remove like this?:

// historically, we had Remove to fully remove a builtin. however there's no use case for now. restore it if needed.

In case you didn't see this from the linked issue. This is why we need the remove and retain:

For v1.8 compatibility, we need to be able to keep adding a builtin until a feature is activated.

For v1.9 compatibility, we need to avoid adding a builtin if a feature is already activated.

also, I'm omitting Replace as it's really useless now. That's because builtins now has access to feature_set. Replace was relic before the widespread use of feature activation mechanism...

lastly, i wonder when Remove would be needed in the (distant) future. so, I'm just saying leaving a comment would be just enough to work as a pointer to this pr in the future.

Yes, that's exactly why I removed Replace. Relic of the past and not useful anymore.

Great yeah I will add a comment, I agree it probably won't be needed in the future

ryoqun · 2022-02-18T14:52:32Z

runtime/src/builtins.rs

+        feature_id: Pubkey,
+    },
+    /// Remove a builtin program if a feature is activated or
+    /// retain a previously added builtin.


nit: /// Specifically, this is needed for rather the exceptional builtin -> precompile transition feature

ryoqun · 2022-02-18T15:53:07Z

runtime/src/bank/builtin_programs.rs

+    };
+
+    #[test]
+    fn test_startup_from_snapshot_after_precompile_transition() {


thanks for writing this. yeah, feature activation needs more test coverage...

ryoqun

lgtm with nits and after ci passes. :)

jackcmay · 2022-02-18T16:40:50Z

runtime/src/builtins.rs

-            ActivationType::NewProgram,
-        ),
+            feature_id: feature_set::add_compute_budget_program::id(),
+        },
        // TODO when feature `prevent_calling_precompiles_as_programs` is


These comments can be removed

Pull request has been modified.

jackcmay · 2022-02-18T17:31:19Z

Looks good @jstarry thanks!

On main and devnet all existing builtin and precompile accounts are owned by the native loader. I added a change to this pr to keep that consistency.

Also added a few nits

t-nelson

lgtm. thanks! third times the charm

t-nelson · 2022-02-18T18:16:31Z

runtime/src/builtins.rs

@@ -51,63 +51,6 @@ macro_rules! with_program_logging {
    };
 }

-/// State transition enum used for adding and removing builtin programs through


This block and the next are purely cosmetic, right? Hard to follow major cosmetics in a functional change set

jstarry · 2022-02-18T18:51:55Z

Cool, I would prefer the owner fix to be separate but looks safe

Pull request has been modified.

codecov · 2022-02-19T01:27:29Z

Codecov Report

Merging #23233 (1a91f9e) into master (c696944) will decrease coverage by 0.0%.
The diff coverage is 88.8%.

@@            Coverage Diff            @@
##           master   #23233     +/-   ##
=========================================
- Coverage    81.3%    81.3%   -0.1%     
=========================================
  Files         570      571      +1     
  Lines      155438   155451     +13     
=========================================
+ Hits       126452   126454      +2     
- Misses      28986    28997     +11

t-nelson · 2022-02-19T02:27:58Z

let's go?

* Add failing test for precompile transition * Skip adding builtins if they will be removed * cargo clean * nits * fix abi check * remove workaround Co-authored-by: Jack May <[email protected]> (cherry picked from commit 1719d23) # Conflicts: # runtime/src/bank.rs # runtime/src/builtins.rs

* Skip adding builtins if they will be removed (#23233) * Add failing test for precompile transition * Skip adding builtins if they will be removed * cargo clean * nits * fix abi check * remove workaround Co-authored-by: Jack May <[email protected]> (cherry picked from commit 1719d23) # Conflicts: # runtime/src/bank.rs # runtime/src/builtins.rs * resolve conflicts Co-authored-by: Justin Starry <[email protected]> Co-authored-by: Jack May <[email protected]>

jstarry · 2022-02-21T12:01:17Z

runtime/src/builtins.rs

+                } else {
+                    // Retaining is no different from adding a new builtin.
+                    Some(BuiltinAction::Add(previous_builtin.clone()))
+                }


I believe this branch introduced a new bug for epoch boundaries. On epoch boundaries, the should_apply_action_for_feature(removal_feature_id) condition will be false if the removal feature id wasn't newly activated. This causes the builtin to be re-added even if it was previously removed.

* Add failing test for precompile transition * Skip adding builtins if they will be removed * cargo clean * nits * fix abi check * remove workaround Co-authored-by: Jack May <[email protected]>

jstarry mentioned this pull request Feb 18, 2022

Fix ed25519 program owner on testnet #23219

Closed

jstarry added 2 commits February 18, 2022 20:50

Add failing test for precompile transition

a753d28

Skip adding builtins if they will be removed

9efe5c1

jstarry force-pushed the fix-builtin-deactivation branch from a525b16 to 9efe5c1 Compare February 18, 2022 12:56

jstarry requested review from jackcmay and ryoqun February 18, 2022 12:56

jstarry added the v1.9 label Feb 18, 2022

cargo clean

fcc54fa

ryoqun reviewed Feb 18, 2022

View reviewed changes

ryoqun previously approved these changes Feb 18, 2022

View reviewed changes

jackcmay reviewed Feb 18, 2022

View reviewed changes

jackcmay force-pushed the fix-builtin-deactivation branch from 3578333 to 63d1c3e Compare February 18, 2022 18:00

t-nelson previously approved these changes Feb 18, 2022

View reviewed changes

t-nelson mentioned this pull request Feb 18, 2022

Fix ed25519 builtin program handling #23182

Merged

jackcmay added 2 commits February 18, 2022 12:24

nits

a5964db

fix abi check

bd40fa4

jackcmay force-pushed the fix-builtin-deactivation branch from 63d1c3e to bd40fa4 Compare February 18, 2022 21:09

remove workaround

1a91f9e

jackcmay force-pushed the fix-builtin-deactivation branch from 8a93aea to 1a91f9e Compare February 18, 2022 22:49

jackcmay merged commit 1719d23 into solana-labs:master Feb 19, 2022

mergify bot mentioned this pull request Feb 19, 2022

Skip adding builtins if they will be removed (backport #23233) #23241

Merged

jstarry mentioned this pull request Feb 21, 2022

Possible (to be confirmed) Consensus (bank hash) mismatch with v1.9.8 #23253

Closed

jstarry commented Feb 21, 2022

View reviewed changes

jstarry mentioned this pull request Feb 21, 2022

Fix builtin handling on epoch boundaries #23256

Merged

jstarry deleted the fix-builtin-deactivation branch February 22, 2022 03:22

t-nelson mentioned this pull request Feb 23, 2022

Hack fix for ICE as seen in CI #23295

Merged

brooksprumo mentioned this pull request Jun 21, 2022

Remove auto traits from ICE work-around #26098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip adding builtins if they will be removed #23233

Skip adding builtins if they will be removed #23233

jstarry commented Feb 18, 2022 •

edited

Loading

ryoqun Feb 18, 2022

jstarry Feb 18, 2022

jstarry commented Feb 18, 2022

ryoqun Feb 18, 2022

jstarry Feb 18, 2022

ryoqun Feb 18, 2022

ryoqun Feb 18, 2022 •

edited

Loading

jstarry Feb 18, 2022

jstarry Feb 18, 2022

ryoqun Feb 18, 2022

ryoqun Feb 18, 2022

ryoqun left a comment

jackcmay Feb 18, 2022

jackcmay commented Feb 18, 2022

t-nelson left a comment

t-nelson Feb 18, 2022

jstarry commented Feb 18, 2022

codecov bot commented Feb 19, 2022

t-nelson commented Feb 19, 2022

jstarry Feb 21, 2022

Skip adding builtins if they will be removed #23233

Skip adding builtins if they will be removed #23233

Conversation

jstarry commented Feb 18, 2022 • edited Loading

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry commented Feb 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun Feb 18, 2022 • edited Loading

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

ryoqun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackcmay commented Feb 18, 2022

t-nelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry commented Feb 18, 2022

codecov bot commented Feb 19, 2022

Codecov Report

t-nelson commented Feb 19, 2022

Choose a reason for hiding this comment

jstarry commented Feb 18, 2022 •

edited

Loading

ryoqun Feb 18, 2022 •

edited

Loading