Details

Reviewers

Group Reviewers

Restricted Project

Maniphest Tasks

Commits

rSTAGING76e36aa9079b: add a flag that restricts sigChecks per-input
rABC76e36aa9079b: add a flag that restricts sigChecks per-input

Summary

This flag is intended for standardness-only, and the primary goal is to
make is so that malicious super-high-density-SigChecks transactions cannot
get into the mempool, where they would frustrate simple block assembly
algorithms.

A transaction conforming to this rule cannot (overall) exceed a density of
1 sigcheck per 36.666 bytes.

(ref: https://gist.github.com/markblundeberg/becf85ac6ba48bebfeb7871ce2de27a7)

Depends on D4835

Test Plan

make check

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markblundeberg created this revision.Dec 2 2019, 19:26

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 2 2019, 19:26

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

markblundeberg added a task: T704: sigChecks implementation.Dec 2 2019, 19:26

markblundeberg added inline comments.Dec 2 2019, 19:28

src/script/interpreter.cpp
1172 ↗	(On Diff #14579)	fix "an the"

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8376: Diff 14579.Dec 2 2019, 19:46

markblundeberg planned changes to this revision.Dec 2 2019, 21:54

The general direction is good, but there are a few problem with this patch.

First, it implements the counting mechanism and the higher level checks at once. It is better to implement the counting alone and test it, then wire that in in the script verification process thing in another.

Add a test that Run EvalScript on various scripts and get the expected results.

src/script/interpreter.cpp
105 ↗	(On Diff #14579)	Pass a reference to some metric counter object instead of an `int*`. If you are worried about verbosity on all callsite, add an overload of `EvalScript` that doesn't need it.
907 ↗	(On Diff #14579)	metricCounter.addCheckSig(1);
1814 ↗	(On Diff #14579)	Move all changes to this in another patch.

redo for building on D4678

Build Bitcoin-ABC / Diffs / Diff Testing started.

There is still more to do on this, but just updating for now.

Now that we've switched to the areAllSigaturesNull approach, I am finding it difficult to write *any* standard script that is affected by the very short script case (shorter than 46 bytes), where the "L / 23" part of the limit becomes active. (previously such a script was easy, using 0-of-2 multisig as seen in prior version of this diff)

So I'm thinking of removing the L/23 part, and just having the (L+60)/43 simple line limit. Below I've plotted the inverse of density (taking into account an input has total size of 41 bytes + len(scriptSig) ), and compared the two rules. Note that sigchecks has to be an integer hence the staircase appearance.

Using just the simpler rule (the blue trace) will increases the theoretical maximum density slightly (from 1/36.666 to 1/33.5), but exceeding 1/36.666 requires scriptSig to be between 26 and 32 bytes (inclusive) and execute two sigchecks. I can't think of any standard spend that would be shorter than 32 bytes scriptSig and even execute one signature check, much less two of them! It's easy to come up with nonstandard spends that do this, but this is intended to be a standardness rule.

src/script/interpreter.cpp
1798 ↗	(On Diff #14963)	not sure about the nSigChecks*23 any more...

markblundeberg planned changes to this revision.Dec 18 2019, 11:03

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8622: Diff 14963.Dec 18 2019, 11:05

Simplify by removing the scriptlen/23 piece.
I've been racking my brain trying to think of any examples that it would
impact and I can't find any.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8715: Diff 15120.Jan 3 2020, 01:55

deadalnix added inline comments.Jan 3 2020, 06:12

src/script/interpreter.cpp
1696 ↗	(On Diff #15120)	Declare before use. Declaring variable at the beginning of function has not been useful since the 80s.
1799 ↗	(On Diff #15120)	It looks like it wasn't a great idea to make nSigCheck signed.
1799 ↗	(On Diff #15120)	Note that this is also different from (L+60) / 43 due to rounding being different.
src/test/script_tests.cpp
2252 ↗	(On Diff #15120)	How come so many flags are required? If there are dependencies on flags, it needs to be checked somewhere.

markblundeberg added inline comments.Jan 3 2020, 23:58

src/script/interpreter.cpp
1799 ↗	(On Diff #15120)	The quantity `metrics.nSigChecks * 43 - 60` becomes negative here, so it definitely should be signed comparison. I'm not sure what rounding differences you're alluding to... can you give an example?
src/test/script_tests.cpp
2252 ↗	(On Diff #15120)	My idea here was to use the 'modern' set of flags rather than testing with a strange never-existing flag set. It's not necessarily that the tests require all these flags. I think only schnorr_multisig and p2sh are actual dependencies in (some of) the tests. Ideally we need that updated script testing system that buckets tests according to satisfying consensus/standardness, with/without/independent of upgrade.

markblundeberg mentioned this in D4834: add sigChecks value to script cache.Jan 5 2020, 02:54

rebase for parent landing ; move decl down

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8741: Diff 15160.Jan 5 2020, 03:52

markblundeberg added a child revision: D4835: VerifyScript: accumulate ScriptExecutionMetrics and return them.Jan 5 2020, 04:11

markblundeberg planned changes to this revision.Jan 5 2020, 04:49

markblundeberg added inline comments.

src/script/interpreter.cpp
1754 ↗	(On Diff #15160)	Hmm I'm going to move above changes into D4835 and flip the dependency, that's cleaner.

markblundeberg mentioned this in D4835: VerifyScript: accumulate ScriptExecutionMetrics and return them.Jan 5 2020, 05:16

markblundeberg removed a child revision: D4835: VerifyScript: accumulate ScriptExecutionMetrics and return them.Jan 5 2020, 05:17

move consensus-affecting bits into D4835 so this is only the flag effect

Build Bitcoin-ABC / Diffs / Diff Testing started.

markblundeberg removed a parent revision: D4678: add nSigChecks counting to EvalScript.Jan 5 2020, 05:19

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8744: Diff 15163.Jan 5 2020, 05:26

Requesting change due to parent. This looks good, but the parent doesn't and this will have to be adapted to the parent and will need to be reviewed again.

This revision now requires changes to proceed.Jan 5 2020, 10:28

rebase for modified parent (no actual change)

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8777: Diff 15215.Jan 8 2020, 06:56

deadalnix added inline comments.Jan 8 2020, 17:55

src/script/interpreter.cpp
1799 ↗	(On Diff #15120)	I think I was wrong about the rounding this, but my level of confidence is not high. The negative problem can be fixed easily by adding 60 on the left side rather than subtracting on the right side. It also happens to be "more correct" as it'll work even if the script size is more than 4G, which I'm sure will happen any time.
src/test/script_tests.cpp
2252 ↗	(On Diff #15120)	So why not MANDATORY_FLAGS \| SCRIPT_VERIFY_INPUT_SIGCHECKS ?

markblundeberg added inline comments.Jan 8 2020, 23:12

src/test/script_tests.cpp
2252 ↗	(On Diff #15120)	MANDATORY_SCRIPT_VERIFY_FLAGS does changes sometimes (whenever we want to modify banning behaviour, as that's what it's for) which would mean needing to rebuild the script tests. Seems a bit weird...