Algod: Simulation run with extra budget per transaction group #5354

ahangsu · 2023-05-03T16:56:35Z

Summary

This PR allows for simulating a txn group with extra budget by specifying additional budget through SimulateRequest.

Test Plan

The test splits into follow 3 parts:

simulate_eval_test
restClient_test
e2e-app-simulate.sh

All tests are testing an app using more than 700 (or 1400) budget.

The logic in both endpoint and simulator should be tested.

codecov · 2023-05-04T16:15:41Z

Codecov Report

Merging #5354 (37ad9b0) into master (7818183) will decrease coverage by 2.77%.
The diff coverage is 55.17%.

@@            Coverage Diff             @@
##           master    #5354      +/-   ##
==========================================
- Coverage   55.26%   52.49%   -2.77%     
==========================================
  Files         454      454              
  Lines       63722    63746      +24     
==========================================
- Hits        35213    33462    -1751     
- Misses      26113    27762    +1649     
- Partials     2396     2522     +126

Impacted Files	Coverage Δ
daemon/algod/api/server/v2/handlers.go	`0.00% <0.00%> (-0.86%)`	⬇️
daemon/algod/api/server/v2/utils.go	`11.94% <0.00%> (-0.11%)`	⬇️
cmd/goal/clerk.go	`8.81% <20.00%> (+0.14%)`	⬆️
ledger/simulation/simulator.go	`77.27% <81.81%> (+2.27%)`	⬆️
ledger/simulation/trace.go	`90.24% <100.00%> (+0.24%)`	⬆️
ledger/simulation/tracer.go	`93.61% <100.00%> (+0.21%)`	⬆️

... and 114 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jasonpaulos

Generally looks good and has nice tests. I do have some design-level questions/thoughts though:

This PR adds an additional flat amount of app call budget to a group. One alternative model I can think of is changing the budget per app call, i.e. changing Proto.MaxAppProgramCost. Do we prefer one of these approaches to the other?
We should consider some limit on the maximum additional app call budget that we allow. As a starting point, I'll suggest 20,000 * 16, since that's the maximum amount of logicsig budget you can use today.
It might be nice to have the additional budget parameter be a signed integer, so you could artificially limit the runtime budget of an app. I think that would be hard to achieve with our REST API code generator though.

daemon/algod/api/algod.oas2.json

ledger/simulation/tracer.go

ledger/simulation/trace.go

ledger/simulation/simulation_eval_test.go

ahangsu · 2023-05-04T19:36:57Z

It might be nice to have the additional budget parameter be a signed integer, so you could artificially limit the runtime budget of an app. I think that would be hard to achieve with our REST API code generator though.

I agree with this part, that having a negative budget would be interesting, if we want to push smart contract simulation to an extreme (with rather limited budget).

It is indeed not quite feasible to do under codegen at this point, for I attempted to specify type to be int64, but I still get uint64 type in model.type. Maybe we should pause a sec here for negative budget support...

ahangsu · 2023-05-05T02:33:59Z

The thing I'm a bit unsure is that, the error handling for too high simulation extra budget is happening in simulateWithTracer before s.evaluate. Are we cool about it?

cmd/goal/clerk.go

jannotti · 2023-05-05T12:30:21Z

daemon/algod/api/algod.oas2.json

