fix(traces): tracing consistency between input and output alloc formatting#2832
fix(traces): tracing consistency between input and output alloc formatting#2832felix314159 wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #2832 +/- ##
================================================
Coverage 88.62% 88.63%
================================================
Files 577 577
Lines 35659 35659
Branches 3490 3490
================================================
+ Hits 31604 31607 +3
+ Misses 3492 3489 -3
Partials 563 563
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
marioevz
left a comment
There was a problem hiding this comment.
It looks ok to me, just a couple of comments. I do agree that having the input and output in the same folder be in different formats is annoying.
I think ideally we should coalesce types used in both sides of the t8n to get rid of this issue from the root.
cc @gurukamath since he is working on refactoring t8n.
There was a problem hiding this comment.
What's the purpose of the changes to this file?
There was a problem hiding this comment.
i had to add the second commit to fix CI failure:
36 tests in test-tests and test-tests-pypy failed. in all casestest_filler.py gave the same exception:
E FileNotFoundError: [Errno 2] No such file or directory:
'/tmp/tmpmepzcz2k/trace-0-0x1997...jsonl'
at .../evm_trace/eip3155.py:324: json_file = open(output_path, "w")
there was a pre-existing bug that was exposed by the first commit of this PR.
so first T8N.__init__ called trace.set_evm_trace(tracers) (in t8n/init.py:336). but set_evm_trace is global (there's one tracer slot for the whole process, and if the next t8n does not use traces the previous trace slot is still live). so every time a T8N was constructed it used that slot, and once a T8N's run() ended the slot was left pointing at that T8N's tracer, holding that T8N's tempdir in its output_basedir attribute. then the next T8N (constructed by the next test) would create a fresh tempfile.TemporaryDirectory() but never re-installed its own tracer if tracing was disabled for that call. EVM execution still fired the leaked previous tracer, which tried to write trace-*.jsonl into a tempdir that had since been cleaned up by TemporaryDirectory.__exit__. so that's why we got FileNotFoundError in CI
and it is a bit weird that output_traces never used mkdir for the output_basedir. so even in the happy path, the trace writer assumed someone else had created the directory.
so the second commit fixes both the t8n-leak-between-instances and also ensures the directory we try to write to exists
| return Alloc.model_validate(accumulated) | ||
|
|
||
|
|
||
| def _alloc_to_canonical_dict(alloc: Alloc) -> Dict[str, Dict[str, Any]]: |
There was a problem hiding this comment.
One small downside of this function is that it's decoupled from the Alloc model, so if we change it (very unlikely) this function would break.
That's right. Once we manage to replace the current t8n's |
🗒️ Description
while looking into how our different tracing flags work I noticed that the same values are represented differently in input vs output alloc. for instance, a storage key would be
0x22in the input (no padding) but0x000..22in the output (32 byte padded). another example is how an empty storage in the input would be shown as{}whereas empty fields are completely emitted in the output (same for empty balances and empty code). also nonces in the input are shown like e.g.0x01(always at least two chars) but shown as0x1in the output (minimal representation)this PR fixes all these inconsistencies and adds a unit tests that ensures input and output alloc formatting remains consistent. the output of course can still be different, this PR is only about the format in which values are shown
i think this is a good feature because if we ever want to actually look at the diff between input and output alloc we currently would have a lot of noise, after the PR only the actual differences would be left (less bloating of our human context windows)
i don't think this PR breaks anything, because it only changes the input alloc formatting and leaves the output alloc (that might be read be client software) exactly as is
Command to run the new unit test
uv run pytest packages/testing/src/execution_testing/cli/pytest_commands/plugins/filler/tests/test_alloc_dump_canonical.py::test_input_alloc_matches_output_alloc_format -v🔗 Related Issues or PRs
N/A.
✅ Checklist
just statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_frommarker.Cute Animal Picture