Page MenuHomePhabricator

add nSigChecks counting to EvalScript
ClosedPublic

Authored by markblundeberg on Dec 10 2019, 07:43.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
T704: sigChecks implementation
Commits
rABC484a7646fb1a: add nSigChecks counting to EvalScript
Summary
Test Plan

make 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.Dec 10 2019, 07:43
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 10 2019, 07:43
deadalnix requested changes to this revision.Dec 11 2019, 01:48
deadalnix added inline comments.
src/script/interpreter.cpp
904 ↗(On Diff #14718)

Please avoid error checking in between doing the checksig and counting it.

1115 ↗(On Diff #14718)

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 ↗(On Diff #14718)

This whole thing is quite convoluted and would benefit from ookign alike the CHECKSIG code path.

src/script/interpreter.h
87 ↗(On Diff #14718)

Fix comment formatting.

89 ↗(On Diff #14718)

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 ↗(On Diff #14718)

You don't need inline for static functions.

This revision now requires changes to proceed.Dec 11 2019, 01:48
markblundeberg added inline comments.Dec 11 2019, 11:10
src/script/interpreter.cpp
904 ↗(On Diff #14718)

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 ↗(On Diff #14718)

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 ↗(On Diff #14718)

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 ↗(On Diff #14718)

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
markblundeberg added inline comments.Dec 12 2019, 08:51
src/script/interpreter.h
89 ↗(On Diff #14718)

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.
deadalnix added inline comments.Dec 13 2019, 02:45
src/script/interpreter.cpp
904 ↗(On Diff #14718)

Because you want the signature check and the accounting of signature check to be together.

1186 ↗(On Diff #14718)

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.

markblundeberg added inline comments.Dec 13 2019, 05:20
src/script/interpreter.cpp
1186 ↗(On Diff #14718)

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.

deadalnix added inline comments.Dec 16 2019, 15:25
src/script/interpreter.cpp
1186 ↗(On Diff #14718)

It cannot, there is a checkpoint.

deadalnix requested changes to this revision.Dec 18 2019, 00:28

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

src/script/interpreter.cpp
904 ↗(On Diff #14718)

^

1115 ↗(On Diff #14718)

^

1186 ↗(On Diff #14718)

It looks like you'll need to rebase that part.

src/script/interpreter.h
87 ↗(On Diff #14718)

^

94 ↗(On Diff #14718)

Please reorder arguments such as the default value for serror can stay. Before or after checker seems like an appropriate place.

95 ↗(On Diff #14718)

Fair enough :)

src/test/sigcheckcount_tests.cpp
100 ↗(On Diff #14718)

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
markblundeberg added inline comments.Dec 18 2019, 00:37
src/test/sigcheckcount_tests.cpp
100 ↗(On Diff #14718)

linter save me please :-)

markblundeberg marked 16 inline comments as done.

rebase & update per comments

Fabien added a subscriber: Fabien.Dec 18 2019, 09:44
Fabien added inline comments.
src/test/sigcheckcount_tests.cpp
100 ↗(On Diff #14718)

Sure, see D4754.

deadalnix requested changes to this revision.Dec 18 2019, 17:42

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 :)

This revision now requires changes to proceed.Dec 18 2019, 17:42
markblundeberg added inline comments.Dec 19 2019, 06:41
src/script/interpreter.cpp
887 ↗(On Diff #14950)

Yeah this is a good idea.

markblundeberg marked 7 inline comments as done.
markblundeberg edited the summary of this revision. (Show Details)

rebase for D4766 landing; split out the scripterror helper into separate diff; address comments

deadalnix accepted this revision.Thu, Jan 2, 06:38
deadalnix added inline comments.
src/test/sigcheckcount_tests.cpp
31 ↗(On Diff #15076)

There are weird spaces in there.

This revision is now accepted and ready to land.Thu, Jan 2, 06:38
markblundeberg added inline comments.Fri, Jan 3, 01:43
src/test/sigcheckcount_tests.cpp
31 ↗(On Diff #15076)

Yeah that's odd. The linter seems to insist on them (if I remove them, they get put back).

markblundeberg marked an inline comment as done.

sub 0 -> 0x00 to get linter to remove weird spaces