AVM: Make `txn FirstValidTime` and `block` opcode available in logicsigs #4371

jannotti · 2022-08-08T16:15:47Z

Thread the ledger into the GroupContext so it can be used during LogicSig evaluation.

To limit the chance of stateless/stateful bugs, the Ledger is "narrowed" to a LedgerForSignature interface that only exposes the past block headers.

codecov · 2022-08-08T16:50:30Z

Codecov Report

Merging #4371 (aee4243) into master (d7ed271) will decrease coverage by 0.06%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master    #4371      +/-   ##
==========================================
- Coverage   55.28%   55.22%   -0.07%     
==========================================
  Files         395      395              
  Lines       50352    50367      +15     
==========================================
- Hits        27838    27814      -24     
- Misses      20119    20157      +38     
- Partials     2395     2396       +1

Impacted Files	Coverage Δ
cmd/goal/clerk.go	`8.75% <0.00%> (-0.02%)`	⬇️
data/txHandler.go	`0.00% <0.00%> (ø)`
node/node.go	`23.01% <0.00%> (-2.39%)`	⬇️
data/transactions/verify/txn.go	`43.85% <50.00%> (-1.03%)`	⬇️
data/transactions/logic/eval.go	`89.57% <76.92%> (+0.06%)`	⬆️
cmd/tealdbg/local.go	`73.95% <100.00%> (+0.09%)`	⬆️
daemon/algod/api/server/v2/dryrun.go	`70.82% <100.00%> (+0.08%)`	⬆️
data/transactions/logic/assembler.go	`85.16% <100.00%> (ø)`
ledger/internal/eval.go	`68.65% <100.00%> (+0.04%)`	⬆️
ledger/tracker.go	`71.79% <0.00%> (-4.71%)`	⬇️
... and 9 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

data/transactions/logic/eval.go

jasonpaulos · 2022-08-08T17:38:23Z

ledger/internal/eval.go

