eval: Add block hooks to eval tracer #5303

Eric-Warehime · 2023-04-13T20:39:32Z

Summary

Adds BeforeBlock and AfterBlock hooks to the EvalTracer interface.

This should be a no-op for most of the things which Simulate is trying to do, but gives the Tracer a sense of which Block a given set invocations covers. That's important since the EvalContext and EvalParams from what I can tell don't have a deterministic way of looking at which round they're evaluating for without direct access to the BlockEvaluator itself.

Test Plan

Updated existing test to ensure calls are properly made during eval lifecycle.

Eric-Warehime · 2023-04-13T22:15:01Z

ledger/simulation/simulator_test.go

@@ -152,6 +152,7 @@ int 1`,
 		mocktracer.AfterOpcode(false),
 		mocktracer.AfterProgram(logic.ModeSig, false),
 		// Txn evaluation
+		mocktracer.BeforeBlock(block.Block().Round()),


This order was surprising to me, but because LogicSig verification happens before the Txns in the block get evaluated the BeforeBlock call will happen after LogicSig verification.

algochoi

Looks pretty reasonable to me - is the plan to compare some sort of deltas between the two before/after block headers or simply to extract certain information from it, e.g. round num?

Eric-Warehime · 2023-04-14T15:48:07Z

Looks pretty reasonable to me - is the plan to compare some sort of deltas between the two before/after block headers or simply to extract certain information from it, e.g. round num?

The reason I want it currently is to get the round num, but I thought it would be useful to have the whole header just in case we need other data from it as well.

You can take a look at #5297 to see how the Tracer I'm writing needs something like this. It's still in draft though so there's a bunch of unfinished stuff in there.

ahangsu

Just took a quick pass, and I think the PR might be interesting on simulation aspect (if I read the PR correctly), but might be hard (or unreasonable) to do. I was thinking:

If we are overloading some runtime config in EvalContext for certain opcodes during simulating multiple txn-groups, then the current implementation is to overload the configs inside cx *EvalContext during BeforeTxnGroup.

I think it might be better to just overload config once in BeforeBlock, assuming the multiple txn-groups are falling into same block, and EvalContext can be maintained through more than one txn-groups.

But that might be not the purpose of this PR, and EvalContext is not avail on block level?

tzaffi

Seems like a safe PR, but I don't see the big picture yet for its impact. I left a couple of commentary nits and a question.

tzaffi · 2023-04-14T16:57:29Z

data/transactions/logic/mocktracer/tracer.go

+	Round basics.Round
+}
+
+// BeforeBlock creates a new Event with the type BeforeBlockEvent


Suggested change

// BeforeBlock creates a new Event with the type BeforeBlockEvent

// BeforeBlock creates a new Event with the type BeforeBlockEvent for a particular round

ignorable nit

tzaffi · 2023-04-14T18:26:11Z

ledger/ledger.go

@@ -833,13 +834,14 @@ func (l *Ledger) VerifiedTransactionCache() verify.VerifiedTransactionCache {
 // provides a cap on the size of a single generated block size, when a non-zero value is passed.
 // If a value of zero or less is passed to maxTxnBytesPerBlock, the consensus MaxTxnBytesPerBlock would
 // be used instead.
-func (l *Ledger) StartEvaluator(hdr bookkeeping.BlockHeader, paysetHint, maxTxnBytesPerBlock int) (*eval.BlockEvaluator, error) {
+func (l *Ledger) StartEvaluator(hdr bookkeeping.BlockHeader, paysetHint, maxTxnBytesPerBlock int, tracer logic.EvalTracer) (*eval.BlockEvaluator, error) {


Can we add an explanation of the optional tracer param as well?

tzaffi · 2023-04-14T20:00:55Z

ledger/eval/eval_test.go

+			tracer := &mocktracer.Tracer{}
+			eval, err := l.StartEvaluator(newBlock.BlockHeader, 0, 0, tracer)


Is this test being modified purely because there is now a prefered way to attach a tracer? I'm trying to understand what new behavior is being tested.

It's both the new way of attaching the tracer, but also the new Before/After Block hooks. Here I've modified how the tracer is attached, and below you can see I've added the mocktracer.AfterBlock(eval.block.Round()), event to assert that the tracer was called as expected.

jasonpaulos

Looks pretty good. I just have minor comments

ledger/simulation/simulator.go

ledger/eval/eval_test.go

data/transactions/logic/tracer.go

Eric-Warehime · 2023-04-18T16:26:46Z

Just took a quick pass, and I think the PR might be interesting on simulation aspect (if I read the PR correctly), but might be hard (or unreasonable) to do. I was thinking:

If we are overloading some runtime config in EvalContext for certain opcodes during simulating multiple txn-groups, then the current implementation is to overload the configs inside cx *EvalContext during BeforeTxnGroup.

I think it might be better to just overload config once in BeforeBlock, assuming the multiple txn-groups are falling into same block, and EvalContext can be maintained through more than one txn-groups.

But that might be not the purpose of this PR, and EvalContext is not avail on block level?

I didn't intend for the block evaluator to carry state across txns/groups like that, but I guess you could do it if you wanted. The EvalContext doesn't have anything to do with the ledger's idea of starting and finishing block evaluation though. It's just involved in contract evaluation.

Eric-Warehime · 2023-04-18T16:28:10Z

Looks pretty reasonable to me - is the plan to compare some sort of deltas between the two before/after block headers or simply to extract certain information from it, e.g. round num?

Right now it's just to extract the round from them--I've included the whole header to have a more generalized interface.

jasonpaulos

LGTM

algochoi

Minor question below but looks good

data/transactions/logic/tracer.go

Add block hooks to eval tracer

acd3f55

Eric-Warehime added the Enhancement label Apr 13, 2023

Fix tests

4d177b9

Eric-Warehime commented Apr 13, 2023

View reviewed changes

Fix tests

3954009

Eric-Warehime marked this pull request as ready for review April 14, 2023 00:42

Eric-Warehime requested review from jasonpaulos, winder, tzaffi, jannotti, shiqizng and algochoi April 14, 2023 00:42

algochoi reviewed Apr 14, 2023

View reviewed changes

ahangsu reviewed Apr 14, 2023

View reviewed changes

tzaffi reviewed Apr 14, 2023

View reviewed changes

jasonpaulos reviewed Apr 14, 2023

View reviewed changes

ledger/simulation/simulator.go Show resolved Hide resolved

ledger/eval/eval_test.go Show resolved Hide resolved

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

PR comments

9f94129

Add diagram

2ef85d8

Eric-Warehime requested review from tzaffi, algochoi, jasonpaulos and ahangsu April 18, 2023 17:02

jasonpaulos approved these changes Apr 18, 2023

View reviewed changes

algochoi approved these changes Apr 18, 2023

View reviewed changes

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

algorandskiy assigned Eric-Warehime Apr 18, 2023

winder approved these changes Apr 19, 2023

View reviewed changes

winder added the Team Lamprey label Apr 19, 2023

winder merged commit 66911af into algorand:master Apr 19, 2023
23 checks passed

This was referenced May 9, 2023

go-algorand 3.16.0-beta Release PR #5370

Closed

go-algorand 3.16.0-beta Release PR #5371

Closed

go-algorand 3.16.0-beta Release PR #5382

Closed

Algo-devops-service mentioned this pull request May 22, 2023

go-algorand 3.16.0-beta Release PR #5406

Merged

This was referenced May 24, 2023

go-algorand 3.16.0-beta Release PR #5417

Merged

go-algorand 3.16.0-beta Release PR #5430

Merged

go-algorand 3.16.0-beta Release PR #5434

Merged

Algo-devops-service mentioned this pull request Jun 12, 2023

go-algorand 3.16.1-stable Release PR #5465

Merged

onetechnical mentioned this pull request Jun 14, 2023

go-algorand 3.16.2-stable Release PR #5469

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eval: Add block hooks to eval tracer #5303

eval: Add block hooks to eval tracer #5303

Eric-Warehime commented Apr 13, 2023

Eric-Warehime Apr 13, 2023

algochoi left a comment

Eric-Warehime commented Apr 14, 2023

ahangsu left a comment

tzaffi left a comment •

edited

tzaffi Apr 14, 2023

tzaffi Apr 14, 2023

tzaffi Apr 14, 2023

Eric-Warehime Apr 18, 2023

jasonpaulos left a comment

Eric-Warehime commented Apr 18, 2023

Eric-Warehime commented Apr 18, 2023

jasonpaulos left a comment

algochoi left a comment

	// BeforeBlock creates a new Event with the type BeforeBlockEvent
	// BeforeBlock creates a new Event with the type BeforeBlockEvent for a particular round

		tracer := &mocktracer.Tracer{}
		eval, err := l.StartEvaluator(newBlock.BlockHeader, 0, 0, tracer)

eval: Add block hooks to eval tracer #5303

eval: Add block hooks to eval tracer #5303

Conversation

Eric-Warehime commented Apr 13, 2023

Summary

Test Plan

Eric-Warehime Apr 13, 2023

Choose a reason for hiding this comment

algochoi left a comment

Choose a reason for hiding this comment

Eric-Warehime commented Apr 14, 2023

ahangsu left a comment

Choose a reason for hiding this comment

tzaffi left a comment • edited

Choose a reason for hiding this comment

tzaffi Apr 14, 2023

Choose a reason for hiding this comment

tzaffi Apr 14, 2023

Choose a reason for hiding this comment

tzaffi Apr 14, 2023

Choose a reason for hiding this comment

Eric-Warehime Apr 18, 2023

Choose a reason for hiding this comment

jasonpaulos left a comment

Choose a reason for hiding this comment

Eric-Warehime commented Apr 18, 2023

Eric-Warehime commented Apr 18, 2023

jasonpaulos left a comment

Choose a reason for hiding this comment

algochoi left a comment

Choose a reason for hiding this comment

tzaffi left a comment •

edited