Page MenuHomePhabricator

save ScriptExecutionMetrics during CScriptCheck
ClosedPublic

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
T704: sigChecks implementation
Commits
rABC8bbb45003d06: save ScriptExecutionMetrics during CScriptCheck
Summary

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
included.

Depends on D4918

Test Plan

ninja all check

Diff Detail

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

Event Timeline

markblundeberg created this revision.Mon, Jan 13, 12:44
Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Jan 13, 12:44
markblundeberg added a comment.EditedMon, Jan 13, 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.Mon, Jan 13, 15:07
deadalnix added inline comments.
src/test/txvalidationcache_tests.cpp
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.Mon, Jan 13, 15:07