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: Merge message processor into invoke context #20308

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Sep 29, 2021

Problem

#20165 (which was reverted in #20301) had the following problem:

The feature flag tx_wide_compute_cap was left in ThisInvokeContext::new(). So it was only applied per transaction, not applied to each instruction, as the constructor got hoisted out of the loop. It diverged on MB and not the CI tests, as the feature flag is activated by default in the tests, but not yet active on MB.

Summary of Changes

The application of the feature flag was moved from ThisInvokeContext::new() to InvokeContext::set_instruction_index().
See: c579bd8

Fixes #

@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

Merging #20308 (457727a) into master (8fee9a2) will increase coverage by 0.0%.
The diff coverage is 94.8%.

@@           Coverage Diff           @@
##           master   #20308   +/-   ##
=======================================
  Coverage    82.8%    82.8%           
=======================================
  Files         485      485           
  Lines      135809   135887   +78     
=======================================
+ Hits       112496   112567   +71     
- Misses      23313    23320    +7     

@jeffwashington
Copy link
Contributor

I confirmed this makes new roots against mnb and rebased against master. @Lichtso asked me to merge this.

@jeffwashington jeffwashington merged commit 57c8abf into solana-labs:master Sep 29, 2021
@Lichtso Lichtso deleted the refactor/merge_message_processor_into_invoke_context branch September 29, 2021 17:15
dankelleher pushed a commit to identity-com/solana that referenced this pull request Nov 24, 2021
)

* Inlines MessageProcessor::execute_instruction() in MessageProcessor::process_message().

* Moves MessageProcessor::create_pre_accounts() into ThisInvokeContext::push().

* Move instruction_recorders slice into InvokeContext.

* Makes account_indices optional in InvokeContext::push().

* Separates initial InvokeContext::push() from ThisInvokeContext::new().

* invoke_context.pop() the base invocation frame.

* Zip message.instructions.iter() and program_indices.iter().

* Moves ThisInvokeContext::new() to the beginning of the loop inside MessageProcessor::process_message().

* Hoists ThisInvokeContext::new() out of loop inside MessageProcessor::process_message().

* Removes unnecessary clone() from ThisInvokeContext::new() in MessageProcessor ::process_message().

* Stop ignoring errors from MessageProcessor::create_pre_accounts().

* Moves MessageProcessor::verify_account_references() and MessageProcessor::verify() into InvokeContext::verify().
frits-metalogix added a commit to identity-com/solana that referenced this pull request Nov 24, 2021
@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