simulate: fix signers

[joe-p]

Summary

This is a new feature in simulate that will close #5914. Auth address correction is done is two places:

1. In simulateWithTracer the txn fields will be analyzed and auth addrs for txns will be updated past on previous rekeyTo fields
2. In AfterProgram the auth addr field for all txns up to the next app call will be corrected

This allows one to simulate with empty signatures without knowing the proper auth addresses when forming the group.

Test Plan

Simple test to ensure the endpoint works as expected in test/e2e-go/restAPI/simulate/simulateRestAPI_test.go and more comprehensive testing in ledger/simulation/simulation_eval_test.go

[joe-p]

@jasonpaulos let me know what you think about this general approach. If it looks good, I can work on the endpoint and more tests

[jasonpaulos]

The direction makes sense to me. This will need extensive testing.

What are you thinking about for how to enable this? At the moment my preference would be a boolean alongside AllowEmptySignatures, which you'd only be allowed to set if AllowEmptySignatures is also set

[joe-p]

@jasonpaulos this comment in the test you initially wrote got me thinking... Should we expect the transaction group to be modified in the response? Is there any precedent for this?

I feel like there is value in seeing the txn group as it was given and also seeing what was modified. Perhaps an additional field that indicates a changed signer per txn, but the actual transaction object remains unchanged?

[joe-p]

Also I'm switching from the terminology "fix auth addr" to "fix signer" to avoid confusion between the auth addr for a given account in the ledger and the signer for the transaction, making it clear the ledger remains unchanged and its just txns that are getting modified.

[codecov]

Codecov Report

Attention: Patch coverage is 77.63158% with 17 lines in your changes missing coverage. Please review.

Project coverage is 55.85%. Comparing base (4e116cb) to head (cf0272f).
Report is 1 commits behind head on master.

Files	Patch %	Lines
daemon/algod/api/server/v2/utils.go	0.00%	7 Missing ⚠️
ledger/simulation/simulator.go	87.50%	3 Missing and 3 partials ⚠️
ledger/simulation/tracer.go	80.00%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5942      +/-   ##
==========================================
- Coverage   55.86%   55.85%   -0.02%     
==========================================
  Files         482      482              
  Lines       68475    68538      +63     
==========================================
+ Hits        38253    38280      +27     
- Misses      27620    27646      +26     
- Partials     2602     2612      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jasonpaulos]

@jasonpaulos this comment in the test you initially wrote got me thinking... Should we expect the transaction group to be modified in the response? Is there any precedent for this?

I feel like there is value in seeing the txn group as it was given and also seeing what was modified. Perhaps an additional field that indicates a changed signer per txn, but the actual transaction object remains unchanged?

I agree that it would be preferable to not modify the transactions as given, and instead provide an additional response field with the overridden signer. The only problem is that the response structure may make it difficult to associate this additional information with inner transactions.

[joe-p]

The only problem is that the response structure may make it difficult to associate this additional information with inner transactions.

Unless you are thinking of functionality that I am not, we're only modifying the signers on the outer transactions, so we only need to add additional information to the outers.

[jasonpaulos]

Unless you are thinking of functionality that I am not, we're only modifying the signers on the outer transactions, so we only need to add additional information to the outers.

Good point, this plan should have no problems then

[joe-p]

@jasonpaulos I need to modify the test case to make sure the change I made in 89c20b5 works as expected with nested app calls, but other than that it's ready for review. <- Done

[jasonpaulos]

Mostly looks good, but I left a few asks and I made this sub-PR with mostly test changes: joe-p#3

[algorandskiy]

LGTM one small suggestion

[joe-p]

Thanks @algorandskiy. Apologies for the force push. Fixed a typo in the commit message and didn't realize it would dismiss @jasonpaulos review status