Page MenuHomePhabricator

add a flag that restricts sigChecks per-input

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


Group Reviewers
Restricted Project
Maniphest Tasks
T704: sigChecks implementation
rABC76e36aa9079b: add a flag that restricts sigChecks per-input

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

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


Depends on D4835

Test Plan
make check

Diff Detail

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; 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
markblundeberg added inline comments.Dec 2 2019, 19:28
1172 ↗(On Diff #14579)

fix "an the"

markblundeberg planned changes to this revision.Dec 2 2019, 21:54
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.

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

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.

1798 ↗(On Diff #14963)

not sure about the nSigChecks*23 any more...

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

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.

deadalnix added inline comments.Fri, Jan 3, 06:12
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.

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.Fri, Jan 3, 23:58
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?

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 planned changes to this revision.Sun, Jan 5, 04:49
markblundeberg added inline comments.
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.Sun, Jan 5, 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.Sun, Jan 5, 10:28

rebase for modified parent (no actual change)

deadalnix added inline comments.Wed, Jan 8, 17:55
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.

2252 ↗(On Diff #15120)


markblundeberg added inline comments.Wed, Jan 8, 23:12
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...

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