Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Dec 15, 2021

Problem

Continuation of #21882.

  • Feature: TransactionContext, InstructionContext and BorrowedAccount #21706 will require the keyed_accounts parameter of mock_process_instruction() to be split in transaction_accounts and instruction_accounts.
  • The tuple of the keyed_account parameter should be replaced by the struct AccountMeta and instruction_accounts (see discussion in #20448).
  • Individual test cases should be isolated and the accounts generated by one and consumed by another explicitly routes through an accounts variable.
  • Tests currently can mock impossible transactions, which have two different accounts for the same key or two different keys for the same account. The transaction_accounts mapping should always be bijective.

Summary of Changes

  • Removes Rc from Refcell<AccountSharedData> in the program-runtime
  • Makes TransactionError::AccountBorrowOutstanding obsolete, but does not remove it yet
  • Adjust all program tests accordingly

Fixes #

@Lichtso Lichtso force-pushed the refactor/remove_rc_from_refcell_account_shared_data branch 3 times, most recently from 7ebd593 to bfbbc8e Compare December 15, 2021 20:19
@codecov
Copy link

codecov bot commented Dec 16, 2021

Codecov Report

Merging #21927 (6d19ba1) into master (46e5350) will increase coverage by 0.0%.
The diff coverage is 98.9%.

@@           Coverage Diff            @@
##           master   #21927    +/-   ##
========================================
  Coverage    81.2%    81.3%            
========================================
  Files         516      516            
  Lines      144353   144777   +424     
========================================
+ Hits       117315   117772   +457     
+ Misses      27038    27005    -33     

@@ -847,50 +847,38 @@ pub struct MockInvokeContextPreparation {
pub fn prepare_mock_invoke_context(
program_indices: &[usize],
instruction_data: &[u8],
keyed_accounts: &[(bool, bool, Pubkey, Rc<RefCell<AccountSharedData>>)],
transaction_accounts: Vec<(Pubkey, AccountSharedData)>,
instruction_accounts: Vec<AccountMeta>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need pubkey in both of these vectors? Would an index suffice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An index would suffice for the instruction_accounts, but then AccountMeta would have to go.
So, as that is only for the tests / mocks I think we can live with that inefficiency for now.
Once #21706 has landed I can extend its structures to the tests as well.

@@ -847,50 +847,38 @@ pub struct MockInvokeContextPreparation {
pub fn prepare_mock_invoke_context(
program_indices: &[usize],
instruction_data: &[u8],
keyed_accounts: &[(bool, bool, Pubkey, Rc<RefCell<AccountSharedData>>)],
transaction_accounts: Vec<(Pubkey, AccountSharedData)>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this same paradign going to be extended beyond mock? If so, are we going to be allocating a lot of vectors as we pass this information around?

Copy link
Contributor Author

@Lichtso Lichtso Dec 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the Vec allocation only happens this way for the tests / mocks, and only for now, as it might be refined to become index based as well (see comment above). The rest of the program-runtime will be directly migrated to the new structs defined in #21706 and they work based on references and indices, so there should not be more allocation happening than what we had until now.

@Lichtso Lichtso merged commit 66fa8f9 into solana-labs:master Dec 17, 2021
@Lichtso Lichtso deleted the refactor/remove_rc_from_refcell_account_shared_data branch December 17, 2021 13:01
@Lichtso Lichtso mentioned this pull request Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants