Page MenuHomePhabricator

add SCRIPTSIGMINPUSHONLY flag and logic
AbandonedPublic

Authored by markblundeberg on Jun 9 2019, 05:49.

Details

Reviewers
jasonbcox
Fabien
Mengerian
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
T667: add rule for **minimal** push only in scriptSig
Summary

This flag enforces that for any given stack created by scriptSig execution,
the scriptSig must have been encoded in a canonical, minimal push-only
form. See T667.

This is intended to be adopted at consensus layer; at the standardness
layer it is made redundant by the broad and dual-purpose MINIMALDATA-rule.
Once adopted, it would subsume the SIG_PUSHONLY rule which could then be
removed.

  • I called it SCRIPTSIGMINPUSHONLY instead of SIG_MINPUSHONLY since the SIG_ prefix sounds like it has to do with signatures somehow.
  • There are a few script_tests involving PUSHONLY that were copied, in addition to a number of MINIMALDATA tests (those regarding pushes, not number encoding) that were copied.
  • A few new script_tests added as well.

Depends on D3270

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
scriptminpushonly
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 6234
Build 10515: Bitcoin ABC Buildbot (legacy)
Build 10514: arc lint + arc unit

Event Timeline

src/script/script_flags.h
117

This is just a matter of taste, but I'd add an underscore to call it "SCRIPT_VERIFY_SCRIPTSIG_MINPUSHONLY".

It just seems slightly more readable that way to me.

src/script/interpreter.cpp
1597

Something interesting: this has been redundant since November and we could remove it (or better -- comment out for posterity).

deadalnix requested changes to this revision.Jun 12 2019, 13:02
deadalnix added inline comments.
src/script/interpreter.cpp
93

What is the problem with using minimaldata, that already do what we are looking for ?

1597

We don't comment out for posterity. We have source control for posterity.

This revision now requires changes to proceed.Jun 12 2019, 13:02
markblundeberg added inline comments.
src/script/interpreter.cpp
93

MINIMALDATA is a different and much larger ruleset. It also has quite low test coverage (since it is only a standardness rule), so from what I can tell, many more tests would have to be added to make it consensus ready.

MINIMALDATA does the following, very little of which is tested:

  • All pushes when executing scriptSig must use minimal push form.
  • All pushes when executing scriptPubKey must use minimal push form. (this does nothing to reduce malleability, and only makes scripts unspendable)
  • All pushes when executing redeemScript must use minimal push form. (this does nothing to reduce malleability, and only makes scripts unspendable)
  • The operand of OP_CHECKLOCKTIMEVERIFY and OP_CHECKSEQUENCEVERIFY must be a number encoded as minimal byte array.
  • The operand of OP_PICK and OP_ROLL must be a number encoded as minimal byte array.
  • The operand of OP_1ADD, OP_1SUB, OP_NEGATE, OP_ABS, OP_NOT, OP_0NOTEQUAL must be a number encoded as minimal byte array.
  • Both operands to the following opcodes must be numbers encoded as minimal byte arrays: OP_ADD, OP_SUB, OP_DIV, OP_MOD, OP_BOOLAND, OP_BOOLOR, OP_NUMEQUAL, OP_NUMEQUALVERIFY, OP_NUMNOTEQUAL, OP_LESSTHAN, OP_GREATERTHAN, OP_LESSTHANOREQUAL, OP_GREATERTHANOREQUAL, OP_MIN, OP_MAX
  • All three operands to OP_WITHIN must be numbers encoded as minimal byte arrays
  • The nPubKeys and nKeysCount arguments to OP_CHECKMULTISIG and OP_CHECKMULTISIGVERIFY must be numbers encoded as minimal byte arrays.
  • The second argument to OP_SPLIT must be a number encoded as minimal byte array.
  • The first argument to OP_NUM2BIN must be a number encoded as minimal byte array.
  • Maybe some other things, I didn't look in full detail and the lack of tests does not give any help here.

The proposed rule does the following:

  • The scriptSig must have canonical form (only pushes, using minimal push forms) for producing a given stack.

See T667