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

MessageProcessor::process_message() returns the accounts data len delta, and the bank uses the value to update its accounts_data_len field #21807

Merged
merged 3 commits into from
Dec 15, 2021

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Dec 11, 2021

Problem

The bank doesn't get accounts data len changes from MessageProcessor, so it cannot account for any changes.

For more context see the main issue #21604

Summary of Changes

MessageProcessor::process_message() returns the accounts data len delta, and the bank uses the value to update its accounts_data_len field

…ta, and the bank uses the value to update its accounts_data_len field
@codecov
Copy link

codecov bot commented Dec 11, 2021

Codecov Report

Merging #21807 (85235a4) into master (eeb97fe) will decrease coverage by 0.0%.
The diff coverage is 81.2%.

@@            Coverage Diff            @@
##           master   #21807     +/-   ##
=========================================
- Coverage    81.3%    81.3%   -0.1%     
=========================================
  Files         515      515             
  Lines      144033   144044     +11     
=========================================
- Hits       117126   117111     -15     
- Misses      26907    26933     +26     

@brooksprumo brooksprumo marked this pull request as ready for review December 11, 2021 23:10
@@ -118,7 +125,7 @@ impl MessageProcessor {
);
timings.accumulate(&invoke_context.timings);
}
Ok(())
Ok(ProcessedMessageInfo::default())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now just return a default value. The next PR (here) will fill this in with real values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning to return the updated meter value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! Here's a snippet from the next PR:

Ok(ProcessedMessageInfo {
accounts_data_len_delta: invoke_context.get_accounts_data_meter().take().consumed(),
})

runtime/src/message_processor.rs Show resolved Hide resolved
@brooksprumo
Copy link
Contributor Author

@jackcmay Let me know if there's any questions on this PR, or if you'd like me to run it by anyone else. TIA!

@@ -56,7 +63,7 @@ impl MessageProcessor {
sysvars: &[(Pubkey, Vec<u8>)],
blockhash: Hash,
lamports_per_signature: u64,
) -> Result<(), TransactionError> {
) -> Result<ProcessedMessageInfo, TransactionError> {
Copy link
Contributor

@Lichtso Lichtso Dec 15, 2021

Choose a reason for hiding this comment

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

As already discussed on Slack, this part here will most likely change when accounts are passed back directly (not by Rc through the parameters). Because the accounts need to be always returned and this struct only on success, it would become somewhat messy. But I am not sure yet, what a good solution would be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remind why we are passing the accounts back in the return value rather then shared access with Rc?

Copy link
Contributor

Choose a reason for hiding this comment

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

Rc around accounts won't work anymore in ABIv2 (see #21706 and #21927).

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see an explanation in those prs as to why we must get rid of Rc

Copy link
Contributor

@Lichtso Lichtso Dec 15, 2021

Choose a reason for hiding this comment

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

You are right, I removed that when splitting the PRs, will add it back in.
The short version is that we can't serialize a Rc, so we would have to wrap / unwrap it twice in process_message() which is completely unnecessary. And ABIv2 is all about getting rid of such unnecessary copy steps.

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.

None yet

3 participants