feat: Clarity eval hook call-tracing improvements#7401
feat: Clarity eval hook call-tracing improvements#7401cylewitruk-stacks wants to merge 22 commits into
Conversation
…l-hook-tracing-improvements
…l-hook-tracing-improvements
There was a problem hiding this comment.
Pull request overview
This PR introduces an opt-in, higher-level Clarity VM hook API for structured call tracing (call begin/arg eval/call finish plus top-level lifecycle events), and adds a reusable CallTraceHook collector that reconstructs resolved call trees with timing and cost attribution. It also wires the new analysis-time budget parameter through block builder transaction-mining call sites to support per-tx analysis deadlines.
Changes:
- Adds
vm::hooks(newEvalHooksurface + call tracing event model) and instruments the VM evaluator to emit structured call/lifecycle notifications. - Adds
vm::hooks::trace::CallTraceHookto collect call trees with resolved args/results, inclusive/exclusive cost, and wall-clock timing. - Extends block-builder transaction mining to pass an additional per-tx
max_analysis_timebudget and updates affected call sites/tests.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| stackslib/src/net/api/tests/postblock_proposal.rs | Updates postblock proposal API tests to pass the new max_analysis_time argument when mining transactions. |
| stackslib/src/net/api/postblock_proposal.rs | Adds per-tx analysis-time budgeting during proposal validation and threads the new argument through replay/validation mining calls. |
| stackslib/src/net/api/blockreplay.rs | Updates replay mining call to include the new max_analysis_time argument. |
| stackslib/src/clarity_vm/tests/ephemeral.rs | Updates ephemeral replay test mining call to include the new max_analysis_time argument. |
| stackslib/src/clarity_vm/clarity.rs | Adjusts with_abort_callback signature/lifetimes to support hook-aware OwnedEnvironment usage. |
| stackslib/src/chainstate/tests/consensus.rs | Updates test mining calls to include the new max_analysis_time argument. |
| stackslib/src/chainstate/stacks/tests/chain_histories.rs | Updates helper mining calls to include the new max_analysis_time argument. |
| stackslib/src/chainstate/stacks/miner.rs | Extends BlockBuilder APIs to accept max_analysis_time and threads miner settings through mining selection paths. |
| stackslib/src/chainstate/nakamoto/tests/node.rs | Updates Nakamoto test mining calls to include the new max_analysis_time argument. |
| stackslib/src/chainstate/nakamoto/shadow.rs | Updates shadow builder mining calls to include the new max_analysis_time argument. |
| stackslib/src/chainstate/nakamoto/miner.rs | Removes builder-stored analysis deadline in favor of per-call max_analysis_time and forwards it through mining. |
| stackslib/src/chainstate/coordinator/tests.rs | Updates coordinator test mining calls to include the new max_analysis_time argument. |
| stacks-node/src/tests/signer/v0/tenure_extend.rs | Updates signer test mining call to include the new max_analysis_time argument. |
| stacks-node/src/tests/nakamoto_integrations.rs | Updates integration test mining call to include the new max_analysis_time argument. |
| clarity/src/vm/tests/mod.rs | Registers the new hooks test module. |
| clarity/src/vm/tests/hooks.rs | Adds extensive unit tests covering lifecycle events, call tracing, lazy special forms, fold, trait dispatch, nested contract calls, and state reads/writes. |
| clarity/src/vm/mod.rs | Moves EvalHook into vm::hooks, instruments eval/apply paths to emit call/lifecycle hook notifications, and refactors builtin dispatch representation. |
| clarity/src/vm/hooks/mod.rs | Introduces the new public hook API: execution lifecycle + call begin/arg eval/finish notifications and supporting types. |
| clarity/src/vm/hooks/internals.rs | Adds internal notifier plumbing (EvalHookNotifier) and a no-op CallTraceFrame wrapper for near-zero overhead when hooks are absent. |
| clarity/src/vm/hooks/trace.rs | Implements CallTraceHook collector to reconstruct resolved call trees with cost/timing attribution and includes unit tests for cost math helpers. |
| clarity/src/vm/functions/mod.rs | Refactors reserved function lookup to return CallableType::Builtin { clarity_name, kind } and supports call tracing metadata. |
| clarity/src/vm/contexts.rs | Adds hook registration, execution lifecycle notifications, call tracing around entry/contract-call routing, and introduces FunctionExecutionOptions. |
| clarity/src/vm/clarity.rs | Updates TransactionConnection::with_abort_callback signature to support hook-aware OwnedEnvironment<'_, 'hooks>. |
| clarity/src/vm/callables.rs | Refactors callable representation to split builtins into CallableType::Builtin + BuiltinKind, adds call trace hook construction, and exposes function name/arg accessors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…l-hook-tracing-improvements
…l-hook-tracing-improvements
|
@hugo-stacks this would be a small breaking change for Clarinet -- worth a look by you guys. Basically, the changes here provide deeper call tracing hooks which allow tx replayers (which use core "append block" flows) to instrument Clarity VM execution more accurately during block/tx replay. |
|
Thanks for the heads up @cylewitruk-stacks I'll keep an eye on it |
Yeah, it does (with new fn's with default impls). Behavior itself shouldn't be changed for the fn's Clarinet currently uses, but the trait moved into the |
|
should this target develop instead? |
…l-hook-tracing-improvements
Coverage Report for CI Build 30105515860Coverage increased (+0.9%) to 86.587%Details
Uncovered Changes
Coverage Regressions12154 previously-covered lines in 169 files lost coverage.
Coverage Stats
💛 - Coveralls |
…l-hook-tracing-improvements
…l-hook-tracing-improvements
…l-hook-tracing-improvements
…l-hook-tracing-improvements
brice-stacks
left a comment
There was a problem hiding this comment.
This looks great! I just requested one more test.
…l-hook-tracing-improvements
…l-hook-tracing-improvements
| if let Err(e) = self.global_context.roll_back() { | ||
| Err(e) | ||
| } else { | ||
| result | ||
| } |
There was a problem hiding this comment.
This is the same semantically as before, just more verbose. Is this just a style refactor?
There was a problem hiding this comment.
Good catch -- I missed to revert this after some mid-impl trial and error which at the time broke the implicit error coercion; will revert.
| cost_input_handle(args.as_slice()) | ||
| } else { | ||
| args.len() as u64 | ||
| Ok(args.len() as u64) | ||
| }; | ||
| runtime_cost(cost_function.clone(), exec_state, cost_input) | ||
| .map_err(VmExecutionError::from) | ||
| .and_then(|_| function.apply(args, exec_state, invoke_ctx)) | ||
| cost_input.and_then(|cost_input| { | ||
| runtime_cost(cost_function.clone(), exec_state, cost_input) | ||
| .map_err(VmExecutionError::from) | ||
| .and_then(|_| function.apply(args, exec_state, invoke_ctx)) | ||
| }) |
There was a problem hiding this comment.
This refactor makes the control flow of the error path much less clear to me -- I think early returning the error rather than passing it through the chained and_then is preferable.
There was a problem hiding this comment.
I'll re-read through this 👍
| &args, | ||
| FunctionExecutionOptions::default(), | ||
| ), | ||
| }, |
There was a problem hiding this comment.
This match feels like it should be defined elsewhere than within this larger match -- before your changeset, I suppose this logic lived in DefinedFunction::apply. It still feels like this match should live in its own function -- nesting matches seems like something to avoid.
There was a problem hiding this comment.
I did go back and forth on this several times, between larger match vs. more indirection... I'll read this over again and give a fresh opinion.
aaronb-stacks
left a comment
There was a problem hiding this comment.
This looks mostly good to me, just had a couple comments on some of the code refactoring.
Adds opt-in Clarity VM hook support for structured call tracing.
Note for reviewers: most of this PR's diff is new tests.
Resolves: https://github.com/stx-labs/core-epics/issues/375
This PR extends EvalHook with:
vm::hooks::trace::CallTraceHook, a reusable collector that reconstructs a call tree with resolved args, results, inclusive/exclusive execution cost, wall time, and nested calls. While fully functional (and public), it was largely used to verify the necessary functionality of the call-trace hooks themselves and serves as a good example for anyone needing to implement their own.Why
stacks-benchand other replay tooling would benefit from understanding which logical VM calls executed during historical transaction execution (without a custom Clarity interpreter), what values they consumed/returned, and where cost was charged. Existing expression-level eval hooks are too low-level for this and require downstream tooling to duplicate VM call-resolution logic.Notes
vm::hooks.did_completeremains unchanged; it is still an external/Clarinet-style completion hook, not emitted by the VM.stackslibblock/transaction-processing code (which is used bystacks-inspectandstacks-bench); that will be in a followup PR -- this is theclarity-side implementation.EvalHookmoved into thevm::hooksnamespace (from a directvm::EvalHookexport, which will break existing consumers -- but it's a simple module path update). I chose to do this to avoidhookstypes polluting the corevmnamespace for opt-in functionality.Testing
fold/step functions, trait-dispatched calls, nested contract calls, and state reads/writes visible through Clarity values.Example output from the debug tracer in tests/hooks.rs: eval_hook_captures_nested_contract_calls_folding_over_contract_calls
Link to test
Checklist
docs/property-testing.md)changelog.d/README.md)