Pool App call budget in group transactions #2711

algochoi · 2021-08-07T02:05:39Z

Summary

Adds feature to allow App calls to pool their budget in a grouped transaction (e.g. in a 16 app call txn group, the entire group has access to 11200 units of pooled cost).
Allows ed25519verify to be called in Application calls, given that there is enough budget in the pool
Added vFuture consensus parameter

Test Plan

Added unit tests in ledger/ to check that group transactions pool the App call budget (700 * number of app calls)
Added tests to check that pooling is not allowed in V28
Added tests to check that ed25519verify is allowed in App calls, given enough budget in the pool

… algochoi/pool-tx-costs

jannotti

Let's discuss the shared budget pointer idea. I think it fits more closely with how we have done some other recent features. (In fact, see my PR for how I'm handling the excess fee credit in app txn creation: Look for "FeeCredit" in https://github.com/algorand/go-algorand/pull/2661/files)

config/consensus.go

data/transactions/logic/eval.go

data/transactions/logic/evalStateful_test.go

ledger/appcow.go

ledger/eval.go

data/transactions/logic/eval.go

algochoi · 2021-08-09T17:29:08Z

ledger/eval_test.go

+		{approvalProgram2, "dynamic cost budget exceeded, executing keccak256: remaining budget is 700 but program cost was 783"},
+	}
+
+	for _, testCase := range cases {


I think at one point we wanted to do the protocol checks in data/transactions/logic/evalStateful_test.go, but there is some ledger code that also needs to be tested (prepareEvalParams) for correct protocol, so I put this test in the ledger tests.

these tests look good but I think we should not test everything here because in ledger package we only set pooled budget for multiple app calls in one group. And the decisions are made in logic.
Let's do the following:

a unit test forprepareEvalParams call on a group with 2 txns with app calls on old and new protos. No ledger construction/blocks needed.

Preserve a high-level test with block evaluator: no pooling on old proto and pooling on new proto. Programs should be simple but valid, something like "byte "a"\n" + strings.Repeat("keccak256\n", 50).

Make more precise testing on ed25519verify in logic package - it is needed anyway, and the EvalParams with proto and budget can be easily set there.

I added the prepareEvalParams tests above like you have suggested. Just to be clear, we should probably only check for correct pooling here, and check for cost/budget in the data/transactions/logic/evalStateful_test.go file?

It is always a good idea to check both happy case and error case. The idea is to have 1) minimal sufficient coverage 2) without knowing to much details about stuff from other packeges.
I I suggest to use the following code to generate test programs of difference cost by varying N:

prog := "byte "a\n" + strings.Repeat("keccak256\n", N) + "pop\nint 1\n";

So here in ledger the following four cases would be enough:

cost ~701 + cost ~100 => FAIL on v29.

same combo OK on vFuture

~701 + ~701 => FAIL on both v29 and vFuture

jannotti · 2021-08-09T17:30:03Z

data/transactions/logic/eval.go

+	if pass && cx.EvalParams.Proto.EnableAppCostPooling {
+		// if eval passes, then budget is always greater than cost, so should not have underflow
+		*cx.EvalParams.PooledApplicationBudget -= cx.cost
+	}


If an app doesn't pass, everything's probably going to get aborted. Nonetheless, I don't see a good reason not to decrease the pooled budget.

This also needs to be super careful about underflow. Going negative here almost certainly leads to the possibility of a the budget being interpreted as a high number elsewhere.

ledger/eval.go

data/transactions/logic/eval.go

… algochoi/pool-tx-costs

codecov-commenter · 2021-08-10T01:08:28Z

Codecov Report

Merging #2711 (4c08880) into master (58bc231) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2711   +/-   ##
=======================================
  Coverage   47.09%   47.10%           
=======================================
  Files         350      350           
  Lines       56309    56321   +12     
=======================================
+ Hits        26520    26528    +8     
- Misses      26817    26821    +4     
  Partials     2972     2972

Impacted Files	Coverage Δ
data/transactions/logic/opcodes.go	`96.00% <ø> (ø)`
config/consensus.go	`84.06% <100.00%> (+0.05%)`	⬆️
data/transactions/logic/eval.go	`90.64% <100.00%> (+0.02%)`	⬆️
ledger/eval.go	`75.97% <100.00%> (+0.21%)`	⬆️
ledger/blockqueue.go	`82.18% <0.00%> (-2.88%)`	⬇️
catchup/service.go	`68.57% <0.00%> (-1.56%)`	⬇️
network/requestTracker.go	`70.25% <0.00%> (-0.44%)`	⬇️
network/wsPeer.go	`74.37% <0.00%> (+2.22%)`	⬆️

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 58bc231...4c08880. Read the comment docs.

algorandskiy

Looks good, some work needed: 1) ed25519verify must be only allowed in stateful TEAL v5+ 2) restructure your TEAL tests in legder: it only should check evaluation on old/new proto with as trivial programs as possible. All TEAL heavy-lifting should go into logic package. Especially this change does not depends on ledger interface for TEAL, only EvalParams preparation, so this preparation in isolation, and a big picture needed to be tested there.

data/transactions/logic/opcodes.go

data/transactions/logic/evalStateful_test.go

data/transactions/logic/eval.go

algorandskiy · 2021-08-10T19:51:33Z

ledger/eval_test.go

