Option to consider bare multisig scripts in TX outputs non-standard #3939

jgarzik commented at 2:19 PM on March 22, 2014: contributor

First and foremost, this defaults to OFF. Perhaps consider changing the default if the community feels strongly about the issue and/or a security issue strongly encourages a change.

This option lets a node consider such transactions non-standard.

This impacts parties using CheckMultiSig for multisig and parties using CheckMultiSig for raw data transport.

This also makes a sigops-related issue harder to express in practice.

WARNING: An issue @gavinandresen raised has not yet been tested. Do not pull at this time.

luke-jr commented at 3:52 AM on March 24, 2014: member

P2SH tests should probably be updated to avoid using bare multisig in IsStandard tests (not the intention of the test): c5127b7

jgarzik commented at 9:35 PM on May 20, 2014: contributor

rebased

laanwj commented at 8:56 AM on May 21, 2014: member

This option needs documentation in HelpMessage() (or is it ondocumented on purpose?)

jgarzik commented at 1:09 PM on May 21, 2014: contributor

+1 @laanwj yes, needs docs

in src/script.h:None in d20faca47e outdated

 190 | @@ -191,6 +191,7 @@ enum
 191 |      SCRIPT_VERIFY_LOW_S     = (1U << 2), // enforce low S values (<n/2) in signatures (depends on STRICTENC)
 192 |      SCRIPT_VERIFY_NOCACHE   = (1U << 3), // do not store results in signature cache (but do query it)
 193 |      SCRIPT_VERIFY_NULLDUMMY = (1U << 4), // verify dummy stack item consumed by CHECKMULTISIG is of zero-length
 194 | +    SCRIPT_VERIFY_BARE_MSIG_OK = (1U << 5), // are bare msig outputs permitted?

Diapolo commented at 1:26 PM on May 21, 2014:

Nit: Nowhere else we use msig, can you use multisig?

jgarzik commented at 1:30 PM on May 21, 2014:

@Diapolo I don't mind s/msig/multisig/ but I do dislike long symbols. Maybe RELAY_MULTISIG or somesuch.

petertodd commented at 11:42 PM on June 24, 2014:

I think a SCRIPT_VERIFY flag is the wrong place for this. The other flags either are consensus critical, or may be in a plausible future soft-fork. They also are all evaluated during script execution. Multisig outputs on the other hand are just an IsStandard() rule and are evaluated via the Solver() template matcher. Additionally being purely a policy rule they would be inappropriate in a libconsensuscore library, again unlike the other script verification flags.

jgarzik commented at 12:28 AM on June 25, 2014:

That's fair.

sipa commented at 12:40 AM on June 25, 2014:

I agree with @petertodd here.

laanwj commented at 6:21 AM on June 25, 2014:

Yes, IsStandard needs a separate set of flags.

in src/init.cpp:None in d20faca47e outdated

 581 | @@ -582,6 +582,7 @@ bool AppInit2(boost::thread_group& threadGroup)
 582 |              InitWarning(_("Warning: -paytxfee is set very high! This is the transaction fee you will pay if you send a transaction."));
 583 |      }
 584 |      bSpendZeroConfChange = GetArg("-spendzeroconfchange", true);
 585 | +    fIsBareMultisigStd = GetArg("-permitbaremultisig", true);

laanwj commented at 7:58 AM on May 27, 2014:

This appears to be inside an #ifdef WALLET_ENABLED. This would mean that the option cannot be disabled if compiled without wallet.

laanwj commented at 2:48 PM on July 9, 2014: member

Needs rebase and nit fix.

Introduce option to disable relay/mining of bare multisig scripts in TX outputs

First and foremost, this defaults to OFF.

This option lets a node consider such transactions non-standard,
meaning they will not be relayed or mined by default, but other miners
are free to mine these as usual.

3da434a2ef

BitcoinPullTester commented at 3:41 PM on July 18, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p3939_3da434a2ef9ac76a0ad4a33921773a9ac8f10ab7/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

jgarzik commented at 6:30 PM on July 18, 2014: contributor

All nits addressed.

petertodd commented at 7:07 PM on July 18, 2014: contributor

ACK

sipa commented at 7:58 PM on July 18, 2014: member

Untested ACK

laanwj commented at 9:14 PM on July 18, 2014: member

Untested ACK

laanwj merged this on Jul 18, 2014

laanwj closed this on Jul 18, 2014

laanwj referenced this in commit ff1fe669d4 on Jul 18, 2014

jgarzik deleted the branch on Aug 24, 2014

DrahtBot locked this on Sep 8, 2021