feat: JS tracer #569
feat: JS tracer #569
Conversation
Test results164 tests 164 ✅ 12m 31s ⏱️ Results for commit 1aded93. ♻️ This comment has been updated with latest results. |
dimazhornyk
requested review from
0xVolosnikov,
RomanBrodetski,
itegulov and
perekopskiy
|
Would be nice to compare it to the Geth/Reth implementation somehow - it's very easy to introduce some divergence in some obscure corner case |
|
Also, do we want to limit this functionality somehow?
|
Yeah, I was also concerned about it. Maybe we can add an integration test that would send a set of requests to our node and the Reth node and compare the outputs. |
I didn't consider it, but it definitely makes sense. I will think about it |
WalkthroughThis PR introduces JavaScript-based EVM tracing via Boa JavaScript engine integration, updates multiple dependencies to newer rc and patch versions (notably zksync-os components and adding boa_engine/boa_gc), and extends test coverage with cross-node tracer comparisons and contract state tracking functionality. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant RPC as RPC Layer
participant EthCall as EthCallHandler
participant JsTracer as JsTracer
participant Boa as Boa Context
participant Host as Host Environment
participant State as State View
Caller->>RPC: debug_trace_call (with JsTracer config)
RPC->>EthCall: call_js_tracer_impl()
EthCall->>JsTracer: new(state_view, js_cfg)
JsTracer->>Boa: Create context
JsTracer->>Host: init_host_env_in_boa_context()
Host->>Host: install_host_bindings(__hostCall)
Host->>Host: install_db_wrapper()
Host->>Boa: bootstrap_tracer(source)
EthCall->>JsTracer: execute transaction (EVM hooks)
loop EVM Execution Steps
JsTracer->>Boa: before_step / after_step hooks
Boa->>Host: invoke __hostCall(method, payload)
Host->>Host: dispatch_host_call()
Host->>State: resolve balance/code/storage
State-->>Host: state data
Host-->>Boa: JSON result
end
JsTracer-->>EthCall: JsonValue trace output
EthCall-->>RPC: trace result
RPC-->>Caller: traced execution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/rpc/src/sandbox.rs (1)
443-471: Small typo inMemoryLimitOOGerror message.The formatted string currently has an extra closing parenthesis (
"out of gas (memory limit reached {}))"). Consider fixing it while you’re here.- EvmError::MemoryLimitOOG => format!("out of gas (memory limit reached {}))", u32::MAX - 31), + EvmError::MemoryLimitOOG => { + format!("out of gas (memory limit reached {})", u32::MAX - 31) + }
♻️ Duplicate comments (1)
integration-tests/tests/tracing.rs (1)
608-608: Remove leftover debug print
println!("{:?}", trace);is still sitting here from debugging and will spam the test logs. Let’s drop it before merging.
🧹 Nitpick comments (6)
lib/rpc/src/lib.rs (1)
19-19: Confirm that makingjs_tracerpublic is the desired API surface.Exporting
js_tracerfrom the crate root exposes the JS tracing types to external crates. If this is still experimental or primarily internal, considerpub(crate)or re‑exporting only a narrow façade (e.g., selected entry points) instead.lib/rpc/src/debug_impl.rs (2)
31-112: Tracer selection indebug_trace_block_by_id_impllooks correct; consider richer error reporting and JS safeguards.Dispatching between the built‑in call tracer and JS tracer based on the requested tracer type is clean, and the per‑tx mapping into
TraceResultwith attached hashes is correct. Right now, any tracing failure (both built‑in and JS) is logged and surfaced as a genericInternalError; if you want closer geth/reth parity and easier debugging, consider:
- Converting failures into
TraceResult::Errorwith a trimmed message, or- At least embedding the underlying error message into the
InternalErrortext.Given the Boa‑based JS tracer can be relatively heavy and receives untrusted input, it’s also worth thinking about limits here (or in the js_tracer/EthCallHandler layer): e.g. maximum JS tracer payload size, capped tx ranges per request, or other guardrails to reduce the blast radius of expensive debug calls.
144-193:debug_trace_call_implbranching between built‑in and JS tracers is consistent with block tracing.The call‑tracer path derives a config from the tracer options and delegates to
EthCallHandler, and the JS path reuses the same plumbing viacall_js_tracer_impl, returning aGethTrace::JSresult, which keeps behavior aligned with block‑level tracing. Similar to the block path, consider:
- Validating JS tracer payload size and shape early (before hitting the engine), and
- Ensuring JS tracer runtime errors are surfaced to clients in a debuggable form rather than being overly generic, subject to whatever constraints you have on the external API.
lib/rpc/src/js_tracer/host.rs (1)
332-339: Consider optimizing the storage overlay check.The storage overlay is checked by iterating over all keys (lines 333-338). For large overlays, this could be inefficient. If this becomes a performance bottleneck, consider maintaining a separate set of addresses that have storage entries.
lib/rpc/src/js_tracer/tracer.rs (1)
462-472: Consider continuing stack dump after errors.If
stack.peek_n(idx)fails (line 465), the loop breaks and the stack dump is incomplete (line 469). Depending on the tracer's needs, consider using a placeholder value (e.g.,"<error>") and continuing the dump, so the tracer receives a complete (albeit partial) stack representation.lib/rpc/src/js_tracer/types.rs (1)
218-226: Simplify commit_map - retain is a no-op.The
retaincall always returnstrue(line 224), so no entries are ever removed. Consider using a simple iteration instead:- fn commit_map(map: &mut HashMap<K, OverlayEntry<V>>) { - map.retain(|_, entry| { - if !entry.committed { - entry.committed = true; - entry.previous = None; - } - true - }); - } + fn commit_map(map: &mut HashMap<K, OverlayEntry<V>>) { + for entry in map.values_mut() { + if !entry.committed { + entry.committed = true; + entry.previous = None; + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Cargo.toml(3 hunks)integration-tests/test-contracts/src/TracingPrimary.sol(2 hunks)integration-tests/tests/js_tracer_cross_node.rs(1 hunks)integration-tests/tests/tracing.rs(2 hunks)lib/genesis/src/lib.rs(1 hunks)lib/multivm/src/apps/mod.rs(6 hunks)lib/revm_consistency_checker/src/helpers.rs(1 hunks)lib/rpc/Cargo.toml(3 hunks)lib/rpc/src/debug_impl.rs(4 hunks)lib/rpc/src/eth_call_handler.rs(5 hunks)lib/rpc/src/js_tracer/host.rs(1 hunks)lib/rpc/src/js_tracer/mod.rs(1 hunks)lib/rpc/src/js_tracer/tracer.rs(1 hunks)lib/rpc/src/js_tracer/types.rs(1 hunks)lib/rpc/src/js_tracer/utils.rs(1 hunks)lib/rpc/src/lib.rs(1 hunks)lib/rpc/src/sandbox.rs(2 hunks)node/bin/src/batcher/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
integration-tests/tests/js_tracer_cross_node.rs (2)
integration-tests/src/lib.rs (1)
ProviderBuilder(279-279)lib/rpc/src/js_tracer/tracer.rs (1)
new(74-107)
lib/rpc/src/eth_call_handler.rs (3)
lib/rpc/src/js_tracer/tracer.rs (1)
new(74-107)lib/storage_api/src/state_override_view.rs (1)
new(26-34)lib/multivm/src/lib.rs (1)
simulate_tx(86-138)
integration-tests/tests/tracing.rs (2)
integration-tests/src/lib.rs (1)
setup(68-70)integration-tests/src/contracts.rs (3)
address(105-107)address(138-140)address(165-167)
lib/rpc/src/js_tracer/tracer.rs (5)
lib/rpc/src/js_tracer/host.rs (1)
init_host_env_in_boa_context(52-74)lib/rpc/src/js_tracer/utils.rs (4)
extract_js_source_and_config(11-37)gas_used_from_resources(7-9)wrap_js_invocation(71-74)serde_json(12-12)lib/rpc/src/sandbox.rs (2)
fmt_error_msg(443-472)maybe_revert_reason(422-438)lib/rpc/src/js_tracer/types.rs (5)
new(162-167)checkpoint(177-179)credit(76-89)debit(91-104)new_pending(58-64)lib/multivm/src/lib.rs (1)
run_block(22-84)
lib/rpc/src/js_tracer/utils.rs (1)
lib/rpc/src/js_tracer/tracer.rs (1)
new(74-107)
lib/rpc/src/js_tracer/host.rs (2)
lib/rpc/src/js_tracer/utils.rs (4)
format_hex_u256(59-65)parse_address(39-47)parse_b256(49-57)anyhow_error_to_js_error(67-69)lib/rpc/src/js_tracer/tracer.rs (1)
new(74-107)
lib/rpc/src/debug_impl.rs (2)
lib/rpc/src/sandbox.rs (1)
call_trace(67-91)lib/rpc/src/js_tracer/tracer.rs (2)
trace_block(873-898)new(74-107)
lib/rpc/src/js_tracer/types.rs (1)
lib/rpc/src/js_tracer/tracer.rs (1)
new(74-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-prover-tests
- GitHub Check: build
- GitHub Check: test
- GitHub Check: format-and-lint
- GitHub Check: cargo-audit
🔇 Additional comments (19)
node/bin/src/batcher/mod.rs (1)
351-355: Error message refactor is clear and correctThe
anyhow::ensure!condition is unchanged, and the new named-format message is valid, more readable, and less error‑prone than positional arguments. No further changes needed here.lib/revm_consistency_checker/src/helpers.rs (1)
125-125: LGTM! Modern Rust formatting with better error context.The inline format argument
{e:?}is cleaner than the older style with separate arguments, and the descriptive prefix "Failed to build TxEnv: " improves error diagnostics.lib/multivm/src/apps/mod.rs (2)
1-153: Clarify the relationship between binary version updates and the JS tracer feature.The PR description mentions implementing a JavaScript tracer with Boa runtime and state access hooks, but this file only updates zksync-os binary dependency versions. Could you clarify how these binary version updates relate to the JS tracer functionality? Do the new binaries (v4 RC1 and v5 0.2.3) include hooks or support required for the JS tracer?
83-83: <!-- [scratchpad_start]
[task review_verify_env_var_migration_final]
Verification complete. All checks passed.[observations]
- Cargo.toml workspace dependencies: forward_system at v0.2.3 ✅
- build.rs auto-generates ZKSYNC_OS_{version}_SOURCE_PATH from git tag ✅
- apps/mod.rs all three occurrences (lines 83, 107, 131) use ZKSYNC_OS_0_2_3_SOURCE_PATH ✅
- No references to old ZKSYNC_OS_0_2_2_SOURCE_PATH remain ✅
[analysis]
- Original review asked to verify build system configuration
- Verification shows the system is AUTOMATICALLY configured via:
- Cargo.toml dependency tag update (v0.2.3)
- build.rs dynamically generates env var from tag
- apps/mod.rs correctly references generated var
- No manual CI/CD configuration needed - this is self-contained in build.rs
- Migration is complete and correct across all three v5 binary paths
- Build will succeed with this configuration
[conclusion]
All concerns from original review have been addressed:
- Build system IS configured (via Cargo.toml dependency tag)
- Environment variable IS properly set up (auto-generated by build.rs)
- Code correctly references the new variable name
- No breaking changes or missing dependencies
[status DONE]
No further action required—build system is properly configured.
The environment variable migration has been correctly implemented. Verification confirms:
- Cargo.toml workspace dependencies updated to
tag = "v0.2.3"for forward_system, zk_ee, and zk_os_basic_system- build.rs automatically generates
ZKSYNC_OS_0_2_3_SOURCE_PATHfrom the v0.2.3 git tag (v0.2.3 → 0_2_3 → env var name)- apps/mod.rs correctly references the new variable at all three locations (lines 83, 107, 131)
- Old variable (ZKSYNC_OS_0_2_2_SOURCE_PATH) completely removed with no dangling references
The build system is self-configuring: dependency tag changes in Cargo.toml automatically trigger correct env var generation via build.rs, requiring no manual CI/CD or environment variable setup.
lib/genesis/src/lib.rs (1)
353-356: No functional change in digest handling; usage is correct.Switching to
digest.as_ref()still yields a 32‑byte slice forcopy_from_slice, so the digest→[u8; 32]conversion remains safe and unchanged in behavior.lib/rpc/src/sandbox.rs (1)
421-438: Crate‑level exposure ofmaybe_revert_reasonpreserves semantics.Returning
Noneon decode failure or empty reasons is still the behavior, and making itpub(crate)cleanly enables reuse from other RPC modules.lib/rpc/src/js_tracer/mod.rs (1)
1-4: JS tracer module visibility layering is sensible.Keeping
host,types, andutilscrate‑private while exposing onlytracergives you a focused public surface and leaves room to evolve internals.Cargo.toml (1)
71-99: Workspace dependency bumps and JS‑tracer crates are coherently aligned.Verification confirms no lingering older version tags (v0.2.2) exist in Cargo.toml. The zk_os stack is consistently pinned to the v0.2.3 line, and Boa (boa_engine, boa_gc) are both at 0.21.0 at the workspace level, preventing version skew across crates.
lib/rpc/Cargo.toml (1)
24-47: Dependency wiring verified as correct.All four dependencies are properly defined in the root Cargo.toml and correctly referenced via
workspace = truein lib/rpc/Cargo.toml. Each dependency is actively used in lib/rpc/src/js_tracer/ code, confirming no dead or orphaned bindings. The workspace configuration aligns with repository practices.lib/rpc/src/js_tracer/host.rs (3)
346-383: LGTM! Proper overflow/underflow handling.The balance delta resolution correctly checks for overflow when applying additions and underflow when applying subtractions, with appropriate error messages.
223-243: The review's characterization of method inconsistency is inaccurate.The actual code shows all three methods handle invalid address parsing identically—they all return default values (
"0x0"for balance/nonce,"0x"for code). The methods only diverge when an account exists but isn't found in state:host_get_nonceandhost_get_codeerror, whilehost_get_balance's behavior depends on theresolve_balance()function (not shown).However, the underlying concern is valid: geth's getNonce, getBalance, and getCode all return default values (0, 0, empty) for non-existent accounts, whereas the current implementation errors on missing accounts. This diverges from geth behavior for existing addresses missing from state—but this is a geth divergence issue, not an inconsistency between methods.
Likely an incorrect or invalid review comment.
18-28: No issues found — design is inherently single-threaded.Verification confirms that the
#[unsafe_ignore_trace]usage onRefCellandRcfields is safe. TheJsTraceris always stack-allocated and used in synchronous, single-threaded contexts (viacall_js_tracer_impl). BoaContext itself is not Send/Sync and enforces single-threaded use by design. The tracer is passed by reference tosimulate_txwithout being moved across threads, preventing any cross-thread access to the non-Send/Sync fields.lib/rpc/src/js_tracer/tracer.rs (4)
496-535: Complex but correct balance delta tracking.The
apply_balance_deltalogic carefully manages committed/pending state and preserves original values for rollback (lines 512-514). The optimization at lines 519-521 removes no-op entries. While complex, the logic appears sound.
621-638: LGTM! Proper frame failure handling with overlay rollback.The frame completion correctly rolls back overlays on failure (line 628) and propagates transaction failure when the top-level frame fails (lines 631-633).
734-783: LGTM! Comprehensive transaction finalization with proper cleanup.The
finish_txmethod correctly handles errors, missing frames, and performs appropriate commit/rollback. The cleanup at lines 779-782 ensures the tracer is ready for the next transaction.
73-107: The clone operation is cheap by design.The
state_view.clone()at line 88 is intentionally inexpensive:
PersistentPreimagescontains only aRocksDBhandle, explicitly documented in code as "cheap to clone"StorageMapViewusesArc<DashMap<>>fields, which clone in O(1) by incrementing the reference countStateViewderivesCloneand is documented as a "thin wrapper"- The parent
StateHandleis documented as "thread-safe and cheaply clonable"No changes required.
lib/rpc/src/js_tracer/types.rs (3)
69-109: LGTM! Proper overflow handling in BalanceDelta.Both
creditanddebitmethods correctly check for overflow usingoverflowing_addand return appropriate errors. Theis_emptyhelper is also correctly implemented.
147-189: LGTM! Sound journal-based overlay design.The
OverlayActionenum and journal-based approach provide a clean mechanism for tracking and reverting overlay changes. The design correctly supports nested checkpoints.
191-204: No changes needed—checkpoint validity is guaranteed by design.The
revert_to_checkpointmethod is safe. Checkpoints are snapshots of journal length captured at frame entry time, and the LIFO stack discipline ensures they are always valid when used. Thewhile journal.len() > checkpointcondition guarantees the journal is never empty whenpop()is called, making theexpect()safe. It functions as a defensive invariant check rather than a panic risk.
0xVolosnikov
left a comment
0xVolosnikov
left a comment
There was a problem hiding this comment.
lgtm
In the long term we should continuously compare our tracers to the Reth/Geth versions to catch potential bugs
4 participants
This PR adds a JS tracer implementation.
It uses Boa JS runtime to call the JS functions when zksync-os execution triggers a corresponding hook.
State access is implemented by using the captured StateView and storage overlays in the JS context.
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.