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

Add AccountsDataMeter to InvokeContext#21813

Merged
brooksprumo merged 8 commits intosolana-labs:masterfrom
brooksprumo:accounts-data-len-3
Dec 28, 2021
Merged

Add AccountsDataMeter to InvokeContext#21813
brooksprumo merged 8 commits intosolana-labs:masterfrom
brooksprumo:accounts-data-len-3

Conversation

@brooksprumo
Copy link
Copy Markdown
Contributor

@brooksprumo brooksprumo commented Dec 11, 2021 •
edited
Loading

Problem

Bank does not know how much the accounts data len changes after processing messages.

For more context see the main issue #21604

Summary of Changes

Add AccountsDataMeter to InvokeContext, and return the new accounts_data_len from MessageProcessor::process_message() so that Bank can update its accounts_data_len.

Does not track accounts_data_len yet. That will be part of a subsequent PR.
(Subsequent PR will likely be—or look like—this one: #21994)

@brooksprumo brooksprumo mentioned this pull request Dec 11, 2021
Merged
@brooksprumo brooksprumo force-pushed the accounts-data-len-3 branch 4 times, most recently from 2037e5e to 3393d2e Compare December 13, 2021 19:36
@brooksprumo brooksprumo added the blocked Unable to proceed label Dec 15, 2021
@brooksprumo brooksprumo added the noCI Suppress CI on this Pull Request label Dec 15, 2021
@brooksprumo
Copy link
Copy Markdown
Contributor Author

brooksprumo commented Dec 15, 2021 •
edited
Loading

Blocked waiting on #21807

@brooksprumo brooksprumo mentioned this pull request Dec 15, 2021
Closed
@brooksprumo brooksprumo force-pushed the accounts-data-len-3 branch 2 times, most recently from 04c2aa0 to ad161a7 Compare December 15, 2021 23:02
@brooksprumo brooksprumo added CI Pull Request is ready to enter CI and removed noCI Suppress CI on this Pull Request blocked Unable to proceed labels Dec 15, 2021
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Dec 15, 2021
brooksprumo
brooksprumo commented Dec 16, 2021
View reviewed changes
Comment thread program-runtime/src/invoke_context.rs Outdated
@brooksprumo brooksprumo marked this pull request as ready for review December 16, 2021 18:49
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 16, 2021 •
edited
Loading

Codecov Report

Merging #21813 (f672328) into master (bb97c8f) will increase coverage by 0.0%.
The diff coverage is 87.0%.

@@           Coverage Diff            @@
##           master   #21813    +/-   ##
========================================
  Coverage    81.2%    81.2%            
========================================
  Files         519      520     +1     
  Lines      145754   145865   +111     
========================================
+ Hits       118466   118564    +98     
- Misses      27288    27301    +13

jackcmay
jackcmay reviewed Dec 16, 2021
View reviewed changes
Comment thread program-runtime/src/invoke_context.rs Outdated
jackcmay
jackcmay reviewed Dec 16, 2021
View reviewed changes
Comment thread program-runtime/src/invoke_context.rs Outdated
jackcmay
jackcmay reviewed Dec 16, 2021
View reviewed changes
Comment thread runtime/src/bank.rs
jackcmay
jackcmay reviewed Dec 16, 2021
View reviewed changes
Comment thread sdk/program/src/instruction.rs
jackcmay
jackcmay reviewed Dec 16, 2021
View reviewed changes
Comment thread sdk/program/src/instruction.rs
brooksprumo
brooksprumo commented Dec 16, 2021
View reviewed changes
Comment thread sdk/program/src/program_error.rs
@brooksprumo brooksprumo force-pushed the accounts-data-len-3 branch 2 times, most recently from f166d72 to 819ef4d Compare December 17, 2021 17:49
@brooksprumo brooksprumo requested a review from jackcmay December 17, 2021 17:56
jackcmay
jackcmay reviewed Dec 22, 2021
View reviewed changes
Comment thread programs/bpf_loader/src/lib.rs Outdated
@brooksprumo brooksprumo requested a review from jackcmay December 27, 2021 17:36
@brooksprumo brooksprumo force-pushed the accounts-data-len-3 branch 2 times, most recently from e540ff3 to ea3c53a Compare December 27, 2021 17:48
Lichtso
Lichtso reviewed Dec 27, 2021
View reviewed changes
Comment thread program-runtime/src/accounts_data_meter.rs
}
self.current = self.current.saturating_add(amount);
} else {
let amount = amount.abs() as u64;
Copy link
Copy Markdown
Contributor

@Lichtso Lichtso Dec 27, 2021

Choose a reason for hiding this comment

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

Edge case (with potential panic) which is not covered?

https://doc.rust-lang.org/std/primitive.i64.html#method.abs

Copy link
Copy Markdown
Contributor Author

@brooksprumo brooksprumo Dec 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Fair point! I figure since we'll never be able to deallocate more than the maximum that can be allocated, amount should never be less than -MAX_ACCOUNTS_DATA_LEN, which means .abs() here will be safe. Is that sufficient? Would you like a comment and/or a debug_assert!()?

Copy link
Copy Markdown
Contributor

@Lichtso Lichtso Dec 27, 2021

Choose a reason for hiding this comment

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

No, makes sense. There should be an implicit debug_assert!() in .abs() already.

@brooksprumo brooksprumo requested a review from Lichtso December 28, 2021 00:10
@brooksprumo
Copy link
Copy Markdown
Contributor Author

brooksprumo commented Dec 28, 2021

@jackcmay @Lichtso If there are no further comments, is it OK to merge, or should I wait for an Approve?

@brooksprumo brooksprumo merged commit 800472d into solana-labs:master Dec 28, 2021
@brooksprumo brooksprumo deleted the accounts-data-len-3 branch December 28, 2021 11:14
@mergify mergify Bot mentioned this pull request Jan 5, 2022
Merged
mergify Bot pushed a commit that referenced this pull request Jan 5, 2022
@brooksprumo
Add AccountsDataMeter to InvokeContext (#21813)
37bc964 
(cherry picked from commit 800472d)

# Conflicts:
#	runtime/src/bank.rs
#	sdk/src/feature_set.rs
brooksprumo added a commit that referenced this pull request Jan 5, 2022
@brooksprumo
Add AccountsDataMeter to InvokeContext (#21813)
71bfe6e 
(cherry picked from commit 800472d)
brooksprumo added a commit that referenced this pull request Jan 5, 2022
@brooksprumo
Add AccountsDataMeter to InvokeContext (#21813)
bde558f 
(cherry picked from commit 800472d)
@brooksprumo brooksprumo mentioned this pull request Jan 5, 2022
Closed
29 tasks
brooksprumo added a commit that referenced this pull request Jan 5, 2022
@brooksprumo
Add AccountsDataMeter to InvokeContext (#21813)
0811146 
(cherry picked from commit 800472d)
mergify Bot added a commit that referenced this pull request Jan 6, 2022
@mergify @brooksprumo
Add AccountsDataMeter to InvokeContext (#21813) (#22299)
687cd47 
(cherry picked from commit 800472d)

Co-authored-by: Brooks Prumo <brooks@solana.com>
@brooksprumo brooksprumo mentioned this pull request Feb 7, 2022
Merged
@brooksprumo brooksprumo added the feature-gate Pull Request adds or modifies a runtime feature gate label Apr 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jackcmay jackcmay Awaiting requested review from jackcmay

1 more reviewer

@Lichtso Lichtso Lichtso approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

feature-gate Pull Request adds or modifies a runtime feature gate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@brooksprumo @jackcmay @Lichtso @solana-grimes