Add opcodes for dynamically indexing into Txn array fields #2847

algochoi · 2021-09-07T13:33:28Z

Summary

This commit adds txnas, gtxnas, and gtxnsas op codes that allow for array indices to be dynamically pushed into the stack instead of being supplied by immediate values. It also adds the args op code to dynamically specify an arg index from the stack.

TODO: Implement stores, loads

Closes #2799

Test Plan

Unit tests

codecov-commenter · 2021-09-07T14:13:00Z

Codecov Report

Merging #2847 (12b0bb8) into master (cd832b3) will increase coverage by 0.00%.
The diff coverage is 47.50%.

@@           Coverage Diff            @@
##           master    #2847    +/-   ##
========================================
  Coverage   47.25%   47.26%            
========================================
  Files         351      351            
  Lines       56363    56481   +118     
========================================
+ Hits        26634    26694    +60     
- Misses      26735    26772    +37     
- Partials     2994     3015    +21

Impacted Files	Coverage Δ
data/transactions/logic/doc.go	`82.75% <ø> (ø)`
data/transactions/logic/opcodes.go	`96.00% <ø> (ø)`
data/transactions/logic/eval.go	`87.79% <44.28%> (-1.61%)`	⬇️
data/transactions/logic/assembler.go	`80.66% <52.00%> (-0.83%)`	⬇️
catchup/peerSelector.go	`98.95% <0.00%> (-1.05%)`	⬇️
cmd/tealdbg/debugger.go	`72.86% <0.00%> (-1.01%)`	⬇️
network/wsPeer.go	`74.37% <0.00%> (-0.84%)`	⬇️
ledger/acctupdates.go	`62.55% <0.00%> (-0.26%)`	⬇️
crypto/merkletrie/node.go	`93.48% <0.00%> (+1.86%)`	⬆️
... and 3 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 cd832b3...12b0bb8. Read the comment docs.

jannotti

We need to think about whether we should add some pseudo op support. Currently, txn ApplicationArgs 0 gets compiled to txna ApplicationArgs 0, because of the two arguments signaling that it's an array ref. txnsa can also be compiled from txns (I think).

I'm not sure we can do much here, but it is a little strange that once you move to the stack versions, the "a" becomes required.

Let me talk through a strawman proposal and see if it makes sense:
txns could be introduced as a substitute for txnas
gtxns could compile to gtxnas if it is using an Array field? (this would be a departure, we normally go by the number of immediates, but the whole point of these new opcodes is that they access an array but do NOT, have an extra immediate)
gtxnss could compile to gtxnsas

I'm really unsure this makes sense. I'm just trying to fit in with our existing choices to not require the "a" in the the txn opcodes during assembly.

data/transactions/logic/README.md

data/transactions/logic/eval.go

data/transactions/logic/eval_test.go

data/transactions/logic/opcodes.go

… algochoi/dynamic-index-array

jannotti · 2021-09-07T20:45:29Z

data/transactions/logic/eval.go

+	if gtxid >= len(cx.TxnGroup) {
+		cx.err = fmt.Errorf("gtxnas lookup TxnGroup[%d] but it only has %d", gtxid, len(cx.TxnGroup))
+		return
+	} else if gtxid < 0 {
+		cx.err = fmt.Errorf("gtxnas lookup %d cannot be negative", gtxid)
+		return
+	}


I'll fix this up when I do loads, stores, but just want to point out that we don't want to complain that the number is negative. In AVM, it isn't. It's only negative (potentially) because this code casted a big uint64 to an int, and that can give you a negative result.

Makes sense!

PS, I liked the way you handled this in args a lot. There, you used uint64 mostly, and casted the len() calls instead of the stack argument. I think I will fix this stuff up that way, instead of the way I had been contemplating.

jannotti · 2021-09-07T20:46:39Z

data/transactions/logic/assembler.go

@@ -980,6 +1034,27 @@ func assembleGtxnsa(ops *OpStream, spec *OpSpec, args []string) error {
 	return nil
 }

