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

The constraints on compute power a program can consume is limited only to its instruction count #11717

Merged
merged 4 commits into from
Aug 21, 2020

Conversation

jackcmay
Copy link
Contributor

Problem

A program's impact on the system is constrained to a cap on the number of BPF instructions it can execute. But program may impact the system in other ways (syscalls, memory allocation, calling other programs). The current constraints are not sufficient

Summary of Changes

Introduce the bones of a compute power metering mechanism that will allow broader and customizable program execution constraints. This first cut takes into account the cost of syscalls as well as calling other programs (but allows free memory allocations from the bump allocator). It can be easily plumbed into fee collection when the time for that comes.

The proposed compute costs are best guesses based on the current limits and should be tuned and expanded in the future.

Fixes #11556

@jackcmay jackcmay changed the title The amount of compute power a program can consume is limited to its instruction count The constraints on compute power a program can consume is limited only to its instruction count Aug 19, 2020
@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #11717 into master will decrease coverage by 0.0%.
The diff coverage is 60.3%.

@@            Coverage Diff            @@
##           master   #11717     +/-   ##
=========================================
- Coverage    82.0%    82.0%   -0.1%     
=========================================
  Files         330      330             
  Lines       77068    77156     +88     
=========================================
+ Hits        63255    63310     +55     
- Misses      13813    13846     +33     

@@ -560,6 +589,7 @@ impl MessageProcessor {
self.programs.clone(), // get rid of clone
log_collector,
self.is_cross_program_supported,
COMPUTE_BUDGET,
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little worried about how we'll deal with tweaking COMPUTE_BUDGET and MAX_INVOCATION_DEPTH in the future. These can cause diverging behavior during a rolling upgrade and changes should be triggered on some slot/epoch boundary. It'd be nice to consider how that will play out now, since technically right now COMPUTE_BUDGET is u64::MAX, and reducing might break existing programs so also needs to be triggered on some slot/epoch boundary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, we need a way to adjust these in the future. These changes mostly mimic the current budget which isn't u64::MAX but instead 100_000 BPF instructions. The only difference for existing programs is a small cost for the logging syscalls. Programs using CPI are the ones that will see a bigger change (cost added for create_progrm_address, invoke, and charged for the cost programs they call incur. I was planning to put in the rolling update stuff later when we need it and was focused on getting these CPI caps in place first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of these changes commit to address your concern?

@ryoqun These should also move into the bank as part of #11750

Copy link
Member

Choose a reason for hiding this comment

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

yeah, nice

@jackcmay jackcmay merged commit 8d362f6 into solana-labs:master Aug 21, 2020
@jackcmay jackcmay deleted the add-compute-metering branch August 21, 2020 22:31
@jackcmay jackcmay added the v1.3 label Aug 24, 2020
mergify bot pushed a commit that referenced this pull request Aug 24, 2020
…y to its instruction count (#11717)

(cherry picked from commit 8d362f6)

# Conflicts:
#	programs/bpf/Cargo.toml
#	programs/bpf_loader/Cargo.toml
#	programs/bpf_loader/src/lib.rs
#	programs/bpf_loader/src/syscalls.rs
#	runtime/src/bank.rs
#	sdk/src/instruction.rs
jackcmay added a commit that referenced this pull request Aug 24, 2020
…y to its instruction count (#11717)

(cherry picked from commit 8d362f6)

# Conflicts:
#	programs/bpf/Cargo.toml
#	programs/bpf_loader/Cargo.toml
#	programs/bpf_loader/src/lib.rs
#	programs/bpf_loader/src/syscalls.rs
#	runtime/src/bank.rs
#	sdk/src/instruction.rs
mergify bot added a commit that referenced this pull request Aug 24, 2020
…y to its instruction count (bp #11717) (#11800)

* The constraints on compute power a program can consume is limited only to its instruction count (#11717)

(cherry picked from commit 8d362f6)

# Conflicts:
#	programs/bpf/Cargo.toml
#	programs/bpf_loader/Cargo.toml
#	programs/bpf_loader/src/lib.rs
#	programs/bpf_loader/src/syscalls.rs
#	runtime/src/bank.rs
#	sdk/src/instruction.rs

* Resolve conflicts

* nudge

Co-authored-by: Jack May <jack@solana.com>
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.

Instruction limits aren't enforced throughout cross program invocations
2 participants