Stack manipulation opcodes - uncover and cover #2541

AyAggarwal · 2021-07-14T00:10:50Z

This PR adds two opcodes.
cover: "remove top of stack, and place it down the stack such that N elements are above it",
uncover: "remove the value at depth N in the stack and shift above items down so the Nth deep value is on top of the stack"

… stackopcodes

codecov-commenter · 2021-07-14T01:17:50Z

Codecov Report

Merging #2541 (90e3521) into master (58fd804) will increase coverage by 0.04%.
The diff coverage is 84.00%.

@@            Coverage Diff             @@
##           master    #2541      +/-   ##
==========================================
+ Coverage   46.97%   47.01%   +0.04%     
==========================================
  Files         348      348              
  Lines       55716    55766      +50     
==========================================
+ Hits        26172    26220      +48     
+ Misses      26600    26598       -2     
- Partials     2944     2948       +4

Impacted Files	Coverage Δ
data/transactions/logic/doc.go	`82.75% <ø> (ø)`
data/transactions/logic/opcodes.go	`96.00% <ø> (ø)`
data/transactions/logic/assembler.go	`81.97% <75.00%> (-0.17%)`	⬇️
data/transactions/logic/eval.go	`90.70% <100.00%> (+0.09%)`	⬆️
ledger/blockqueue.go	`82.18% <0.00%> (-2.88%)`	⬇️
catchup/service.go	`70.31% <0.00%> (ø)`
network/wsPeer.go	`75.20% <0.00%> (+3.06%)`	⬆️

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 58fd804...90e3521. Read the comment docs.

AyAggarwal · 2021-07-14T02:34:05Z

Hello All, the PR build is failing due to "scripts/travis/codegen_verification.sh". Will look into this tomorrow morning (PST)

jannotti

I think the shifts can be done clearer and faster with copy()

jannotti · 2021-07-14T02:37:11Z

data/transactions/logic/eval.go

+func opCover(cx *evalContext) {
+	depth := int(uint(cx.program[cx.pc+1]))
+	idx := len(cx.stack) - 1 - depth
+	// Need to check stack size explicitly here because checkArgs() doesn't understand dig


Cover, not dig

jannotti · 2021-07-14T02:38:54Z

data/transactions/logic/eval.go

+		return
+	}
+	topIndex := len(cx.stack) - 1
+	Idxsv := cx.stack[idx]


Use lower case for locals

AyAggarwal · 2021-07-14T07:02:38Z

updated to use copy

jannotti · 2021-07-14T12:45:48Z

I'm realizing there is a tricky issue with these opcodes - normally our assembler and the Check() call try to keep track of the types on the stack. If you push an int, then a bytes, then use +, you will get an error at assembly time. (In the presence of branches, we downgrade this error to a warning).

This can only happen because each opcode (so far) only manipulates the top of the stack. The OpSpec describes that manipulation in terms of types. But these opcodes are different. They manipulate the stack deeply, and potentially change the types of many elements (the element doesn't change, but the type at a particular location does, as they all shift).

We need to figure out how that can be made to work. It might just be that they need their own check function (like the branches have) or it might be that they can explicitly manipulate the type stack during assembly. They might need both, or even more.

First step, let's get a unit test in that shows the problem. Something like:

int 4; byte "john"; int 5; cover 2; pop; +

I think this will not type check, but it is correct (if I haven't messed up).

jdtzmn

Mostly good, nice work on this! I left a few comments.

data/transactions/logic/eval.go

jdtzmn · 2021-07-14T14:34:52Z

data/transactions/logic/eval_test.go

 	testPanics(t, "int 3; int 2; int 1; dig 11; int 2; ==; return", 3)
 }

+func TestCover(t *testing.T) {


It would be nice to see test cases here that check that other elements are shifted correctly

will add these

algochoi

Good work! Just very minor comments.

Based on the Travis error, maybe try running make sanity on the root folder or go fmt again?

algochoi · 2021-07-14T14:26:12Z

data/transactions/logic/eval.go

+		cx.err = fmt.Errorf("cover %d with stack size = %d", depth, len(cx.stack))
+		return
+	}
+	topidx := len(cx.stack) - 1


nit: topIdx

algochoi · 2021-07-14T14:26:38Z

data/transactions/logic/eval.go

+		cx.err = fmt.Errorf("uncover %d with stack size = %d", depth, len(cx.stack))
+		return
+	}
+	topidx := len(cx.stack) - 1


