Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Return the accounts data len delta after processing messages#22986

Merged
brooksprumo merged 1 commit intosolana-labs:masterfrom
brooksprumo:accounts-data-len/delta-from-process-message
Feb 9, 2022
Merged

Return the accounts data len delta after processing messages#22986
brooksprumo merged 1 commit intosolana-labs:masterfrom
brooksprumo:accounts-data-len/delta-from-process-message

Conversation

@brooksprumo
Copy link
Copy Markdown
Contributor

@brooksprumo brooksprumo commented Feb 7, 2022

Problem

Tracking and consuming from Bank::accounts_data_len could race if multiple threads call Bank::process_message(). The original behavior in #21807 was correct, and #21813 introduced the race.

Summary of Changes

  • Change AccountsDataMeter to track the accounts data len delta
  • MessageProcessor returns the delta (from above) instead of the final accounts data len
  • Bank updates its accounts data len from the delta instead of storing the final accounts data len

Related to #21604

@brooksprumo brooksprumo marked this pull request as ready for review February 7, 2022 20:44
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 7, 2022

Codecov Report

Merging #22986 (7e37ae0) into master (eaf2df9) will decrease coverage by 0.0%.
The diff coverage is 81.2%.

@@            Coverage Diff            @@
##           master   #22986     +/-   ##
=========================================
- Coverage    81.2%    81.2%   -0.1%     
=========================================
  Files         563      563             
  Lines      152938   152946      +8     
=========================================
- Hits       124328   124306     -22     
- Misses      28610    28640     +30     

/// amount of accounts data space used. If `amount` is negative, we are *decreasing* the
/// amount of accounts data space used.
pub fn consume(&mut self, amount: i64) -> Result<(), InstructionError> {
if amount == 0 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I found the name of this function and it's description like reading double negatives. Maybe consume is not the best name for this function because we are not just using up a resource, but also replenishing it. Maybe rename to adjust_delta, adjust_remaining or something along those lines?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yah, I definitely agree with you. Since this function is not new, I will address this in a subsequent PR (and so I don't need to have CI run again on this PR). Let me know if that's not ok.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tracking this here: #23022

Copy link
Copy Markdown
Contributor

@jackcmay jackcmay left a comment

Choose a reason for hiding this comment

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

Looks great besides the nit

@brooksprumo brooksprumo merged commit 869cfc9 into solana-labs:master Feb 9, 2022
@brooksprumo brooksprumo deleted the accounts-data-len/delta-from-process-message branch February 9, 2022 01:24
mergify Bot pushed a commit that referenced this pull request Feb 9, 2022
mergify Bot added a commit that referenced this pull request Feb 9, 2022
…#23023)

(cherry picked from commit 869cfc9)

Co-authored-by: Brooks Prumo <brooks@solana.com>
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Feb 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants