From 7990b5c06c8f6f1ae27930a3ef8bf3b9d9865f4d Mon Sep 17 00:00:00 2001 From: John Jannotti Date: Sun, 21 Aug 2022 22:29:29 -0400 Subject: [PATCH 1/3] Handle Teal programs with manual constant blocks better This PR fixes #4034, by forcing the use of "push*" when mixing manual constant blocks with the int/byte/addr/method pseudo-ops. (And a related disassembly bug). However, to avoid this "deoptimization" when manual blocks are used to add program metadata, deadcode constant blocks are ignored for this check. --- data/transactions/logic/assembler.go | 106 ++++++++++++++++------ data/transactions/logic/assembler_test.go | 92 +++++++++++++++++-- data/transactions/logic/eval_test.go | 9 ++ 3 files changed, 171 insertions(+), 36 deletions(-) diff --git a/data/transactions/logic/assembler.go b/data/transactions/logic/assembler.go index aa531c9bf0..a4bb2970aa 100644 --- a/data/transactions/logic/assembler.go +++ b/data/transactions/logic/assembler.go @@ -225,10 +225,12 @@ type OpStream struct { intc []uint64 // observed ints in code. We'll put them into a intcblock intcRefs []intReference // references to int pseudo-op constants, used for optimization hasIntcBlock bool // prevent prepending intcblock because asm has one + hasPseudoInt bool // were any `int` pseudo ops used? bytec [][]byte // observed bytes in code. We'll put them into a bytecblock bytecRefs []byteReference // references to byte/addr pseudo-op constants, used for optimization hasBytecBlock bool // prevent prepending bytecblock because asm has one + hasPseudoByte bool // were any `byte` (or equivalent) pseudo ops used? // tracks information we know to be true at the point being assembled known ProgramKnowledge @@ -373,18 +375,18 @@ func (ops *OpStream) returns(spec *OpSpec, replacement StackType) { func (ops *OpStream) Intc(constIndex uint) { switch constIndex { case 0: - ops.pending.WriteByte(0x22) // intc_0 + ops.pending.WriteByte(OpsByName[ops.Version]["intc_0"].Opcode) case 1: - ops.pending.WriteByte(0x23) // intc_1 + ops.pending.WriteByte(OpsByName[ops.Version]["intc_1"].Opcode) case 2: - ops.pending.WriteByte(0x24) // intc_2 + ops.pending.WriteByte(OpsByName[ops.Version]["intc_2"].Opcode) case 3: - ops.pending.WriteByte(0x25) // intc_3 + ops.pending.WriteByte(OpsByName[ops.Version]["intc_3"].Opcode) default: if constIndex > 0xff { ops.error("cannot have more than 256 int constants") } - ops.pending.WriteByte(0x21) // intc + ops.pending.WriteByte(OpsByName[ops.Version]["intc"].Opcode) ops.pending.WriteByte(uint8(constIndex)) } if constIndex >= uint(len(ops.intc)) { @@ -394,8 +396,10 @@ func (ops *OpStream) Intc(constIndex uint) { } } -// Uint writes opcodes for loading a uint literal -func (ops *OpStream) Uint(val uint64) { +// IntLiteral writes opcodes for loading a uint literal +func (ops *OpStream) IntLiteral(val uint64) { + ops.hasPseudoInt = true + found := false var constIndex uint for i, cv := range ops.intc { @@ -405,7 +409,11 @@ func (ops *OpStream) Uint(val uint64) { break } } + if !found { + if ops.hasIntcBlock { + ops.errorf("int %d used without %d in intcblock", val, val) + } constIndex = uint(len(ops.intc)) ops.intc = append(ops.intc, val) } @@ -420,18 +428,18 @@ func (ops *OpStream) Uint(val uint64) { func (ops *OpStream) Bytec(constIndex uint) { switch constIndex { case 0: - ops.pending.WriteByte(0x28) // bytec_0 + ops.pending.WriteByte(OpsByName[ops.Version]["bytec_0"].Opcode) case 1: - ops.pending.WriteByte(0x29) // bytec_1 + ops.pending.WriteByte(OpsByName[ops.Version]["bytec_1"].Opcode) case 2: - ops.pending.WriteByte(0x2a) // bytec_2 + ops.pending.WriteByte(OpsByName[ops.Version]["bytec_2"].Opcode) case 3: - ops.pending.WriteByte(0x2b) // bytec_3 + ops.pending.WriteByte(OpsByName[ops.Version]["bytec_3"].Opcode) default: if constIndex > 0xff { ops.error("cannot have more than 256 byte constants") } - ops.pending.WriteByte(0x27) // bytec + ops.pending.WriteByte(OpsByName[ops.Version]["bytec"].Opcode) ops.pending.WriteByte(uint8(constIndex)) } if constIndex >= uint(len(ops.bytec)) { @@ -444,6 +452,8 @@ func (ops *OpStream) Bytec(constIndex uint) { // ByteLiteral writes opcodes and data for loading a []byte literal // Values are accumulated so that they can be put into a bytecblock func (ops *OpStream) ByteLiteral(val []byte) { + ops.hasPseudoByte = true + found := false var constIndex uint for i, cv := range ops.bytec { @@ -454,6 +464,9 @@ func (ops *OpStream) ByteLiteral(val []byte) { } } if !found { + if ops.hasBytecBlock { + ops.errorf("byte/addr/method used without value in bytecblock") + } constIndex = uint(len(ops.bytec)) ops.bytec = append(ops.bytec, val) } @@ -468,23 +481,31 @@ func asmInt(ops *OpStream, spec *OpSpec, args []string) error { if len(args) != 1 { return ops.error("int needs one argument") } + + if ops.hasIntcBlock && ops.Version >= backBranchEnabledVersion { + // We don't understand control-flow, so use pushint + ops.warnf("int %s used with explicit intcblock. must pushint", args[0]) + pushint := OpsByName[ops.Version]["pushint"] + return asmPushInt(ops, &pushint, args) + } + // check txn type constants i, ok := txnTypeMap[args[0]] if ok { - ops.Uint(i) + ops.IntLiteral(i) return nil } // check OnCompetion constants oc, isOCStr := onCompletionMap[args[0]] if isOCStr { - ops.Uint(oc) + ops.IntLiteral(oc) return nil } val, err := strconv.ParseUint(args[0], 0, 64) if err != nil { return ops.error(err) } - ops.Uint(val) + ops.IntLiteral(val) return nil } @@ -696,6 +717,12 @@ func asmByte(ops *OpStream, spec *OpSpec, args []string) error { if len(args) == 0 { return ops.errorf("%s operation needs byte literal argument", spec.Name) } + if ops.hasBytecBlock && ops.Version >= backBranchEnabledVersion { + // We don't understand control-flow, so use pushbytes + ops.warnf("byte %s used with explicit bytecblock. must pushbytes", args[0]) + pushbytes := OpsByName[ops.Version]["pushbytes"] + return asmPushBytes(ops, &pushbytes, args) + } val, consumed, err := parseBinaryArgs(args) if err != nil { return ops.error(err) @@ -734,11 +761,10 @@ func asmMethod(ops *OpStream, spec *OpSpec, args []string) error { func asmIntCBlock(ops *OpStream, spec *OpSpec, args []string) error { ops.pending.WriteByte(spec.Opcode) + ivals := make([]uint64, len(args)) var scratch [binary.MaxVarintLen64]byte l := binary.PutUvarint(scratch[:], uint64(len(args))) ops.pending.Write(scratch[:l]) - ops.intcRefs = nil - ops.intc = make([]uint64, len(args)) for i, xs := range args { cu, err := strconv.ParseUint(xs, 0, 64) if err != nil { @@ -746,9 +772,19 @@ func asmIntCBlock(ops *OpStream, spec *OpSpec, args []string) error { } l = binary.PutUvarint(scratch[:], cu) ops.pending.Write(scratch[:l]) - ops.intc[i] = cu + if !ops.known.deadcode { + ivals[i] = cu + } } - ops.hasIntcBlock = true + if !ops.known.deadcode { + if ops.hasPseudoInt { + ops.error("intcblock following int") + } + ops.intcRefs = nil + ops.intc = ivals + ops.hasIntcBlock = true + } + return nil } @@ -763,8 +799,7 @@ func asmByteCBlock(ops *OpStream, spec *OpSpec, args []string) error { // intcblock, but parseBinaryArgs would have // to return a useful consumed value even in // the face of errors. Hard. - ops.error(err) - return nil + return ops.error(err) } bvals = append(bvals, val) rest = rest[consumed:] @@ -777,9 +812,14 @@ func asmByteCBlock(ops *OpStream, spec *OpSpec, args []string) error { ops.pending.Write(scratch[:l]) ops.pending.Write(bv) } - ops.bytecRefs = nil - ops.bytec = bvals - ops.hasBytecBlock = true + if !ops.known.deadcode { + if ops.hasPseudoByte { + ops.error("bytecblock following byte/addr/method") + } + ops.bytecRefs = nil + ops.bytec = bvals + ops.hasBytecBlock = true + } return nil } @@ -1649,6 +1689,16 @@ func (ops *OpStream) assemble(text string) error { } } + // Before back branches, the pseudo ops can and do complain during first pass + if ops.Version >= backBranchEnabledVersion { + if ops.hasIntcBlock && ops.hasPseudoInt { + ops.error("program has int instruction with a manual intcblock") + } + if ops.hasBytecBlock && ops.hasPseudoByte { + ops.error("program has byte instruction with a manual bytecblock") + } + } + if ops.Version >= optimizeConstantsEnabledVersion { ops.optimizeIntcBlock() ops.optimizeBytecBlock() @@ -2019,7 +2069,7 @@ func (ops *OpStream) prependCBlocks() []byte { vlen := binary.PutUvarint(scratch[:], ops.Version) prebytes.Write(scratch[:vlen]) if len(ops.intc) > 0 && !ops.hasIntcBlock { - prebytes.WriteByte(0x20) // intcblock + prebytes.WriteByte(OpsByName[ops.Version]["intcblock"].Opcode) vlen := binary.PutUvarint(scratch[:], uint64(len(ops.intc))) prebytes.Write(scratch[:vlen]) for _, iv := range ops.intc { @@ -2028,7 +2078,7 @@ func (ops *OpStream) prependCBlocks() []byte { } } if len(ops.bytec) > 0 && !ops.hasBytecBlock { - prebytes.WriteByte(0x26) // bytecblock + prebytes.WriteByte(OpsByName[ops.Version]["bytecblock"].Opcode) vlen := binary.PutUvarint(scratch[:], uint64(len(ops.bytec))) prebytes.Write(scratch[:vlen]) for _, bv := range ops.bytec { @@ -2266,7 +2316,7 @@ func disassemble(dis *disassembleState, spec *OpSpec) (string, error) { return "", err } - dis.intc = append(dis.intc, intc...) + dis.intc = intc for i, iv := range intc { if i != 0 { out += " " @@ -2279,7 +2329,7 @@ func disassemble(dis *disassembleState, spec *OpSpec) (string, error) { if err != nil { return "", err } - dis.bytec = append(dis.bytec, bytec...) + dis.bytec = bytec for i, bv := range bytec { if i != 0 { out += " " diff --git a/data/transactions/logic/assembler_test.go b/data/transactions/logic/assembler_test.go index d0bcff2954..23ea0fb946 100644 --- a/data/transactions/logic/assembler_test.go +++ b/data/transactions/logic/assembler_test.go @@ -17,6 +17,7 @@ package logic import ( + "bytes" "encoding/hex" "fmt" "strings" @@ -732,7 +733,7 @@ func TestOpUint(t *testing.T) { for v := uint64(1); v <= AssemblerMaxVersion; v++ { t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) { ops := newOpStream(v) - ops.Uint(0xcafebabe) + ops.IntLiteral(0xcafebabe) prog := ops.prependCBlocks() require.NotNil(t, prog) s := hex.EncodeToString(prog) @@ -748,9 +749,8 @@ func TestOpUint64(t *testing.T) { for v := uint64(1); v <= AssemblerMaxVersion; v++ { t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) { - t.Parallel() ops := newOpStream(v) - ops.Uint(0xcafebabecafebabe) + ops.IntLiteral(0xcafebabecafebabe) prog := ops.prependCBlocks() require.NotNil(t, prog) s := hex.EncodeToString(prog) @@ -878,6 +878,80 @@ func TestAssembleBytesString(t *testing.T) { } } +func TestManualCBlocks(t *testing.T) { + partitiontest.PartitionTest(t) + t.Parallel() + + // Despite appearing twice, 500s are pushints because of manual intcblock + ops := testProg(t, "intcblock 1; int 500; int 500; ==", AssemblerMaxVersion) + require.Equal(t, ops.Program[4], OpsByName[ops.Version]["pushint"].Opcode) + + ops = testProg(t, "intcblock 2 3; intcblock 4 10; int 5", AssemblerMaxVersion) + text, err := Disassemble(ops.Program) + require.NoError(t, err) + require.Contains(t, text, "pushint 5") + + ops = testProg(t, "intcblock 2 3; intcblock 4 10; intc_3", AssemblerMaxVersion) + text, err = Disassemble(ops.Program) + require.NoError(t, err) + require.Contains(t, text, "intc_3\n") // That is, no commented value for intc_3 is shown + + // In old straight-line versions, allow mixing int and intc if the ints all + // reference manual block. Since conditionals do make it possible that + // different cblocks could be in effect depending on earlier path choices, + // maybe we should not even allow this. + checkSame(t, 3, + "intcblock 4 5 1; intc_0; intc_2; +; intc_1; ==", + "intcblock 4 5 1; int 4; int 1; +; intc_1; ==", + "intcblock 4 5 1; intc_0; int 1; +; int 5; ==") + checkSame(t, 3, + "bytecblock 0x44 0x55 0x4455; bytec_0; bytec_1; concat; bytec_2; ==", + "bytecblock 0x44 0x55 0x4455; byte 0x44; bytec_1; concat; byte 0x4455; ==", + "bytecblock 0x44 0x55 0x4455; bytec_0; byte 0x55; concat; bytec_2; ==") + + // But complain if they do not + testProg(t, "intcblock 4; int 3;", 3, Expect{1, "int 3 used without 3 in intcblock"}) + testProg(t, "bytecblock 0x44; byte 0x33;", 3, Expect{1, "byte/addr/method used without value in bytecblock"}) + + // Or if the ref comes before the constant block, even if they match + testProg(t, "int 5; intcblock 4;", 3, Expect{1, "intcblock following int"}) + testProg(t, "int 4; intcblock 4;", 3, Expect{1, "intcblock following int"}) + testProg(t, "addr RWXCBB73XJITATVQFOI7MVUUQOL2PFDDSDUMW4H4T2SNSX4SEUOQ2MM7F4; bytecblock 0x44", 3, Expect{1, "bytecblock following byte/addr/method"}) + + // But we can't complain precisely once backjumps are allowed, so we force + // compile to push*. (We don't analyze the CFG, so we don't know if we can + // use what is in the user defined block. Perhaps we could special case + // single cblocks at start of program. + checkSame(t, 4, + "intcblock 4 5 1; int 4; int 1; +; int 5; ==", + "intcblock 4 5 1; pushint 4; pushint 1; +; pushint 5; ==") + checkSame(t, 4, + "bytecblock 0x44 0x55 0x4455; byte 0x44; byte 0x55; concat; byte 0x4455; ==", + "bytecblock 0x44 0x55 0x4455; pushbytes 0x44; pushbytes 0x55; concat; pushbytes 0x4455; ==") + // Can't switch to push* after the fact. + testProg(t, "int 5; intcblock 4;", 4, Expect{1, "intcblock following int"}) + testProg(t, "int 4; intcblock 4;", 4, Expect{1, "intcblock following int"}) + testProg(t, "addr RWXCBB73XJITATVQFOI7MVUUQOL2PFDDSDUMW4H4T2SNSX4SEUOQ2MM7F4; bytecblock 0x44", 4, Expect{1, "bytecblock following byte/addr/method"}) + + // Ignore manually added cblocks in deadcode, so they can be added easily to + // existing programs. There are proposals to put metadata there. + ops = testProg(t, "int 4; int 4; +; int 8; ==; return; intcblock 10", AssemblerMaxVersion) + require.Equal(t, ops.Program[1], OpsByName[ops.Version]["intcblock"].Opcode) + require.EqualValues(t, ops.Program[3], 4) // 1 4 + require.Equal(t, ops.Program[4], OpsByName[ops.Version]["intc_0"].Opcode) + ops = testProg(t, "b skip; intcblock 10; skip: int 4; int 4; +; int 8; ==;", AssemblerMaxVersion) + require.Equal(t, ops.Program[1], OpsByName[ops.Version]["intcblock"].Opcode) + require.EqualValues(t, ops.Program[3], 4) + + ops = testProg(t, "byte 0x44; byte 0x44; concat; len; return; bytecblock 0x11", AssemblerMaxVersion) + require.Equal(t, ops.Program[1], OpsByName[ops.Version]["bytecblock"].Opcode) + require.EqualValues(t, ops.Program[4], 0x44) // 1 1 0x44 + require.Equal(t, ops.Program[5], OpsByName[ops.Version]["bytec_0"].Opcode) + ops = testProg(t, "b skip; bytecblock 0x11; skip: byte 0x44; byte 0x44; concat; len; int 4; ==", AssemblerMaxVersion) + require.Equal(t, ops.Program[1], OpsByName[ops.Version]["bytecblock"].Opcode) + require.EqualValues(t, ops.Program[4], 0x44) +} + func TestAssembleOptimizedConstants(t *testing.T) { partitiontest.PartitionTest(t) t.Parallel() @@ -2605,12 +2679,14 @@ func checkSame(t *testing.T, version uint64, first string, compares ...string) { if version == 0 { version = assemblerNoVersion } - ops, err := AssembleStringWithVersion(first, version) - require.NoError(t, err, first) + ops := testProg(t, first, version) for _, compare := range compares { - other, err := AssembleStringWithVersion(compare, version) - assert.NoError(t, err, compare) - assert.Equal(t, other.Program, ops.Program, "%s unlike %s", first, compare) + other := testProg(t, compare, version) + if bytes.Compare(other.Program, ops.Program) != 0 { + t.Log(Disassemble(ops.Program)) + t.Log(Disassemble(other.Program)) + } + assert.Equal(t, ops.Program, other.Program, "%s unlike %s", first, compare) } } diff --git a/data/transactions/logic/eval_test.go b/data/transactions/logic/eval_test.go index 19970acfb3..32367c9f86 100644 --- a/data/transactions/logic/eval_test.go +++ b/data/transactions/logic/eval_test.go @@ -896,6 +896,15 @@ func TestBytecTooFar(t *testing.T) { testPanics(t, "byte 0x23; bytec_1; btoi", 1) } +func TestManualCBlockEval(t *testing.T) { + // TestManualCBlock in assembler_test.go demonstrates that these will use + // an inserted constant block. + testAccepts(t, "int 4; int 4; +; int 8; ==; return; intcblock 10", 2) + testAccepts(t, "b skip; intcblock 10; skip: int 4; int 4; +; int 8; ==;", 2) + testAccepts(t, "byte 0x2222; byte 0x2222; concat; len; int 4; ==; return; bytecblock 0x11", 2) + testAccepts(t, "b skip; bytecblock 0x11; skip: byte 0x2222; byte 0x2222; concat; len; int 4; ==", 2) +} + func TestTxnBadField(t *testing.T) { partitiontest.PartitionTest(t) From 019df3b3429a4bb18f35ccedf1897d0e84800dfd Mon Sep 17 00:00:00 2001 From: John Jannotti Date: Tue, 30 Aug 2022 15:19:21 -0400 Subject: [PATCH 2/3] Further checks for programs using multiple cblocks --- data/transactions/logic/assembler.go | 72 +++++++++++++++-------- data/transactions/logic/assembler_test.go | 45 ++++++++++++++ data/transactions/logic/eval_test.go | 3 + 3 files changed, 97 insertions(+), 23 deletions(-) diff --git a/data/transactions/logic/assembler.go b/data/transactions/logic/assembler.go index a4bb2970aa..97c61dee08 100644 --- a/data/transactions/logic/assembler.go +++ b/data/transactions/logic/assembler.go @@ -224,12 +224,12 @@ type OpStream struct { intc []uint64 // observed ints in code. We'll put them into a intcblock intcRefs []intReference // references to int pseudo-op constants, used for optimization - hasIntcBlock bool // prevent prepending intcblock because asm has one + cntIntcBlock int // prevent prepending intcblock because asm has one hasPseudoInt bool // were any `int` pseudo ops used? bytec [][]byte // observed bytes in code. We'll put them into a bytecblock bytecRefs []byteReference // references to byte/addr pseudo-op constants, used for optimization - hasBytecBlock bool // prevent prepending bytecblock because asm has one + cntBytecBlock int // prevent prepending bytecblock because asm has one hasPseudoByte bool // were any `byte` (or equivalent) pseudo ops used? // tracks information we know to be true at the point being assembled @@ -411,7 +411,7 @@ func (ops *OpStream) IntLiteral(val uint64) { } if !found { - if ops.hasIntcBlock { + if ops.cntIntcBlock > 0 { ops.errorf("int %d used without %d in intcblock", val, val) } constIndex = uint(len(ops.intc)) @@ -464,7 +464,7 @@ func (ops *OpStream) ByteLiteral(val []byte) { } } if !found { - if ops.hasBytecBlock { + if ops.cntBytecBlock > 0 { ops.errorf("byte/addr/method used without value in bytecblock") } constIndex = uint(len(ops.bytec)) @@ -482,13 +482,28 @@ func asmInt(ops *OpStream, spec *OpSpec, args []string) error { return ops.error("int needs one argument") } - if ops.hasIntcBlock && ops.Version >= backBranchEnabledVersion { + // After backBranchEnabledVersion, control flow is confusing, so if there's + // a manual cblock, use push instead of trying to use what's given. + if ops.cntIntcBlock > 0 && ops.Version >= backBranchEnabledVersion { // We don't understand control-flow, so use pushint ops.warnf("int %s used with explicit intcblock. must pushint", args[0]) pushint := OpsByName[ops.Version]["pushint"] return asmPushInt(ops, &pushint, args) } + // There are no backjumps, but there are multiple cblocks. Maybe one is + // conditional skipped. Too confusing. + if ops.cntIntcBlock > 1 { + pushint, ok := OpsByName[ops.Version]["pushint"] + if ok { + return asmPushInt(ops, &pushint, args) + } + return ops.errorf("int %s used with manual intcblocks. Use intc.", args[0]) + } + + // In both of the above clauses, we _could_ track whether a particular + // intcblock dominates the current instruction. If so, we could use it. + // check txn type constants i, ok := txnTypeMap[args[0]] if ok { @@ -717,12 +732,29 @@ func asmByte(ops *OpStream, spec *OpSpec, args []string) error { if len(args) == 0 { return ops.errorf("%s operation needs byte literal argument", spec.Name) } - if ops.hasBytecBlock && ops.Version >= backBranchEnabledVersion { + + // After backBranchEnabledVersion, control flow is confusing, so if there's + // a manual cblock, use push instead of trying to use what's given. + if ops.cntBytecBlock > 0 && ops.Version >= backBranchEnabledVersion { // We don't understand control-flow, so use pushbytes ops.warnf("byte %s used with explicit bytecblock. must pushbytes", args[0]) pushbytes := OpsByName[ops.Version]["pushbytes"] return asmPushBytes(ops, &pushbytes, args) } + + // There are no backjumps, but there are multiple cblocks. Maybe one is + // conditional skipped. Too confusing. + if ops.cntBytecBlock > 1 { + pushbytes, ok := OpsByName[ops.Version]["pushbytes"] + if ok { + return asmPushBytes(ops, &pushbytes, args) + } + return ops.errorf("byte %s used with manual bytecblocks. Use bytec.", args[0]) + } + + // In both of the above clauses, we _could_ track whether a particular + // bytecblock dominates the current instruction. If so, we could use it. + val, consumed, err := parseBinaryArgs(args) if err != nil { return ops.error(err) @@ -777,12 +809,14 @@ func asmIntCBlock(ops *OpStream, spec *OpSpec, args []string) error { } } if !ops.known.deadcode { + // If we previously processed an `int`, we thought we could insert our + // own intcblock, but now we see a manual one. if ops.hasPseudoInt { ops.error("intcblock following int") } ops.intcRefs = nil ops.intc = ivals - ops.hasIntcBlock = true + ops.cntIntcBlock++ } return nil @@ -813,12 +847,14 @@ func asmByteCBlock(ops *OpStream, spec *OpSpec, args []string) error { ops.pending.Write(bv) } if !ops.known.deadcode { + // If we previously processed a pseudo `byte`, we thought we could + // insert our own bytecblock, but now we see a manual one. if ops.hasPseudoByte { ops.error("bytecblock following byte/addr/method") } ops.bytecRefs = nil ops.bytec = bvals - ops.hasBytecBlock = true + ops.cntBytecBlock++ } return nil } @@ -1680,7 +1716,7 @@ func (ops *OpStream) assemble(text string) error { ops.error(err) } - // backward compatibility: do not allow jumps behind last instruction in v1 + // backward compatibility: do not allow jumps past last instruction in v1 if ops.Version <= 1 { for label, dest := range ops.labels { if dest == ops.pending.Len() { @@ -1689,16 +1725,6 @@ func (ops *OpStream) assemble(text string) error { } } - // Before back branches, the pseudo ops can and do complain during first pass - if ops.Version >= backBranchEnabledVersion { - if ops.hasIntcBlock && ops.hasPseudoInt { - ops.error("program has int instruction with a manual intcblock") - } - if ops.hasBytecBlock && ops.hasPseudoByte { - ops.error("program has byte instruction with a manual bytecblock") - } - } - if ops.Version >= optimizeConstantsEnabledVersion { ops.optimizeIntcBlock() ops.optimizeBytecBlock() @@ -1850,7 +1876,7 @@ func replaceBytes(s []byte, index, originalLen int, newBytes []byte) []byte { // This function only optimizes constants introduces by the int pseudo-op, not // preexisting intcblocks in the code. func (ops *OpStream) optimizeIntcBlock() error { - if ops.hasIntcBlock { + if ops.cntIntcBlock > 0 { // don't optimize an existing intcblock, only int pseudo-ops return nil } @@ -1893,7 +1919,7 @@ func (ops *OpStream) optimizeIntcBlock() error { // This function only optimizes constants introduces by the byte or addr // pseudo-ops, not preexisting bytecblocks in the code. func (ops *OpStream) optimizeBytecBlock() error { - if ops.hasBytecBlock { + if ops.cntBytecBlock > 0 { // don't optimize an existing bytecblock, only byte/addr pseudo-ops return nil } @@ -2068,7 +2094,7 @@ func (ops *OpStream) prependCBlocks() []byte { prebytes := bytes.Buffer{} vlen := binary.PutUvarint(scratch[:], ops.Version) prebytes.Write(scratch[:vlen]) - if len(ops.intc) > 0 && !ops.hasIntcBlock { + if len(ops.intc) > 0 && ops.cntIntcBlock == 0 { prebytes.WriteByte(OpsByName[ops.Version]["intcblock"].Opcode) vlen := binary.PutUvarint(scratch[:], uint64(len(ops.intc))) prebytes.Write(scratch[:vlen]) @@ -2077,7 +2103,7 @@ func (ops *OpStream) prependCBlocks() []byte { prebytes.Write(scratch[:vlen]) } } - if len(ops.bytec) > 0 && !ops.hasBytecBlock { + if len(ops.bytec) > 0 && ops.cntBytecBlock == 0 { prebytes.WriteByte(OpsByName[ops.Version]["bytecblock"].Opcode) vlen := binary.PutUvarint(scratch[:], uint64(len(ops.bytec))) prebytes.Write(scratch[:vlen]) diff --git a/data/transactions/logic/assembler_test.go b/data/transactions/logic/assembler_test.go index 23ea0fb946..723274eee5 100644 --- a/data/transactions/logic/assembler_test.go +++ b/data/transactions/logic/assembler_test.go @@ -952,6 +952,51 @@ func TestManualCBlocks(t *testing.T) { require.EqualValues(t, ops.Program[4], 0x44) } +func TestManualCBlocksPreBackBranch(t *testing.T) { + partitiontest.PartitionTest(t) + t.Parallel() + + // Before backbranch enabled, the assembler is willing to assemble an `int` + // reference after an intcblock as an intc. It uses the most recent seen + // non-deadcode intcblock, so it *could* be wrong. + testProg(t, "intcblock 10 20; int 10;", backBranchEnabledVersion-1) + // By the same token, assembly complains if that intcblock doesn't have the + // constant. In v3, and v3 only, it *could* pushint. + testProg(t, "intcblock 10 20; int 30;", backBranchEnabledVersion-1, Expect{1, "int 30 used..."}) + + // Since the second intcblock is dead, the `int 10` "sees" the first block, not the second + testProg(t, "intcblock 10 20; b skip; intcblock 3 4 5; skip: int 10;", backBranchEnabledVersion-1) + testProg(t, "intcblock 10 20; b skip; intcblock 3 4 5; skip: int 3;", backBranchEnabledVersion-1, + Expect{1, "int 3 used..."}) + + // Here, the intcblock in effect is unknowable, better to force the user to + // use intc (unless pushint is available to save the day). + + // backBranchEnabledVersion-1 contains pushint + testProg(t, "intcblock 10 20; txn NumAppArgs; bz skip; intcblock 3 4 5; skip: int 10;", backBranchEnabledVersion-1) + testProg(t, "intcblock 10 20; txn NumAppArgs; bz skip; intcblock 3 4 5; skip: int 3;", backBranchEnabledVersion-1) + + // backBranchEnabledVersion-2 does not + testProg(t, "intcblock 10 20; txn NumAppArgs; bz skip; intcblock 3 4 5; skip: int 10;", backBranchEnabledVersion-2, + Expect{1, "int 10 used with manual intcblocks. Use intc."}) + testProg(t, "intcblock 10 20; txn NumAppArgs; bz skip; intcblock 3 4 5; skip: int 3;", backBranchEnabledVersion-2, + Expect{1, "int 3 used with manual intcblocks. Use intc."}) + + // REPEAT ABOVE, BUT FOR BYTE BLOCKS + + testProg(t, "bytecblock 0x10 0x20; byte 0x10;", backBranchEnabledVersion-1) + testProg(t, "bytecblock 0x10 0x20; byte 0x30;", backBranchEnabledVersion-1, Expect{1, "byte/addr/method used..."}) + testProg(t, "bytecblock 0x10 0x20; b skip; bytecblock 0x03 0x04 0x05; skip: byte 0x10;", backBranchEnabledVersion-1) + testProg(t, "bytecblock 0x10 0x20; b skip; bytecblock 0x03 0x04 0x05; skip: byte 0x03;", backBranchEnabledVersion-1, + Expect{1, "byte/addr/method used..."}) + testProg(t, "bytecblock 0x10 0x20; txn NumAppArgs; bz skip; bytecblock 0x03 0x04 0x05; skip: byte 0x10;", backBranchEnabledVersion-1) + testProg(t, "bytecblock 0x10 0x20; txn NumAppArgs; bz skip; bytecblock 0x03 0x04 0x05; skip: byte 0x03;", backBranchEnabledVersion-1) + testProg(t, "bytecblock 0x10 0x20; txn NumAppArgs; bz skip; bytecblock 0x03 0x04 0x05; skip: byte 0x10;", backBranchEnabledVersion-2, + Expect{1, "byte 0x10 used with manual bytecblocks. Use bytec."}) + testProg(t, "bytecblock 0x10 0x20; txn NumAppArgs; bz skip; bytecblock 0x03 0x04 0x05; skip: byte 0x03;", backBranchEnabledVersion-2, + Expect{1, "byte 0x03 used with manual bytecblocks. Use bytec."}) +} + func TestAssembleOptimizedConstants(t *testing.T) { partitiontest.PartitionTest(t) t.Parallel() diff --git a/data/transactions/logic/eval_test.go b/data/transactions/logic/eval_test.go index 32367c9f86..aea652693d 100644 --- a/data/transactions/logic/eval_test.go +++ b/data/transactions/logic/eval_test.go @@ -897,6 +897,9 @@ func TestBytecTooFar(t *testing.T) { } func TestManualCBlockEval(t *testing.T) { + partitiontest.PartitionTest(t) + t.Parallel() + // TestManualCBlock in assembler_test.go demonstrates that these will use // an inserted constant block. testAccepts(t, "int 4; int 4; +; int 8; ==; return; intcblock 10", 2) From 7893fa414d66261d2c61bbb4c1ca6c54ae3cf7da Mon Sep 17 00:00:00 2001 From: John Jannotti Date: Tue, 30 Aug 2022 21:07:30 -0400 Subject: [PATCH 3/3] Add coverage to an old error case --- data/transactions/logic/assembler_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/data/transactions/logic/assembler_test.go b/data/transactions/logic/assembler_test.go index 723274eee5..33b72a1424 100644 --- a/data/transactions/logic/assembler_test.go +++ b/data/transactions/logic/assembler_test.go @@ -771,6 +771,7 @@ func TestOpBytes(t *testing.T) { require.NotNil(t, prog) s := hex.EncodeToString(prog) require.Equal(t, mutateProgVersion(v, "0126010661626364656628"), s) + testProg(t, "byte 0x7; len", v, Expect{1, "...odd length hex string"}) }) } }