Feature/contract to contract -> master #3397

jannotti · 2022-01-07T19:53:46Z

No description provided.

* Three new globals for to help contract-to-contract usability * detritis * Check error * doc comments

* Three new globals for to help contract-to-contract usability * detritis * Check error * doc comments * opcode, docs, and tests * specs update

data/transactions/logic/eval.go

algorandskiy

Some comments, part 1

cmd/tealdbg/local.go

cmd/tealdbg/localLedger_test.go

config/consensus.go

daemon/algod/api/server/v2/dryrun.go

data/transactions/logic/backwardCompat_test.go

algorandskiy · 2022-01-31T17:56:04Z

data/transactions/logic/backwardCompat_test.go

-		require.Error(t, err)
-		require.Contains(t, err.Error(), "greater than protocol supported version")
-		_, err = Eval(ops.Program, ep)
+		ep, _, _ := makeSampleEnvWithVersion(1)


I do not think it is a good idea to use a fake protocol makeSampleTxn in a test checking specific pre-app behavior. I'd rather avoid modifying these backward compact tests at all

The thing we are checking here is whether the version handling is done properly. So we set the allowed logic version to a known value, and confirm that anything higher is rejected.

If we want to separately test that protocol.ConsensusV23 has a particular version, that seems ok, but why not test that every single consensus version is exactly what it always has been? It certainly doesn't seem like a test for the logic package.

algorandskiy · 2022-01-31T17:58:08Z

data/transactions/logic/backwardCompat_test.go

-			ep.Proto = &proto
-			ep.Ledger = ledger
-			ep.TxnGroup = txgroup
+			ep, tx, _ := makeSampleEnvWithVersion(1)


same as above, can't we let it be as is? especially defaultEvalParams stil lexist and looks like no need to update the test.

I can't see why we would test that config.Consensus[protocl.ConsensusV23] has a specific logicsig version when what we're trying to see is if fields are properly rejected by having the wrong version. You might as well just require.Equal(t, config.Consensus[protocol.ConsensusV23].LogicVersion, 4)

ConsensusV23 is pre-app version where teal programs might have v0 and v1, and the test checks if pre-app TEAL programs fail with with rekey set and etc. The new code makes a fake proto with TEAL v1 only, and if fact does not check real scenario.

data/transactions/logic/eval.go

tsachiherman · 2022-01-31T18:27:43Z

data/transactions/logic/eval.go

+		if _, ok := cx.Txn.EvalDelta.LocalDeltas[accountIdx]; !ok {
+			cx.Txn.EvalDelta.LocalDeltas[accountIdx] = basics.StateDelta{}
+		}
+		cx.Txn.EvalDelta.LocalDeltas[accountIdx][key] = tv.ToValueDelta()


Shouldn't the size of LocalDeltas be capped to config.MaxEvalDeltaAccounts ?

That's an interesting question, and something I'm still grappling with for allocbounds. I don't think that our code explicitly enforces many of the allocbounds. Instead, the allocbounds are calculated so that they can not be exceeded (they are usually the max across any protocol version, or something even more conservative).

I do think we have a problem if we ever get that wrong (as we've been talking about the last few days). But I don't know if should just sprinkle checks around.

I suppose I would feel safer if I wrote some sort of func (ad ApplyData) checkAllocBounds() that recursively checked them, and invoked it at the end of evaluation. Does that sound reasonable?

The high level design was to write the code so that it wouldn't have the ability to generate unrealistic sizes of arrays/maps. Then, the msgp allocbound were set to align with these.

I don't believe that we have too many locations that have these issue. I think that it would be less error prone to perform that inline rather than "export" that process. ( i.e. keep the limits next to the variable )

In that regard, this is true here. The allocbound is huge (2048) but the number of different accounts that can be the accountIdx here is limited to 5. (The number of accounts in the static array, plus one for sender.)

data/transactions/logic/eval.go

algorandskiy

part 2...

data/transactions/logic/eval.go

algorandskiy · 2022-01-31T23:15:07Z

data/transactions/logic/eval.go

+		return
+	}
+
+	addr, _, err := cx.accountReference(cx.stack[last])


mutableAccountReference? as the call to ledger.Balance below assumes this is preexisting account, and mutableAccountReference returns non-error only for existing addresses

mutable only means that the the account being looked up must have an index in the static array (or be the sender so we can use 0). The idea is that we have added some rules to allow access to more accounts, but we have no way to write a LocalDelta unless we have an index. So we can't allow access to those extra accounts for writes. That means we only need mutableAccount() when doing a put or delete.

data/transactions/logic/eval.go

algorandskiy

