Page MenuHomePhabricator

save ScriptExecutionMetrics during CScriptCheck

Authored by markblundeberg on Jan 13 2020, 12:44.



CScriptCheck represents a full closure of the call to VerifyScript.
Currently it just saves the outputted error code; update it to also save
the metrics return value.

A small test (taking advantage of an existing test that uses CScriptCheck) is

Depends on D4918

Test Plan

ninja all check

Diff Detail

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markblundeberg created this revision.Jan 13 2020, 12:44
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 13 2020, 12:44
markblundeberg added a comment.EditedJan 13 2020, 12:52

Note: As far as closures go, CScriptCheck is slightly weird because it saves (some of) the results into the closure, rather than returning them.

In the future it would be worth looking into perhaps just storing the input parameters, and then returning a struct with all the outputs: {bool success; ScriptError error; ScriptExecutionMetrics metrics;}. This would save a bit of memory during validation of blocks that have lots of uncached scripts (such as during IBD, or if running in blocksonly mode). But it would be a bit of an overhaul, especially due to how it affects checkqueue.h and the many associated tests. For now it's not worth having such a complication, and I don't think it will be any harder if we do it later (probably easier overall if we first complete other backports).

deadalnix accepted this revision.Jan 13 2020, 15:07
deadalnix added inline comments.
436 ↗(On Diff #15392)

These raw int access make me uncomfortable. Someone is going to want to do something smart with this at some point and we'll regret it.

This revision is now accepted and ready to land.Jan 13 2020, 15:07