Algod: Additional simulation result information #4439
Conversation
Dynamic budget checks in the evaluator complicate the gatekeeping. This functionality will be easier to add in the future after the `DebuggerHook` interface has been extended.
This test confirms an error spotted by @jannotti where `errors.As(…)` was always true. algorand#4322 (comment)
| t.Helper() | ||
|
|
||
| if seen[node] { | ||
| return |
| preEncodedTxPath(generatedResponseGraph).children = preEncodedTxPath(customResponseGraph).children | ||
| } | ||
|
|
||
| generatedResponseGraph.AssertEquals(t, customResponseGraph) |
There was a problem hiding this comment.
elaborate but helpful
| return vb, missingSignatures, err | ||
| } | ||
|
|
||
| // Simulate simulates a transaction group using the simulator. Will error if the transaction group is not well-formed. | ||
| func (s Simulator) Simulate(txgroup []transactions.SignedTxn) (Result, error) { |
daemon/algod/api/algod.oas2.json
Outdated
| "failed-at": { | ||
| "description": "If present, indicates which transaction in this group caused the failure", | ||
| "type": "array", |
There was a problem hiding this comment.
Why is this an array? The description appears to say it will be a single integer.
There was a problem hiding this comment.
It's an array because we show you the path to the failing txn. E.g. if the second txn is an app call whose third inner failed, failed-at would be [1,2]
There was a problem hiding this comment.
Can we explain that succinctly(?) in the description? Perhaps it's too much.
There was a problem hiding this comment.
I made an attempt in 9557a66, let me know what you think
There was a problem hiding this comment.
I think including that in the description is nice - the endpoint only returns the first error so it could be ambiguous whether failed-at is reporting every error in the group, or the path to the failed transaction.
daemon/algod/api/algod.oas2.json
Outdated
| "missing-signatures": { | ||
| "description": "\\[ms\\] Whether any transactions would have failed during a live broadcast because they were missing signatures.", | ||
| "would-succeed": { | ||
| "description": "Indicates whether the simulated transactions would have succeeded during an actual submission.", |
There was a problem hiding this comment.
I suppose this is true if all of the transaction groups would have succeeded? I think I'd just drop it, but I don't feel strongly.
It seems a bit strange that we have three levels of results being aggregated, Txn, TxnGroup, and Group of TxnGroups. At each level, we describe success a little differently. At the transaction level, we put it inside the txn-result, then at the group level, we aggregate it as a "failure-message", and then at the very top, we aggregate the other way, as "would-succeed". We could strive for more unity here, or we could skip the aggregation entirely.
There was a problem hiding this comment.
The motivation behind "would-succeed" at this level was to provide a dead simple way of figuring out if your transactions would succeed if you sent them to the real submission endpoint. Yes, this is redundant information, since you could deduce the same thing by checking that no TxnGroup has a failure message and no Txn is missing signatures.
But in the future you may have to check more things, like no additional foreign resources were needed and you didn't exceed the standard opcode budget. True, these features would likely be opt-in so they're less likely to confuse people, but still think there's a benefit to summarizing all the possible reasons for failure in a simple boolean.
There was a problem hiding this comment.
I'm ok with this, but I'm actually so convinced that I think it should exist on individual txn-groups as well (we can add in a later PR, since it will be an addition). The idea that there will be multiple ways that we might allow a group to "succeed" even though they would not actually succeed on chain is what convinces me. And, I think there will be a good use for simulation in unit tests where you want to confirm that, say, txgroups 1, 2 , 3 and 5 would succeed, but 4 would not. That is, the convenience you're offering is good at the txgroup level, because you may want some of the groups to fail (you are testing that they will, in fact). But that the other groups are perfectly correct.
There was a problem hiding this comment.
Those are good points. I'll admit I haven't given much thought to how multiple groups would be handled, but your ideas make sense.
Summary
This PR builds upon #4322 by exposing additional evaluation details with the help of the new debugger hooks.
Specifically, the simulate endpoint is more structured and includes a
FailedAtpointer to the transaction (including inner transactions) where evaluation failed, as well as each transaction'sApplyData. And if a transaction fails, we include theApplyDatawith as much detail as possible at the point of failure.As a minor note, the simulate endpoint can now be configured to encode its response as either JSON or msgpack. By default, JSON will be used.
Test Plan
Many unit tests added to ensure the more detailed response structure behaves as expected.