track ScriptExecutionMetrics in CheckInputs
AbandonedPublic
Actions

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 Passed

Unit

No Test Coverage

Build Status

Buildable 8928
Build 15830: Default Diff Build & Tests
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

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 B8928: Diff 15445.Jan 14 2020, 15:12

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.

markblundeberg mentioned this in D4834: add sigChecks value to script cache.Jan 19 2020, 00:43

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.