Page MenuHomePhabricator

add a flag that restricts sigChecks per-input
ClosedPublic

Authored by markblundeberg on Dec 2 2019, 19:26.

Details

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

src/script/interpreter.cpp
1172 ↗(On Diff #14579)

fix "an the"

deadalnix requested changes to this revision.Dec 2 2019, 22:01

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.

markblundeberg retitled this revision from add sigChecks counting and a flag that restricts it per-input to add a flag that restricts sigChecks per-input.
markblundeberg edited the summary of this revision. (Show Details)
markblundeberg edited the test plan for this revision. (Show Details)

redo for building on D4678

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.

rule.png (480×640 px, 42 KB)

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...

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.

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.

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 marked 4 inline comments as done.

rebase for parent landing ; move decl down

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 edited the summary of this revision. (Show Details)

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

deadalnix requested changes to this revision.Jan 5 2020, 10:28

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)

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 ?

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...

This revision is now accepted and ready to land.Jan 11 2020, 17:06