Algod: Modify EvalTracer design and improve testing for failures#5071
Conversation
| ad.EvalDelta = transactions.EvalDelta{} | ||
| return errors.New("Approval program failed") | ||
| } | ||
| ad.EvalDelta = cx.txn.EvalDelta |
There was a problem hiding this comment.
The real ledger does not set EvalDeltas if an error happens, however the test ledger implicitly does right now. This is because ad is pointer to cx.txn.ApplyData, so this line is a noop.
I added some lines above this to explicitly clear the AD's EvalDelta if a failure happens, and that's good enough to get my tests to pass.
There was a problem hiding this comment.
maybe add this comment inline
| // along with go-algorand. If not, see <https://www.gnu.org/licenses/>. | ||
|
|
||
| package logic | ||
| package logic_test |
There was a problem hiding this comment.
By changing to the logic_test package, we can now import the mocktracer package without circular import issues!
Codecov Report
@@ Coverage Diff @@
## master #5071 +/- ##
==========================================
+ Coverage 53.44% 53.46% +0.02%
==========================================
Files 431 431
Lines 54364 54373 +9
==========================================
+ Hits 29056 29073 +17
+ Misses 23053 23043 -10
- Partials 2255 2257 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
What about a second paradigm where you separate out errors into its own class, i.e partial-ApplyData on an error is stored in a special erroredApplyData, and instead of calling AfterTxn or AfterProgram you go straight to an AfterError method. |
|
@bbroder-algo having a separate function for errors, like
|
jannotti
left a comment
jannotti
left a comment
There was a problem hiding this comment.
It seems fine - I tried to pay most of my attention to places I thought might change or slow down execution. I put relatively less effort into ensuring I believe the traces are correct.
One place you might to test is that all the handling is correct if an opcode panics. That seems like a challenging case, especially an opcode in an inner txn.
TestPanic shows nice trick to have such a test.
| // Due to the LIFO behavior of defer statements, if we want the tracer to catch panics, we | ||
| // need to have a `recover` defer statement defined after the tracer-invoking defer | ||
| // statement. However, since it's possible for the tracer itself to panic, we still want to | ||
| // keep the outmost `recover` defer statement defined at the top of this function. | ||
| defer func() { | ||
| if x := recover(); x != nil { | ||
| err = makePanicError(x, cx.EvalParams, "Eval") | ||
| pass = false | ||
| } | ||
| }() |
There was a problem hiding this comment.
Definitely looking for feedback on this. As the comment mentions, go defer statements are executed LIFO, so without this additional recovery, the above defer which invokes tracer methods will not see any PanicErrors, as those are only created by the defer at the top of this function.
By adding another defer layer that can recover from panics, we make them visible to the tracer.
An alternative here would be to just move the single recovery defer from the beginning of the function to after we define the tracer deferred logic. I opted against it since I didn't want to decrease coverage of potential panicking code, especially since it's in theory possible for tracer methods themselves to panic.
(There's a new test case, TestEvalWithTracerPanic, which confirm a tracer method panicking gets caught during eval.)
There was a problem hiding this comment.
I think I follow what's going on, but let me ask in order to be sure. You are trying to catch the panic "early", and build the stack trace so that you have it available to give out to the tracer. Might you simply call the Tracer with a simpler error here, and then panic again with x?
That is, only one defer, something like:
defer func() {
x := recover();
if x != nil {
// A panic error occurred during the eval loop. Report it now.
cx.Tracer.AfterOpcode(cx, fmt.Errorf("some nonsense"))
}
// Ensure we update the tracer before exiting
cx.Tracer.AfterProgram(cx, err)
if x != nil {
panic(x)
}
}()
I recognize this gives AfterOpcode slightly less information to go on. But I'm trying to make it so that a panic is always handled through the single stack trace creating handler. Done this way, when there's a Tracer, we build the stack here, and we don't go through the existing panic handler, since we're no longer panicking.
My motivation is that I think I can see a future where we really want to preserve the difference of "normal" error returns from evaluation vs a panic, if/when we consider supporting a itxn_try_submit which I think should continue to fail for panics, rather than cleanly reporting 0 for a failed txn.
There was a problem hiding this comment.
Good idea. I implemented it in 912d5de, let me know if you see any issues
algochoi
left a comment
algochoi
left a comment
There was a problem hiding this comment.
Looks good, I mostly went through the tests using a debugger to double-check that they behave as expected. I'll defer the logic/eval nuances to JJ.
| // possible during each inner transaction, as well as before all inners, between the two inner | ||
| // groups, and after all inners. For app call failures, there are scenarios for both rejection and | ||
| // runtime errors, which should invoke tracer hooks slightly differently. | ||
| func GetTestScenarios() map[string]TestScenarioGenerator { |
|
testing is excellent, passes for me, covers the cases discussed in the PR, I spent additional time staring at the changes to eval and the new panic handling/code segment relocations. |
| if pe, ok := err.(PanicError); ok { | ||
| require.Equal(t, panicString, pe.PanicValue) | ||
| pes := pe.Error() | ||
| require.True(t, strings.Contains(pes, "panic")) |
There was a problem hiding this comment.
There's require.Contains() if you'd like.
5 participants
Summary
Follow up to #4438, which introduced the new
EvalTracerinterface for tracing transaction and program evaluation.This PR improves how evaluation failures are reported to
EvalTracers. Prior to this, if a failure occurred in a deeply nested inner transaction, evaluation would immediately stop, and theAfterOpcodeandAfterProgramtracer methods would get invoked with the error, starting with the deeply nested inner transaction, and unwinding up the layers to the top-level transaction which encompasses the error. The issue is that theAfterTxnandAfterTxnGroupmethods would not get invoked at all during failures, so anEvalTracerwould have no idea that the context ever changed from the inner transaction which spawned the failure.This PR reconciles the situation by making it so that
AfterTxnandAfterTxnGroupalso get called when an error happens in an inner transaction context. NowEvalTracers will see the correct context changes when evaluation halts because of an error.(You might be wondering why we want
EvalTracers to get called after an error happens at all, and the answer to this is so that they can gather detailed information about the failure, such as partially-completedApplyDeltas).Test Plan
New tests are added which cover transaction and program failures. Specifically, the
mocktracerpackage now contains test scenarios which cover many possible failures in an app call and its inner transactions.