@@ -1483,6 +1483,7 @@ type evalTxValidator struct {
 	txcache          verify.VerifiedTransactionCache
 	block            bookkeeping.Block
 	verificationPool execpool.BacklogPool
+	ledger           logic.LedgerForLogic


I don't see where this field is ever assigned

Agreed. What on earth.

It seems as though LogicSigs can be evaluated along two paths, and I've clearly not fixed the path that uses evalTxValidator. Before I fix it, I'd like to find or write failing tests.

@algorandskiy do we have tests evalTxValidator? It looks to me like it's use in txn validation, so I don't understand how my code is passing most tests, including e2e tests, without a way to get block headers.

I suppose the only ones that would fail are ones with a block or txn FirstValidTime in them (since only they access SigLedger). But we have an e2e test that does that, and it passes.

I'm committing a check in EvalSignature for SigLedger == nil. I suppose we should have it anyway, but my goal is to see if maybe we do have a test that exercises this code, but it was not failing because it was not using the new opcodes, so it didn't complain about the nil SigLedger.

algorandskiy · 2022-08-08T17:31:14Z

data/transactions/logic/evalStateful_test.go

-	testApp(t, "int 4294967299; block BlkSeed; len; int 32; ==", ep) // current - 1
-	testApp(t, "int 4294967300; block BlkSeed; len; int 32; ==", ep,
-		"not available") // can't get current round's blockseed
+	testApp(t, "int 4294967300; block BlkSeed; len; int 32; ==", ep)


this is confusing.
cow.Round() is always the current (latest +1). So the old test is valid and the new one is not.

my understanding is we supply cow to contracts and ledger to logicsig. But the same Round() method returns the different values.
Maybe one should be Latest() and another Round()?

I think I know what's going on now. I added a Round() method to ledger.Ledger, so I could use it as a LedgerForSignature. I should have used Latest()+1, but used Latest(). I need to get them doing the same thing, and then the test will go back to what it was.

This should be back to making sense.

test/scripts/e2e_subs/hdr-access-logicsig.sh

algorandskiy · 2022-08-08T18:32:15Z

data/transactions/logic/eval.go

@@ -4848,7 +4861,7 @@ func (cx *EvalContext) availableRound(r uint64) (basics.Round, error) {
 	if firstAvail > cx.txn.Txn.LastValid || firstAvail == 0 { // early in chain's life
 		firstAvail = 1
 	}
-	current := cx.Ledger.Round()
+	current := cx.SigLedger.Round()
 	round := basics.Round(r)
 	if round < firstAvail || round >= current {


this is interesting, Round() is either current, or latest, depending on a context, and the condition round >= current is technically correct for both contexts but looks as a maintainability problem.

I've changed it so that regardless of how the SigLedger is supplied (either as a copy of Ledger, or on its own for LogicSig evaluation) Round() is Latest()+1. I think that is fairly good, although I remain worried about transactions added to a long queue that will be dropped asking for a header that doesn't exist yet.

ledger/internal/applications.go

data/transactions/logic/assembler.go

jannotti · 2022-08-08T21:07:20Z

I've removed Round() from SigLedger. The rule for what blocks are available is now simpler and depends only on the transaction, which seems nice for LogicSigs.

But the big mystery is how to get a test to fail for the clearly erroneous way the evalTxValidator is initialized in ledger/internal/eval.go's Eval

algorandskiy

LGTM. One comment about Round -> round.
I'll take another look on morning

data/transactions/logic/eval.go

ledger/internal/applications.go

…o hdr-in-logicsig

data/transactions/logic/eval.go

algorandskiy · 2022-08-09T15:37:33Z

data/transactions/logic/evalStateful_test.go

-	testApp(t, "int 4294967299; block BlkSeed; len; int 32; ==", ep) // current - 1
+
+	// These first two tests show that current-1 is not available now, though a
+	// resonable extension is to allow such access for apps (not sigs).


nit: resonable -> reasonable

Make txn FirstValidTime and block opcode available in logicsigs

621713f

jannotti requested review from michaeldiamant, algorandskiy and algochoi August 8, 2022 16:15

jannotti changed the title ~~Make txn FirstValidTime and block opcode available in logicsigs~~ AVM: Make txn FirstValidTime and block opcode available in logicsigs Aug 8, 2022

jannotti added the Bug-Fix label Aug 8, 2022

Add e2e test

5795cef

algochoi reviewed Aug 8, 2022

View reviewed changes

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

jasonpaulos reviewed Aug 8, 2022

View reviewed changes

algorandskiy reviewed Aug 8, 2022

View reviewed changes

Get the two ledgers using the same thing for Round()

5f96e4d

michaeldiamant reviewed Aug 8, 2022

View reviewed changes

test/scripts/e2e_subs/hdr-access-logicsig.sh Outdated Show resolved Hide resolved

algorandskiy reviewed Aug 8, 2022

View reviewed changes

michaeldiamant reviewed Aug 8, 2022

View reviewed changes

ledger/internal/applications.go Outdated Show resolved Hide resolved

tzaffi reviewed Aug 8, 2022

View reviewed changes

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

jannotti added 3 commits August 8, 2022 14:56

Code review and mroe detailed check that logicsig is working

8b532b0

Stringent check to try to catch problem

eb475c4

Update dryrun and debugger with trival SigLedgers

6e1b384

jannotti added 4 commits August 8, 2022 20:25

Fix some tests based on new rules

98289e7

Good test, thanks to @jasonpaulos

d0a6e2b

Give NoHeaderLedger to goal dryrun

71a4d5f

Give NoHeaderLedger to goal dryrun

8a0f216

jannotti force-pushed the hdr-in-logicsig branch from 71a4d5f to 8a0f216 Compare August 9, 2022 01:45

algorandskiy reviewed Aug 9, 2022

View reviewed changes

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

ledger/internal/applications.go Outdated Show resolved Hide resolved

jannotti added 2 commits August 9, 2022 07:56

unexport Round

9504d00

Merge branch 'hdr-in-logicsig' of github.com:jannotti/go-algorand int…

aee4243

…o hdr-in-logicsig

jannotti marked this pull request as ready for review August 9, 2022 13:09

jannotti requested a review from algorandskiy August 9, 2022 13:09

jannotti requested a review from jasonpaulos August 9, 2022 13:09

michaeldiamant reviewed Aug 9, 2022

View reviewed changes

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

michaeldiamant approved these changes Aug 9, 2022

View reviewed changes

algorandskiy approved these changes Aug 9, 2022

View reviewed changes

algorandskiy merged commit 566be49 into algorand:master Aug 9, 2022

jannotti deleted the hdr-in-logicsig branch August 9, 2022 15:52

Algo-devops-service mentioned this pull request Aug 9, 2022

go-algorand 3.9.0-beta Release PR #4383

Merged

Algo-devops-service mentioned this pull request Sep 1, 2022

go-algorand 3.9.2-stable Release PR #4506

Merged

This was referenced Sep 12, 2022

Add E2E test verifying behavior when transactions are not gossiped #4538

Open

Test for uninitialized ledger in testLogic #4559

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AVM: Make `txn FirstValidTime` and `block` opcode available in logicsigs #4371

AVM: Make `txn FirstValidTime` and `block` opcode available in logicsigs #4371

jannotti commented Aug 8, 2022 •

edited

Loading

codecov bot commented Aug 8, 2022 •

edited

Loading

jasonpaulos Aug 8, 2022

jannotti Aug 8, 2022

jannotti Aug 8, 2022

jannotti Aug 8, 2022

jannotti Aug 8, 2022 •

edited

Loading

jannotti Aug 8, 2022

algorandskiy Aug 8, 2022

algorandskiy Aug 8, 2022

jannotti Aug 8, 2022

jannotti Aug 8, 2022

algorandskiy Aug 8, 2022

jannotti Aug 8, 2022

jannotti commented Aug 8, 2022

algorandskiy left a comment

algorandskiy Aug 9, 2022

AVM: Make txn FirstValidTime and block opcode available in logicsigs #4371

AVM: Make txn FirstValidTime and block opcode available in logicsigs #4371

Conversation

jannotti commented Aug 8, 2022 • edited Loading

codecov bot commented Aug 8, 2022 • edited Loading

Codecov Report

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 Aug 8, 2022 • edited Loading

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

jannotti commented Aug 8, 2022

algorandskiy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AVM: Make `txn FirstValidTime` and `block` opcode available in logicsigs #4371

AVM: Make `txn FirstValidTime` and `block` opcode available in logicsigs #4371

jannotti commented Aug 8, 2022 •

edited

Loading

codecov bot commented Aug 8, 2022 •

edited

Loading

jannotti Aug 8, 2022 •

edited

Loading