+        "extra-app-budget": {
+          "description": "The extra budget added for each app call during each transaction group simulation",
+          "type": "integer",
+          "x-algorand-format": "uint64"


Why do we use x-algorand-format here, but not above? (in the request)

I did a bit research here, saw https://github.com/algorand/generator/blob/e5add2d3096bba45d51e44cdd9e9725427e02208/src/main/java/com/algorand/sdkutils/generators/OpenApiParser.java#L131 and algorand/java-algorand-sdk#183 (comment), I think this x-algorand-format should be deleted, for it has only things to do with sdk codegen, and we don't have to specify the integer type again.

ledger/simulation/trace.go

jannotti · 2023-05-05T12:38:20Z

ledger/simulation/tracer.go

@@ -140,6 +140,12 @@ func (tracer *evalTracer) BeforeTxnGroup(ep *logic.EvalParams) {
 		tracer.result.TxnGroups[0].AppBudgetAdded = uint64(*ep.PooledApplicationBudget)
 	}

+	// Override transaction group budget if specified in request, retrieve from tracer.result
+	if tracer.result.EvalOverrides.ExtraAppBudget != nil && ep.PooledApplicationBudget != nil {


I'm only now realizing this allows extra budget in apps but not logicsigs. Do we want that?

It would be interesting, but not very commonly used I guess?

I think we may want that at some point, but I see no reason to attempt to also achieve that now.

And since logicsig budget is not pooled among a group, I suspect we'd want to increase each program's budget to get that feature to work, which isn't really compatible with the scheme here

I assume that with this PR as it is now, one could, in a simulated transaction group, create an wrapper app that incorporates the logicsig code, then call into it that app, with elevated opcode budget? i.e. there's nothing a logicsig can do that an app can't do.

daemon/algod/api/server/v2/handlers.go

ledger/simulation/simulator.go

ledger/simulation/trace.go

…nt64

jasonpaulos

Just some minor comments

ledger/simulation/simulation_eval_test.go

test/scripts/e2e_subs/e2e-app-simulate.sh

cmd/goal/clerk.go

test/scripts/e2e_subs/e2e-app-simulate.sh

cmd/goal/clerk.go

extra budget running req, resp, goal arg

b0bf410

ahangsu force-pushed the simulate-extra-budget branch from 99d7671 to b0bf410 Compare May 3, 2023 16:57

ahangsu added the New Feature label May 3, 2023

ahangsu added 3 commits May 3, 2023 13:19

minor

3c17622

eval test

5ecc0aa

restclient test

95f1594

ahangsu force-pushed the simulate-extra-budget branch from 97b6336 to 95f1594 Compare May 3, 2023 18:16

e2e test

9d026ca

ahangsu marked this pull request as ready for review May 3, 2023 19:01

typo in e2e test log

002bcfb

ahangsu requested review from jasonpaulos and bbroder-algo May 3, 2023 19:13

ahangsu added 3 commits May 4, 2023 10:29

generate TEAL file, rather than handwritten one

c112a57

minor, shrink by optional cmdline arg

ca4b285

Merge branch 'master' into simulate-extra-budget

fb15dd6

jasonpaulos reviewed May 4, 2023

View reviewed changes

ahangsu added 2 commits May 4, 2023 15:16

renaming from allow-* to extra-app-budget

fb69bc4

request extra budget ptr to instance

9b305be

guard around ep.PooledApplicationBudget for nil ptr

32abe6b

algoanne added the Team Scytale label May 4, 2023

ahangsu added 2 commits May 4, 2023 19:30

extra testcase, that test cost budget exceed, even if with extra budget

0b0eb97

update error returning if exceed simulation extra budget limit

d8ba678

ahangsu requested review from jannotti and jasonpaulos May 5, 2023 02:07

jannotti reviewed May 5, 2023

View reviewed changes

ahangsu force-pushed the simulate-extra-budget branch from 1069eee to 33d910f Compare May 5, 2023 15:38

review comments response 1

be8d6cb

ahangsu force-pushed the simulate-extra-budget branch from 33d910f to be8d6cb Compare May 5, 2023 15:41

jasonpaulos reviewed May 5, 2023

View reviewed changes

daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved

ledger/simulation/simulator.go Outdated Show resolved Hide resolved

ledger/simulation/trace.go Outdated Show resolved Hide resolved

omitempty, InvalidRequestError

cece257

ahangsu force-pushed the simulate-extra-budget branch from 9d87e9d to d305f96 Compare May 5, 2023 18:56

extra-app-budget -> extra-opcode-budget, overrides ptr to instance ui…

3563c28

…nt64

ahangsu force-pushed the simulate-extra-budget branch from d305f96 to 3563c28 Compare May 5, 2023 19:03

remove x-algorand-format fields

650456a

bbroder-algo previously approved these changes May 5, 2023

View reviewed changes

jasonpaulos reviewed May 5, 2023

View reviewed changes

more test in e2e for --allow-extra-opcode-budget, pr comment response

2eebb81

ahangsu dismissed bbroder-algo’s stale review via 2eebb81 May 5, 2023 21:34

jasonpaulos reviewed May 5, 2023

View reviewed changes

test/scripts/e2e_subs/e2e-app-simulate.sh Outdated Show resolved Hide resolved

cmd/goal/clerk.go Outdated Show resolved Hide resolved

flag description fix and e2e error line fix

37ad9b0

jasonpaulos approved these changes May 5, 2023

View reviewed changes

bbroder-algo approved these changes May 5, 2023

View reviewed changes

bbroder-algo merged commit 9e980d3 into algorand:master May 5, 2023
24 checks passed

ahangsu deleted the simulate-extra-budget branch May 5, 2023 22:16

This was referenced May 9, 2023

go-algorand 3.16.0-beta Release PR #5370

Closed

go-algorand 3.16.0-beta Release PR #5371

Closed

go-algorand 3.16.0-beta Release PR #5382

Closed

Algo-devops-service mentioned this pull request May 22, 2023

go-algorand 3.16.0-beta Release PR #5406

Merged

This was referenced May 24, 2023

go-algorand 3.16.0-beta Release PR #5417

Merged

go-algorand 3.16.0-beta Release PR #5430

Merged

go-algorand 3.16.0-beta Release PR #5434

Merged

Algo-devops-service mentioned this pull request Jun 12, 2023

go-algorand 3.16.1-stable Release PR #5465

Merged

onetechnical mentioned this pull request Jun 14, 2023

go-algorand 3.16.2-stable Release PR #5469

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Algod: Simulation run with extra budget per transaction group #5354

Algod: Simulation run with extra budget per transaction group #5354

ahangsu commented May 3, 2023 •

edited

codecov bot commented May 4, 2023 •

edited

jasonpaulos left a comment

ahangsu commented May 4, 2023

ahangsu commented May 5, 2023

jannotti May 5, 2023

ahangsu May 5, 2023

jannotti May 5, 2023

ahangsu May 5, 2023

jasonpaulos May 5, 2023

bbroder-algo May 5, 2023

jasonpaulos left a comment

Algod: Simulation run with extra budget per transaction group #5354

Algod: Simulation run with extra budget per transaction group #5354

Conversation

ahangsu commented May 3, 2023 • edited

Summary

Test Plan

codecov bot commented May 4, 2023 • edited

Codecov Report

jasonpaulos left a comment

Choose a reason for hiding this comment

ahangsu commented May 4, 2023

ahangsu commented May 5, 2023

jannotti May 5, 2023

Choose a reason for hiding this comment

ahangsu May 5, 2023

Choose a reason for hiding this comment

jannotti May 5, 2023

Choose a reason for hiding this comment

ahangsu May 5, 2023

Choose a reason for hiding this comment

jasonpaulos May 5, 2023

Choose a reason for hiding this comment

bbroder-algo May 5, 2023

Choose a reason for hiding this comment

jasonpaulos left a comment

Choose a reason for hiding this comment

ahangsu commented May 3, 2023 •

edited

codecov bot commented May 4, 2023 •

edited