Dynamic teal #2126

jannotti · 2021-05-03T20:38:40Z

No description provided.

data/transactions/logic/eval.go

algorandskiy

Few comments and suggestions.
Please also consider adding backward branching tests into backward compatibility tests - like the program rejected before teal4 and accepted after

data/transactions/logic/eval.go

algorandskiy · 2021-05-05T18:56:36Z

data/transactions/logic/eval.go

-
-	// Evaluate the program
-	pass, err = eval(program, &cx)
-
-	return pass, err
+	return eval(program, &cx)


not needed change. This place was quite convenient place for debugging (breakpoints/dumpning some info)

algorandskiy · 2021-05-05T20:58:33Z

data/transactions/logic/assembler.go

-func checkPushBytes(cx *evalContext) int {
+func checkPushBytes(cx *evalContext) error {
 	opPushBytes(cx)
-	return 1
+	return cx.err


it looks like check* functions needs to be moved to check.go from the assembler

data/transactions/logic/eval.go

algorandskiy · 2021-05-05T21:16:25Z

data/transactions/logic/eval.go

@@ -774,6 +771,41 @@ func opAddw(cx *evalContext) {
 	cx.stack[last].Uint = sum
 }

+func uint128(hi uint64, lo uint64) *big.Int {
+	whole := new(big.Int).SetUint64(hi)


it is interesting why does not big package like stack-allocated Ints... Have you tried

whole := &big.Int{} whole.SetUint64(hi) ...

?

I think that's fine too, but why prefer two lines / statements instead of a single expression?

I wanted to use syntax that gave a single expression that produced a big.Int from a u64, especially since I needed similar syntax two lines later to create the low word.

data/transactions/logic/eval_test.go

jannotti · 2021-05-06T01:29:22Z

@algorandskiy You asked about testing old backjumps. This test does that

func TestAssembleRejectNegJump(t *testing.T) {
	t.Parallel()
	source := `wat:
int 1
bnz wat
int 2`
	for v := uint64(1); v < backBranchEnabledVersion; v++ {
		t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) {
			testProg(t, source, v, expect{3, "label wat is a back reference..."})
		})
	}
	for v := uint64(backBranchEnabledVersion); v <= AssemblerMaxVersion; v++ {
		t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) {
			testProg(t, source, v)
		})
	}
}

It tests that you get an error in all version up to backBranchEnabledVersion and then it works from that version onward.

This allows callers to do less work. The consensus protocol was already being supplied, so Check() has everything it needed to do the right thing. This also consolidates the code that will act differently for dynamic teal costs.

test/scripts/e2e_subs/v26/teal-v3-only.sh

algorandskiy

Looks good except removing cost checks from txn verifier for pre-eval checks. Scenario: submit a long v2 program with jump to the end on a new and old node before protocol upgrade. It will fail on the old (sum up check) and pass on new.

algorandskiy · 2021-05-11T22:39:23Z

cmd/goal/clerk.go

@@ -1097,7 +1094,7 @@ var dryrunCmd = &cobra.Command{
 			}
 			pass, err := logic.Eval(txn.Lsig.Logic, ep)
 			// TODO: optionally include `inspect` output here?
-			fmt.Fprintf(os.Stdout, "tx[%d] cost=%d trace:\n%s\n", i, cost, sb.String())
+			fmt.Fprintf(os.Stdout, "tx[%d] trace:\n%s\n", i, sb.String())


why not printing the cost after eval?

Because it's hidden inside the evaluation function and not exposed. If we find it important enough, I could rework the interfaces.

data/transactions/logic/README_in.md

data/transactions/logic/opcodes.go

data/transactions/verify/txn.go

algorandskiy · 2021-05-11T23:09:04Z

LGTM

brianolson

Lots of good stuff here. A few small tweaks.

Probably worth having a paragraph in README_in.md about the callstack, how it's invisible except to two instructions, and any depth limit.

data/transactions/logic/eval.go

brianolson · 2021-05-12T18:37:15Z

test/scripts/e2e_subs/v26/teal-v3-only.sh

+cat >${TEMPDIR}/true.teal<<EOF
+#pragma version 3
+int 1
+return


don't need return

Right this is a remnant from the teal-v2 test, where I wanted to be clear that I had v3 code (where return was introduced). I think it still makes some sense here because of that, but the one below should be changed to some v4 opcode.

Nope, I'm wrong. return was in v2. So it's entirely extraneous. I guess I got in the habit of doing that in tests when I wasn't sure of the stack.

test/scripts/e2e_subs/v26/teal-v3-only.sh

jannotti · 2021-05-12T18:44:14Z

Lots of good stuff here. A few small tweaks.

Probably worth having a paragraph in README_in.md about the callstack, how it's invisible except to two instructions, and any depth limit.

