Details

src/script/interpreter.cpp
904	Please avoid error checking in between doing the checksig and counting it.
1115	It would be better to put this near the for loop do the connection with nSigsCount is evident. You can also increment by one for each loop iterration just before or just after calling VerifySig. Putting it isolated on its own is how you end up people missing it when they edit the code later on, same for review. Out of sight, out of mind.
1186	This whole thing is quite convoluted and would benefit from ookign alike the CHECKSIG code path.
src/script/interpreter.h
87	Fix comment formatting.
89	Do you really expect negative number of sigops? The way I envision this is that you have 2 structs, one that collects the metric, with data private and that friend EvalScript. Then you get another struct, that has the same metrics, that has getter for them, and a public function to absorb the first struct. You'll have to get something like this at some point to aggregate a bunch of metrics from a bunch of scripts that run in parallel at some point. Maybe not strictly necessary for this patch, but think about it. Maybe `ScriptMetricCollector` and `ScriptMetricAggregator` or something like that.
95	You don't need inline for static functions.

This revision now requires changes to proceed.Dec 11 2019, 01:48

src/script/interpreter.cpp
904	Any reason why? As described in the spec, the sigChecks count for a failing scripts is unspecified. It really shouldn't make a difference under any circumstances (if it matters, then other much more important things have gone horribly wrong).
1186	Can you be more specific? The required behaviour is: scan all signatures (not just scan some of signatures like in the main loop above), and if any of them is non-null then increment sigchecks by nKeysCount, once.
src/script/interpreter.h
89	No, but I do plan to do integer arithmetic (and not modulo arithmetic/bitwise manips) with the sigchecks count, and the common wisdom is that you don't use modulo ints ("unsigned") just because an arithmetical quantity is nonnegative.
95	That's true, it will compile, but I'd like to avoid gcc throwing up a bunch of warnings about unused function coming from this header when it's only static.

markblundeberg requested review of this revision.Dec 11 2019, 11:12

src/script/interpreter.h
89	For aggregation, my plan is like so: A fresh `ScriptExecutionMetrics` is generated for every call to `VerifyScript` (i.e., a new one for every input and for every distinct set of flags). So, at first aggregation only happens across the sequential `EvalScript` calls that happen on the same stack. Each `ScriptExecutionMetrics` needs to be unshared and starts fresh for that input, so that any per-input limitations know what happened for that input specifically and separately. In CScriptChecks::operator(), once the VerifyScript is done it can then merge the per-input metrics into a coarser aggregator, which can be one of two types: add the per-input metrics to some transaction-aggregate metrics, during mempool validation (would be needed for doing transaction vbytes size calculation); once CheckInputs is done running all inputs, throw the aggregate metrics into the script cache (as relevant). subtract the per-input metrics from a block-aggregate atomic 'metrics-remaining', during block parallel validation; if the atomic metrics-remaining falls below 0 then bail.

src/script/interpreter.cpp
904	Because you want the signature check and the accounting of signature check to be together.
1186	The only situation where it makes a difference is if you use the count in a case where nullfail is not present. Making the code more complex to support something that never happens is a bad idea.

src/script/interpreter.cpp
1186	As I described in telegram, that's the plan eventually after sigops is removed -- calculate sigchecks on all blocks, both historical and current. By using this limit, it means that CPU usage per block will be limited even if the node somehow gets sidetracked onto an alternate history chain prior to nullfail, that is designed to introduce high CPU usage. Yes, it can happen- this could happen maybe because the user disabled checkpoints, or because the checkpoints don't work. FWIW I believe this way is perfectly clear -- "are all signatures null? if not, then add N sigchecks". Putting something into the loop makes it much more awkward since the loop never performs N iterations, except the special case of N==M and success. If you have a better way to code this, please show me the code.

src/script/interpreter.cpp
1186	It cannot, there is a checkpoint.

A bunch of smaller stuff, but this looks good overall.

src/script/interpreter.cpp
904	^
1115	^
1186	It looks like you'll need to rebase that part.
src/script/interpreter.h
87	^
94	Please reorder arguments such as the default value for serror can stay. Before or after checker seems like an appropriate place.
95	Fair enough :)
src/test/sigcheckcount_tests.cpp
100	You should avoid these kind of empty lines. They make the code look sloppy, which cause the reviewer to expect the code to be bad - when really it doesn't look like it actually is.

This revision now requires changes to proceed.Dec 18 2019, 00:28

src/test/sigcheckcount_tests.cpp
100	linter save me please :-)

rebase & update per comments

Fabien added inline comments.

src/test/sigcheckcount_tests.cpp
100	Sure, see D4754.

More small changes. The overall diff is sound.

src/script/interpreter.cpp
887 ↗	(On Diff #14950)	If that test is to be performed regardless, I think it may be a good idea to call `checker.CheckSig` in the if branch, like it's done for CDS.
src/test/sigcheckcount_tests.cpp
17 ↗	(On Diff #14950)	Formating.
38 ↗	(On Diff #14950)	This is all very packed, a few empty lines would make this more readable. This would also beneit from being const if this is actually const - which I think it is. If this is not const, it would be preferable to make this a local state.
65 ↗	(On Diff #14950)	Why inline?
82 ↗	(On Diff #14950)	A lot of the above stuff would benefit from being within the test suite.
93 ↗	(On Diff #14950)	This should be const or local.
131 ↗	(On Diff #14950)	Capital letters usually refers to constants. Can't you just use i, j like every good loop counter?
201 ↗	(On Diff #14950)	Have some mercy on your reviewer and add some empty lines :)