Accept any sequence of PUSHDATAs in OP_RETURN outputs #5079

pull petertodd wants to merge 3 commits into bitcoin:master from petertodd:op-return-accept-multiple-pushdatas changing 4 files +42 −15
  1. petertodd commented at 2:26 pm on October 13, 2014: contributor

    Previously only one PUSHDATA was allowed, needlessly limiting applications such as matching OP_RETURN contents with bloom filters that operate on a per-PUSHDATA level. Now any combination that passes IsPushOnly() is allowed, so long as the total size of the scriptPubKey is less than 42 bytes. (unchanged modulo non-minimal PUSHDATA encodings)

    Also, this fixes the odd bug where previously the PUSHDATA could be replaced by any single opcode, even sigops consuming opcodes such as CHECKMULTISIG. (20 sigops!)

    Finally, fix a weird corner-case bug is also fixed for TX_SCRIPTHASH outputs.

  2. in src/script/standard.cpp: in 8d67967587 outdated 
    64+    // So long as script passes the IsUnspendable() test and all but the first
65+    // byte passes the IsPushOnly() test we don't care what exactly is in the
66+    // script.
67+    size_t nMaxOpReturnRelay = GetBoolArg("-datacarrier", true) ? MAX_OP_RETURN_RELAY : 1;
68+    if (scriptPubKey.IsUnspendable() && 1 <= scriptPubKey.size() // protect the begin() + 1 below
69+                                     && scriptPubKey.size() <= nMaxOpReturnRelay)
    TheBlueMatt commented at 7:47 am on October 14, 2014:
    This conditional could be written much more clearly. It is not clear that IsUnspendable() is actually checking for OP_RETURN at the beginning.
    petertodd commented at 11:20 am on October 14, 2014:
    Fixed
  3. TheBlueMatt commented at 7:47 am on October 14, 2014: member
    Aside from that one thing, utACK.
  4. petertodd force-pushed on Oct 14, 2014
  5. in src/script/standard.cpp: in ba78c7640a outdated 
    63+    //
64+    // So long as script passes the IsUnspendable() test and all but the first
65+    // byte passes the IsPushOnly() test we don't care what exactly is in the
66+    // script.
67+    size_t nMaxOpReturnRelay = GetBoolArg("-datacarrier", true) ? MAX_OP_RETURN_RELAY : 1;
68+    if (1 <= scriptPubKey.size() && scriptPubKey[0] == OP_RETURN
    TheBlueMatt commented at 5:20 pm on October 14, 2014:
    1 smaller than scriptPubKey.size, it is. Can we avoid yoda conditionals?
    theuni commented at 8:25 pm on October 14, 2014:
    This is scriptPubKey.IsUnspendable(), btw
    petertodd commented at 8:27 pm on October 14, 2014:
    @theuni Lol, that’s what the code was prior to @TheBlueMatt asking me to change it to something explicit. :)
    petertodd commented at 8:32 pm on October 14, 2014:
    @TheBlueMatt I’ve got no strong feelings about Star Wars, so yoda removed.
  6. TheBlueMatt commented at 5:20 pm on October 14, 2014: member
    WIth or without yodaspeak, utACK.
  7. in src/script/standard.cpp: in ba78c7640a outdated 
    66+    // script.
