policy: Add new constant MAX_STANDARD_MULTISIG_KEYS #6902

pull dajohi wants to merge 1 commits into bitcoin:master from dajohi:multisig_keys changing 2 files +9 −2

dajohi commented at 2:12 PM on October 29, 2015: contributor

No description provided.

in src/policy/policy.cpp:None in 2e6b4a9745 outdated

  43 | @@ -44,8 +44,9 @@ bool IsStandard(const CScript& scriptPubKey, txnouttype& whichType)
  44 |      {
  45 |          unsigned char m = vSolutions.front()[0];
  46 |          unsigned char n = vSolutions.back()[0];
  47 | -        // Support up to x-of-3 multisig txns as standard
  48 | -        if (n < 1 || n > 3)
  49 | +        // Support up to x-of-MAX_STANDARD_MULTISIG_KEYS multisig txns as
  50 | +        // standard

dcousens commented at 2:29 PM on October 29, 2015:

Maybe Support up to m-of-MAX_ ... (note, the m, not x)

dcousens commented at 2:30 PM on October 29, 2015: contributor

utACK
policy: Add new constant MAX_STANDARD_MULTISIG_KEYS aa42c87487
dajohi commented at 3:13 PM on October 29, 2015: contributor

@dcousens Agree. Updated.
laanwj added the label TX fees and policy on Oct 30, 2015
laanwj commented at 11:43 PM on October 30, 2015: member

Trivial ACK
gmaxwell commented at 11:49 PM on October 30, 2015: contributor

I think the result of this is confusing, if you go read policy.h post patch you think that bitcoin multisig will not allow more keys. But this isn't true, the restriction is specific to bare multisig-- something virtually no one cares about-- which was more or less apparent from the code in context but isn't otherwise; and in the updated version the new comment is arguably outright factually incorrect.

FWIW, the constant movement of trivial bits of code behavior across multiple files is very frustrating to me. The net result of this is that I am less likely to review future changes that changes around that test in IsStandard, because I have to go scatter and read many different files (which involves first figuring out where its defined) to have any idea of what the code actually does.
luke-jr commented at 11:52 PM on October 30, 2015: member

IMO this should become an option or bare multisig made unconditionally non-IsStandard.
sipa commented at 11:58 PM on October 30, 2015: member

@gmaxwell Seems that at least the comment should be fixed to say it's about bare multisig.

in src/policy/policy.h:None in aa42c87487

  25 | @@ -26,6 +26,12 @@ static const unsigned int MAX_P2SH_SIGOPS = 15;
  26 |  /** The maximum number of sigops we're willing to relay/mine in a single tx */
  27 |  static const unsigned int MAX_STANDARD_TX_SIGOPS = MAX_BLOCK_SIGOPS/5;
  28 |  /**
  29 | + * The maximum number of public keys allowed in a multi-signature transaction
  30 | + * output script to be considered standard.
  31 | + */
  32 | +static const unsigned int MAX_STANDARD_MULTISIG_KEYS = 3;

dcousens commented at 12:10 AM on October 31, 2015:

Maybe MAX_STANDARD_BARE_MULTISIG_KEYS, in light of @gmaxwell's comment

dcousens commented at 12:16 AM on October 31, 2015: contributor

Agreed the indirection of extracting constants can be frustrating for review, but, IMHO the only [equally balanced] alternative is to remove the constants themselves.

This is probably an exception though, given it is a 1:1 mapping and the motivation for extraction is solely a style choice. I think the underlying question should be: if we are encouraging [already motivated] users to modify these constants (by extracting them all to 1 place), should we be pointing them at tweaking constants, or tweaking the policy code itself?

If we have an answer to that, this could be a trivial ACK or NACK.
laanwj closed this on Nov 2, 2015
laanwj commented at 3:39 AM on November 2, 2015: member

Too controversial for a trivial change, closing
DrahtBot locked this on Sep 8, 2021

Contributors

Labels