+		{approvalProgram2, "dynamic cost budget exceeded, executing keccak256: remaining budget is 700 but program cost was 783"},
+	}
+
+	for _, testCase := range cases {


these tests look good but I think we should not test everything here because in ledger package we only set pooled budget for multiple app calls in one group. And the decisions are made in logic.
Let's do the following:

a unit test forprepareEvalParams call on a group with 2 txns with app calls on old and new protos. No ledger construction/blocks needed.

Preserve a high-level test with block evaluator: no pooling on old proto and pooling on new proto. Programs should be simple but valid, something like "byte "a"\n" + strings.Repeat("keccak256\n", 50).

Make more precise testing on ed25519verify in logic package - it is needed anyway, and the EvalParams with proto and budget can be easily set there.

… algochoi/pool-tx-costs

algorandskiy · 2021-08-11T13:55:46Z

ledger/eval_test.go

+	for i, testCase := range cases {
+		t.Run(fmt.Sprintf("i=%d", i), func(t *testing.T) {


this is not must but something you might to consider: there are two almost identical loops.
If fact, it can be done as a single loop over cases.

Remove fake proto from eval construction above

Make params array consisting on 3 elements: fake params with Application: true, MaxAppProgramCost: 700, vFuture, v29.

Modify evalTestCase struct and add number of app calls in tx group (numAppCalls)

Run loops over cases, params, and calculate pooled cost as proto.MaxAppProgramCost * numAppCalls, and compare against a value EvalParams

Good suggestion, will refactor this part.

algorandskiy

LGTM, one question about pushint in tests.

ledger/eval_test.go

… algochoi/pool-tx-costs

algochoi added 3 commits August 6, 2021 18:55

Add pooling for grouped app calls and add unit tests

c6c550a

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

01d5eb2

… algochoi/pool-tx-costs

Fix doc errors

d1aae0c

algochoi changed the title ~~Algochoi/pool tx costs~~ Pool App call budget in group transactions Aug 7, 2021

jannotti requested changes Aug 7, 2021

View reviewed changes

algochoi added 2 commits August 7, 2021 23:03

Pool app call budget in ep

7ee3baa

Check if pooledBudget is not nil in budget call

6265a82

algochoi commented Aug 9, 2021

View reviewed changes

jannotti reviewed Aug 9, 2021

View reviewed changes

ledger/eval.go Outdated Show resolved Hide resolved

jannotti reviewed Aug 9, 2021

View reviewed changes

data/transactions/logic/eval.go Outdated Show resolved Hide resolved

algochoi added 3 commits August 9, 2021 10:41

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

b95de28

… algochoi/pool-tx-costs

Fix app call integration test output and minor syntax changes

e29d240

Minor change in comment

1412935

algochoi requested review from algorandskiy and jannotti August 10, 2021 00:33

algorandskiy requested changes Aug 10, 2021

View reviewed changes

algochoi added 2 commits August 10, 2021 17:45

Modify some tests to get better coverage

8462940

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

9dba150

… algochoi/pool-tx-costs

algorandskiy reviewed Aug 11, 2021

View reviewed changes

algochoi added 3 commits August 11, 2021 10:42

Refactor tests

d38b7e5

Clean up

c4a2f2c

Remove a hardcoded constant

16e147d

algorandskiy reviewed Aug 11, 2021

View reviewed changes

ledger/eval_test.go Show resolved Hide resolved

algochoi requested a review from algorandskiy August 11, 2021 18:22

algorandskiy previously approved these changes Aug 11, 2021

View reviewed changes

ledger/eval_test.go Show resolved Hide resolved

jannotti reviewed Aug 11, 2021

View reviewed changes

ledger/eval_test.go Show resolved Hide resolved

Add pointer equivalence test

5f98b82

algochoi dismissed algorandskiy’s stale review via 5f98b82 August 11, 2021 21:57

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

4c08880

… algochoi/pool-tx-costs

jannotti self-requested a review August 12, 2021 01:55

jannotti approved these changes Aug 12, 2021

View reviewed changes

jannotti merged commit 848f132 into algorand:master Aug 12, 2021

algochoi mentioned this pull request Aug 13, 2021

Fix error message for dynamic cost exceeded algorand/algorand-sdk-testing#140

Merged

This was referenced Aug 19, 2021

DO NOT MERGE --- go-algorand 2.10.0-stable #2773

Closed

go-algorand 2.10.1-beta #2800

Merged

bricerisingalgorand mentioned this pull request Aug 27, 2021

go-algorand 2.10.1-stable #2813

Merged

algochoi deleted the algochoi/pool-tx-costs branch February 17, 2022 22:22

		for i, testCase := range cases {
		t.Run(fmt.Sprintf("i=%d", i), func(t *testing.T) {

Pool App call budget in group transactions #2711

Pool App call budget in group transactions #2711

Uh oh!

Conversation

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

Uh oh!

Summary

Test Plan

Uh oh!

jannotti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

algochoi Aug 9, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

algochoi Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

algorandskiy Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

algorandskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

algorandskiy Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

algochoi Aug 11, 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!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

algochoi commented Aug 7, 2021 •

edited

Loading

algorandskiy Aug 10, 2021 •

edited

Loading

jannotti Aug 9, 2021 •

edited

Loading

codecov-commenter commented Aug 10, 2021 •

edited

Loading

algorandskiy Aug 10, 2021 •

edited

Loading