Teal4 math #2139

jannotti · 2021-05-07T02:48:39Z

Summary

Add a lot of math oriented opcodes that have been requested by teal users.

sqrt, exp, expw, divmodw

byteslice oriented math - a set of opcodes that will treat byteslices as arbitrary precision uints.

Test Plan

Unit tests

Byte math costs were established with a bunch of new operation benchmarks.

jasonpaulos · 2021-05-14T15:29:44Z

data/transactions/logic/doc.go

Why not have b~ for bitwise invert as well?

Is it because byteslices are variable length and the normal invert is kind of undefined in this scenario?

Because it's confusing. If you start with 0xff00 and invert it, you get 0xff (note, only one byte). So then if you invert it again, you get "" (empty byteslice == 0). The results of these things are always only as long as needed to represent the answer as a bigint. So I think we instead have to recommend people use a bunch of 1s, and xor, to get the functional equivalent of bit negation. That way their choice of length for the 1s slice explicit forces the final result to be the same length.

I'm going to add an opcode to make such 1-strings easy to build.

Saying all this outloud led to a better way. bitwise operation no longer truncate to the shortest byteslice that can hold the value. They return a byteslice as long as their longest input. This is more likely to be what people want, as they would use these operations when treating the slices as arrays of bits. It also makes the definition of b~ more straightforward.

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.

algorandskiy

Nice work, couple comments

algorandskiy · 2021-05-18T17:44:12Z

data/transactions/logic/eval.go

