Page MenuHomePhabricator

track ScriptExecutionMetrics in CheckInputs
AbandonedPublic

Authored by markblundeberg on Jan 14 2020, 15:05.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
T704: sigChecks implementation
Summary

This makes CheckInputs do the following:

  • If metrics are found in cache, return them.
  • If cache miss and checks are not deferred, then accumulate the total of inputs' metrics, store them in cache if requested, and then finally return those.
  • If cache miss and checks are deferred, return empty metrics.

Due to D4940 even though some standard txes right now may have huge sigchecks
counts > 65535, they will still be cacheable so there is no performance penalty.

Depends on D4940 and parents

Test Plan

ninja check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
sigchecks_checkinputs
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8928
Build 15830: Bitcoin ABC Buildbot
Build 15829: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Jan 14 2020, 15:05
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 14 2020, 15:05
deadalnix requested changes to this revision.Jan 15 2020, 22:48
deadalnix added inline comments.
src/script/script_metrics.h
24

You find yourself with that pod/non pod problem once again.

This clearly indicates that you are trying to build an abstraction, not to bundle various related data together.

It is somewhat strange that you have a += operator defined, but not a + operator defined. The reverse, why not, if you want to make an immutable struct or something, but this is weird.

src/validation.cpp
794

!= ?

806

Is this a bug though? We don't know what future flag we'll have, and by the time we introduce one, it won't be obvious this message needs to be updated.

1271

The thing that you want to actually check is that the maximum tx size won't give you numbers above 65k sigops. Because if not, then we won't cache precisely the script we'd benefit the most from caching.

This revision now requires changes to proceed.Jan 15 2020, 22:48
markblundeberg added inline comments.Jan 16 2020, 07:53
src/script/script_metrics.h
24

Yeah, effectively I'm using += as shorthand for 'merge' / 'accumulate'.

For example if one of the values was a boolean that tracks whether some operation has been executed at least once, then, then this would actually perform |= on said boolean, instead of addition. (I have no idea why such a thing would be useful, but that's the meaning.)

src/validation.cpp
806

Hmm... yes, I'd say it's a bug that indicates the standard flag needs to be changed. It's analogous to the CheckInputsFromMempoolAndCache above -- we could just silently reject such transactions without logging, since it's unambigously the correct response to a consensus-invalid tx, but it does mean there is a bug in standardness rules.

1271

The only way we can guarantee such a thing is if either flags & SCRIPT_VERIFY_INPUT_SIGCHECKS is true at this point and it has the current definition, or parallel validation is on. Otherwise, the total sigchecks can be as high as 5000000 (which is the number the current code is testing).

Is this going to be guaranteed? Yes, but only weakly and indirectly:

  • During ConnectBlock, CheckInputs will always be called without flags & SCRIPT_VERIFY_INPUT_SIGCHECKS. It is only because of the input-granular parallel validation that the transaction total never gets constructed. If parallel validation were ever turned off (or if it were changed to tx-granular parallelism instead of input-granular parallelism), then we could exceed 65535.
  • During mempool admission, CheckInputs will be called with flags & SCRIPT_VERIFY_INPUT_SIGCHECKS after activation, ideally. But if for some reason this flag isn't turned on, then we could exceed 65535. For example, the second call to CheckInputs happens using consensus flags only, and we can only get to that point by passing the standard flags check, which (hopefully) has identical metrics.
  • SCRIPT_VERIFY_INPUT_SIGCHECKS itself might be redefined, thereby changing the maximum density. If maximum density were to fall below ~16 (currently 33.5) then we could exceed 65535.

Anyway, that check you want about ensuring the cache can fit standard things is done in https://reviews.bitcoinabc.org/D4834. This static assert is to ensure no UB even in the worst case.

If you're really worried about having uncacheable transactions due to weird things, we can allocate 23 bits to store sigchecks in the cache instead of 16, which will then be enough even for nonstandard transactions.

markblundeberg requested review of this revision.Jan 18 2020, 04:05
markblundeberg added inline comments.
src/validation.cpp
794

no operator!= is defined.

deadalnix requested changes to this revision.Jan 19 2020, 14:17
deadalnix added inline comments.
src/script/script_metrics.h
24

You seem to be telling me that it is the responsibility of the class to manage the semantic of its innards. It therefore follows that this is not the responsibility of the code using it.

You have a design problem staring you into the face right now.

src/validation.cpp
806

OK

1271

This static assert is to ensure no UB even in the worst case.

Note that this is a good argument pointing to the fact that using an int to count sigops wasn't such a good idea. You end up having to do a bunch of check that the number is positive and also have to add check for overflow anyways.

I nw understand the problem of having sigops count that go over the limit before there is enforcement. But this still lead a ton of checks, and I'm not convinced that this is ideal. Why not simply not accumulate sigops when the flag is not set? No you'd only get one test at instead of the battery you need sprinkled all over the patchset.

This revision now requires changes to proceed.Jan 19 2020, 14:17
deadalnix added inline comments.Jan 19 2020, 14:18
src/validation.cpp
1271

Note that you end up having to do something similar to what is proposed here in D4940 anyways, so it doesn't looks like you are saving anything at all.

markblundeberg abandoned this revision.Jan 25 2020, 04:23

we will not be using ScriptExecutionMetrics