tracer: Default to ledger tracer when starting new evaluator #5521
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5521 +/- ##
==========================================
- Coverage 55.81% 55.77% -0.05%
==========================================
Files 446 446
Lines 63228 63231 +3
==========================================
- Hits 35290 35266 -24
- Misses 25572 25590 +18
- Partials 2366 2375 +9
... and 15 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| func (l *Ledger) StartEvaluator(hdr bookkeeping.BlockHeader, paysetHint, maxTxnBytesPerBlock int, tracer logic.EvalTracer) (*eval.BlockEvaluator, error) { | ||
| tracerForEval := tracer |
There was a problem hiding this comment.
Stylistic and ignoreable nit: introducing a new variable isn't necessary. You can either rename the param tracerForEval or even without renaming simply overwrite it with l.tracer when it's nil.
There was a problem hiding this comment.
Another point to consider. Currently, and in the pre-existing code the tracer param overrides l.tracer even when it's non-nil. This feels like an error to me and perhaps deserves an error (though it may very well break some tests and even live uses). Something like:
if tracer != nil && l.tracer != nil {
return nil, fmt.Errorf("ledger StartEvaluator(): ambiguous non-nil EvalTracers provided both in Ledger and as tracer param")
}
There was a problem hiding this comment.
We need to make sure this doesn't break the simulate endpoint, which attaches its own tracer with this function.
There was a problem hiding this comment.
@jasonpaulos are there regression tests?
There was a problem hiding this comment.
I don't believe there are any tests of simulate with the config option EnableTxnEvalTracer=true yet. This would be a good test to ensure simulate isn't affected by this PR.
Probably the simplest way to achieve this would be to either augment or copy TestSimulateTransaction to also test with the new DevModeTxnTracerNetwork.json.
There was a problem hiding this comment.
Yeah the intention right now is to allow StartEvaluator to have the final say in which tracer is attached to the newly created evaluator. The ledger's tracer is provided as the default in case nothing is passed in.
This lets simulate/txn eval tracer work for their respective use cases currently, but maybe we would want to revisit this decision in the future if/when we have more tracers competing for access to the evaluator.
There was a problem hiding this comment.
Added a test which is a copy of the TestSimulateTransaction test w/ a couple of assertions changed because devmode lets us be confident that the round increases deterministically.
|
FWIW I ran a simple payment and app call (https://github.com/algorand/py-algorand-sdk/blob/develop/examples/apps.py) on the simulate endpoint using this branch and it looks ok as well |
| func (l *Ledger) StartEvaluator(hdr bookkeeping.BlockHeader, paysetHint, maxTxnBytesPerBlock int, tracer logic.EvalTracer) (*eval.BlockEvaluator, error) { | ||
| tracerForEval := tracer |
Summary
Addresses #5483 by assigning the Block Evaluator's tracer to be the tracer associated with the ledger on creation if there is no tracer passed in.
This lets devmode and private networks accumulate txn group deltas when the tracer is enabled on the ledger.
Test Plan
Added e2e test which retrieves txn group deltas while in dev mode.