#35641 kernel: Add script evaluation tracer
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35641 kernel: Add script evaluation tracer | Waiting on author | 1 (+1) | Mild | 549 + 111 tests | |||
|
|
|
| ||||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Adds an optional compile-time script execution tracer exposed through the libbitcoinkernel C API and C++ wrapper. When compiled with `-DENABLE_SCRIPT_TRACE=ON`, hooks in `EvalScript` emit execution state snapshots (stack, altstack, opcode position, execution flag, op count, sigversion, tapleaf hash, codeseparator position, and error code) at execution start, each instruction step, and completion. When the CMake option is disabled (default), the trace hooks compile to no-ops.
Problem: External consumers of libbitcoinkernel cannot observe intermediate script execution states, preventing them from implementing step-by-step script debuggers or execution visualizers against Bitcoin Core's interpreter. Additionally, testing and fuzzing harnesses cannot easily verify internal interpreter behavior when later errors shadow earlier states.
Evidence of importance:
- The author demonstrated the use case by building a proof-of-concept debugger against rust-bitcoinkernel (PR description: 'The initial sketch for this work was prepared on rust-bitcoinkernel... That branch also includes a very rudimentary script debugger showcasing the use case for this patch')
- The author cites PR #33961 as an example where test bugs occurred because 'error conditions shadow actual behaviour', which intermediate trace inspection could prevent
Scope: The feature is gated behind the CMake flag `-DENABLE_SCRIPT_TRACE=ON`. In default builds, the macro hooks in `src/script/interpreter.cpp` expand to `static_assert(true)`.
Risk of a mistake: medium (validation, consensus). A mistake in the macro hooks or state references in EvalScript could alter script evaluation behavior or crash validation if compiled with tracing enabled.
Changed since the previous assessment: No substantial architectural or API changes; the update represents a rebase and minor implementation polish.
Reviewability
Paused · Waiting on author
Open objection from fametrano with no author reply for 7 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: respond to fametrano's concern regarding ScriptTraceGetCallback mutex contention during parallel evaluation 2026-10-25
Agreement
Mild
objection that needs a change was answered, no reply from the objector (w0xlt: tracer incorrectly reports control-flow opcodes inside inactive branches as skipped even though they are executed by the); objection that needs an answer has none yet (fametrano)
Thread notes: Added Tested ACK and mutex contention objection from fametrano. The PR is now waiting on the author to respond to fametrano's proposed atomic check to avoid lock contention.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| alexanderwiederin (member) | question, interface | whether multi-threaded applications are supported and desired | answered · author explained thread safety and that multithreading is not the main use case | answered | yes | 2026-07-06: 'Does this support multi-threaded applications? If not, do we want it to?' Status from: 2026-07-06: sedited: 'It should be thread-safe, but I don't think this is particularly interesting to run in a multi-threaded context.' |
| alexanderwiederin (member) | question, maintenance | whether a CI job is needed to run with ENABLE_SCRIPT_TRACE=ON | answered · author replied unsure where to put it and questioned if worthwhile yet | answered | yes | 2026-07-10: 'Do we need a CI job to run with `ENABLE_SCRIPT_TRACE=ON`?' Status from: 2026-07-10: sedited: 'I wasn't sure where to put such a job, so I didn't introduce it here.' |
| alexanderwiederin (member) | suggestion, interface | define what each error code means | answered · author replied preferring to expose the error list in a follow-up PR | answered | yes | 2026-07-10: 'I think we have to define what each code means. What do you think?' Status from: 2026-07-10: sedited: 'I think we should leave this here - it's fairly straight forward to get the error code header from somewhere. I'll try t' |
| alexanderwiederin (member) | suggestion, approach | snapshot the callback in ScriptTraceScope's constructor to take mutex acquisition and std::function copy out of per-opcode path | fixed · fix pushed · author implemented callback snapshotting in constructor | answered | yes | 2026-07-10: 'Could `ScriptTraceScope` snapshot the callback in its constructor? That takes both the mutex acquisition and the std::function copy out of the per-opcode path.' Status from: 2026-07-13: sedited: 'Done.' |
| elmeriniemela (none) | question, correctness | whether it is expected that a trace can be invalid while the script error code is OK | answered · author explained this is expected because EvalScript does not enforce final stack conditions | answered | yes | 2026-07-24: 'one weirdness that I found is that a trace can be invalid, but the related script error code is still OK.' Status from: 2026-07-25: sedited: 'What you are seeing is the intended behaviour here. Your LLM seems to already have inferred the correct scenario from th' |
| haanhvu (none) | suggestion, maintenance | add a comment that user data is destroyed in any case to avoid problems like double free | answered · author declined, viewing it as self-explanatory | answered | yes | 2026-07-24: 'Should we clearly comment that user data is destroyed in any case, whether tracing enabled/succeeds or not, to avoid problems like double free?' Status from: 2026-08-28: sedited: 'I don't think this needs an extra comment. To me it seems self-explanatory.' |
| haanhvu (none) | objection, safety | BOOST_CHECK_EQUAL on vector size allows test execution to continue and access out-of-bounds indices, causing undefined behavior | fixed · fix pushed · author fixed by making boost size check a hard requirement | answered | yes | 2026-07-29: 'BOOST_CHECK_EQUAL(states.size(), 11) is non-fatal, but the test checks states[0] through states[10] after this. So if the size check fails, the test still continues and may access out-of-bound items, ' Status from: 2026-08-28: sedited: 'made the boost size check a hard requirement.' |
| haanhvu (none) | suggestion, correctness | verify that unregistering the tracer actually stops tracing | fixed · fix pushed · author added verification that unregistering works | answered | yes | 2026-07-30: 'Should we verify unregistering actually stops tracing?' Status from: 2026-08-28: sedited: 'added a quick verification that unregistering the tracer actually works.' |
| haanhvu (none) | suggestion, style | std::move state into m_state to avoid an extra copy of ScriptTraceFrame | answered · author declined as not mattering for a test | answered | yes | 2026-08-01: 'Nit: `state` is already passed by value here, so `m_state.emplace_back(state)` performs an extra copy of the `ScriptTraceFrame`. Would it make sense to move it instead?' Status from: 2026-08-28: sedited: 'I don't think this matters in a test. Besides, the new type is hardly worth the move now.' |
| haanhvu (none) | suggestion, correctness | extend tests to cover callback replacement contract | fixed · fix pushed · author added test verification that re-registering the tracer works | answered | yes | 2026-08-01: 'Should we extend the test to cover this callback replacement contract?' Status from: 2026-08-28: sedited: 'added a verification that re-registering the tracer works.' |
| stringintech (contributor) | suggestion, interface | make frame opaque with getters instead of a public struct | fixed · fix pushed · author wrapped the frame into nested opaque types and getters | answered | yes | 2026-08-11: 'would it make sense to make the frame opaque like other `btck_*` types, with getters, instead of a public struct' Status from: 2026-08-28: sedited: 'wrapped the frame into nested opaque types and getters.' |
| stringintech (contributor) | suggestion, interface | use exhaustive switch for kind and sig_version conversions | fixed · fix pushed · author switched to exhaustive enum conversions | answered | yes | 2026-08-11: 'Perhaps it is better to use an exhaustive switch for `kind` and `sig_version` as well, in both directions' Status from: 2026-08-28: sedited: 'use exhaustive enum conversions.' |
| stringintech (contributor) | suggestion, approach | drop the closure for f_exec and use a fixed dummy in begin and end frames | fixed · fix pushed · author dropped f_exec preloading closure and documented dummy values in begin and end frames | answered | yes | 2026-08-11: 'can we drop the closure and give it a fixed dummy in begin and end frames?' Status from: 2026-08-28: sedited: 'dropped f_exec pre-loading with a lambda. As suggested, the begin and end frames now contain dummy values' |
| stringintech (contributor) | objection, correctness | null error pointer causes successful evaluation to report SCRIPT_ERR_UNKNOWN_ERROR in trace end frames | fixed · fix pushed · author fixed by adding assertion for passing in error | answered | yes | 2026-08-11: 'a null m_error makes a successful eval report SCRIPT_ERR_UNKNOWN_ERROR. The kernel path always passes &error, but in case any non-kernel callers want to use the tracer, would it make sense to assert i' Status from: 2026-08-28: sedited: 'added the suggested assertion for passing in the error.' |
| haanhvu (none) | suggestion, correctness | verify other fields besides m_kind and m_script_error in tests | answered · author declined making tests exhaustive | answered | yes | 2026-08-14: 'Should we also verify other fields besides `m_kind` and `m_script_error` to see if the deep-copy conversion works fully correctly?' Status from: 2026-08-28: sedited: 'I'm not sure if this is still relevant after the last push. In any case, I don't want the tests here to be exhaustive.' |
| alexanderwiederin (member) | objection, correctness | test suite fails when compiled with ENABLE_SCRIPT_TRACE=ON due to callback pushing into destroyed vector in subsequent tests | fixed · fix pushed · author fixed by unsetting the callback after trace tests complete | changed | yes | 2026-08-31: 'Add ScriptTraceUnsetCallback() to avoid the next test case push into a destroyed vector. Test suite currently does not pass with the feature flag.' Status from: 2026-08-31: sedited: 'unsetting the callback after the trace tests are complete.' |
| alexanderwiederin (member) | suggestion, interface | add BITCOINKERNEL_WARN_UNUSED_RESULT to kernel function | fixed · fix pushed · author added BITCOINKERNEL_WARN_UNUSED_RESULT annotation | answered | yes | 2026-08-31: 'This needs a `BITCOINKERNEL_WARN_UNUSED_RESULT`' Status from: 2026-10-06: sedited: 'added suggested `BITCOINKERNEL_WARN_UNUSED_RESULT`' |
| w0xlt (contributor) | objection, correctness | EvalScript triggers an assertion failure when tracing is enabled if called with a null serror pointer (e.g. in CheckSignetBlockSolution) | fixed · fix pushed · author pushed fix making error code optional instead of asserting | changed | yes | 2026-09-01: 'The tracing code assumes serror is non-null whenever a callback is registered, but EvalScript permits nullptr... This triggers the assertion assert(!m_callback || error);' Status from: 2026-09-24: sedited: 'making the error code "optional" by encoding `-1` as a nullopt. This gets rid of the assertion' |
| w0xlt (contributor) | objection, correctness | oversized scripts are rejected before ScriptTraceScope is instantiated, preventing BEGIN and END trace frames from being emitted | answered · author declined changing interpreter and noted kernel lib catches the length check | answered | yes | 2026-09-01: 'EvalScript rejects oversized scripts before constructing ScriptTraceScope, so a registered callback receives neither BEGIN nor END.' Status from: 2026-09-24: sedited: 'I don't want to change anything in the interpreter in this pull request. When running through the kernel lib's script ve' |
| w0xlt (contributor) | question, style | whether function-like macros are needed instead of expanding ifdefs at call sites | answered · author declined changing, preferring function macros to minimize interpreter line changes | answered | yes | 2026-09-01: 'Is there a specific reason these need to be function-like macros?' Status from: 2026-09-24: sedited: 'I'd like to keep the lines changed in the interpreter to an absolute minimum. My preference style-wise is the function m' |
| w0xlt (contributor) | objection, correctness | tracer incorrectly reports control-flow opcodes inside inactive branches as skipped even though they are executed by the interpreter | answered · author declined changing fExec meaning, added docs clarification and a test case | changed | yes | 2026-09-24: 'The tracer uses `fExec` to indicates whether the current conditional branch is active. But the interpreter needs to process control-flow opcodes even inside inactive branches... the tracer incorrectly' Status from: 2026-09-25: sedited: 'added a sentence in the docs that conditional opcodes are indeed processed when fexec is set. I don't think it is a good' |
| alexanderwiederin (member) | suggestion, interface | add BITCOINKERNEL_WARN_UNUSED_RESULT annotations to kernel trace header functions | fixed · fix pushed · author added suggested BITCOINKERNEL_WARN_UNUSED_RESULT annotations | answered | yes | 2026-10-05: 'Missing some `BITCOINKERNEL_WARN_UNUSED_RESULT` I think.' Status from: 2026-10-06: sedited: 'added suggested `BITCOINKERNEL_WARN_UNUSED_RESULT`' |
| alexanderwiederin (member) | suggestion, interface | add BITCOINKERNEL_WARN_UNUSED_RESULT to kernel function | fixed · fix pushed · author added BITCOINKERNEL_WARN_UNUSED_RESULT annotation | answered | yes | 2026-10-05: 'This needs a `BITCOINKERNEL_WARN_UNUSED_RESULT`' Status from: 2026-10-06: sedited: 'added suggested `BITCOINKERNEL_WARN_UNUSED_RESULT`' |
| alexanderwiederin (member) | suggestion, interface | add BITCOINKERNEL_WARN_UNUSED_RESULT to kernel function | fixed · fix pushed · author added BITCOINKERNEL_WARN_UNUSED_RESULT annotation | answered | yes | 2026-10-05: 'This needs a `BITCOINKERNEL_WARN_UNUSED_RESULT`' Status from: 2026-10-06: sedited: 'added suggested `BITCOINKERNEL_WARN_UNUSED_RESULT`' |
| fametrano (contributor) | objection, approach | ScriptTraceGetCallback locks g_script_trace_mutex on every EvalScript call even with no tracer registered, causing mutex contention during parallel script verification | open | answered | no | 2026-10-25: 'ScriptTraceGetCallback() still locks g_script_trace_mutex on every EvalScript call, so parallel script checks contend on it.' |
Support:
- furszy (member) (Concept ACK, 2026-07-02): haven't checked the implementation. But this is something that would benefit testing greatly.
- w0xlt (contributor) (Concept ACK, 2026-07-02): Strong Concept ACK [not substantive]
- theStack (member) (Concept ACK, 2026-07-08): Concept ACK [not substantive]
- alexanderwiederin (member) (ACK, 2026-07-15): ACK 114e48e968a087f74c1ab611ac2a31a9266812e1 [not substantive]
- elmeriniemela (none) (2026-07-22) [tested]: tested with a clanker... Added python bindings... Made a visualizer using the traces... FWIW, found no issues
- haanhvu (none) (Approach ACK, 2026-08-14): Approach ACK [not substantive]
- fametrano (contributor) (Tested ACK, 2026-10-25) [both: C API, ScriptTraceScope, ScriptTraceGetCallback(), test_kernel]: I drove the tracer through the C API over 32,535 spends: legacy, P2SH, P2WSH and tapscript, valid and invalid, with branches not taken, OP_CODESEPARATOR, OP_SUCCESSx and scripts that fail on their last opcode. I compared each frame with a step trace of a script interpreter I maintain. All 158,683 st
Participants:
- furszy (member, support, 1 statements 2026-07-02 to 2026-07-02): Concept ACK noting it would benefit testing greatly
- w0xlt (contributor, objection, 5 statements 2026-07-02 to 2026-09-24): Strong Concept ACK, but objected to assertion on null serror, missing frames on oversized scripts, and incorrect exec reporting for control flow
- alexanderwiederin (member, objection, 15 statements 2026-07-06 to 2026-10-05): ACKed earlier revision, but pointed out test suite failures with ENABLE_SCRIPT_TRACE=ON and missing BITCOINKERNEL_WARN_UNUSED_RESULT annotations
- theStack (member, support, 1 statements 2026-07-08 to 2026-07-08): Concept ACK
- elmeriniemela (none, support, 2 statements 2026-07-22 to 2026-07-24): tested with python bindings and visualizer, finding no issues
- haanhvu (none, objection, 7 statements 2026-07-24 to 2026-08-14): Approach ACK, but raised out-of-bounds UB concern on test assertions which was resolved
- stringintech (contributor, objection, 4 statements 2026-08-11 to 2026-08-11): Suggested opaque types, exhaustive enums, and asserting non-null error pointer in trace scope
- fametrano (contributor, objection, 1 statements 2026-10-25 to 2026-10-25): Tested ACK after extensive verification, but raised objection to mutex contention in EvalScript when tracing is enabled without a registered callback
Files
111 lines under test/bench/ci.
- src/kernel/bitcoinkernel.cpp +146/-1
- src/kernel/bitcoinkernel.h +135/-0
- src/script/trace.h +118/-0
- src/test/kernel/test_kernel.cpp +107/-0
- src/kernel/bitcoinkernel_wrapper.h +96/-0
- src/script/trace.cpp +40/-0
- src/script/interpreter.cpp +8/-1
- src/kernel/CMakeLists.txt +6/-0
- src/test/kernel/CMakeLists.txt +4/-0
Card
The short text the ranking pass compares across PRs.
Adds an optional compile-time script execution tracing framework exposed via the libbitcoinkernel C API and C++ wrapper. External libbitcoinkernel consumers currently have no way to inspect intermediate script interpreter states, preventing the development of external script debuggers or fine-grained execution analyzers. The author provides a working prototype debugger in rust-bitcoinkernel to demonstrate practical utility and notes that intermediate frames can help catch interpreter bugs shadowed by later failure codes in tests. The feature is disabled by default via CMake and has no dependencies.
Data
dossier JSON · extract JSON · model ledger, generated 2026-10-26T01:21, confidence high, input hash 677a530429c17e5e
thread read through 2026-10-25 (54 statements), head 0161ebdbf6
labels: Validation
thread read through 2026-10-25 (54 statements), head 0161ebdbf6
labels: Validation
Categories
Kernel (libbitcoinkernel)
P3 · new feature
Exposes intermediate script execution states to libbitcoinkernel consumers for debuggers and visualizers. The PR adds an optional compile-time API that downstream tools (such as rust-bitcoinkernel) can build on, but it is an opt-in debugging extension rather than core block validation or chainstate functionality, making it worthwhile but deferrable.
Membership: Adds C API functions and C++ wrapper headers in src/kernel/ and kernel test coverage for script execution tracing.
Factors: security/stability 0, bug 0, performance 0, user value 2, leverage 1