67+    size_t nMaxOpReturnRelay = GetBoolArg("-datacarrier", true) ? MAX_OP_RETURN_RELAY : 1;
68+    if (1 <= scriptPubKey.size() && scriptPubKey[0] == OP_RETURN
69+                                 && scriptPubKey.size() <= nMaxOpReturnRelay)
70+    {
71+        CScript scriptData(scriptPubKey.begin() + 1, scriptPubKey.end());
    sipa commented at 7:10 pm on October 14, 2014:
    Making a full copy (apart from one byte) of every script in every transaction being relayed seems wasteful. Why is there a requirement that it’s push only?
    petertodd commented at 7:25 pm on October 14, 2014:
    sigops are counted even in scriptPubKeys, so accepting a transaction with an OP_RETURN output containing a bunch of CHECKMULTISIGS - for instance - would use up a lot of your per-block sigop quota.
    TheBlueMatt commented at 7:44 pm on October 14, 2014:
    could easily adapt IsPushOnly() to be IsPushOnly(int startingAt), or even an iterator.
    petertodd commented at 8:03 pm on October 14, 2014:
    @TheBlueMatt Any ideas on the best way of doing that?
  8. petertodd force-pushed on Oct 14, 2014
  9. devrandom commented at 3:37 am on October 15, 2014: none

    You could add a IsPushOnly with an arg as follows:

    0bool CScript::IsPushOnly() const
1{
2  return this->IsPushOnly(begin());
3}
4
5bool CScript::IsPushOnly(const_iterator pc) const
6{
7...

    and then call it with begin()+1 in the OP_RETURN flow.

    Also add the new definition in script.h

  10. sipa commented at 3:39 am on October 15, 2014: member
    Sounds like a good suggestion, @devrandom.
  11. petertodd force-pushed on Nov 4, 2014
  12. petertodd commented at 5:44 pm on November 4, 2014: contributor
    @devrandom I implemented your suggestion re: IsPushOnly() and rebased.
  13. devrandom commented at 5:52 pm on November 4, 2014: none
    +1
  14. Flavien commented at 7:08 pm on November 13, 2014: contributor
    Since the OP_RETURN doomsday some had predicted did not happen, could we also increase the space in the OP_RETURN? 20 bytes would already go a long way.
  15. luke-jr commented at 7:19 pm on November 13, 2014: member
    @Flavien 0.10 is adding a -datacarriersize option to make configuring the size easy. Changing the default value is IMO outside the scope of this PR and should be proposed independently.
  16. Make TX_SCRIPTHASH clear vSolutionsRet first 
    Previously unlike other transaction types the TX_SCRIPTHASH would not
clear vSolutionsRet, which means that unlike other transaction types if
it was called twice in a row you would get the result of the previous
invocation as well.
    19c0afa744
  17. Add IsPushOnly(const_iterator pc) 
    Allows IsPushOnly() to be applied to just part of the script for
OP_RETURN outputs.
    04a62754d6
  18. petertodd force-pushed on Nov 13, 2014
  19. petertodd commented at 9:10 pm on November 13, 2014: contributor
    Rebased
  20. Accept any sequence of PUSHDATAs in OP_RETURN outputs 
    Previously only one PUSHDATA was allowed, needlessly limiting
applications such as matching OP_RETURN contents with bloom filters that
operate on a per-PUSHDATA level. Now any combination that passes
IsPushOnly() is allowed, so long as the total size of the scriptPubKey
is less than 42 bytes. (unchanged modulo non-minimal PUSHDATA encodings)

Also, this fixes the odd bug where previously the PUSHDATA could be
replaced by any single opcode, even sigops consuming opcodes such as
CHECKMULTISIG. (20 sigops!)
    c26370192a
  21. petertodd commented at 9:10 pm on November 13, 2014: contributor
    @Flavien Yeah, that belongs in a separate pull-req. Also, if you want this merged, do please test it and ACK…
  22. laanwj added the label TX fees and policy on Dec 5, 2014
  23. devrandom commented at 11:58 pm on February 4, 2015: none
    tested ACK
  24. maraoz commented at 1:52 pm on April 8, 2015: contributor
    concept ACK
  25. jtimon commented at 5:21 pm on April 27, 2015: contributor

    If this was rebased today, it wouldn’t pass the tests. I rebased and fixed that. You were adding 2 for the op_return and the op_pushdata, but you also have to add the number expressing the size and the size of that number itself depends on the pushdata type used. So replacing 2 with 3 to fix the tests is not a very convincing option IMO. I would prefer to change the default from 80 to 83 and say that it also counds the op_return and whatever pushdata ops you need.

    You were also altering the behaviour of the -datacarrier option and I fixed that too. Ultimately I think we could get rid of that option since users can just express the same desire using -datacarriersize=0.

    Also since you are touching this part of the code already, it is trivial to move the nMaxDatacarrierBytes checks from Solver to IsStandard where it belongs (to be moved with #6068 to policy/policy).

    You can read and get my suggestions (diff only from what you already had, which had a clean rebase) here: https://github.com/jtimon/bitcoin/compare/pr_5079_rebased...jtimon:pr_5079

    Apart from that, utACK.

  26. petertodd commented at 10:21 pm on May 3, 2015: contributor
    @jtimon I disagree with some of the things you say above in minor ways, but the big picture is that you’re version looks fine to me. How about you submit it as a separate pull-req and I’ll close this one?
  27. jtimon commented at 11:06 am on May 7, 2015: contributor
    Care to be more specific about where you disagree? Sure, I could replace this PR. But if I do that, I would only do it after #6068 (was #5595) if that ever gets merged.
  28. petertodd commented at 7:58 pm on May 9, 2015: contributor

    Oh, actually re-reeading it I think it’s all fine.

    Well, it’s up to you, but I’d see this as separate to #6068, in the sense that it doesn’t need to be a blocker.

  29. dexX7 commented at 2:02 am on June 4, 2015: contributor

    matching OP_RETURN contents with bloom filters that operate on a per-PUSHDATA level

    This seems very useful in my opinon.

  30. dexX7 commented at 6:19 pm on June 13, 2015: contributor

    @jtimon: https://github.com/jtimon/bitcoin/compare/pr_5079_rebased...jtimon:pr_5079

    If the size check is removed from Solver(), then there are a few consequences.

    The solver is still successful, even if payloads are larger than nMaxDatacarrierBytes. On the first glimpse this has no negative effect, but since typeRet is set TX_NULL_DATA in these cases, too, then it also affects ExtractDestinations(), and ultimately ScriptPubKeyToJSON() plus friends.

    The RPC calls "getrawtransaction", "decoderawtransaction" and "decodescript" then no longer return "type" : "nonstandard", but "nulldata".

    Nevertheless, I think this is the right thing to do, and it would be in line with the behavior related to non-standard m-of-n multisig scripts.

  31. in src/script/standard.cpp: in c26370192a 
    71+    // byte passes the IsPushOnly() test we don't care what exactly is in the
72+    // script.
73+    if (scriptPubKey.size() >= 1 && scriptPubKey[0] == OP_RETURN
74+                                 && scriptPubKey.size() <= nMaxDatacarrierBytes+2) // to account for the pushdata opcodes
75+    {
76+        if (scriptPubKey.IsPushOnly(scriptPubKey.begin()+1))
    jtimon commented at 10:55 pm on July 8, 2015:
    This can be incorporated to the previous if, just another &&. What makes this condition different from the others?
  32. in src/script/standard.cpp: in c26370192a 
    50@@ -51,13 +51,10 @@ bool Solver(const CScript& scriptPubKey, txnouttype& typeRet, vector<vector<unsi
51 
52         // Sender provides N pubkeys, receivers provides M signatures
53         mTemplates.insert(make_pair(TX_MULTISIG, CScript() << OP_SMALLINTEGER << OP_PUBKEYS << OP_SMALLINTEGER << OP_CHECKMULTISIG));
54-
55-        // Empty, provably prunable, data-carrying output
56-        if (GetBoolArg("-datacarrier", true))
    jtimon commented at 11:01 pm on July 8, 2015:
    I agree with getting rid of the -datacarrier=0 option, it’s just a redundant way to say -datacarriersize=0. But if you’re removing the option, you should also get rid of https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L400 and raise a deprecated error when -datacarrier is used before https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L872
  33. in src/script/standard.cpp: in c26370192a 
    69+    //
70+    // So long as script passes the IsUnspendable() test and all but the first
71+    // byte passes the IsPushOnly() test we don't care what exactly is in the
72+    // script.
73+    if (scriptPubKey.size() >= 1 && scriptPubKey[0] == OP_RETURN
74+                                 && scriptPubKey.size() <= nMaxDatacarrierBytes+2) // to account for the pushdata opcodes
    jtimon commented at 11:09 pm on July 8, 2015:

    The scriptPubKey.size() <= nMaxDatacarrierBytes+2 check belongs in policy/policy.cpp, specifically, in line https://github.com/bitcoin/bitcoin/blob/master/src/policy/policy.cpp#L52 , replacing that line with

    0    } else if (whichType == TX_NULL_DATA && scriptPubKey.size() > nMaxDatacarrierBytes)
1        return false;

    To do that you will need to also move nMaxDatacarrierBytes and MAX_OP_RETURN_RELAY to policy/policy.o. That would be cleaner after #6068 since the global would be moved only once directly to a CStandardPolicy attribute. But moving the condition and global already makes sense IMO.

    jtimon commented at 11:10 pm on July 8, 2015:
    Also, nMaxDatacarrierBytes is only changed from its default value on init, again, why nMaxDatacarrierBytes+2 here instead of just nMaxDatacarrierBytes? If it’s to maintain the current default policy, you can just add that constant 2 to MAX_OP_RETURN_RELAY
    dcousens commented at 0:59 am on September 19, 2015:
    +1 on @jtimon’s comments
  34. in src/test/transaction_tests.cpp: in c26370192a 
    360+    // OP_RESERVED *is* considered to be a PUSHDATA type opcode by IsPushOnly()!
361+    t.vout[0].scriptPubKey = CScript() << OP_RETURN << OP_RESERVED << -1 << 0 << ParseHex("01") << 2 << 3 << 4 << 5 << 6 << 7 << 8 << 9 << 10 << 11 << 12 << 13 << 14 << 15 << 16;
362+    BOOST_CHECK(IsStandardTx(t, reason));
363+    t.vout[0].scriptPubKey = CScript() << OP_RETURN << 0 << ParseHex("01") << 2 << ParseHex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
364+    BOOST_CHECK(IsStandardTx(t, reason));
365+
    jtimon commented at 11:19 pm on July 8, 2015:

    What about adding…

    0BOOST_CHECK_EQUAL(MAX_OP_RETURN_RELAY, t.vout[0].scriptPubKey.size());

    …or similar somewhere here?

  35. jtimon commented at 2:17 pm on July 12, 2015: contributor
    Please, rebase. Even if it’s not strictly necessary, MAX_OP_RETURN_RELAY has changed its value and IsStandard() has moved to policy/policy. It will also become easier to compare with #6424, which I just opened (it’s a rebased version of this with my nits on top, ready to be squashed).
  36. laanwj commented at 8:44 am on July 21, 2015: member
    What still needs to happen here? Edit: re-triggered travis to see the test failures.
  37. jtimon commented at 9:13 am on July 21, 2015: contributor

    @laanwj just merge #6424 instead? Well, after I get some more review and I remove the last “fixup?” commit. @dexX7

    The RPC calls “getrawtransaction”, “decoderawtransaction” and “decodescript” then no longer return “type” : “nonstandard”, but “nulldata”.

    Yes, and there’s no problem with that.

    Nevertheless, I think this is the right thing to do, and it would be in line with the behavior related to non-standard m-of-n multisig scripts.

    So do you agree or not, I’m confused…

  38. dexX7 commented at 11:12 am on July 21, 2015: contributor

    @jtimon: So do you agree or not, I’m confused…

    With #5079 (comment) I was just pointing out that your change has an impact on how the scripts are labeled via RPC, but I think it’s fine, and I agree with:

    @jtimon: Yes, and there’s no problem with that.

  39. jtimon commented at 11:20 am on July 21, 2015: contributor
    @dexX7 Oh, I see, it was just to inform. Thanks.
  40. jgarzik commented at 5:23 pm on September 15, 2015: contributor

    ut ACK as-is

    Suggested improvement:

    • first “if” checks only for OP_RETURN
    • second “if” detects TX_NULL_DATA
    • otherwise no need to scan templates - it is an OP_RETURN - we know it is “unspendable+garbage” and therefore TX_NONSTANDARD
  41. dcousens commented at 1:00 am on September 19, 2015: contributor
    concept / utACK, though waiting on @jtimon’s comments to be addressed. +1 on @jgarzik’s suggestions.
  42. jtimon commented at 4:46 am on September 19, 2015: contributor
    Was this rebased? There was already a rebased version (with my nits fixed) in #6424 …
  43. laanwj commented at 4:14 pm on October 1, 2015: member
    Closing in favor of @jtimon’s #6424
  44. laanwj closed this on Oct 1, 2015
  45. MarcoFalke locked this on Sep 8, 2021


petertodd TheBlueMatt theuni sipa devrandom Flavien luke-jr maraoz jtimon dexX7 dcousens laanwj jgarzik

Labels
TX fees and policy

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 07:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me