This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Refactor: process_instruction() #20448

Merged

Lichtso merged 7 commits into solana-labs:master from Lichtso:refactor/process_instruction

Oct 8, 2021

Contributor

Lichtso commented Oct 5, 2021 •

edited

Loading

Problem

The runtime currently uses InvokeContext::remove_first_keyed_account() to pop off the front of the program KeyedAccounts of each invoke stack frame. That necessitates the invoke stack frame to be writable and we want to avoid that in ABIv2 (#19191). Because, if the invoke stack frames are immutable, they can be shared as read-only between runtime, loaders and programs alike with less internal validation checks.

Summary of Changes

Replaces InvokeContext::remove_first_keyed_account() by introducing a num_program_accounts parameter (which is acting as a cursor) to process_instruction().
Also, augments mock ups and tests with the missing beginning of the program chain which was not simulated until now.

Fixes #

vercel bot temporarily deployed to Preview

October 5, 2021 20:21

Inactive

Lichtso force-pushed the refactor/process_instruction branch from b051b76 to a851842 Compare

October 5, 2021 20:31

vercel bot temporarily deployed to Preview

October 5, 2021 20:31

Inactive

digidawww approved these changes

View reviewed changes

Lichtso force-pushed the refactor/process_instruction branch from a851842 to fe09137 Compare

October 6, 2021 08:07

vercel bot temporarily deployed to Preview

October 6, 2021 08:07

Inactive

Lichtso changed the title ~~Refactor: process_Instruction()~~ Refactor: process_instruction()

Lichtso force-pushed the refactor/process_instruction branch from fe09137 to 4e29b54 Compare

October 6, 2021 11:33

vercel bot temporarily deployed to Preview

October 6, 2021 11:33

Inactive

Lichtso force-pushed the refactor/process_instruction branch 2 times, most recently from 17fe3c1 to 05cce99 Compare

October 6, 2021 15:35

vercel bot temporarily deployed to Preview

October 6, 2021 15:35

Inactive

Lichtso force-pushed the refactor/process_instruction branch from 05cce99 to cf86549 Compare

October 6, 2021 17:22

vercel bot temporarily deployed to Preview

October 6, 2021 17:24

Inactive

codecov bot commented Oct 6, 2021 •

edited

Loading

Codecov Report

Merging #20448 (579579a) into master (258c3bc) will decrease coverage by 0.2%.
The diff coverage is 90.2%.

@@            Coverage Diff            @@
##           master   #20448     +/-   ##
=========================================
- Coverage    82.3%    82.1%   -0.3%     
=========================================
  Files         497      492      -5     
  Lines      136959   137083    +124     
=========================================
- Hits       112854   112673    -181     
- Misses      24105    24410    +305

jackcmay reviewed

View reviewed changes

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

jackcmay reviewed

View reviewed changes

programs/bpf_loader/src/lib.rs Outdated Show resolved Hide resolved

jackcmay reviewed

View reviewed changes

programs/bpf_loader/src/lib.rs Outdated

@@ @@ -283,22 +310,22 @@ fn process_loader_upgradeable_instruction( @@
                   match limited_deserialize(instruction_data)? {
                       UpgradeableLoaderInstruction::InitializeBuffer => {
-                          let buffer = keyed_account_at_index(keyed_accounts, 0)?;
+                          let buffer = keyed_account_at_index(keyed_accounts, 1)?;

Contributor

jackcmay Oct 6, 2021

I see the assert above but maybe its clearer to just pass the skip variable through?

jackcmay reviewed

View reviewed changes

programs/bpf_loader/src/lib.rs

@@ @@ -1042,6 +1074,38 @@ mod tests { @@
                       }
                   }
+                  type KeyedAccountTuple<'a> = (bool, bool, &'a Pubkey, &'a RefCell<AccountSharedData>);

Contributor

jackcmay Oct 6, 2021

Any reason this isn't a structure to help with readability?

jackcmay reviewed

View reviewed changes

programs/bpf_loader/src/lib.rs

-                      keyed_accounts[0].account.borrow_mut().set_data(vec![0; 6]);
-                      let mut invoke_context = MockInvokeContext::new(keyed_accounts);
+                      keyed_accounts[0] = (true, false, &program_key, &program_account);
+                      keyed_accounts[0].3.borrow_mut().set_data(vec![0; 6]);

Contributor

jackcmay Oct 6, 2021

tuple indexing, yuck

jackcmay reviewed

View reviewed changes

programs/bpf_loader/src/lib.rs

-                          KeyedAccount::new_readonly(&clock_id, false, &clock_account),
-                          KeyedAccount::new_readonly(
-                              &upgrade_authority_address,
+                      let keyed_accounts: Vec<KeyedAccountTuple> = vec![

Contributor

jackcmay Oct 6, 2021

Why are these now tuples and not KeyedAccounts?

jackcmay reviewed

View reviewed changes

programs/stake/src/stake_instruction.rs Outdated


		let me = &keyed_account_at_index(keyed_accounts, 0)?;

		let me = &keyed_account_at_index(keyed_accounts, 1)?;

Contributor

jackcmay Oct 6, 2021

These magic numbers are confusing and there is not info here to indicate what they are based on. Also, is _skip_program_accounts == 1 here, if so, why not base on that?

Contributor Author

Lichtso Oct 7, 2021 •

edited

Loading

You are right that we should document what keyed_account_at_index() is based / operating on.
Because now, it is always the beginning of the program chain, followed by the instruction accounts.

I also thought about parametrizing keyed_account_at_index() with skip_program_accounts.
However, that would introduce yet another method like keyed_account_at_base_and_index(keyed_accounts, skip_program_accounts, index), or keyed_account_at_index(keyed_accounts, skip_program_accounts + index) which is getting very long and will be short-lived as it is replaced by the ABIv2 account access pattern.

Contributor Author

Lichtso commented Oct 7, 2021

About the ugly KeyedAccountTuple:
Some of the tests already used create_keyed_accounts_unified() and that is the format the parameter expects.

I made all the tests to have the wrapper process_instruction() around super::process_instruction() which adds the missing processor_account in front of the program chain and passes the tuples to create_keyed_accounts_unified().

Now, why switch some of the test to create_keyed_accounts_unified() instead of switching the rest to KeyedAccount::new_***()? Because the distinction between the KeyedAccount struct and Rc<RefCell<AccountSharedData>> will be replaced in ABIv2 by an unified access pattern. But I haven't come up with a nice solution to the create_keyed_accounts_unified() parameter for tests and mock ups yet.

Lichtso force-pushed the refactor/process_instruction branch from cf86549 to d683925 Compare

October 7, 2021 10:59

vercel bot temporarily deployed to Preview

October 7, 2021 10:59

Inactive

Lichtso added 5 commits

October 7, 2021 22:19


          Adds first_instruction_account parameter to process_instruction().

e5d6b95


          Removes InvokeContext::remove_first_keyed_account() from all BPF load…

dad0219

…ers.


          Removes InvokeContext::remove_first_keyed_account() from all builtin …

2c16373

…programs.


          Removes InvokeContext::remove_first_keyed_account() from all mock ups.

22e58ce


          Deprecates InvokeContext::remove_first_keyed_account().

921c64e

Lichtso force-pushed the refactor/process_instruction branch from 91e41da to 61ccc47 Compare

October 7, 2021 20:28

vercel bot temporarily deployed to Preview

October 7, 2021 20:28

Inactive

Lichtso force-pushed the refactor/process_instruction branch from 61ccc47 to 1495ecd Compare

October 7, 2021 22:14

vercel bot temporarily deployed to Preview

October 7, 2021 22:14

Inactive

jackcmay reviewed

View reviewed changes

program-runtime/src/instruction_processor.rs Outdated

-                                      return process_instruction(program_id, instruction_data, invoke_context);
+                                      return process_instruction(
+                                          program_id,
+,

Contributor

jackcmay Oct 7, 2021

Name these or at leat provide a comment describing why they are the value that they are

jackcmay previously approved these changes

View reviewed changes

Contributor

jackcmay left a comment

Look good, just one remaining nit about documenting a couple of magic numbers

vercel bot temporarily deployed to Preview

October 8, 2021 06:19

Inactive

mergify bot dismissed jackcmay’s stale review

October 8, 2021 06:19

Pull request has been modified.

Lichtso added 2 commits

October 8, 2021 08:20


          Documents index base of keyed_account_at_index().

af7a920


          Adds dynamic offset to call sites of "keyed_account_at_index()".

579579a

Lichtso force-pushed the refactor/process_instruction branch from dd1982a to 579579a Compare

October 8, 2021 06:21

vercel bot temporarily deployed to Preview

October 8, 2021 06:21

Inactive

Lichtso merged commit 4e65487 into solana-labs:master

Lichtso deleted the refactor/process_instruction branch

October 8, 2021 09:43

Lichtso mentioned this pull request

ABI v2 Proposal #19191

Closed

jstarry reviewed

View reviewed changes

runtime/src/builtins.rs

                   instruction_data: &[u8],
                   invoke_context: &mut dyn InvokeContext,
               ) -> Result<(), InstructionError> {
+                  debug_assert_eq!(first_instruction_account, 1);

Contributor

jstarry Nov 18, 2021

@Lichtso it's possible to hit this debug assert with the tx I described here: https://github.com/solana-labs/solana/pull/21238/files?diff=split&w=0#r752263887

dankelleher pushed a commit to identity-com/solana that referenced this pull request


          Refactor: process_instruction() (solana-labs#20448)

8b7da50

* Adds first_instruction_account parameter to process_instruction().

* Removes InvokeContext::remove_first_keyed_account() from all BPF loaders.

* Removes InvokeContext::remove_first_keyed_account() from all builtin programs.

* Removes InvokeContext::remove_first_keyed_account() from all mock ups.

* Deprecates InvokeContext::remove_first_keyed_account().

* Documents index base of keyed_account_at_index().

* Adds dynamic offset to call sites of "keyed_account_at_index()".

frits-metalogix added a commit to identity-com/solana that referenced this pull request


          Revert "Refactor: process_instruction() (solana-labs#20448)"

ea622d4

This reverts commit 8b7da50.

Lichtso mentioned this pull request

Conensus (bank hash) mismatch between master and v1.8.5 (mainnet beta) #21397

Closed

Lichtso restored the refactor/process_instruction branch

November 29, 2021 17:06

Lichtso deleted the refactor/process_instruction branch

November 30, 2021 15:28

Lichtso mentioned this pull request

Fix #21397 #21535

Merged

This was referenced Dec 15, 2021

Refactor: Removes Rc from Refcell<AccountSharedData> in the program-runtime #21927

Merged

Consensus (bank hash) mismatch between v1.8.10 and tip of master/v1.9.1 (mainnet) #21986

Closed

Fix #21986 #22035

Merged

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet