add default_cost as mandatory field for Builtin #30639

tao-stones · 2023-03-08T05:19:31Z

Problem

Builtin should consume compute units when executed; if no exact units being deducted from compute budget, a default cost should be used. In order to do so, a mandatory default_cost field is added to Builtin, which is passed down to invoke_context::BuiltinProgram where it can be used for compute budget.

Summary of Changes

add mandatory default_cost to Builtin, assigned pre-defined value from block_cost_limits.rs
updated macros
pass default_cost to invoke_context::BuiltinProgram (A follow up PR would use that value to deduct builtin's cost from compute budget)

Fixes #

tao-stones · 2023-03-08T05:48:50Z

Following programs are not in builtins covered by this PR, are they not being executed via invoke_context, or perhaps I missed them out somehow?

                (solana_program::ed25519_program::id(), 720),
                (solana_program::feature::id(), 60),
                (solana_program::incinerator::id(), 60),
                (solana_sdk::native_loader::id(), 60),
                (solana_program::secp256k1_program::id(), 720),

joncinque

Looks really great!

I also double-checked all the values to be safe. It's funny this also showed that the old map had solana_sdk::stake::config::id() which isn't even a program, so I hope this will make things much better!

programs/bpf_loader/src/deprecated.rs

runtime/src/builtins.rs

joncinque · 2023-03-08T14:50:43Z

Regarding the other programs, here's some info:

(solana_program::ed25519_program::id(), 720): this is a precompile, so it's run during transaction sanitize, directly by the bank, outside of the instruction processor, in

solana/sdk/src/transaction/mod.rs

Line 1014 in d8a6953

pub fn verify_precompiles(&self, feature_set: &feature_set::FeatureSet) -> Result<()> {
(solana_program::secp256k1_program::id(), 720): same as ed25519
(solana_program::feature::id(), 60): I don't think it's possible to actually run this, it's just an owner for the feature accounts -- here's that instruction

solana/sdk/program/src/feature.rs

Line 48 in d8a6953

pub fn activate_with_lamports(
(solana_program::incinerator::id(), 60): this isn't a program either, it's just a special address whose lamports are automatically burned at the end of each block
(solana_sdk::native_loader::id(), 60): this isn't a program either, it just owns the native programs

tao-stones · 2023-03-08T16:08:38Z

I also double-checked all the values to be safe. It's funny this also showed that the old map had solana_sdk::stake::config::id() which isn't even a program, so I hope this will make things much better!

It is not ideal to maintain a static list of builtins separately, this change will make it better. I am going to close #30617 in favor of this, and start updating all broken tests. And thank you for the advices.

joncinque · 2023-03-08T16:16:55Z

Anytime! Feel free to ping me when this is ready

tao-stones · 2023-03-09T14:41:34Z

Anytime! Feel free to ping me when this is ready

@joncinque all tests have been updated, thank @samkim-crypto for quickly measure cost for zkp. PR is ready for review.

tao-stones · 2023-03-09T15:41:22Z

program-runtime/src/invoke_context.rs

@@ -43,6 +43,7 @@ pub type ProcessInstructionWithContext = fn(&mut InvokeContext) -> Result<(), In
 pub struct BuiltinProgram {
    pub program_id: Pubkey,
    pub process_instruction: ProcessInstructionWithContext,
+    pub default_compute_unit_cost: u64,


default_compute_unit_cost will be used in follow-up PR to consume units from TX's compute budget when builtins are processed.

samkim-crypto

Looks good on my part! I'd be interested to review the follow up PR as well (since this relates to zk proof program).

For the case of the zk proof program, the costs should ideally be determined by the instruction, but this should suffice for now as an approximate.

joncinque

Looks good overall! Just a question for the default compute unit usage on the zk proof program. Once that's resolved, this will be good to go

runtime/src/builtins.rs

joncinque · 2023-03-10T02:15:00Z

runtime/src/builtins.rs

+                125906, // the average CUs for 6 zkp program instructions,
+                        // see comment at https://github.com/solana-labs/solana/pull/30639


Sorry to come back to this, but I'd prefer setting an extremely high default so that the zk program is forced to always manually consume compute units, maybe even u64::MAX. It would stink to open up a DOS opportunity just because we forget to consume compute units in a new expensive instruction. With u64::MAX, it should fail tests immediately, right?

@samkim-crypto what do you think?

Yep, if the idea is to use the default compute budget when the individual instructions fail to manually consume compute units, then making it always fail by setting it to a high number would be the way to go.

Currently (well, it is currently behind a feature gate), the proof program does consume units at the program level, so it is not completely clear when the default units will kick in. I was hoping to see how this is done in the follow-up PR.

This is a good concern. Think about it, the risk of new instruction (of an existing program) would use much more units than pre-define program default cost applies not just zkp program, but to all.

It is always preferred builtin programs to manually deduct units during process. But for those don't have the luxury to do so at the moment, a default projects the worst scenario should be good enough.

For zkp program, my reason for using average instead of worse scenario (407_121 CU) is that compute budget defines default unit per instruction as 200_000, and max as 1_400_000 cu per tx. So I think anything close to 200_000 is reasonable, cause you can't have more than 7 of that in single tx. But if we really want to be careful, then worse scenario is ok too. u64::max might be overkill - that'd fail tx immediately. This is my 2 cents.

@samkim-crypto sorry I didn't see your comment before post mine above.

The use case of default cost is described in this comment #30639 (comment). Basically if builtin.process_instruction() does not deduct units from compute budget, then the program's default cost will be used.

In the case of proof program, since it'd consume 100_000 during process, the default will not be used.

if the idea is to use the default compute budget when the individual instructions fail to manually consume compute units

regarding this, currently no builtin manually consumes units (i mean nothing was deducted from compute budget after builtin.process_instruction()). So if sets default cost to max, then all tx has builtin will fail.

(BTW, I think this discussion is very helpful, thanks guys)

Ok that all makes sense. How about we split the difference, and use the CUs from VerifyTransfer as the default? That should be the most used instruction. And since it's over 200k, we'll have to request more compute units in tests, which means we're much less likely to forget.

sounds good to me, changed it in comment 959f686

Co-authored-by: Jon Cinque <[email protected]>

joncinque

Looks great!

codecov · 2023-03-10T18:11:49Z

Codecov Report

Merging #30639 (8085015) into master (13107b4) will increase coverage by 0.0%.
The diff coverage is 83.8%.

@@           Coverage Diff           @@
##           master   #30639   +/-   ##
=======================================
  Coverage    81.6%    81.7%           
=======================================
  Files         723      723           
  Lines      201674   201682    +8     
=======================================
+ Hits       164762   164784   +22     
+ Misses      36912    36898   -14

tao-stones · 2023-03-10T21:28:36Z

I'd be interested to review the follow up PR as well (since this relates to zk proof program).

@samkim-crypto the follow up PR is #30680

* add default_cost as mandatory field for Builtin * updated tests * set zkp program default to VerifyTransfer CUs --------- Co-authored-by: Jon Cinque <[email protected]>

Lichtso · 2023-03-13T12:01:40Z

Sorry @taozhu-chicago that I only saw this PR now. Problem is, that @pgarg66 and I are in the process of removing invoke_context::BuiltinProgram and replace it with rbpf::vm::BuiltInProgram, so that we can unify the call interface of built-in programs and syscalls. Thus, this PR probably should be reverted and implemented differently.

tao-stones · 2023-03-13T17:22:13Z

Sorry @taozhu-chicago that I only saw this PR now. Problem is, that @pgarg66 and I are in the process of removing invoke_context::BuiltinProgram and replace it with rbpf::vm::BuiltInProgram, so that we can unify the call interface of built-in programs and syscalls. Thus, this PR probably should be reverted and implemented differently.

No worries, I should have tagged you on this PR. I knew you and @pgarg66 are working in the area, didn't know what changes you guys are actually making. In any case, let me catch up on your changes, see if there are chance to apply/merge this PRs. If not, OK to revert. The end goal is to find a quick way to make builtin programs to consume units (as in #30680)

…30639)" This reverts commit 7b95c8e.

…0749) This reverts commit 7b95c8e.

tao-stones requested a review from joncinque March 8, 2023 05:19

tao-stones mentioned this pull request Mar 8, 2023

Move address_lookup_table program id declaration to solana-program #30617

Closed

tao-stones force-pushed the builtin_adds_default_cost_field branch from 52c2d0c to bc2571c Compare March 8, 2023 05:58

joncinque reviewed Mar 8, 2023

View reviewed changes

programs/bpf_loader/src/deprecated.rs Show resolved Hide resolved

runtime/src/builtins.rs Outdated Show resolved Hide resolved

runtime/src/builtins.rs Outdated Show resolved Hide resolved

tao-stones force-pushed the builtin_adds_default_cost_field branch from 30c1782 to 1b664e5 Compare March 9, 2023 00:10

tao-stones requested review from joncinque and samkim-crypto March 9, 2023 14:37

tao-stones commented Mar 9, 2023

View reviewed changes

samkim-crypto previously approved these changes Mar 10, 2023

View reviewed changes

joncinque reviewed Mar 10, 2023

View reviewed changes

tao-stones dismissed samkim-crypto’s stale review via 9958545 March 10, 2023 04:55

tao-stones and others added 6 commits March 10, 2023 16:31

add default_cost as mandatory field for Builtin

4966a36

rename to default_compute_unit_cost for clarity; updated tests

6cc0eb6

update sbf tests

d5dbe2b

setting zkp program cost

611e7da

Update comment

22bcce2

Co-authored-by: Jon Cinque <[email protected]>

set zkp program default to VerifyTransfer CUs

8085015

tao-stones force-pushed the builtin_adds_default_cost_field branch from 959f686 to 8085015 Compare March 10, 2023 16:31

joncinque approved these changes Mar 10, 2023

View reviewed changes

tao-stones merged commit 7b95c8e into solana-labs:master Mar 10, 2023

tao-stones deleted the builtin_adds_default_cost_field branch March 10, 2023 20:02

tao-stones mentioned this pull request Mar 10, 2023

builtin program consumes default cost if instruction not manually deduct units #30680

Closed

tao-stones mentioned this pull request Mar 13, 2023

Feature gate builtin consumes static units during processing instruction #30702

Merged

tao-stones added a commit to tao-stones/solana that referenced this pull request Mar 16, 2023

Revert "add default_cost as mandatory field for Builtin (solana-labs#…

5f0e884

…30639)" This reverts commit 7b95c8e.

tao-stones added a commit to tao-stones/solana that referenced this pull request Mar 16, 2023

Revert "add default_cost as mandatory field for Builtin (solana-labs#…

d4a0d02

…30639)" This reverts commit 7b95c8e.

tao-stones mentioned this pull request Mar 16, 2023

Revert "add default_cost as mandatory field for Builtin (#30639)" #30749

Merged

tao-stones added a commit that referenced this pull request Mar 16, 2023

Revert "add default_cost as mandatory field for Builtin (#30639)" (#3…

17ba051

…0749) This reverts commit 7b95c8e.

tao-stones mentioned this pull request Jun 2, 2023

use one definition of builtin program cost in runtime and banking stage #31930

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add default_cost as mandatory field for Builtin #30639

add default_cost as mandatory field for Builtin #30639

tao-stones commented Mar 8, 2023

tao-stones commented Mar 8, 2023

joncinque left a comment

joncinque commented Mar 8, 2023

tao-stones commented Mar 8, 2023

joncinque commented Mar 8, 2023

tao-stones commented Mar 9, 2023

tao-stones Mar 9, 2023

samkim-crypto left a comment

joncinque left a comment

joncinque Mar 10, 2023 •

edited

Loading

samkim-crypto Mar 10, 2023 •

edited

Loading

tao-stones Mar 10, 2023

tao-stones Mar 10, 2023 •

edited

Loading

tao-stones Mar 10, 2023

tao-stones Mar 10, 2023

joncinque Mar 10, 2023

tao-stones Mar 10, 2023

joncinque left a comment

codecov bot commented Mar 10, 2023

tao-stones commented Mar 10, 2023

Lichtso commented Mar 13, 2023 •

edited

Loading

tao-stones commented Mar 13, 2023

		125906, // the average CUs for 6 zkp program instructions,
		// see comment at https://github.com/solana-labs/solana/pull/30639

add default_cost as mandatory field for Builtin #30639

add default_cost as mandatory field for Builtin #30639

Conversation

tao-stones commented Mar 8, 2023

Problem

Summary of Changes

tao-stones commented Mar 8, 2023

joncinque left a comment

Choose a reason for hiding this comment

joncinque commented Mar 8, 2023

tao-stones commented Mar 8, 2023

joncinque commented Mar 8, 2023

tao-stones commented Mar 9, 2023

tao-stones Mar 9, 2023

Choose a reason for hiding this comment

samkim-crypto left a comment

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

joncinque Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

samkim-crypto Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

tao-stones Mar 10, 2023

Choose a reason for hiding this comment

tao-stones Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

tao-stones Mar 10, 2023

Choose a reason for hiding this comment

tao-stones Mar 10, 2023

Choose a reason for hiding this comment

joncinque Mar 10, 2023

Choose a reason for hiding this comment

tao-stones Mar 10, 2023

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 10, 2023

Codecov Report

tao-stones commented Mar 10, 2023

Lichtso commented Mar 13, 2023 • edited Loading

tao-stones commented Mar 13, 2023

joncinque Mar 10, 2023 •

edited

Loading

samkim-crypto Mar 10, 2023 •

edited

Loading

tao-stones Mar 10, 2023 •

edited

Loading

Lichtso commented Mar 13, 2023 •

edited

Loading