EvalTracer: Txn group deltas Tracer #5297
Conversation
| @@ -161,7 +162,7 @@ func (d *Tracer) BeforeTxnGroup(ep *logic.EvalParams) { | |||
| } | |||
|
|
|||
| // AfterTxnGroup mocks the logic.EvalTracer.AfterTxnGroup method | |||
| func (d *Tracer) AfterTxnGroup(ep *logic.EvalParams, evalError error) { | |||
| func (d *Tracer) AfterTxnGroup(ep *logic.EvalParams, update *ledgercore.StateDelta, evalError error) { | |||
There was a problem hiding this comment.
nit: Inconsistent naming. No strong preference for update, delta or deltas.
| func (d *Tracer) AfterTxnGroup(ep *logic.EvalParams, update *ledgercore.StateDelta, evalError error) { | |
| func (d *Tracer) AfterTxnGroup(ep *logic.EvalParams, deltas *ledgercore.StateDelta, evalError error) { |
data/transactions/logic/tracer.go
Outdated
| @@ -129,7 +130,11 @@ type EvalTracer interface { | |||
| // AfterTxnGroup is called after a transaction group has been executed. This includes both | |||
| // top-level and inner transaction groups. The argument ep is the EvalParams object for the | |||
| // group; if the group is an inner group, this is the EvalParams object for the inner group. | |||
| AfterTxnGroup(ep *EvalParams, evalError error) | |||
| // | |||
| // no-op methods we don't care about | ||
| logic.NullEvalTracer |
There was a problem hiding this comment.
Curious what others think about this. There are weird side effects of using inheritance in go like this (i.e. it doesn't actually use polymorphism, calls from the embedded interface don't call the parent overrides). These don't apply to NullEvalTracer, but I'm generally hesitant about showing off the pattern.
The alternative is empty function definitions, which doesn't seem that bad.
There was a problem hiding this comment.
Yeah it does use "inheritance" in ways that don't seem idiomatic. This is how it's being done in other (all other?) tracers that I've seen so far in the codebase. https://github.com/algorand/go-algorand/blob/master/ledger/simulation/tracer.go#L30
There was a problem hiding this comment.
We considered a few different options before settling on the current NullEvalTracer idea. Originally we actually broke up the EvalTracer interface into many small ones, since we anticipated most tracers would only want to implement only a few related methods. Having to add a bunch of empty methods for things you don't care about seemed annoying, and it also slowed down development since there were frequent changes to methods.
It's definitely a bit weird though
There was a problem hiding this comment.
Thanks for the background. You might look into multiple optional interfaces and a cast check. We've been using that pattern for optional functions in Conduit. It's a little more work but a really nice interface to implement.
Here's one example. Plugins can optionally implement the Complete interface for a callback after a round is finished https://github.com/algorand/conduit/blob/585a568f091d333a79b27e872ef781206d90859d/conduit/pipeline/pipeline.go#L73
| } | ||
|
|
||
| // GetDeltasForRound supplies all StateDelta objects for txn groups in a given rnd | ||
| func (tracer *TxnGroupDeltaTracer) GetDeltasForRound(rnd basics.Round) ([]ledgercore.StateDelta, error) { |
There was a problem hiding this comment.
This probably needs a mutex in the next PR
| require.NoError(t, err) | ||
| newBlock := bookkeeping.MakeBlock(blkHeader) | ||
| tracer := MakeTxnGroupDeltaTracer(4) | ||
| eval, err := l.StartEvaluator(newBlock.BlockHeader, 0, 0, tracer) |
Summary
PR for adding a txn-group deltas tracer.
Also adds
ledgercore.StateDeltato theAfterTxnGrouphook in theEvalTracer. Functionality is gated by a new config variable.#5272
Test Plan
Unit tests included.