nit: topIdx

I think Go convention is to camelCase local variables

data/transactions/logic/eval_test.go

algochoi · 2021-07-14T14:36:20Z

data/transactions/logic/eval.go

+		return
+	}
+	topidx := len(cx.stack) - 1
+	idxsv := cx.stack[idx]


nit: I think this should be sv (based on the dig opcode)

algochoi · 2021-07-14T14:54:03Z

data/transactions/logic/eval.go

+		return
+	}
+	topidx := len(cx.stack) - 1
+	topsv := cx.stack[topidx]


nit: maybe sv (based on the dig opcode)?

jannotti · 2021-07-15T01:34:28Z

It looks to me like all we need to do to address the type checking is to manipulate the type stack in the assemble function for cover/uncover. Currently they use asmDefault. So they'll need custom assemblers, but it should pretty much just call the default assembler, and manipulate the stack.

… stackopcodes

jannotti · 2021-07-19T14:34:31Z

They are byte immediates. Still, it seems as though this double cast is unneeded. I recall seeing it before, messing with it a bit, and for some reason leaving it. I can't see why, right now. In C, I guess something similar would be needed, as char is not defined to be signed or unsigned. But byte is definitely unsigned in Go, and the simple cast to int should preserve the sign because there's room in int for the entire range of byte.

…

On Mon, Jul 19, 2021 at 10:09 AM Jacob Daitzman ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In data/transactions/logic/eval.go <#2541 (comment)>: > @@ -1700,6 +1700,36 @@ func opDig(cx *evalContext) { cx.stack = append(cx.stack, sv) } +func opCover(cx *evalContext) { + depth := int(uint(cx.program[cx.pc+1])) Aren't uint64 immediates required to be non-negative? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#2541 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADL7TZG4725TJYM6TFF64DTYQW2DANCNFSM5AKLPDSQ> .

jdtzmn · 2021-07-20T14:15:54Z

data/transactions/logic/assembler.go

+		}
+		returns[len(returns)-1] = sv
+	}
+	fmt.Println(returns)


nit: fmt.Println

jdtzmn · 2021-07-20T14:17:03Z

data/transactions/logic/assembler.go

