save ScriptExecutionMetrics during CScriptCheck
ClosedPublic
Actions

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

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Maniphest Tasks

T704: sigChecks implementation

Commits

rSTAGING8bbb45003d06: save ScriptExecutionMetrics during CScriptCheck
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

Lint Not Applicable

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

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8892: Diff 15392.Jan 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.

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.