Inner clearstate by jannotti · Pull Request #3556 · algorand/go-algorand

jannotti · 2022-02-03T02:39:15Z

Prevents inner txns and bad opcode budget use in clear state programs.

Should also implement the "CSP requires 700 budget and can't spend more than 700 budget" rule, but tests still required.

Co-authored-by: Michael Diamant <michaeldiamant@users.noreply.github.com>

tzaffi · 2022-02-04T22:41:49Z

+}
+
+func (e ClearStateBudgetError) Error() string {
+	return fmt.Sprintf("ClearState execution with only %d", e.offered)


nit: can the error message refer to PooledApplicationBudget or something like that?

Changed:
return fmt.Sprintf("Attempted ClearState execution with low OpcodeBudget %d", e.offered)
That should call to mind the global OpcodeBudget which is the problem.

jasonpaulos

Looks correct, I just have questions/comments on tests.

Additionally, I don't think there's any tests for the changes in clear state budget? One suggestion would be a test where an app calls an inner clear state that has an infinite loop and we verify the global OpcodeBudget only decreases by ~700. Another test I'd like to see is one that makes sure ClearStateBudgetError actually fails the txn.

jannotti · 2022-02-04T23:42:14Z

Looks correct, I just have questions/comments on tests.

Additionally, I don't think there's any tests for the changes in clear state budget? One suggestion would be a test where an app calls an inner clear state that has an infinite loop and we verify the global OpcodeBudget only decreases by ~700. Another test I'd like to see is one that makes sure ClearStateBudgetError actually fails the txn.

Correct, I still need both tests. Going into csp with less than 700 should NOT clear, but should fail. A csp that tries to take 701 should fail, but not kill caller, and should clear state.

codecov-commenter · 2022-02-05T00:55:50Z

Codecov Report

Merging #3556 (ecefe2b) into feature/contract-to-contract (84b52e7) will increase coverage by 0.07%.
The diff coverage is 60.00%.

@@                       Coverage Diff                        @@
##           feature/contract-to-contract    #3556      +/-   ##
================================================================
+ Coverage                         47.80%   47.88%   +0.07%     
================================================================
  Files                               370      370              
  Lines                             59959    60019      +60     
================================================================
+ Hits                              28665    28740      +75     
+ Misses                            27994    27977      -17     
- Partials                           3300     3302       +2

Impacted Files	Coverage Δ
data/transactions/logic/opcodes.go	`100.00% <ø> (ø)`
ledger/apply/application.go	`79.52% <33.33%> (-0.67%)`	⬇️
data/transactions/transaction.go	`32.38% <46.15%> (+1.40%)`	⬆️
data/transactions/logic/eval.go	`85.60% <68.42%> (-0.32%)`	⬇️
config/consensus.go	`84.74% <100.00%> (+0.04%)`	⬆️
ledger/internal/appcow.go	`81.36% <100.00%> (+0.11%)`	⬆️
ledger/blockqueue.go	`82.18% <0.00%> (-2.88%)`	⬇️
catchup/service.go	`68.88% <0.00%> (-0.50%)`	⬇️
data/transactions/msgp_gen.go	`41.35% <0.00%> (+0.49%)`	⬆️
network/wsPeer.go	`68.61% <0.00%> (+0.55%)`	⬆️
... and 9 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 84b52e7...ecefe2b. Read the comment docs.

jasonpaulos

Changes look good to me

jannotti added 2 commits February 2, 2022 13:13

Test current state of inner clear state behavior

fca0a47

First half of clear state fixes. Prevents inner calls in CSP

bd29bd8

Should also implement the "CSP requires 700 budget and can't spend more than 700 budget" rule, but tests still required.

jannotti force-pushed the inner-clearstate branch from 1cbe56e to bd29bd8 Compare February 3, 2022 19:44

Match program version. Don't allow downgrade < 6.

03b124a

jannotti requested a review from jasonpaulos February 4, 2022 19:31

jannotti force-pushed the inner-clearstate branch from 2d2e2df to e863765 Compare February 4, 2022 19:34

partition test

ce12e59

jannotti force-pushed the inner-clearstate branch from e863765 to ce12e59 Compare February 4, 2022 19:34

michaeldiamant reviewed Feb 4, 2022

View reviewed changes

Comment thread data/transactions/logic/eval.go Outdated

typo

97cc248

Co-authored-by: Michael Diamant <michaeldiamant@users.noreply.github.com>

michaeldiamant reviewed Feb 4, 2022

View reviewed changes

Comment thread data/transactions/logic/eval.go Outdated

jannotti and others added 3 commits February 4, 2022 14:46

typo

9c9f592

Co-authored-by: Michael Diamant <michaeldiamant@users.noreply.github.com>

Make the default clear_state program match versions to approval

551eb10

Correct some tests that now need programs present

1eea69f

tzaffi reviewed Feb 4, 2022

View reviewed changes

jasonpaulos reviewed Feb 4, 2022

View reviewed changes

Comment thread ledger/internal/apptxn_test.go

Fix test to actually test INNER clear state

5e5ac43

jannotti added 5 commits February 5, 2022 13:27

Add tests for opcode budget and CSPs

27f90a6

Improve tracing for inner app calls

91979a5

Error now reports what the cost *was* before the attempted inst.

a18cb7b

Simplify type switch and maybe kick CI?

5f94fdd

Fewer copies during inner processing

ecefe2b

jannotti merged commit abe3c6a into algorand:feature/contract-to-contract Feb 6, 2022

michaeldiamant reviewed Feb 7, 2022

View reviewed changes

Comment thread config/consensus.go

michaeldiamant reviewed Feb 7, 2022

View reviewed changes

Comment thread data/transactions/logic/eval.go

michaeldiamant mentioned this pull request Feb 7, 2022

Document clear state program constraints with inner transactions algorandfoundation/docs#733

Merged

jasonpaulos reviewed Feb 7, 2022

View reviewed changes

michaeldiamant mentioned this pull request Feb 7, 2022

Document rationale for disallowing clear state program inner transactions #3586

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inner clearstate#3556

Inner clearstate#3556
jannotti merged 14 commits intoalgorand:feature/contract-to-contractfrom
jannotti:inner-clearstate

jannotti commented Feb 3, 2022

Uh oh!

Uh oh!

Uh oh!

tzaffi Feb 4, 2022 •

edited

Loading

Uh oh!

jannotti Feb 5, 2022

Uh oh!

jasonpaulos left a comment •

edited

Loading

Uh oh!

Uh oh!

jannotti commented Feb 4, 2022

Uh oh!

codecov-commenter commented Feb 5, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

jasonpaulos left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

jannotti commented Feb 3, 2022

Uh oh!

Uh oh!

Uh oh!

tzaffi Feb 4, 2022 • 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!

jannotti Feb 5, 2022

Choose a reason for hiding this comment

Uh oh!

jasonpaulos left a comment • 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!

jannotti commented Feb 4, 2022

Uh oh!

codecov-commenter commented Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

jasonpaulos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tzaffi Feb 4, 2022 •

edited

Loading

jasonpaulos left a comment •

edited

Loading

codecov-commenter commented Feb 5, 2022 •

edited

Loading