+		returns[i] = StackAny
+	}
+	idx := len(ops.typeStack) - depth
+	if idx >= 0 {


Will the idx here ever be less than 0? Won't that be caught in the opCover function?

I believe this check occurs first, as jj said below opCover should now be sure that it has the correct size stack

This is actually because the arrays will create out of bounds error if you don't check here. It doesn't panic though

jdtzmn · 2021-07-20T14:23:22Z

data/transactions/logic/assembler.go

+	idx := len(ops.typeStack) - depth
+	if idx >= 0 {
+		sv := ops.typeStack[len(ops.typeStack)-1]
+		for i := idx; i < len(ops.typeStack)-1; i++ {


Could this be shifted more cleanly using copy()?

It could, but I'm to blame for setting the precedent with dig. We need cleaner helper functions for building these arrays in the type* functions. Then we can also do select, dup, dup.

Agreed. Does it make sense to include those changes in this PR or should it be done in a separate one?

I think we should do those separately. I'm still mulling over the right interface, but I think I'm imagining two things, one I'm fairly sure about, and one less so.

Fairly sure: the typeCheck functions should check the appropriate depth first, and return early if the stack is short (with an appropriately sized slice to cause checkStack to complain about length). This means the rest of the typeCheck function can assume the stack is deep enough, and it'll be easier to write the code (using copy!) that returns slices that match parts of the existing stack.

Less sure: the interface should be reworked a little so that immediates are parsed early in one place, and supplied in a structured way to the typeCheck and assemble functions. That way it only happens once, and assemble functions are no longer expected to do that parsing or error checking.

I agree with the first one for sure, the second point is good as well - would that still mean if a program failed in typeCheck it fails during assembly?

jdtzmn · 2021-07-20T14:36:32Z

data/transactions/logic/eval.go

+		cx.err = fmt.Errorf("cover %d with stack size = %d", depth, len(cx.stack))
+		return
+	}
+	topIdx := len(cx.stack) - 1


This may read better if you move topIdx above idx like so:

topIdx := len(cx.stack - 1) idx := topIdx - depth

jannotti · 2021-07-21T00:51:51Z

data/transactions/logic/eval.go

+	// Need to check stack size explicitly here because checkArgs() doesn't understand cover
+	// so we can't expect our stack to be prechecked.


This may not be true any more with the changes to checkStack. I think that now opCover is sure to have a big enough stack to work with.

good point, i'll re visit this

jannotti · 2021-07-21T00:54:31Z

data/transactions/logic/eval_test.go

+	testAccepts(t, "int 4; int 3; int 2; int 1; uncover 3; pop; int 1; ==; return", 5)
+	testAccepts(t, "int 4; int 3; int 2; int 1; uncover 3; pop; pop; int 2; ==; return", 5)
+	testAccepts(t, "int 1; int 3; int 2; int 1; uncover 3; pop; pop; int 2; ==; return", 5)
+	testPanics(t, obfuscate("int 4; int 3; int 2; int 1; uncover 11; int 3; ==; return"), 5)


Can you add one that panics, but just barely? In other words, confirm theres no off-by-one error by uncovering with an argument that's just one too high.

… stackopcodes

algorandskiy

Looks good, one more test request and a minor code style fix

algorandskiy · 2021-07-27T15:24:00Z

data/transactions/logic/doc.go

 	"Byteslice Logic":      {"b|", "b&", "b^", "b~"},
 	"Loading Values":       {"intcblock", "intc", "intc_0", "intc_1", "intc_2", "intc_3", "pushint", "bytecblock", "bytec", "bytec_0", "bytec_1", "bytec_2", "bytec_3", "pushbytes", "bzero", "arg", "arg_0", "arg_1", "arg_2", "arg_3", "txn", "gtxn", "txna", "gtxna", "gtxns", "gtxnsa", "global", "load", "store", "gload", "gloads", "gaid", "gaids"},
-	"Flow Control":         {"err", "bnz", "bz", "b", "return", "pop", "dup", "dup2", "dig", "swap", "select", "assert", "callsub", "retsub"},
+	"Flow Control":         {"err", "bnz", "bz", "b", "return", "pop", "dup", "dup2", "dig", "cover", "uncover", "swap", "select", "assert", "callsub", "retsub"},


just wondering how did "pop", "dup", "dup2", "dig", "cover", "uncover" end up to be flow control

I think it's because it used to only be pop and dup so we didn't create a category for stack manipulation.

algorandskiy · 2021-07-27T15:31:45Z

data/transactions/logic/eval_test.go

+
+func TestUncover(t *testing.T) {
+	t.Parallel()
+	testAccepts(t, "int 4; int 3; int 2; int 1; uncover 2; int 3; ==; return", 5)


please add unvcover 0 test as an edge case test

algorandskiy · 2021-07-27T15:35:14Z

data/transactions/logic/eval.go

+	depth := int(cx.program[cx.pc+1])
+	idx := len(cx.stack) - 1 - depth
+	// Need to check stack size explicitly here because checkArgs() doesn't understand uncover
+	// so we can't expect our stack to be prechecked.
+	if idx < 0 {
+		cx.err = fmt.Errorf("uncover %d with stack size = %d", depth, len(cx.stack))
+		return
+	}
+	topIdx := len(cx.stack) - 1


make it the same as prev function

Suggested change

depth := int(cx.program[cx.pc+1])

idx := len(cx.stack) - 1 - depth

// Need to check stack size explicitly here because checkArgs() doesn't understand uncover

// so we can't expect our stack to be prechecked.

if idx < 0 {

cx.err = fmt.Errorf("uncover %d with stack size = %d", depth, len(cx.stack))

return

}

topIdx := len(cx.stack) - 1

depth := int(cx.program[cx.pc+1])

topIdx := len(cx.stack) - 1

idx := topIdx - depth

// Need to check stack size explicitly here because checkArgs() doesn't understand uncover

// so we can't expect our stack to be prechecked.

if idx < 0 {

cx.err = fmt.Errorf("uncover %d with stack size = %d", depth, len(cx.stack))

return

}

… stackopcodes

jannotti

Looks good.

Ayush Aggarwal added 4 commits July 12, 2021 02:53

resolve git conflicts

27f621f

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

75e6c2d

… stackopcodes

f

4cdc045

passing go tests

7e4f9cd

AyAggarwal requested review from algochoi, jannotti, jdtzmn and shiqizng July 14, 2021 02:33

jannotti reviewed Jul 14, 2021

View reviewed changes

copy instead of loops

5aed820

AyAggarwal added the Team Scytale label Jul 14, 2021

AyAggarwal linked an issue Jul 14, 2021 that may be closed by this pull request

ABI - AVM Support - selector and dig/bury #2346

Closed

2 tasks

jdtzmn suggested changes Jul 14, 2021

View reviewed changes

algochoi reviewed Jul 14, 2021

View reviewed changes

Ayush Aggarwal added 2 commits July 14, 2021 15:24

added tests and cosmetic fixes

147874e

resolve merge conflicts

4668fb7

Ayush Aggarwal added 6 commits July 16, 2021 00:40

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

8c47816

… stackopcodes

merging new stack infra

48c762c

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

ddc6d9b

… stackopcodes

implemented type stackchanges

e83a70f

one more test

97c9a4a

formatting

136e264

removed uint cast

117ece1

AyAggarwal requested a review from jdtzmn July 19, 2021 20:06

jdtzmn suggested changes Jul 20, 2021

View reviewed changes

jannotti previously approved these changes Jul 21, 2021

View reviewed changes

Ayush Aggarwal added 2 commits July 22, 2021 16:42

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

a3331be

… stackopcodes

resolved comments and added more tests

90e3521

AyAggarwal dismissed jannotti’s stale review via 90e3521 July 22, 2021 21:01

jdtzmn previously approved these changes Jul 23, 2021

View reviewed changes

AyAggarwal requested a review from jannotti July 26, 2021 20:09

algorandskiy reviewed Jul 27, 2021

View reviewed changes

Ayush Aggarwal added 2 commits July 27, 2021 11:40

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

19641c4

… stackopcodes

added edge case and style changes

cc6cd06

AyAggarwal dismissed jdtzmn’s stale review via cc6cd06 July 27, 2021 15:44

algorandskiy requested a review from jdtzmn July 27, 2021 15:49

algorandskiy approved these changes Jul 27, 2021

View reviewed changes

jannotti approved these changes Jul 27, 2021

View reviewed changes

algojohnlee merged commit 5908fe5 into master Jul 27, 2021

tsachiherman deleted the stackopcodes branch August 5, 2021 13:08

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

jasonpaulos mentioned this pull request Sep 8, 2021

Simplify recursive subroutine calling in TEAL 5 algorand/pyteal#110

Closed

		// Need to check stack size explicitly here because checkArgs() doesn't understand cover
		// so we can't expect our stack to be prechecked.

Stack manipulation opcodes - uncover and cover #2541

Stack manipulation opcodes - uncover and cover #2541

Uh oh!

Conversation

AyAggarwal commented Jul 14, 2021

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

AyAggarwal commented Jul 14, 2021

Uh oh!

jannotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AyAggarwal commented Jul 14, 2021

Uh oh!

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

Uh oh!

Uh oh!

jdtzmn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

algochoi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

algochoi Jul 14, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannotti commented Jul 15, 2021

Uh oh!

jannotti commented Jul 19, 2021 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 14, 2021 •

edited

Loading

jannotti commented Jul 14, 2021 •

edited

Loading

algochoi Jul 14, 2021 •

edited

Loading