I like the change and exceed testing. It might be worth exploring a case of self opting-in and setting own local state when signed with a logic sig.
Another point is to check the old cow ApplyData calculation and the new ApplyData in logic produce roughly the same results (exclude nil vs empty maps and so on).
I'll make another readout of logic.eval and revisit previous questions.

Btw, I found divw and bsqrt are not relevant to this PR but looks like it is too much work to extract it.

data/transactions/logic/evalAppTxn_test.go

data/transactions/logic/evalStateful_test.go

ledger/apply/asset.go

data/transactions/logic/eval_test.go

algorandskiy · 2022-02-01T18:17:33Z

ledger/internal/appcow.go

+			evalDelta.Logs = params.TxnGroup[gi].EvalDelta.Logs
+			evalDelta.InnerTxns = params.TxnGroup[gi].EvalDelta.InnerTxns
+		} else {
+			evalDelta = params.TxnGroup[gi].EvalDelta


Since state modification from logic.eval goes into ledger state functions, shouldn't we have a test comparing deltas from logic to what BuildEvalDelta would build?

algorandskiy · 2022-02-02T16:36:08Z

Please consider adding the following test:

App A calls B
B opt-ins into self and sets some local state
check B.Address() account for existing non-empty local state.

jannotti · 2022-02-02T16:51:12Z

Please consider adding the following test:

App A calls B

B opt-ins into self and sets some local state

check B.Address() account for existing non-empty local state.

An app cannot opt its account into itself, because that would require re-entrancy. (To optin into yourself, you'd need to issue an app call to yourself with OnCompletion=OptIn)

Is the main idea to see if, when A calls B, if B makes changes, those changes are visible to A?

algorandskiy · 2022-02-02T16:57:02Z

Looks like we also missing global/local state reads in scenarios like these:

A calls B, B sets B's state
A calls C, C reads B's state
A reads B's state

From the code I see it should work but I do not see tests checking this (no add_global_get opcode is on only in old tests).

* Test current state of inner clear state behavior * First half of clear state fixes. Prevents inner calls in CSP Should also implement the "CSP requires 700 budget and can't spend more than 700 budget" rule, but tests still required. * Match program version. Don't allow downgrade < 6. * partition test * typo Co-authored-by: Michael Diamant <[email protected]> * typo Co-authored-by: Michael Diamant <[email protected]> * Make the default clear_state program match versions to approval * Correct some tests that now need programs present * Fix test to actually test INNER clear state * Add tests for opcode budget and CSPs * Improve tracing for inner app calls * Error now reports what the cost *was* before the attempted inst. * Simplify type switch and maybe kick CI? * Fewer copies during inner processing Co-authored-by: Michael Diamant <[email protected]>

* test for global modifications by various inner apps * CR adjustments to feeCredit * Test local state sharing too. * explain clear state details

github-actions · 2022-02-08T16:38:00Z

data/transactions/logic/eval.go

+		return
+	}
+
+	// if deleting a non-existant value, do nothing, matching ledger behavior


🚫 [Lint Errors] _{reported by reviewdog 🐶}
existant is a misspelling of existent (misspell)

github-actions · 2022-02-08T16:38:00Z

data/transactions/logic/eval.go

@@ -3440,7 +3744,19 @@ func opAppGlobalDel(cx *EvalContext) {
 		return
 	}

-	err := cx.Ledger.DelGlobal(key)
+	// if deleting a non-existant value, do nothing, matching ledger behavior


🚫 [Lint Errors] _{reported by reviewdog 🐶}
existant is a misspelling of existent (misspell)

algorandskiy

Let's merge and add some enhanced verification of cow content vs EvalDelta as part of race build later.

## Summary Create an update path that would enable the following features: 1. Batch Verification (#3031) 2. State Proof Keys (#2990) 3. TEAL 6 (#3397) 4. Expired Participation Keys (#2924) 5. Fix rewards calculation bug (#3403) ## Test Plan Unit tests added.

jannotti and others added 30 commits October 6, 2021 20:49

The new inner appl fields

cd116c0

Merge branch 'master' into inner-appl

1f34627

Unit tests for field setting on appls

088a724

Construct EvalDelta in AVM rather than by inspecting ledger

3b742fb

Merge 'master' into inner-appl, adapt to ledger/internal mocks.

bb4e0cd

Obey the linter!

4797e9b

more LedgerForEvaluation accomodation

fc4455c

Test inner evaldeltas

2ad431b

Checks on calling old AVM apps, or re-entrancy

5046b23

Allow opcode budget to be added to by executing inner apps.

6d80f94

TxID and GroupID for inner transactions

2fdee53

gitxn/gitxna

a4e71f0

Lint, spec generate

f81082c

Merge branch 'master' into inner-appl

a9f66ae

txn simplifications

566c840

Encode "arrayness" in the txn field spec

19fdabc

Pavel's CR comments

8fdff9a

Update tests to distinguish assembly / eval errors

cc690fb

Test itxn_field assembly separate from eval

a64bd3b

factor out the array index parsing of all the txn assembly

b86e11f

Consistent errors and parsign for many opcodes

3f348fd

Cleanup immediate parsing, prep txn effects testing

ff34b35

EvalParams is now a single object used to evaluate each txn in turn.

4c2d6e5

Simplifications for the Dawg (the Review Dog)

d1efc26

Use a copy for the EvalParams.TxnGroup

099f843

Set the logicsig on txns in the GroupContext, so check() can see it

1995986

Update test for explicit empty check

0f357e8

Merge branch 'master' into inner-appl

9519f9a

Three new globals for to help contract-to-contract usability (#3237)

d371ef2

* Three new globals for to help contract-to-contract usability * detritis * Check error * doc comments

Gloadss (#3248)

d28eca1

* Three new globals for to help contract-to-contract usability * detritis * Check error * doc comments * opcode, docs, and tests * specs update

tsachiherman reviewed Jan 31, 2022

View reviewed changes

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

tsachiherman reviewed Jan 31, 2022

View reviewed changes

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

tsachiherman reviewed Jan 31, 2022

View reviewed changes

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

algorandskiy reviewed Jan 31, 2022

View reviewed changes

tsachiherman reviewed Jan 31, 2022

View reviewed changes

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

tsachiherman reviewed Jan 31, 2022

View reviewed changes

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

algorandskiy reviewed Jan 31, 2022

View reviewed changes

jannotti mentioned this pull request Feb 1, 2022

AVM: SHA3-256 hash function opcode #3544

Closed

algorandskiy reviewed Feb 1, 2022

View reviewed changes

Tsachi cr (#3538)

7c2329c

jannotti and others added 4 commits February 6, 2022 13:45

Merge branch 'master' into feature/contract-to-contract

9fca1ee

Global test (#3587)

cdbf4f0

* test for global modifications by various inner apps * CR adjustments to feeCredit * Test local state sharing too. * explain clear state details

Match old BuildEvalDelta behaviour - no delete missing keys (#3594)

2c7b906

github-actions bot reviewed Feb 8, 2022

View reviewed changes

spelling (and clarity) fix

46529d3

tsachiherman approved these changes Feb 8, 2022

View reviewed changes

algorandskiy approved these changes Feb 8, 2022

View reviewed changes

jannotti merged commit 0111cb1 into master Feb 8, 2022

tsachiherman mentioned this pull request Feb 10, 2022

V31 consensus upgrade #3553

Merged

onetechnical mentioned this pull request Feb 11, 2022

go-algorand 3.4.0-beta #3613

Merged

egieseke mentioned this pull request Feb 16, 2022

Script to generate go-algorand release notes #3520

Closed

michaeldiamant mentioned this pull request Feb 17, 2022

Support TEAL 6 txn fields LastLog, StateProofPK and opcodes divw, itxnas, gitxnas algorand/pyteal#174

Merged

onetechnical mentioned this pull request Mar 1, 2022

go-algorand 3.4.2-stable #3700

Merged

algojohnlee deleted the feature/contract-to-contract branch November 2, 2022 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/contract to contract -> master #3397

Feature/contract to contract -> master #3397

jannotti commented Jan 7, 2022

algorandskiy left a comment

algorandskiy Jan 31, 2022

jannotti Jan 31, 2022

algorandskiy Jan 31, 2022

jannotti Jan 31, 2022

algorandskiy Feb 2, 2022

tsachiherman Jan 31, 2022

jannotti Jan 31, 2022

tsachiherman Jan 31, 2022

jannotti Feb 2, 2022

algorandskiy left a comment

algorandskiy Jan 31, 2022

jannotti Feb 1, 2022

algorandskiy left a comment

algorandskiy Feb 1, 2022

algorandskiy commented Feb 2, 2022

jannotti commented Feb 2, 2022

algorandskiy commented Feb 2, 2022

github-actions bot Feb 8, 2022

github-actions bot Feb 8, 2022

algorandskiy left a comment

Feature/contract to contract -> master #3397

Feature/contract to contract -> master #3397

Conversation

jannotti commented Jan 7, 2022

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

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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algorandskiy commented Feb 2, 2022

jannotti commented Feb 2, 2022

algorandskiy commented Feb 2, 2022

github-actions bot Feb 8, 2022

Choose a reason for hiding this comment

github-actions bot Feb 8, 2022

Choose a reason for hiding this comment

algorandskiy left a comment

Choose a reason for hiding this comment