test/utils: Extract evmone::tooling::t8n() from evmone-t8n main#1535
Merged
Conversation
Move the state-transition body of evmone-t8n's main() into a reusable
evmone::tooling::t8n() helper next to run.cpp in the testutils library.
The function operates only on std::istream/std::ostream and a small
config struct; opening files and assembling the per-tx trace filename
remain main()'s responsibility.
T8NArgs collects all configuration and I/O in one place:
- non-owning raw std::istream*/std::ostream* for inputs and outputs
(TODO(C++26): switch to std::optional<stream&>),
- an open_trace callback (std::function<std::ostream&(size_t, const
evmc::bytes32&)>) invoked once per executed transaction so the
caller controls the per-tx trace sink without t8n() touching the
filesystem,
- opcode_count_file kept as std::string until a follow-up makes the
VM-side option filesystem-free.
A small StreamRedirect RAII guard scopes the std::clog rdbuf swap to
each transaction's execution so the original buffer is restored on
exception as well as normal return. evmone-t8n's main shrinks from
326 to 127 lines and constructs the VM itself; testutils therefore
does not pick up a link dependency on evmone.
Add an integration test (cancun_create_tx_trace) exercising --trace,
asserting both the original trace-<i>-0x<hash>.jsonl filename and the
recorded opcode sequence.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1535 +/- ##
==========================================
+ Coverage 96.71% 96.97% +0.26%
==========================================
Files 160 162 +2
Lines 14337 14459 +122
Branches 3367 3386 +19
==========================================
+ Hits 13866 14022 +156
+ Misses 332 307 -25
+ Partials 139 130 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
t8n() now constructs its own evmc::VM internally. This removes the
sticky-config concern flagged earlier: a caller-supplied VM that t8n()
mutates with vm.set_option("trace") and vm.set_option("opcode.count")
no longer leaks back. testutils gains a PRIVATE link dep on evmone
(it already does in practice via every consumer; the dep is now
declared on the library that uses the symbol).
Split `std::ifstream alloc_in, env_in, txs_in, blob_params_in;` into
separate declarations to satisfy clang-tidy's
`readability-isolate-declaration` check (the previous form failed CI).
Add `test/unittests/tooling_t8n_test.cpp` with three focused tests for
the API surface that integration tests do not exercise directly:
- no-inputs/no-outputs smoke,
- default outputs populated with the expected JSON fields,
- open_trace callback invoked once per executed transaction with the
correct tx_index and tx_hash, and the redirected std::clog reaching
the callback-supplied stream.
Match T8NArgs's prefix naming style: rename the four `*_in` ifstreams to `in_*` and the three `*_out` ofstreams to `out_*`. Assignments like `args.out_result = &out_result;` now read symmetrically.
Turn the input-only helper into a generic-lambda `bind_stream(auto&, const fs::path&)` and route all three output ofstreams through it. The result/alloc paths always pass through `output_dir / name` (the directory base alone keeps the path non-empty, so behavior is unchanged); the body uses an explicit empty-name check so an absent --output.body still skips the write.
- Make empty-name skip uniform across all three output streams: thread every output through a small `output_path(name)` helper that returns an empty fs::path when name is empty, so `bind_stream` decides consistently. Previously result/alloc silently opened the bare output_dir on an absent flag (badbit set, writes ignored) while body used an explicit guard. - Drop the now-redundant `evmone::state`, `evmc::evmc`, and `evmone` direct link deps from evmone-t8n: main() no longer constructs the VM itself, and evmone::testutils PUBLIC-links evmone::state / evmc::evmc_cpp and PRIVATE-links evmone, so the transitive chain already covers them. - Add a TODO next to the latent `loaded_tx_hash_opt.value()` UB in the hash-mismatch path (pre-existing; flagged by review).
Add a fourth tooling_t8n test that exercises the previously-uncovered `*args.out_body << hex0x(rlp::encode(transactions))` branch. Reuses the same Shanghai/legacy-create fixture; asserts the body output is hex-prefixed and non-empty.
PR patch coverage was 91.94%. Locally with gcov, the uncovered lines on the patch split into four cheap groups; this commit closes them: - `test/utils/t8n.cpp` hash-mismatch throw (L143-145): new tooling_t8n.mismatched_tx_hash_throws unit test supplies a tx with a wrong "hash" field and asserts std::logic_error. - `test/utils/t8n.cpp` `block.difficulty != 0` branch (L82): add currentDifficulty (and currentRandom, so the difficulty value isn't reinterpreted as prev_randao) to ENV_JSON in the unit tests. The no-env tests still exercise the calculate_difficulty fallback. - `test/t8n/t8n.cpp` --version / --output.body / --state.reward -1 branches (L42-43, L67, L83): new integration tests `t8n/version`, `t8n/prague_pre_state_only` (which exercises --output.body and --state.reward -1 together). - `test/t8n/t8n.cpp` catch block (L119-123): new `t8n/bad_fork` integration test invokes the tool with an unknown --state.fork and asserts the "unknown revision" error message. Coverage on the patch files locally: test/utils/t8n.cpp: 94.33% -> 97.16% (8 uncovered -> 4) test/t8n/t8n.cpp : 95.65% -> 100% (4 uncovered -> 0) The four lines remaining in test/utils/t8n.cpp need revision/state-specific fixtures (pre-Byzantium post_state mpt_hash, non-empty Prague requests, deposit-event failure) that are out of scope for this PR.
The `if (rev < EVMC_BYZANTIUM) receipt.post_state = state::mpt_hash(state);` branch in t8n() was previously unexercised. The existing TX_JSON fixture works at Homestead because the tx is signed legacy-style (v=0x1b) and is pre-EIP-155 compatible; the inner CREATE fails on PUSH0 (Shanghai+) but the outer tx still produces a TransactionReceipt, hitting the new branch. t8n.cpp coverage: 97.16% -> 97.87%. The three lines that remain (L203/206/240) are Prague-specific deposit/request handling paths that need a fixture with deposit-log-emitting contract code in alloc - that requires more setup than a unit test should carry.
The previous `HasSubstr("\"receipts\"")` check passes even for rejected
txs because the receipts array is initialized empty at the top of the
txs-present branch. Switch to `HasSubstr("\"transactionHash\"")`, which
is only emitted from the success path - the same path that contains the
`rev < EVMC_BYZANTIUM` line we want to cover.
There was a problem hiding this comment.
Pull request overview
This PR refactors the evmone-t8n tool by extracting the core state-transition logic into a reusable evmone::tooling::t8n() helper within the evmone::testutils library, and adds tests to validate tracing behavior.
Changes:
- Added
evmone::tooling::t8n(const T8NArgs&)andT8NArgsto run t8n using stream-based I/O plus an optional per-tx trace sink callback. - Simplified
evmone-t8n’smain()to focus on CLI parsing, file opening, and trace filename construction. - Added unit and integration coverage for tracing and basic output behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utils/t8n.hpp | Introduces T8NArgs and the tooling::t8n() API. |
| test/utils/t8n.cpp | Implements the extracted t8n execution logic, including per-tx std::clog redirection for tracing. |
| test/utils/CMakeLists.txt | Adds the new sources and updates testutils link dependencies. |
| test/unittests/tooling_t8n_test.cpp | Adds unit tests for the new helper (smoke, outputs, tracing, hash mismatch). |
| test/unittests/CMakeLists.txt | Registers the new unit test source. |
| test/t8n/t8n.cpp | Refactors evmone-t8n main to use tooling::t8n() and stream binding helpers. |
| test/t8n/CMakeLists.txt | Updates evmone-t8n link libraries after refactor. |
| test/integration/t8n/CMakeLists.txt | Adds an integration test asserting trace filename and opcode sequence output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| add_executable(evmone-t8n) | ||
| target_link_libraries(evmone-t8n PRIVATE evmone::state evmone::testutils evmc::evmc evmone evmone-buildinfo) | ||
| target_link_libraries(evmone-t8n PRIVATE evmone::testutils evmone-buildinfo) |
Comment on lines
7
to
11
| target_link_libraries( | ||
| evmone.testutils | ||
| PUBLIC evmone::state evmc::evmc_cpp nlohmann_json::nlohmann_json | ||
| PRIVATE evmc::mocked_host | ||
| PRIVATE evmone evmc::mocked_host | ||
| ) |
Comment on lines
+137
to
+142
| const auto loaded_tx_hash_opt = | ||
| evmc::from_hex<bytes32>(j_txs[i]["hash"].get<std::string>()); | ||
|
|
||
| // TODO: Distinguish malformed-hex from hash-mismatch; currently a | ||
| // malformed loaded hash throws bad_optional_access from .value() below. | ||
| if (loaded_tx_hash_opt != computed_tx_hash) |
- Fix the latent `bad_optional_access` in the hash-mismatch path: separate the malformed-hex case from the mismatch case so the error thrown is always a std::logic_error with a clear message, never the cryptic bad_optional_access from `loaded_tx_hash_opt.value()`. Replaces the TODO comment with the actual fix. - Document the VM-ownership asymmetry between run() and t8n() in t8n.hpp: run() takes a caller-supplied VM because it sets no persistent options; t8n() builds its own to keep "trace" / "opcode.count" mutations local.
Comment on lines
7
to
11
| target_link_libraries( | ||
| evmone.testutils | ||
| PUBLIC evmone::state evmc::evmc_cpp nlohmann_json::nlohmann_json | ||
| PRIVATE evmc::mocked_host | ||
| PRIVATE evmone evmc::mocked_host | ||
| ) |
|
|
||
| add_executable(evmone-t8n) | ||
| target_link_libraries(evmone-t8n PRIVATE evmone::state evmone::testutils evmc::evmc evmone evmone-buildinfo) | ||
| target_link_libraries(evmone-t8n PRIVATE evmone::testutils evmone-buildinfo) |
Comment on lines
+47
to
+48
| /// a caller's VM. (`run()` above takes a caller-supplied `evmc::VM&` because | ||
| /// it does not set any persistent options on the VM.) |
Comment on lines
+15
to
+17
| #include <test/utils/utils.hpp> | ||
| #include <iomanip> | ||
| #include <iostream> |
- Fix the stale "run() above" reference in t8n.hpp (the doc landed in t8n.hpp, but run() is declared in run.hpp, not above). - Make t8n.cpp self-contained: add explicit includes for the standard headers it uses (vector, optional, algorithm, iterator, system_error, stdexcept) that were previously pulled in only transitively through project headers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Move the state-transition body of evmone-t8n's main() into a reusable evmone::tooling::t8n() helper next to run.cpp in the testutils library. The function operates only on std::istream/std::ostream and a small config struct; opening files and assembling the per-tx trace filename remain main()'s responsibility.
T8NArgs collects all configuration and I/O in one place:
A small StreamRedirect RAII guard scopes the std::clog rdbuf swap to each transaction's execution so the original buffer is restored on exception as well as normal return. evmone-t8n's main shrinks from 326 to 127 lines and constructs the VM itself; testutils therefore does not pick up a link dependency on evmone.
Add an integration test (cancun_create_tx_trace) exercising --trace, asserting both the original trace--0x.jsonl filename and the recorded opcode sequence.