+
+	if cx.err == nil {
+		postheight := len(cx.stack)
+		if spec.Name != "return" && postheight-preheight != len(spec.Returns)-len(spec.Args) {


the check is correct but the message would be -2 != -1 or similar that might be confusing?

This error would only occur because of a bug in our implementation, not in teal code. I don't know how user-friendly it needs to be. The info we need is there: There's a bug in the named opcode and it didn't manage the stack properly.

I simply do not remember checks for popped vs pushed. But I guess you are right and these problem would be already discovered.

Yes, that's true, those checks are new. But they are checks on the results of individual opcode operations, so I don't think it is possible to construct a strange program that would trip them. If they catch a problem, it would mean that one of our existing opcodes has a pretty important bug that we have missed for a long time. I think that's unlikely. But it does help mistakes as we write new opcodes.

algorandskiy · 2021-05-18T17:47:28Z

data/transactions/logic/eval.go

+	preheight := len(cx.stack)
 	spec.op(cx)
+
+	if cx.err == nil {


I think this needs to go under consensus params switch, since there is a change upgraded nodes reject some specially-crafted program, and all allow it => fork

I don't think so. It only checks things that would have previously caused an error. But now it does it in a central place, so that future opcodes won't have to do it themselves. In particular, in the past only concat could create a 4k slice, so only concat checked its output. Now we check here, in case we have introduced a new way to create a 4k slice (I don't think we have, since byteslice math is input limited, but this seems like a useful thing to centralize, especially as we might add other ways to hit 4k)

algorandskiy · 2021-05-18T18:00:06Z

data/transactions/logic/eval.go

+	opSwap(cx)
+	opLt(cx)


I thought entire point of duplicating code in each opcode was in reducing number of additional operations.
Like opLe now is opSwap + opLt + opNot that is (5 + 6 + 3) = 14 instead of 6 in old version (counted mem read/write, swap is expensive since no direct mem to mem copy in a single cpu operation).
IMO this is questionable, maybe benchmark it?

I did this when I found writing the equivalent byteslice operators to be tedious, repetitive, and error-prone. It's hard for me to imagine any appreciable performance effect here, with all this data in cache. I'll write a little benchmark to convince myself.

Here are the result. I put them in order of overhead introduced by the shorter, clearer code. Each one is on more function call than the last.

Benchmark < 16.9-17.8 ns/op

Benchmark >= 17.3-17.5 ns/op

Benchmark > 20.0-23.3 ns/op

Benchmark <= 21.8-22.0 ns/op

3 ns seems like an ok price to pay, for the largest overhead.

The biggest jump is between 2 & 3, because it involves opSwap. I suppose I could be convinced to repeat the code to eliminate that one. Then they'd all be so close the difference is in the noise.

Thank you for benchmarking it! I think I agree, 3ns/op is OK price for cleaner code.
Extrapolate: suppose there are 300 (1/3 of program) of comparison instructions then there is 300*3ns =~ 1 us overhead. For 10k programs in a block we'll have 10ms execution time increase. I guess this is acceptable.

data/transactions/logic/eval_test.go

algorandskiy · 2021-05-18T18:07:55Z

data/transactions/logic/eval_test.go

+	testAccepts(t, "int 2; log2; int 2; ==", 4)
+	testAccepts(t, "int 4; log2; int 3; ==", 4)
+	testAccepts(t, "int 5; log2; int 3; ==", 4)
+	testAccepts(t, "int 8; log2; int 4; ==", 4)


I though log2(8) = 3

Yikes, yes, thank you! Off by one everywhere.

The reason I made this error is that integer log2 is just "what's the highest bit set in this integer (minus 1)".

I changed this so it's "correct", and fails when the supplied integer is 0. But I wonder if I should just rename it "highbit" and give it the old semantics. That sounds clearer to me, and is probably what people would actually need this opcode for (so they can decide how far to bitshift a value, for example). That's why this is an operation that's supported by the bits package. With the log2 semantics, callers need to check if the value is 0 first, to avoid panics.

I decided to call the op "bitlen" and define it on uints and byte-arrays. It returns the index of the highest bit, so it returns 0 on 0, rather than panic. (And 1 => 1, 2 => 2, 4=> 3, 8 => 4). It corresponds directly to bits.Len in Go.

data/transactions/logic/eval_test.go

jasonpaulos

This looks good to me, including the teal.tmLanguage.json file.

algorandskiy

Looks good. Please consider adding one more test for all wide opcodes in a sequence to ensure format compatibility - like mulw, addw, divmodw, and something like (A * B / B == A) for mulw + divmodw. A seprate PR is OK.

algorandskiy · 2021-05-21T23:45:03Z

data/transactions/logic/eval.go

+	result := new(big.Int)
+	opBytesBinOp(cx, result, result.Add)


algorandskiy · 2021-05-21T23:48:36Z

data/transactions/logic/eval.go

+	a, b := opBytesBinaryLogicPrep(cx)
+	for i := range a {
+		a[i] = a[i] | b[i]
+	}


another nice one

algorandskiy · 2021-05-21T23:57:28Z

@brianolson please take a look

brianolson

one tiny testing quibble, but the 0.05% change probably doesn't matter.
Otherwise no objections. LGTM.

brianolson · 2021-05-24T14:06:41Z

data/transactions/logic/eval_test.go

+	hashes := []string{"sha256", "keccak256", "sha512_256"}
+	for _, hash := range hashes {
+		b.Run(hash+"-small", func(b *testing.B) { // hash 32 bytes
+			benchmarkOperation(b, "byte 0x1234567890", hash,


nit: for each of these its only hashing 5 bytes on the first execution, and then {32,128,512} for the next 1999. It could be perfectly consistent by using a 32 byte constant and running dup; concat; dup; concat; {hash} as the repeated op?

onetechnical · 2021-05-25T20:20:24Z

Brian's comments to be addressed in a separate PR.

Add a lot of math oriented opcodes that have been requested by teal users. sqrt, exp, expw, divmodw byteslice oriented math - a set of opcodes that will treat byteslices as arbitrary precision uints.

onetechnical added the Team Scytale label May 7, 2021

jannotti force-pushed the teal4-math branch 2 times, most recently from b12c44c to 64df154 Compare May 12, 2021 15:00

jasonpaulos reviewed May 14, 2021

View reviewed changes

jannotti force-pushed the teal4-math branch from 5fa0efa to 36e5c99 Compare May 15, 2021 02:58

jannotti added 23 commits May 14, 2021 23:17

back branches

7a25b60

callsub, retsub

ef2086f

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

6cb8116

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 that v4 teal is not available early

c37a394

mistaken checkin of commented test, removed

b42949c

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

a5c47e3

shl, shr

9850abe

name divmodw properly

9564db7

sqrt, exp, expw

fc69448

log2

6f9723d

Change name of divw to divmodw in test introduced after the change.

a41722a

byteslice math

4e90f49

spec updates

c2079ff

spec updates

810a361

Improve testing by checking return types of opcodes

77f2144

More safety: opcodes must change stack according to spec.

d163857

Fix the ed25519verify benchmark. Simplify many test.

5f2d570

Post rebase fixups

b6f90b4

spec typo

fad1cdf

enforce some byte-array size limits

2aef070

Byte math detail

7d3bf0b

bit manipulation of bytes keeps longest length

5934c32

simplify bitwise ops on byte-arrays

34359c1

jannotti force-pushed the teal4-math branch from 36e5c99 to 34359c1 Compare May 15, 2021 03:28

Fix benchmarking to expect error in Eval, not Check

13affc9

jannotti force-pushed the teal4-math branch from 10387b9 to 13affc9 Compare May 17, 2021 18:59

Fix tmLanguage, small spec updates.

3afd2fa

jannotti marked this pull request as ready for review May 17, 2021 20:38

algorandskiy reviewed May 18, 2021

View reviewed changes

jannotti added 5 commits May 18, 2021 16:45

Better tests on constant blocks

b31fc55

Proper definition of log2

740184d

Benchmarking, and some associated costs for opcodes

6b01ff8

Changed log2 to bitlen, make it work on byte-arrays too.

92d0520

Spec correction. Byte logic ops are not input length limited

8d5effc

jannotti requested review from brianolson and cce May 21, 2021 18:23

jasonpaulos approved these changes May 21, 2021

View reviewed changes

algorandskiy approved these changes May 21, 2021

View reviewed changes

brianolson reviewed May 24, 2021

View reviewed changes

algojohnlee merged commit c8df55f into algorand:master May 25, 2021

jannotti deleted the teal4-math branch January 28, 2022 15:40

algochoi mentioned this pull request May 27, 2022

Compare performance from old PRs 1 algochoi/go-algorand#17

Closed

Teal4 math #2139

Teal4 math #2139

Uh oh!

Conversation

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

Uh oh!

Summary

Test Plan

Uh oh!

jasonpaulos May 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

algorandskiy 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!

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!

algorandskiy May 20, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

algorandskiy 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!

algorandskiy commented May 21, 2021

Uh oh!

brianolson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onetechnical commented May 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jannotti commented May 7, 2021 •

edited

Loading

jasonpaulos May 14, 2021 •

edited

Loading

algorandskiy May 20, 2021 •

edited

Loading

jasonpaulos left a comment •

edited

Loading