Refactor: Replaces `KeyedAccount` by `BorrowedAccount` in BPF loaders #23056

Lichtso · 2022-02-10T14:51:35Z

Problem

There are use sites of KeyedAccount remaining in the BPF loader and the built-in programs.

Summary of Changes

Replaces KeyedAccount by BorrowedAccount.

Fixes #

codecov · 2022-02-10T23:05:41Z

Codecov Report

Merging #23056 (757d833) into master (eff085f) will decrease coverage by 0.0%.
The diff coverage is 87.2%.

@@            Coverage Diff            @@
##           master   #23056     +/-   ##
=========================================
- Coverage    81.3%    81.3%   -0.1%     
=========================================
  Files         572      572             
  Lines      155876   156213    +337     
=========================================
+ Hits       126815   127086    +271     
- Misses      29061    29127     +66

programs/bpf_loader/src/lib.rs

jackcmay · 2022-02-16T06:53:05Z

programs/bpf_loader/src/lib.rs

        }
        UpgradeableLoaderInstruction::Write { offset, bytes } => {
-            let buffer = keyed_account_at_index(keyed_accounts, first_instruction_account)?;
-            let authority = keyed_account_at_index(keyed_accounts, first_instruction_account + 1)?;


This is probably an error order mismatch, we used to check the authority account was present up front, but now check it much later after other checks

jackcmay · 2022-02-16T06:57:48Z

programs/bpf_loader/src/lib.rs

                if authority_address != upgrade_authority_address {
                    ic_logger_msg!(log_collector, "Buffer and upgrade authority don't match");
                    return Err(InstructionError::IncorrectAuthority);
                }
-                if upgrade_authority_signer {
+                if !instruction_context.is_signer(first_instruction_account + 7)? {


This is another case where the account being present was checked much earlier before. There are probably other cases in this function

programs/bpf_loader/src/lib.rs

jackcmay · 2022-02-16T07:14:55Z

runtime/src/system_instruction_processor.rs

    let keyed_accounts = invoke_context.get_keyed_accounts()?;
    let instruction = limited_deserialize(instruction_data)?;

    trace!("process_instruction: {:?}", instruction);
    trace!("keyed_accounts: {:?}", keyed_accounts);

-    let _ = keyed_account_at_index(keyed_accounts, first_instruction_account)?;


Checking first account is now gone, that's a behavior change

jackcmay · 2022-02-16T07:17:56Z

This is a good step in the right direction but a doozy of a PR. I found a few of what looks like un-featurized behavior changes and I'm pretty sure there are a lot more. Might be easier to break this up into smaller changes to help review.

Lichtso · 2022-02-16T08:15:14Z

Thanks @jackcmay !

The error order mismatches are already on my radar and unfortunately we don't have tests for them.
Anyway, I think we could add a check_number_of_instruction_accounts(x) or something in the first line of every instructions match-case.
Edit: Tried it and it fails when there are sysvars in between which check if they have the right pubkey. So, it is no that easy.

About the magic numbers, would going for enums or constants with the instruction account indices be better? If so, which do you prefer?

jackcmay · 2022-02-16T18:48:39Z

Would be cool if the account indexes could be derived from the instruction comments so we have a single source of authority. We could use an enum per instruction and have the instruction documentation refer to those enums rather than repeat the info. In the short term something local to the program would be ok and consts seems like the better choice but either one is fine. As long as we have some way to cleaning up the first + x syntax that is hard to read, validate, and error prone.

jackcmay · 2022-03-01T22:38:05Z

programs/bpf_loader/src/lib.rs

-            let buffer = keyed_account_at_index(keyed_accounts, first_instruction_account + 3)?;
-            let rent = get_sysvar_with_account_check::rent(
-                keyed_account_at_index(keyed_accounts, first_instruction_account + 4)?,
+            instruction_context.check_number_of_instruction_accounts(1)?;


Why isn't thius 5?

You are right, but it should be 4 I think, because the sysvar checks its own presence in get_sysvar_with_account_check2().

jackcmay · 2022-03-01T22:48:28Z

programs/bpf_loader/src/lib.rs

-            let buffer = keyed_account_at_index(keyed_accounts, first_instruction_account + 2)?;
-            let rent = get_sysvar_with_account_check::rent(
-                keyed_account_at_index(keyed_accounts, first_instruction_account + 4)?,
+            instruction_context.check_number_of_instruction_accounts(1)?;


Again, you are right but it should be 3.

4 seems clearer since that is the number of accounts we expect

I don't want to mix the "normal" parameters and sysvars as we might disable sysvars being passed as accounts using a feature gate.

Its more clear to include all the accounts especially since the sysvars might not always be last. If we change it the feature gate should include the change in the number of accounts expected

Its more clear to include all the accounts especially since the sysvars might not always be last

Exactly, and combined with the answer to why check_number_of_instruction_accounts is called twice this is the reason not to include them. Currently we have to do a separate check for every consecutive run of "normal" accounts until it is interrupted by a sysvar. But once we feaure gate the sysvar account check, we can merge all these check_number_of_instruction_accounts into one which covers all account indices at once.

jackcmay · 2022-03-01T22:51:10Z

programs/bpf_loader/src/lib.rs

                ic_logger_msg!(log_collector, "ProgramData account not large enough");
                return Err(InstructionError::AccountDataTooSmall);
            }
-            if programdata.lamports()? + buffer.lamports()? < programdata_balance_required {
+            if programdata.get_lamports() + buffer.get_lamports() < programdata_balance_required {


Be helpful if these name changes came in a different PR (before) to reduce the noise when trying to review and find order/behavior differences.

This is not a rename of methods, it uses BorrowedAccount instead of KeyedAccount now.
The ? at the end of each is gone as well.

Was thinking that borrowed account exportlamports() and then once all the rework is complete switch the name over. That way the reworking of accounts/error order is easier to grok in this pr

jackcmay · 2022-03-01T22:53:36Z

programs/bpf_loader/src/lib.rs

                    ic_logger_msg!(log_collector, "Buffer and upgrade authority don't match");
                    return Err(InstructionError::IncorrectAuthority);
                }
-                if authority.signer_key().is_none() {
+                if !instruction_context.is_signer(


The difference in index base here can lead to confusion

True and already on my TODO list of things to iron out once I refactored all builtin programs.
Did not want to bring that change into this PR as well.

jackcmay · 2022-03-01T22:56:04Z

programs/bpf_loader/src/lib.rs

-                    .map(|account| account.unsigned_key());
-
-            match account.state()? {
+            instruction_context.check_number_of_instruction_accounts(2)?;


Just 2, because NewAuthority is optional.

jackcmay · 2022-03-02T09:22:58Z

programs/bpf_loader/src/lib.rs

-            let authority = keyed_account_at_index(keyed_accounts, first_instruction_account + 7)?;
-            let upgrade_authority_address = Some(*authority.unsigned_key());
-            let upgrade_authority_signer = authority.signer_key().is_none();
+            instruction_context.check_number_of_instruction_accounts(8)?;


Why call this twice and shouldn't it be 7?

It was: let authority = keyed_account_at_index(keyed_accounts, first_instruction_account + 7)?

So 8 slots total, no optional ones.
And it is called twice when sysvars are mixed in between as they can throw a different error.

…abs#23056)

…olana-labs#23056)" 6c56eb9

* Revert "Replaces KeyedAccount by BorrowedAccount in the BPF loader. (#23056)" 6c56eb9 * Revert "Replaces `KeyedAccount` by `BorrowedAccount` in `system_instruction_processor`. (#23217)" ee7e411 * Revert "Replaces `KeyedAccount` by `BorrowedAccount` in `nonce_keyed_account`. (#23214)" 1a68f81 * Revert "Replaces KeyedAccount by BorrowedAccount in the config processor. (#23302)" a14c7c3 * Revert "Replaces `KeyedAccount` by `BorrowedAccount` in vote processor (#23348)" e2fa6a0 * Revert "Refactor: Prepare stake_instruction.rs to remove `KeyedAccount`s (#23375)" ee3fc39

Lichtso added the work in progress This isn't quite right yet label Feb 10, 2022

Lichtso force-pushed the refactor/remove_keyed_accounts_3 branch from 9d8c9c0 to 3da0beb Compare February 10, 2022 15:10

Lichtso force-pushed the refactor/remove_keyed_accounts_3 branch 4 times, most recently from 59b9fe2 to 4e09fc7 Compare February 14, 2022 11:47

Lichtso mentioned this pull request Feb 14, 2022

Removes KeyedAccounts in system_instruction_processor tests #23118

Merged

Lichtso force-pushed the refactor/remove_keyed_accounts_3 branch 3 times, most recently from bd6a5d4 to 08e032f Compare February 15, 2022 22:07

jackcmay reviewed Feb 16, 2022

View reviewed changes

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

jackcmay reviewed Feb 16, 2022

View reviewed changes

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

jackcmay reviewed Feb 16, 2022

View reviewed changes

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

jackcmay reviewed Feb 16, 2022

View reviewed changes

Lichtso mentioned this pull request Feb 16, 2022

Cleanup: get_program_key() and get_loader_key() in TransactionContext #23191

Merged

This was referenced Feb 17, 2022

Replaces KeyedAccount by BorrowedAccount in nonce_keyed_account. #23214

Merged

Replaces KeyedAccount by BorrowedAccount in system_instruction_processor. #23217

Merged

Lichtso force-pushed the refactor/remove_keyed_accounts_3 branch 3 times, most recently from 5ecb141 to 449ecc4 Compare February 23, 2022 14:02

Lichtso mentioned this pull request Feb 23, 2022

Replaces KeyedAccount by BorrowedAccount in the config processor. #23302

Merged

Lichtso force-pushed the refactor/remove_keyed_accounts_3 branch from 449ecc4 to c583fa0 Compare February 23, 2022 19:15

This was referenced Feb 24, 2022

Removes KeyedAccount from tests in vote processor. #23333

Merged

Replaces KeyedAccount by BorrowedAccount in vote processor #23348

Merged

Replaces KeyedAccount by BorrowedAccount in the BPF loader.

757d833

Lichtso force-pushed the refactor/remove_keyed_accounts_3 branch from c583fa0 to 757d833 Compare March 1, 2022 15:30

Lichtso merged commit 6c56eb9 into solana-labs:master Mar 1, 2022

Lichtso deleted the refactor/remove_keyed_accounts_3 branch March 1, 2022 21:55

jackcmay reviewed Mar 1, 2022

View reviewed changes

Lichtso changed the title ~~Refactor: Remove KeyedAccount~~ Refactor: Replaces KeyedAccount by BorrowedAccount in BPF loaders Mar 2, 2022

jackcmay reviewed Mar 2, 2022

View reviewed changes

Lichtso mentioned this pull request Mar 2, 2022

Fix: instruction_account_indices in builtin programs #23440

Merged

jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Mar 4, 2022

Replaces KeyedAccount by BorrowedAccount in the BPF loader. (solana-l…

3203dd2

…abs#23056)

Lichtso added a commit to Lichtso/solana that referenced this pull request Mar 14, 2022

Revert "Replaces KeyedAccount by BorrowedAccount in the BPF loader. (s…

905b2d1

…olana-labs#23056)" 6c56eb9

Lichtso mentioned this pull request Mar 14, 2022

Revert: KeyedAccount refactoings in builtin programs #23649

Merged

Lichtso removed the work in progress This isn't quite right yet label Mar 14, 2022

Lichtso added a commit to Lichtso/solana that referenced this pull request Mar 14, 2022

Revert "Replaces KeyedAccount by BorrowedAccount in the BPF loader. (s…

e996ee4

…olana-labs#23056)" 6c56eb9

Lichtso added a commit to Lichtso/solana that referenced this pull request Mar 14, 2022

Revert "Replaces KeyedAccount by BorrowedAccount in the BPF loader. (s…

f6e8032

…olana-labs#23056)" 6c56eb9

Lichtso added a commit to Lichtso/solana that referenced this pull request Mar 15, 2022

Revert "Replaces KeyedAccount by BorrowedAccount in the BPF loader. (s…

ef8bd0e

…olana-labs#23056)" 6c56eb9

Lichtso added a commit to Lichtso/solana that referenced this pull request Mar 15, 2022

Revert "Replaces KeyedAccount by BorrowedAccount in the BPF loader. (s…

7ced005

…olana-labs#23056)" 6c56eb9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Replaces `KeyedAccount` by `BorrowedAccount` in BPF loaders #23056

Refactor: Replaces `KeyedAccount` by `BorrowedAccount` in BPF loaders #23056

Lichtso commented Feb 10, 2022

codecov bot commented Feb 10, 2022 •

edited

Loading

jackcmay Feb 16, 2022

jackcmay Feb 16, 2022

jackcmay Feb 16, 2022

jackcmay commented Feb 16, 2022

Lichtso commented Feb 16, 2022 •

edited

Loading

jackcmay commented Feb 16, 2022

jackcmay Mar 1, 2022

Lichtso Mar 2, 2022

jackcmay Mar 1, 2022

Lichtso Mar 2, 2022

jackcmay Mar 2, 2022

Lichtso Mar 2, 2022

jackcmay Mar 2, 2022

Lichtso Mar 2, 2022

jackcmay Mar 1, 2022

Lichtso Mar 2, 2022 •

edited

Loading

jackcmay Mar 2, 2022

jackcmay Mar 1, 2022

Lichtso Mar 2, 2022

jackcmay Mar 1, 2022

Lichtso Mar 2, 2022

jackcmay Mar 2, 2022

Lichtso Mar 2, 2022

Refactor: Replaces KeyedAccount by BorrowedAccount in BPF loaders #23056

Refactor: Replaces KeyedAccount by BorrowedAccount in BPF loaders #23056

Conversation

Lichtso commented Feb 10, 2022

Problem

Summary of Changes

codecov bot commented Feb 10, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackcmay commented Feb 16, 2022

Lichtso commented Feb 16, 2022 • edited Loading

jackcmay commented Feb 16, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lichtso Mar 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Refactor: Replaces `KeyedAccount` by `BorrowedAccount` in BPF loaders #23056

Refactor: Replaces `KeyedAccount` by `BorrowedAccount` in BPF loaders #23056

codecov bot commented Feb 10, 2022 •

edited

Loading

Lichtso commented Feb 16, 2022 •

edited

Loading

Lichtso Mar 2, 2022 •

edited

Loading