Deprecate account meta executable read/update in bpf loaders #34194

HaoranYi · 2023-11-21T15:24:41Z

Problem

Part of effort for #33970

We are deprecating executable meta flags on account. Instead of relying on executable flag on account to determine whether the account is executable, we are going to check the owner of the account and the loader's meta data in account data to determine whether the account is executable.

Summary of Changes

use owner + metadata in accounts data to determine whether the account is executable in transaction loading/processing.
add disable_bpf_loader_instructions feature to disable bpf loader management instructions,
add deprecate_executable_meta_update_in_bpf_loader feature to deprecate executable meta update in bpf loader

Feature Gate Issues:

34424
34425

SIMD

Fixes #

runtime/src/accounts/mod.rs

Lichtso · 2023-11-21T15:58:50Z

It just occurred to me that there is one more complicating factor here, the migration of built-in programs to on-chain programs: solana-foundation/solana-improvement-documents#88

So, in the end almost all programs (except for the loaders) will be on-chain and be owned by the loaders. But until then we still have to support built-in programs which also count as "executable" sometimes, but not always. E.g. built-ins don't need to be loaded into the LoadedPrograms cache.

Thus, we might need a boolean parameter to the is_executable() function replacement or two versions:

One for user programs only: Which returns true if the owner is in PROGRAM_OWNERS
One for built-ins: Which also returns true if the owner is native_loader::id()

Related code sites:

solana/programs/bpf_loader/src/lib.rs

Line 430 in 481c357

if native_loader::check_id(owner_id) {
solana/programs/bpf_loader/src/syscalls/cpi.rs

Line 1037 in 481c357

if native_loader::check_id(program_id)
solana/program-runtime/src/invoke_context.rs

Line 458 in 481c357

if native_loader::check_id(owner_id) {

sdk/src/account.rs

Lichtso · 2023-11-22T15:10:21Z

runtime/src/accounts/mod.rs

-            loaded_accounts[0],
-            (Err(TransactionError::InvalidProgramForExecution), None,)
-        );
+        assert!(loaded_accounts[0].0.is_ok());


This seems to be an un-feature-gated change to consensus.

yeah. this needs to be feature-gated.

Actually, I would prefer if we would find a way to do this refactoring transparently, without consensus changes.

ok. let me think about it.

Another question:

https://github.com/solana-labs/solana/blob/master/sdk/src/transaction_context.rs#L1082

With the change of is_executable() on BorrowedAccount, this seems not correct any more? upgradable account can_data_be_changed?

@@ -1079,7 +1081,9 @@ impl<'a> BorrowedAccount<'a> { #[cfg(not(target_os = "solana"))] pub fn can_data_be_changed(&self) -> Result<(), InstructionError> { // Only non-executable accounts data can be changed - if self.is_executable() { + if self.is_executable() + && !solana_program::bpf_loader_upgradeable::check_id(self.account.owner()) + { return Err(InstructionError::ExecutableDataModified); }

HaoranYi · 2023-11-22T19:52:35Z

The following are the behavior changes with this PR found in test

if program owner changes to non bpf-loader, while its executable flags are
set. IncorrectProgramId --> AccountNotExecutable
if program data is unitialized, while its executable flags are
set. InvalidAccountData --> AccountNotExecutable
for account, whose owner is bpf_loader, while its executable flags are set.
running LoaderInstruction::write, Ok --> ExecutableDataModified

@Lichtso Can we review them to be sure that these scenarios are not possible practically?

Lichtso · 2023-11-22T21:05:58Z

Such a test should be deleted because transferring the ownership of an executable account is explicitly forbidden:

solana/sdk/src/transaction_context.rs

Line 752 in 53c723a

if self.is_executable() {
Again, makes no sense. Once an account is marked as executable it becomes immutable (

solana/sdk/src/transaction_context.rs

Line 1082 in 53c723a

if self.is_executable() {

), so it could never leave the uninitialized state. Thus, the executable flag is only set after the state is initialized.
Uff, this will actually be a problem. The first two loaders did not write any metadata into the program account, so the only way to distinguish the writing-in-progress from the finalized state is the executable flag. Obviously, that can not be emulated without the executable flag so we do need a feature gate here. Also, we will probably have to disable deployments for everything but the upgradeable loader (v3), which was planned anyways.

HaoranYi · 2023-11-22T21:31:52Z

Makes sense. I will update.

…

On Wed, Nov 22, 2023, 15:06 Alexander Meißner ***@***.***> wrote: 1. Such a test should be deleted because transferring the ownership of an executable account is explicitly forbidden: https://github.com/solana-labs/solana/blob/53c723ae3f6b3013b9bfe275ea683ef2088737b3/sdk/src/transaction_context.rs#L752 2. Again, makes no sense. Once an account is marked as executable it becomes immutable ( https://github.com/solana-labs/solana/blob/53c723ae3f6b3013b9bfe275ea683ef2088737b3/sdk/src/transaction_context.rs#L1082), so it could never leave the uninitialized state. Thus, the executable flag is only set after the state is initialized. 3. Uff, this will actually be a problem. The first two loaders did not write any metadata into the program account, so the only way to distinguish the writing-in-progress from the finalized state is the executable flag. Obviously, that can not be emulated without the executable flag so we do need a feature gate here. Also, we will probably have to disable deployments for everything but the upgradeable loader (v3), which was planned anyways. — Reply to this email directly, view it on GitHub <#34194 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABVSJCHCDJEAEVIBLUIZYDYFZSMFAVCNFSM6AAAAAA7UYRPOOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRTGUYDEMBQGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

HaoranYi · 2023-11-27T17:43:39Z

I have tried to add a feature gate for executable flag. But it turns out that
leads to quite a few API changesfor Account and BorrowedAccount, which is
not very clean. I think we may want to avoid adding a feature flag. Instead,
can we partially disable usage of executable flag for the loaders? i.e. for bpf_loader_upgradeable
and loader_v4, we disable the usage of executable flag, while for
'bpf_loader' and 'bpf_loader_deprecated', we still use executable flags. When
we disable deployments for these two loaders, we can remove the code that use
executable flags for them? Wdyt? @Lichtso

codecov · 2023-11-28T05:53:06Z

Codecov Report

Attention: 43 lines in your changes are missing coverage. Please review.

Comparison is base (d6146af) 81.8% compared to head (d59193c) 81.8%.
Report is 10 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #34194    +/-   ##
========================================
  Coverage    81.8%    81.8%            
========================================
  Files         824      824            
  Lines      222262   222393   +131     
========================================
+ Hits       181860   182065   +205     
+ Misses      40402    40328    -74

HaoranYi · 2023-11-30T01:43:47Z

https://explorer.solana.com/tx/4UzC7fuDiB5spFJmb1tBd6b44zvMYNsbHNBzFnawW5PM1z9ZzmfMpAagvbjuMxSPUHuhHqbKzWzgxrtYDezL18mA

Fails because BPFUpgradableLoader was hijacked by fn account_shared_data_from_program, which creates a dummy account with empty data. That's why it fails with this PR.

aaf2748 fixed it.

I have deploy this PR again on my node again to run against mnt overnight.

runtime/src/accounts/mod.rs

sdk/src/transaction_context.rs

programs/bpf_loader/src/lib.rs

sdk/src/account.rs

HaoranYi · 2023-12-04T15:08:50Z

FYI. A node with this change has been running for the past 5 days against mainnet! So far, it looks good!

HaoranYi · 2023-12-15T15:13:54Z

I added two featuregates in this PR:
a) disable bpfloader v1 deployment instructions
b) skip updating executable during bpf program deployment.

Aso, I created two SIMDs for these two features.

@Lichtso @pgarg66 @brooksprumo Please take a look. Let me know if you have any more comments. Thanks!

Lichtso · 2023-12-18T20:54:07Z

It seems that deprecate_executable_meta_update_in_bpf_loader only gates one call site of set_executable(), not all of them. Also, the switch from using the account executable flag to the inferred one in is_executable() needs to be feature gated (can be done inside is_executable() with deprecate_executable_meta_update_in_bpf_loader) because the deprecated loader can not be emulated (thus we need to disable its deployment with disable_bpf_loader_instructions beforehand).

Lichtso · 2023-12-18T23:36:58Z

sdk/src/account.rs

+    // the account. Once we disable the deployment of these two loaders, we can
+    // remove the following dependency on `executable`.
+    if bpf_loader::check_id(account.owner()) || bpf_loader_deprecated::check_id(account.owner()) {
+        return account.executable();


return true;
That will be safe once deployments on loader-v2 are disabled. And we don't switch to this emulation here before that feature is activated.

Lichtso · 2023-12-20T11:55:25Z

@HaoranYi Can you split off the changes where you wire in feature_set in all the methods of BorrowedAccount into a separate PR? Otherwise I would say this looks good, can you run it on a validator against MNB?

HaoranYi · 2023-12-20T15:52:54Z

@HaoranYi Can you split off the changes where you wire in feature_set in all the methods of BorrowedAccount into a separate PR? Otherwise I would say this looks good, can you run it on a validator against MNB?

Yeah. #34542 is the refactor PR that pass feature_set into BorrowedAccount.
I just started a validator on my node, will keep it running.

HaoranYi · 2024-01-02T15:10:25Z

@HaoranYi Can you split off the changes where you wire in feature_set in all the methods of BorrowedAccount into a separate PR? Otherwise I would say this looks good, can you run it on a validator against MNB?

Yeah. #34542 is the refactor PR that pass feature_set into BorrowedAccount. I just started a validator on my node, will keep it running.

My node has been running fine with the PR over the holidays.

Lichtso · 2024-01-03T13:31:05Z

programs/bpf_loader/src/lib.rs

@@ -1818,7 +1848,7 @@ mod tests {
            Err(InstructionError::NotEnoughAccountKeys),
        );



For all the tests of loader-v2 it would be preferable to just force deactivate the feature instead of altering the tests.

yeah. restored.

Lichtso

After loader-v2 is disabled its management instructions will be removed. Thus, we need to rework the tests in programs/sbf/tests/programs.rs to use loader-v3 instead. However, that can happen in a separate PR.

mock account data with executable_meta in precompiled program and update test_bank_hash_consistency test pr: return const slice and add comments pr: use ReadableAccount use const to get rid of magic number add featuregate disable_bpf_loader_instructions to disable bpf loader management instructions, and deprecate_executable_meta_update_in_bpf_loader to deprecate executable flag update in bpf loader deprecate usage of executable in Account fix a test fix sbp bench fix sbf program tests add feature gate to account and borrowed account apis fix tests more test fixes

t-nelson · 2024-02-05T22:52:00Z

please DO NOT introduce multiple feature gates in the same pull request!

HaoranYi changed the title ~~nov 21 exec flag2~~ use PROGRAM_OWNER to check executable on account when loading transaction accounts Nov 21, 2023

HaoranYi requested review from jeffwashington and Lichtso November 21, 2023 15:47

Lichtso reviewed Nov 21, 2023

View reviewed changes

runtime/src/accounts/mod.rs Outdated Show resolved Hide resolved

HaoranYi force-pushed the nov_21_exec_flag2 branch from 0985839 to e4c0c13 Compare November 21, 2023 15:54

HaoranYi force-pushed the nov_21_exec_flag2 branch 3 times, most recently from 1194285 to 462fb24 Compare November 21, 2023 22:37

Lichtso reviewed Nov 22, 2023

View reviewed changes

sdk/src/account.rs Show resolved Hide resolved

HaoranYi force-pushed the nov_21_exec_flag2 branch from 87a5450 to bbfa8c5 Compare November 22, 2023 14:46

Lichtso reviewed Nov 22, 2023

View reviewed changes

HaoranYi force-pushed the nov_21_exec_flag2 branch from 3b58901 to 2c1f47a Compare November 27, 2023 21:16

HaoranYi requested a review from brooksprumo November 28, 2023 19:59

HaoranYi force-pushed the nov_21_exec_flag2 branch 5 times, most recently from df9d94c to 954848a Compare November 30, 2023 23:25

HaoranYi requested a review from pgarg66 November 30, 2023 23:32

brooksprumo reviewed Dec 1, 2023

View reviewed changes

HaoranYi changed the title ~~use PROGRAM_OWNER to check executable on account when loading transaction accounts~~ use owner+meta_data to check executable on account when loading transaction accounts Dec 1, 2023

pgarg66 reviewed Dec 1, 2023

View reviewed changes

sdk/src/account.rs Outdated Show resolved Hide resolved

HaoranYi changed the title ~~use owner+meta_data to check executable on account when loading transaction accounts~~ Deprecate account meta executable read/update in bpf loaders Dec 12, 2023

HaoranYi force-pushed the nov_21_exec_flag2 branch from 84e2791 to aedde94 Compare December 15, 2023 14:41

brooksprumo mentioned this pull request Dec 18, 2023

Implements ReadableAccount for RefMut Account and AccountSharedData #34370

Closed

Lichtso reviewed Dec 18, 2023

View reviewed changes

HaoranYi force-pushed the nov_21_exec_flag2 branch 2 times, most recently from 8872bf2 to 8272151 Compare December 19, 2023 15:25

HaoranYi force-pushed the nov_21_exec_flag2 branch from 3d1ec85 to a84f34b Compare December 20, 2023 14:59

HaoranYi mentioned this pull request Dec 20, 2023

pass feature_set to BorrowedAccount #34542

Merged

HaoranYi force-pushed the nov_21_exec_flag2 branch from a84f34b to 3bdb51d Compare December 22, 2023 15:07

Lichtso reviewed Jan 3, 2024

View reviewed changes

Lichtso previously approved these changes Jan 3, 2024

View reviewed changes

HaoranYi added 2 commits January 3, 2024 18:06

restore bpf_loader v2 tests

d59193c

HaoranYi dismissed Lichtso’s stale review via d59193c January 3, 2024 18:06

HaoranYi force-pushed the nov_21_exec_flag2 branch from 68cd6b9 to d59193c Compare January 3, 2024 18:06

Lichtso approved these changes Jan 3, 2024

View reviewed changes

pgarg66 approved these changes Jan 3, 2024

View reviewed changes

HaoranYi merged commit 5a3a10e into solana-labs:master Jan 3, 2024
45 checks passed

HaoranYi deleted the nov_21_exec_flag2 branch January 3, 2024 21:12

HaoranYi mentioned this pull request Jan 8, 2024

Upgrade sbf tests to use bpf loader v3 #34691

Merged

acheroncrypto mentioned this pull request Jan 29, 2024

Feature Gate: Deprecate executable meta flag update in bpf loader #34425

Open

Lichtso mentioned this pull request Feb 5, 2024

Backport - feature disable_bpf_loader_instructions to v1.17 #35093

Merged

acheroncrypto mentioned this pull request Mar 26, 2024

test-validator: Cloned upgradeable programs don't work anza-xyz/agave#436

Closed

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore anza-xyz/agave#475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate account meta executable read/update in bpf loaders #34194

Deprecate account meta executable read/update in bpf loaders #34194

HaoranYi commented Nov 21, 2023 •

edited

Loading

Lichtso commented Nov 21, 2023 •

edited

Loading

Lichtso Nov 22, 2023

HaoranYi Nov 22, 2023 •

edited

Loading

Lichtso Nov 22, 2023

HaoranYi Nov 22, 2023 •

edited

Loading

HaoranYi commented Nov 22, 2023

Lichtso commented Nov 22, 2023

HaoranYi commented Nov 22, 2023 via email

HaoranYi commented Nov 27, 2023

codecov bot commented Nov 28, 2023 •

edited

Loading

HaoranYi commented Nov 30, 2023

HaoranYi commented Dec 4, 2023 •

edited

Loading

HaoranYi commented Dec 15, 2023 •

edited

Loading

Lichtso commented Dec 18, 2023 •

edited

Loading

Lichtso Dec 18, 2023

Lichtso commented Dec 20, 2023

HaoranYi commented Dec 20, 2023

HaoranYi commented Jan 2, 2024

Lichtso Jan 3, 2024

HaoranYi Jan 3, 2024

Lichtso left a comment •

edited

Loading

t-nelson commented Feb 5, 2024

		@@ -1818,7 +1848,7 @@ mod tests {
		Err(InstructionError::NotEnoughAccountKeys),
		);

Deprecate account meta executable read/update in bpf loaders #34194

Deprecate account meta executable read/update in bpf loaders #34194

Conversation

HaoranYi commented Nov 21, 2023 • edited Loading

Problem

Summary of Changes

Feature Gate Issues:

SIMD

Lichtso commented Nov 21, 2023 • edited Loading

Lichtso Nov 22, 2023

Choose a reason for hiding this comment

HaoranYi Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Lichtso Nov 22, 2023

Choose a reason for hiding this comment

HaoranYi Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

HaoranYi commented Nov 22, 2023

Lichtso commented Nov 22, 2023

HaoranYi commented Nov 22, 2023 via email

HaoranYi commented Nov 27, 2023

codecov bot commented Nov 28, 2023 • edited Loading

Codecov Report

HaoranYi commented Nov 30, 2023

HaoranYi commented Dec 4, 2023 • edited Loading

HaoranYi commented Dec 15, 2023 • edited Loading

Lichtso commented Dec 18, 2023 • edited Loading

Lichtso Dec 18, 2023

Choose a reason for hiding this comment

Lichtso commented Dec 20, 2023

HaoranYi commented Dec 20, 2023

HaoranYi commented Jan 2, 2024

Lichtso Jan 3, 2024

Choose a reason for hiding this comment

HaoranYi Jan 3, 2024

Choose a reason for hiding this comment

Lichtso left a comment • edited Loading

Choose a reason for hiding this comment

t-nelson commented Feb 5, 2024

HaoranYi commented Nov 21, 2023 •

edited

Loading

Lichtso commented Nov 21, 2023 •

edited

Loading

HaoranYi Nov 22, 2023 •

edited

Loading

HaoranYi Nov 22, 2023 •

edited

Loading

codecov bot commented Nov 28, 2023 •

edited

Loading

HaoranYi commented Dec 4, 2023 •

edited

Loading

HaoranYi commented Dec 15, 2023 •

edited

Loading

Lichtso commented Dec 18, 2023 •

edited

Loading

Lichtso left a comment •

edited

Loading