Dryrun return cost by AyAggarwal · Pull Request #2746 · algorand/go-algorand

AyAggarwal · 2021-08-16T05:34:01Z

This PR adds a Cost Parameter to DryrunResponse. It is calculated by giving the evalParams object in dryrun.go the maximum budget allowed (maxappcost * maxgroupsize) and checking the difference after stateful evaluation. LogicSig dryruns will return cost of 0.

Added a single test with sqrt and default opcodes

… dryrunCost

jannotti · 2021-08-16T11:03:37Z

Seems like there's a gofmt error.

Make sure you have a test that uses more than 700 units.

I don't think cost should be returned at all in the json for logic sigs. That seems misleading. (It's possible it isn't, as we often omit 0s in responses.)

algorandskiy · 2021-08-16T13:40:49Z

+	t.Log("App Cost:")
+	t.Log(response.Cost)


not needed debug output

algorandskiy

Protocol modification does not look correct

jannotti

Looks like we need to decide how to handle some protocol parmater things, and test that the api response is updated to include cost.

codecov-commenter · 2021-08-18T05:02:07Z

Codecov Report

Merging #2746 (1f2817f) into master (80bd5ed) will decrease coverage by 0.00%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master    #2746      +/-   ##
==========================================
- Coverage   47.14%   47.14%   -0.01%     
==========================================
  Files         349      349              
  Lines       56322    56345      +23     
==========================================
+ Hits        26553    26563      +10     
- Misses      26800    26813      +13     
  Partials     2969     2969