+func assembleGtxnsas(ops *OpStream, spec *OpSpec, args []string) error {
+	if len(args) != 1 {
+		return ops.error("gtxnsas expects one immediate argument")


Let's try to use %s and spec.Name as often as possible.

Agreed - I think we can potentially save ourselves time in the future by doing this.

jasonpaulos

Mostly good, I just have some test suggestions. Also it would be nice if some code could be reused between the similar opcodes, but that can wait until later.

jasonpaulos · 2021-09-07T20:41:23Z

data/transactions/logic/eval_test.go

 int 0
 ==
 &&
 gtxn 1 ExtraProgramPages
 int 2
 ==
 &&
+`
+
+	gtxnTextV5 := gtxnTextV4 + `int 0


This is missing txnas

I put txnas in the testTxnProgramTextV5 section I think.

jasonpaulos · 2021-09-07T20:51:48Z

data/transactions/logic/eval_test.go

+	txgroup[0] = txn
+	ep := defaultEvalParams(nil, &txn)
+	ep.TxnGroup = txgroup
+	_, err := Eval(ops.Program, ep)


I think you're just checking that the programs runs, not that it passes? It would be a stronger test to instead check that a simple use of txnas passes. I'd suggest something like the following, and you could adapt it for gtxnas and gtxnsas too.

int 0 txnas Accounts txna Accounts 0 == int 0 txnas ApplicationArgs txna ApplicationArgs 0 == && int 1 txnas ApplicationArgs txna ApplicationArgs 1 == && return

Good point, I will check these with e.g. testAccepts(t, "int 0; txnas Accounts; txna Accounts 0; ==", 5)

I merged what existed, I'll add these.

I concluded these were fine. The following tests checked pass, and they were grabbing the accounts and comparing them to app args.

data/transactions/logic/eval_test.go

algochoi added 2 commits September 7, 2021 09:30

Add txnas opcodes

fc520d0

Delete comments

f37928b

barnjamin mentioned this pull request Sep 7, 2021

Dynamic Array Reference algorand/pyteal#109

Closed

Add args opcode

bb2cbf6

jannotti requested changes Sep 7, 2021

View reviewed changes

algochoi added 4 commits September 7, 2021 11:29

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

7c1f1ec

… algochoi/dynamic-index-array

Address PR comments

1cb18d5

Fix op docs

d18878e

Fix cast

1bf8bfd

algochoi requested review from jannotti and jasonpaulos September 7, 2021 20:19

Fix gtxnsas opdoc

12b0bb8

jannotti approved these changes Sep 7, 2021

View reviewed changes

jannotti reviewed Sep 7, 2021

View reviewed changes

jasonpaulos reviewed Sep 7, 2021

View reviewed changes

jannotti merged commit cde7eed into algorand:master Sep 7, 2021

onetechnical mentioned this pull request Sep 15, 2021

go-algorand 3.0.0-beta #2893

Merged

algojack mentioned this pull request Sep 24, 2021

go-algorand 3.0.1-stable #2949

Merged

algochoi deleted the algochoi/dynamic-index-array branch February 17, 2022 22:22

geo2a mentioned this pull request Nov 9, 2021

[TEAL] Extra documentation runtimeverification/avm-semantics#12

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add opcodes for dynamically indexing into Txn array fields #2847

Add opcodes for dynamically indexing into Txn array fields #2847

algochoi commented Sep 7, 2021 •

edited

codecov-commenter commented Sep 7, 2021 •

edited

jannotti left a comment •

edited

jannotti Sep 7, 2021

algochoi Sep 7, 2021

jannotti Sep 7, 2021

jannotti Sep 7, 2021

algochoi Sep 7, 2021

jasonpaulos left a comment •

edited

jasonpaulos Sep 7, 2021

algochoi Sep 7, 2021

jasonpaulos Sep 7, 2021

algochoi Sep 7, 2021

jannotti Sep 7, 2021

jannotti Sep 7, 2021

Add opcodes for dynamically indexing into Txn array fields #2847

Add opcodes for dynamically indexing into Txn array fields #2847

Conversation

algochoi commented Sep 7, 2021 • edited

Summary

Test Plan

codecov-commenter commented Sep 7, 2021 • edited

Codecov Report

jannotti left a comment • edited

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

Choose a reason for hiding this comment

jasonpaulos left a comment • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algochoi commented Sep 7, 2021 •

edited

codecov-commenter commented Sep 7, 2021 •

edited

jannotti left a comment •

edited

jasonpaulos left a comment •

edited