I'll add something in TEAL cannot do section - I'll create a new line about lack of indirect jumps and this will naturally fit there. (Though I'll do it the next PR, as I want to avoid having this thing run through CI again, until/unless I get feedback that needs a code change.)

My current thinking is NOT to restrict the call depth, and to simply let that be limited by the runtime of the program. That means a program of loop: callsub loop would get 20,000 deep in a logicsig, but that's just a 20k int array that immediately goes away. Didn't seem to matter. Thoughts?

jasonpaulos

Looks good!

My current thinking is NOT to restrict the call depth, and to simply let that be limited by the runtime of the program. That means a program of loop: callsub loop would get 20,000 deep in a logicsig, but that's just a 20k int array that immediately goes away. Didn't seem to matter. Thoughts?

From a programmer/debugger standpoint this seems like a good approach, though I can't comment on whether that's a significant memory usage or not.

data/transactions/logic/assembler.go

data/transactions/logic/assembler_test.go

data/transactions/logic/TEAL_opcodes.md

tsachiherman

Few minor issues mentioned above, but overall looks great.
I would be happy to see more extensive testing around the new teal opcodes - maybe some teal code that is doing a dual/triple recursion ?

jannotti · 2021-05-14T15:28:03Z

maybe some teal code that is doing a dual/triple recursion ?

I'll do a mutually recursive odd() / even() implementation.

jannotti · 2021-05-14T16:12:17Z

Aw crap. I committed some changes to address @tsachiherman comments. So now approvals are dismissed. Once I get through CI, I'll ping for them again. Sorry.

Add support for dynamic TEAL.

jannotti added 5 commits April 26, 2021 20:59

basics for divw, teal v4

b4a55af

Docs and tests for divw

1b98e69

back branches

cdd1a59

Allow labels to proceed instructions on a line

c6ddaa1

callsub, retsub

2d8e221

algorandskiy reviewed May 4, 2021

View reviewed changes

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

algorandskiy reviewed May 5, 2021

View reviewed changes

jannotti added 3 commits May 5, 2021 21:32

code review suggestions

736be24

callsub arg correction, infinite recursion test

8b8aba2

check back branches for alignment

fbc896b

onetechnical added the Team Scytale label May 6, 2021

This was linked to issues May 6, 2021

TEAL - allow backward jumps or indirect jumps #1211

Closed

divw and friends #1967

Closed

jannotti added 7 commits May 6, 2021 22:14

make dry run test closer to reality

a7346a3

Move cost checking into Check() for static cost checks.

60ce9ee

This allows callers to do less work. The consensus protocol was already being supplied, so Check() has everything it needed to do the right thing. This also consolidates the code that will act differently for dynamic teal costs.

ignore langspec files. we might consdier checkin instead

ca53930

test that v4 teal is not available early

0430d1c

check programs against last static version

4137ada

mistaken checkin of commented test, removed

1802d7b

Use TEAL v4, rather than new consenus parameter to go dynamic.

f8e281d

jdtzmn reviewed May 11, 2021

View reviewed changes

test/scripts/e2e_subs/v26/teal-v3-only.sh Outdated Show resolved Hide resolved

jannotti added 2 commits May 11, 2021 15:44

Spec updates for backjumps

838d411

typo

511cf69

jannotti requested review from tsachiherman and brianolson May 11, 2021 21:57

algorandskiy reviewed May 11, 2021

View reviewed changes

jannotti marked this pull request as ready for review May 11, 2021 23:02

algorandskiy previously approved these changes May 12, 2021

View reviewed changes

brianolson reviewed May 12, 2021

View reviewed changes

jasonpaulos previously approved these changes May 13, 2021

View reviewed changes

tsachiherman reviewed May 14, 2021

View reviewed changes

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

tsachiherman reviewed May 14, 2021

View reviewed changes

data/transactions/logic/assembler_test.go Show resolved Hide resolved

tsachiherman reviewed May 14, 2021

View reviewed changes

data/transactions/logic/TEAL_opcodes.md Show resolved Hide resolved

tsachiherman reviewed May 14, 2021

View reviewed changes

Further explanation of callstack

12d90f8

brianolson previously approved these changes May 14, 2021

View reviewed changes

code review

acbc483

jannotti dismissed stale reviews from brianolson, jasonpaulos, and algorandskiy via acbc483 May 14, 2021 16:10

algorandskiy approved these changes May 14, 2021

View reviewed changes

tsachiherman merged commit 009d57f into algorand:master May 14, 2021

tsachiherman pushed a commit to tsachiherman/go-algorand that referenced this pull request Jul 7, 2021

Dynamic teal (algorand#2126)

8d6d1a1

Add support for dynamic TEAL.

jannotti deleted the dynamic-teal branch January 28, 2022 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic teal #2126

Dynamic teal #2126

jannotti commented May 3, 2021

algorandskiy left a comment

algorandskiy May 5, 2021

algorandskiy May 5, 2021

algorandskiy May 5, 2021

jannotti May 6, 2021

jannotti commented May 6, 2021 •

edited

Loading

algorandskiy left a comment

algorandskiy May 11, 2021

jannotti May 11, 2021

algorandskiy commented May 11, 2021

brianolson left a comment

brianolson May 12, 2021

jannotti May 12, 2021

jannotti May 12, 2021

jannotti commented May 12, 2021

jasonpaulos left a comment •

edited

Loading

tsachiherman left a comment

jannotti commented May 14, 2021

jannotti commented May 14, 2021

Dynamic teal #2126

Dynamic teal #2126

Conversation

jannotti commented May 3, 2021

algorandskiy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannotti commented May 6, 2021 • edited Loading

algorandskiy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algorandskiy commented May 11, 2021

brianolson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannotti commented May 12, 2021

jasonpaulos left a comment • edited Loading

Choose a reason for hiding this comment

tsachiherman left a comment

Choose a reason for hiding this comment

jannotti commented May 14, 2021

jannotti commented May 14, 2021

jannotti commented May 6, 2021 •

edited

Loading

jasonpaulos left a comment •

edited

Loading