Impacted Files	Coverage Δ
cmd/goal/clerk.go	`9.11% <0.00%> (-0.03%)`	⬇️
daemon/algod/api/server/v2/dryrun.go	`61.53% <84.61%> (+2.54%)`	⬆️
ledger/roundlru.go	`90.56% <0.00%> (-5.67%)`	⬇️
agreement/cryptoVerifier.go	`75.73% <0.00%> (-2.21%)`	⬇️
agreement/proposalManager.go	`96.07% <0.00%> (-1.97%)`	⬇️
data/transactions/verify/txn.go	`45.08% <0.00%> (-0.90%)`	⬇️
network/requestTracker.go	`70.25% <0.00%> (-0.87%)`	⬇️
catchup/service.go	`69.35% <0.00%> (-0.78%)`	⬇️
ledger/acctupdates.go	`62.63% <0.00%> (+0.16%)`	⬆️
cmd/tealdbg/debugger.go	`73.86% <0.00%> (+1.00%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80bd5ed...1f2817f. Read the comment docs.

AyAggarwal · 2021-08-20T06:19:35Z

did 3 things:

added better commenting to explain what is going on
preserved user's EnableAppCostPooling choice and added logic to reject if any program is above maxAppCost if the user did not enable originally
implemented maxCost estimate by using appCost * numAppCalls, and REJECT programs that surpass this. added tests for this case

shiqizng

just a couple questions, but others look fine to me.

shiqizng · 2021-08-25T19:15:15Z

+			require.Empty(t, response.Error)
+			require.Equal(t, 2, len(response.Txns))
+
+			//dryrun call will execute but fail because the first program exceeds max possible cost


This is not true for the test case 2, right?

yes, I'll remove this old comment

shiqizng · 2021-08-25T19:37:39Z

        -re {(ApprovalProgram)} {set PROGRAM_TYPE $expect_out(1,string); exp_continue}
        "PASS" {set PASSED 1; close}
    }
+    if { $COST == 0 } {


wouldn't this always be true as long as line 12 is executed?

this looks for "cost:" string in the output. if it is there then COST = 1 and this condition does trigger termination.

shiqizng

looks good to me

jasonpaulos

Looks good, but I have a suggestion to improve the cost budget exceeded message

Ouddated, goal clerk changed and emits cost

Ayush Aggarwal added 3 commits August 16, 2021 01:01

added cost field to dryron response

9348e65

Merge branch 'master' of https://github.com/algorand/go-algorand into…

73b49f1

… dryrunCost

hardcoded proto parameters in line

8dd0dcb

AyAggarwal requested review from jannotti and jasonpaulos August 16, 2021 05:35

algorandskiy reviewed Aug 16, 2021

View reviewed changes

algorandskiy requested changes Aug 16, 2021

View reviewed changes

Comment thread daemon/algod/api/server/v2/dryrun.go

jannotti previously requested changes Aug 16, 2021

View reviewed changes

Comment thread daemon/algod/api/server/v2/dryrun.go Outdated

Comment thread daemon/algod/api/server/v2/dryrun.go Outdated

Comment thread daemon/algod/api/server/v2/dryrun_test.go

jasonpaulos reviewed Aug 17, 2021

View reviewed changes

Comment thread daemon/algod/api/server/v2/generated/types.go Outdated

fixed dryrun protocol version to use vfuture

4914b2b

algorandskiy reviewed Aug 18, 2021

View reviewed changes

Comment thread daemon/algod/api/algod.oas3.yml Outdated

Comment thread daemon/algod/api/server/v2/dryrun.go

Comment thread daemon/algod/api/server/v2/dryrun.go Outdated

updated with feedback

5b627eb

github-actions Bot reviewed Aug 20, 2021

View reviewed changes

Comment thread daemon/algod/api/server/v2/dryrun.go Outdated

added more testing

45b2b4b

fmt

fb0a41d

shiqizng reviewed Aug 24, 2021

View reviewed changes

Comment thread daemon/algod/api/algod.oas2.json Outdated

algorandskiy added 2 commits August 25, 2021 13:24

Move cost report to txn and add more test

96b6c1e

Add cost output to dryrun-remote and a test

77d9308

algorandskiy previously approved these changes Aug 25, 2021

View reviewed changes

algorandskiy dismissed their stale review via c0b47cf August 25, 2021 19:46

shiqizng reviewed Aug 25, 2021

View reviewed changes

algorandskiy added the Team Scytale label Aug 25, 2021

algorandskiy self-assigned this Aug 25, 2021

Produce an error if cost exceeded the budget

15eeffb

algorandskiy force-pushed the dryrunCost branch from c0b47cf to 15eeffb Compare August 25, 2021 19:57

Fix cumulative cost check against allowed budget

baa7036

shiqizng previously approved these changes Aug 25, 2021

View reviewed changes

jasonpaulos previously approved these changes Aug 25, 2021

View reviewed changes

Comment thread daemon/algod/api/server/v2/dryrun.go Outdated

Update cost error message per review

1f2817f

algorandskiy dismissed stale reviews from jasonpaulos and shiqizng via 1f2817f August 25, 2021 21:18

jasonpaulos approved these changes Aug 25, 2021

View reviewed changes

shiqizng approved these changes Aug 26, 2021

View reviewed changes

algojohnlee merged commit 2b03a60 into master Aug 26, 2021

onetechnical deleted the dryrunCost branch August 26, 2021 14:15

onetechnical mentioned this pull request Sep 15, 2021

go-algorand 3.0.0-beta #2893

Merged

algojack mentioned this pull request Sep 24, 2021

go-algorand 3.0.1-stable #2949

Merged

Conversation

AyAggarwal commented Aug 16, 2021

Uh oh!

jannotti commented Aug 16, 2021

Uh oh!

Uh oh!

algorandskiy Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

algorandskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jannotti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AyAggarwal commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

shiqizng left a comment

Choose a reason for hiding this comment

Uh oh!

shiqizng Aug 25, 2021

Choose a reason for hiding this comment

Uh oh!

algorandskiy Aug 25, 2021

Choose a reason for hiding this comment

Uh oh!

shiqizng Aug 25, 2021

Choose a reason for hiding this comment

Uh oh!

algorandskiy Aug 25, 2021

Choose a reason for hiding this comment

Uh oh!

shiqizng left a comment

Choose a reason for hiding this comment

Uh oh!

jasonpaulos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-commenter commented Aug 18, 2021 •

edited

Loading

AyAggarwal commented Aug 20, 2021 •

edited

Loading