Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simulate: Hash of Program Bytecodes in Simulation #5658

Merged
merged 4 commits into from
Aug 17, 2023

Conversation

ahangsu
Copy link
Contributor

@ahangsu ahangsu commented Aug 12, 2023

Summary

This PR implements #5567, which returns hash digests of program bytecode being executed during simulation.

Test Plan

Extending pre-existing simulation tests and rest client tests on execution trace, and activate initial-state-config which returns program bytes.

@CLAassistant
Copy link

CLAassistant commented Aug 12, 2023

CLA assistant check
All committers have signed the CLA.

@ahangsu ahangsu self-assigned this Aug 12, 2023
@codecov
Copy link

codecov bot commented Aug 12, 2023

Codecov Report

Merging #5658 (9a6c040) into master (3425e85) will increase coverage by 0.03%.
Report is 9 commits behind head on master.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master    #5658      +/-   ##
==========================================
+ Coverage   55.15%   55.19%   +0.03%     
==========================================
  Files         465      465              
  Lines       65024    65042      +18     
==========================================
+ Hits        35862    35897      +35     
+ Misses      26769    26760       -9     
+ Partials     2393     2385       -8     
Files Changed Coverage Δ
daemon/algod/api/server/v2/utils.go 7.43% <0.00%> (-0.19%) ⬇️
ledger/simulation/trace.go 83.05% <ø> (ø)
ledger/simulation/tracer.go 95.55% <100.00%> (+0.20%) ⬆️

... and 13 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tonyfloatersu tonyfloatersu force-pushed the sim-program-initial-state branch from 4f727a7 to 3d55c0b Compare August 12, 2023 02:31
@ahangsu ahangsu force-pushed the sim-program-initial-state branch from 3d55c0b to a9cdb25 Compare August 12, 2023 02:44
@ahangsu ahangsu force-pushed the sim-program-initial-state branch 2 times, most recently from ca52227 to fb12544 Compare August 15, 2023 00:29
@ahangsu ahangsu changed the title Simulate: Program Initial State in Simulation Simulate: Program Bytes in Simulation Aug 15, 2023
@ahangsu ahangsu marked this pull request as ready for review August 15, 2023 04:17
@ahangsu ahangsu force-pushed the sim-program-initial-state branch from a4e6070 to 0ea486c Compare August 15, 2023 04:34
cmd/goal/clerk.go Outdated Show resolved Hide resolved
cmd/goal/clerk.go Outdated Show resolved Hide resolved
cmd/goal/clerk.go Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
@ahangsu ahangsu force-pushed the sim-program-initial-state branch 4 times, most recently from 606f9b8 to 4966124 Compare August 16, 2023 00:26
@ahangsu ahangsu force-pushed the sim-program-initial-state branch from 4966124 to 0fe8f3b Compare August 16, 2023 00:26
@ahangsu ahangsu changed the title Simulate: Program Bytes in Simulation Simulate: Hash of Program Bytecodes in Simulation Aug 16, 2023
@ahangsu ahangsu requested a review from jannotti August 16, 2023 00:37
@ahangsu
Copy link
Contributor Author

ahangsu commented Aug 16, 2023

By live discussion, while unknown if it is interesting to return program byte code, I decided to divert this PR into exposing executed program bytecode's hash in simulation exec trace, and this should help debugger to pair sourcemap + teal source code with execution trace.

Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good way to let debuggers know they've got the right program ready to step through.
Since this is a small value, and quick to compute, I don't think we should add options to include or not include this. The number of config flags is getting a little out of hand. Let's just always include it. I think that with JSON responses it's legitimate to add something new and still consider in "unbreaking", right?

@ahangsu ahangsu force-pushed the sim-program-initial-state branch from 88578db to 88fc595 Compare August 17, 2023 17:46
@ahangsu
Copy link
Contributor Author

ahangsu commented Aug 17, 2023

yea, even better, we can just return the hash as some always return part. JSON wont just break easily if we add something to it, sdks just ignore such field (for now)

@ahangsu
Copy link
Contributor Author

ahangsu commented Aug 17, 2023

oh snap, I should include hash for every test now.

bbroder-algo
bbroder-algo previously approved these changes Aug 17, 2023
Copy link
Contributor

@bbroder-algo bbroder-algo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A hash-slinging slasher.

jasonpaulos
jasonpaulos previously approved these changes Aug 17, 2023
Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good only minor nits

daemon/algod/api/server/v2/utils.go Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
@ahangsu ahangsu force-pushed the sim-program-initial-state branch from 8e4aefb to c1764cf Compare August 17, 2023 19:22
bbroder-algo
bbroder-algo previously approved these changes Aug 17, 2023
@ahangsu ahangsu requested a review from jasonpaulos August 17, 2023 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants