-
Notifications
You must be signed in to change notification settings - Fork 302
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
Instruction tracing with EIP-3155 JSONL output #325
Conversation
Codecov Report
@@ Coverage Diff @@
## master #325 +/- ##
========================================
Coverage 99.77% 99.78%
========================================
Files 29 29
Lines 3999 4100 +101
========================================
+ Hits 3990 4091 +101
Misses 9 9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
lib/evmone/tracing.cpp
Outdated
m_out << std::dec; // Set number formatting to dec, JSON does not support other forms. | ||
|
||
m_out << "{"; | ||
m_out << R"("start":true)"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the EIP-3155 compatible format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now change, but we additional "start call" log line.
@@ -46,6 +46,11 @@ evmc_set_option_result set_option(evmc_vm* c_vm, char const* c_name, char const* | |||
} | |||
return EVMC_SET_OPTION_INVALID_VALUE; | |||
} | |||
else if (name == "trace") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this is missing coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be done in the end.
dd907fe
to
174cc7e
Compare
053b68c
to
07e739c
Compare
Funny. I output stack items from top to bottom, but apparently the |
lib/evmone/tracing.cpp
Outdated
void on_execution_start( | ||
evmc_revision rev, const evmc_message& msg, bytes_view code) noexcept override | ||
{ | ||
m_contexts.emplace(code.data(), msg.gas, evmc_get_instruction_names_table(rev)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we assume that all nested executions are on the same revision, and then save rev
or opcode_names
once for entire tracer in case m_contexts
is empty, instead of putting it into each context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can assume this because this will not cause crashes if wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this and this requires some special conditions. E.g. we save it when depth=0
? Then unit tests starting on other depth stops working. So maybe set it when not set? But when to null it then? At exit when depth=0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested to look at m_contexts.empty()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this works nicely, thanks.
However, I think it is better to do it separately not to modify the HistrogramTracer now.
lib/evmone/tracing.cpp
Outdated
|
||
m_out << "{"; | ||
m_out << R"("kind":")" | ||
<< ((msg.kind == EVMC_CREATE || msg.kind == EVMC_CREATE2) ? "create" : "call") << '"'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not standard, why not print all possible kinds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is known not being properly propagated. The EVM does not care. Only for Ewasm it was important to distinguish create from call. In first version I implemented full to_string(evmc_call_kind)
. Do you think this was better? We can also drop this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to_string(evmc_call_kind)
is better than create
+call
, but if it's not needed by anyone, then better to drop it.
@MariusVanDerWijden it should. So if the ops are |
Yes, I have figured it out based on the |
test/unittests/tracing_test.cpp
Outdated
@@ -27,10 +27,13 @@ class tracing : public Test | |||
vm{*static_cast<evmone::VM*>(m_baseline_vm.get_raw_pointer())} | |||
{} | |||
|
|||
std::string trace(bytes_view code, int32_t depth = 0) | |||
std::string trace( | |||
bytes_view code, int32_t depth = 0, evmc_call_kind kind = EVMC_CALL, uint32_t flags = 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this helper doesn't really need kind
parameter now.
16de9b9
to
0e5cbc3
Compare
Implementation of EVM tracing following the EIP-3155 (draft). It outputs log line with JSON (jsonl) for every instruction to standard error output.
Differences from the spec:
"stateRoot"
is omitted as there is no way to compute it,"error"
is taken from the instruction trace log, replaces"pass"
or"successful"
."gasCost"
is omitted. This is difficult to compute as "dynamic" gas cost is calculated together with execution but the spec forces these to be separated."depth"
is omitted as already presented in "start call"."returnData"
is not implemented. This is doable, but seems unnecessary because this is available in"output"
in "end call"."refund"
is omitted, currently not doable."memory"
was initially implemented, then removed because makes traces huge. It is recommended to enabled it with a flag. This is easy to implement in future.Example (SHA1):
sha1.txt