Dryrun: Return EvalDeltas for failed executions in Dryrun

[algochoi]

Summary

This PR returns the EvalDelta for stateful evaluation, regardless of whether it succeeds or not. This allows users to check the EvalDelta even for failed dryrun execution.

Closes #2687 .

Test Plan

Added unit tests for dryrun:

• Check global deltas match the expected deltas when dryrun fails
• Check local deltas match the expected deltas when dryrun fails

[codecov-commenter]

Codecov Report

Merging #3929 (a6518ae) into master (bd18d04) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3929   +/-   ##
=======================================
  Coverage   49.79%   49.79%           
=======================================
  Files         409      409           
  Lines       69145    69147    +2     
=======================================
+ Hits        34429    34430    +1     
+ Misses      30997    30996    -1     
- Partials     3719     3721    +2     

Impacted Files	Coverage Δ
daemon/algod/api/server/v2/dryrun.go	70.31% <100.00%> (+0.18%)	⬆️
ledger/blockqueue.go	82.18% <0.00%> (-2.88%)	⬇️
data/transactions/verify/txn.go	44.15% <0.00%> (-0.87%)	⬇️
ledger/acctupdates.go	68.77% <0.00%> (-0.66%)	⬇️
ledger/tracker.go	74.45% <0.00%> (+1.29%)	⬆️
network/wsPeer.go	68.05% <0.00%> (+2.22%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd18d04...a6518ae. Read the comment docs.

[jannotti]

I don't know how strongly I feel about my objexction. The patch is so small, I really like that. I don't actually see the work being done - is that because it just naturally falls out that if the eval delta is built, it gets serialized out to caller?

[jasonpaulos]

Looks like a good improvement to me, I only have minor comments about some test semantics.

Higher level question: have you considered comparing the msgpack or json encoded deltas instead of diving into the individual fields? That might be a way to get nearly the same equality confidence without as much code.

[jasonpaulos]

Looks good to me

[algorandskiy]

The change looks good but please fix the exceed license header

[jannotti]

In many cases it's to distinguish 0 from "empty" so the REST api can marshall it properly.

…

On Thu, May 5, 2022 at 11:08 AM algoidurovic ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In daemon/algod/api/server/v2/dryrun_test.go <#3929 (comment)>: > @@ -1634,3 +1656,148 @@ int 1`) logResponse(t, &response) } } + +func checkStateDelta(t *testing.T, + response *generated.StateDelta, + expectedDelta *generated.StateDelta, +) { + for i, vd := range *response { + assert.Equal(t, (*expectedDelta)[i].Key, vd.Key) + + // Pointer checks: make sure we don't try to derefence a nil. + if (*expectedDelta)[i].Value.Bytes == nil { + assert.Nil(t, vd.Value.Bytes) + } else { + assert.NotNil(t, vd.Value.Bytes) + assert.Equal(t, *(*expectedDelta)[i].Value.Bytes, *vd.Value.Bytes) This seems to be appear consistently in this file -- so I don't necessarily have a problem with you following the same convention -- but I don't understand why we're using pointers everywhere. Afaict, most uses don't need to be pointers, especially with slice types like generated.StateDelta. — Reply to this email directly, view it on GitHub <#3929 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADL7T2TTMCVVBSSYOW6MQLVIPP6LANCNFSM5UQNQM6Q> . You are receiving this because you commented.Message ID: ***@***.***>

[algoidurovic]

In many cases it's to distinguish 0 from "empty" so the REST api can marshall it properly.
…
On Thu, May 5, 2022 at 11:08 AM algoidurovic @.> wrote: @.* commented on this pull request. ------------------------------ In daemon/algod/api/server/v2/dryrun_test.go <#3929 (comment)>: > @@ -1634,3 +1656,148 @@ int 1`) logResponse(t, &response) } } + +func checkStateDelta(t *testing.T, + response *generated.StateDelta, + expectedDelta *generated.StateDelta, +) { + for i, vd := range *response { + assert.Equal(t, (*expectedDelta)[i].Key, vd.Key) + + // Pointer checks: make sure we don't try to derefence a nil. + if (*expectedDelta)[i].Value.Bytes == nil { + assert.Nil(t, vd.Value.Bytes) + } else { + assert.NotNil(t, vd.Value.Bytes) + assert.Equal(t, (expectedDelta)[i].Value.Bytes, vd.Value.Bytes) This seems to be appear consistently in this file -- so I don't necessarily have a problem with you following the same convention -- but I don't understand why we're using pointers everywhere. Afaict, most uses don't need to be pointers, especially with slice types like generated.StateDelta. — Reply to this email directly, view it on GitHub <#3929 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADL7T2TTMCVVBSSYOW6MQLVIPP6LANCNFSM5UQNQM6Q . You are receiving this because you commented.Message ID: @.>

Makes sense. I think this doesn't apply to all the uses of pointers in this PR but the pattern is prevalent enough that I don't take issue with it. I just generally try to avoid syntax like *(*ptr)[i]... if it isn't necessary but this is optional and shouldn't hold up the PR.

[algoidurovic]

LGTM

[jannotti]

I probably confused the issue by mentioning length, but now that you use assert.Equal, you don't need the length check.

[algochoi]

thanks for catching, should be deleted in the latest commit