Accept any sequence of PUSHDATAs in OP_RETURN outputs #5079

petertodd · 2014-10-13T14:26:43Z

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.

TheBlueMatt · 2014-10-14T07:47:22Z

src/script/standard.cpp

+    // script.
+    size_t nMaxOpReturnRelay = GetBoolArg("-datacarrier", true) ? MAX_OP_RETURN_RELAY : 1;
+    if (scriptPubKey.IsUnspendable() && 1 <= scriptPubKey.size() // protect the begin() + 1 below
+                                     && scriptPubKey.size() <= nMaxOpReturnRelay)


This conditional could be written much more clearly. It is not clear that IsUnspendable() is actually checking for OP_RETURN at the beginning.

TheBlueMatt · 2014-10-14T07:47:33Z

Aside from that one thing, utACK.

TheBlueMatt · 2014-10-14T17:20:25Z

src/script/standard.cpp

+    // byte passes the IsPushOnly() test we don't care what exactly is in the
+    // script.
+    size_t nMaxOpReturnRelay = GetBoolArg("-datacarrier", true) ? MAX_OP_RETURN_RELAY : 1;
+    if (1 <= scriptPubKey.size() && scriptPubKey[0] == OP_RETURN


1 smaller than scriptPubKey.size, it is. Can we avoid yoda conditionals?

This is scriptPubKey.IsUnspendable(), btw

@theuni Lol, that's what the code was prior to @TheBlueMatt asking me to change it to something explicit. :)

@TheBlueMatt I've got no strong feelings about Star Wars, so yoda removed.

TheBlueMatt · 2014-10-14T17:20:53Z

WIth or without yodaspeak, utACK.

sipa · 2014-10-14T19:10:02Z

src/script/standard.cpp

+    if (1 <= scriptPubKey.size() && scriptPubKey[0] == OP_RETURN
+                                 && scriptPubKey.size() <= nMaxOpReturnRelay)
+    {
+        CScript scriptData(scriptPubKey.begin() + 1, scriptPubKey.end());


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?

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.

could easily adapt IsPushOnly() to be IsPushOnly(int startingAt), or even an iterator.

@TheBlueMatt Any ideas on the best way of doing that?

devrandom · 2014-10-15T03:37:06Z

You could add a IsPushOnly with an arg as follows:

bool CScript::IsPushOnly() const
{
  return this->IsPushOnly(begin());
}

bool CScript::IsPushOnly(const_iterator pc) const
{
...

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

Also add the new definition in script.h

sipa · 2014-10-15T03:39:22Z

Sounds like a good suggestion, @devrandom.

petertodd · 2014-11-04T17:44:52Z

@devrandom I implemented your suggestion re: IsPushOnly() and rebased.

devrandom · 2014-11-04T17:52:27Z

+1

Flavien · 2014-11-13T19:08:38Z

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.

luke-jr · 2014-11-13T19:19:14Z

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

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.

Allows IsPushOnly() to be applied to just part of the script for OP_RETURN outputs.

petertodd · 2014-11-13T21:10:04Z

Rebased

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

petertodd · 2014-11-13T21:10:59Z

@Flavien Yeah, that belongs in a separate pull-req. Also, if you want this merged, do please test it and ACK...

devrandom · 2015-02-04T23:58:24Z

tested ACK

maraoz · 2015-04-08T13:52:55Z

concept ACK

jtimon · 2015-04-27T17:21:44Z

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: jtimon/bitcoin@pr_5079_rebased...jtimon:pr_5079

Apart from that, utACK.

petertodd · 2015-05-03T22:21:00Z

@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?

jtimon · 2015-05-07T11:06:59Z

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.

petertodd · 2015-05-09T19:58:52Z

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.

dexX7 · 2015-06-04T02:02:22Z

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

This seems very useful in my opinon.

dexX7 · 2015-06-13T18:19:44Z

@jtimon: jtimon/bitcoin@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.

jtimon · 2015-07-08T22:55:51Z

src/script/standard.cpp

+    if (scriptPubKey.size() >= 1 && scriptPubKey[0] == OP_RETURN
+                                 && scriptPubKey.size() <= nMaxDatacarrierBytes+2) // to account for the pushdata opcodes
+    {
+        if (scriptPubKey.IsPushOnly(scriptPubKey.begin()+1))


This can be incorporated to the previous if, just another &&. What makes this condition different from the others?

jtimon · 2015-07-12T14:17:29Z

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

laanwj · 2015-07-21T08:44:47Z

What still needs to happen here?
Edit: re-triggered travis to see the test failures.

jtimon · 2015-07-21T09:13:50Z

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

dexX7 · 2015-07-21T11:12:38Z

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

jtimon · 2015-07-21T11:20:08Z

@dexX7 Oh, I see, it was just to inform. Thanks.

jgarzik · 2015-09-15T17:23:56Z

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

dcousens · 2015-09-19T01:00:20Z

concept / utACK, though waiting on @jtimon's comments to be addressed. +1 on @jgarzik's suggestions.

jtimon · 2015-09-19T04:46:39Z

Was this rebased? There was already a rebased version (with my nits fixed) in #6424 ...

laanwj · 2015-10-01T16:14:32Z

Closing in favor of @jtimon's #6424

TheBlueMatt reviewed Oct 14, 2014
View reviewed changes

petertodd force-pushed the op-return-accept-multiple-pushdatas branch from 8d67967 to ba78c76 Compare October 14, 2014 11:20

TheBlueMatt reviewed Oct 14, 2014
View reviewed changes

sipa reviewed Oct 14, 2014
View reviewed changes

petertodd force-pushed the op-return-accept-multiple-pushdatas branch from ba78c76 to f386100 Compare October 14, 2014 20:31

petertodd force-pushed the op-return-accept-multiple-pushdatas branch from f386100 to 6865d23 Compare November 4, 2014 17:44

petertodd added 2 commits November 13, 2014 16:05

Make TX_SCRIPTHASH clear vSolutionsRet first

19c0afa

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.

Add IsPushOnly(const_iterator pc)

04a6275

Allows IsPushOnly() to be applied to just part of the script for OP_RETURN outputs.

petertodd force-pushed the op-return-accept-multiple-pushdatas branch from 6865d23 to c263701 Compare November 13, 2014 21:09

laanwj added the TX fees and policy label Dec 5, 2014

jgarzik mentioned this pull request Jul 8, 2015

Allow OP_RETURN with two PUSHDATA #6396

Closed

jtimon reviewed Jul 8, 2015
View reviewed changes

jtimon mentioned this pull request Jul 12, 2015

Policy: Decouple Solver() from nMaxDatacarrierBytes (for free) #6424

Merged

laanwj closed this Oct 1, 2015

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Accept any sequence of PUSHDATAs in OP_RETURN outputs #5079

Accept any sequence of PUSHDATAs in OP_RETURN outputs #5079

Uh oh!

Conversation

petertodd commented Oct 13, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 14, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 14, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devrandom commented Oct 15, 2014

Uh oh!

sipa commented Oct 15, 2014

Uh oh!

petertodd commented Nov 4, 2014

Uh oh!

devrandom commented Nov 4, 2014

Uh oh!

Flavien commented Nov 13, 2014

Uh oh!

luke-jr commented Nov 13, 2014

Uh oh!

petertodd commented Nov 13, 2014

Uh oh!

petertodd commented Nov 13, 2014

Uh oh!

devrandom commented Feb 4, 2015

Uh oh!

maraoz commented Apr 8, 2015

Uh oh!

jtimon commented Apr 27, 2015

Uh oh!

petertodd commented May 3, 2015

Uh oh!

jtimon commented May 7, 2015

Uh oh!

petertodd commented May 9, 2015

Uh oh!

dexX7 commented Jun 4, 2015

Uh oh!

dexX7 commented Jun 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Jul 12, 2015

Uh oh!

laanwj commented Jul 21, 2015

Uh oh!

jtimon commented Jul 21, 2015

Uh oh!

dexX7 commented Jul 21, 2015

Uh oh!

jtimon commented Jul 21, 2015

Uh oh!

jgarzik commented Sep 15, 2015

Uh oh!

dcousens commented Sep 19, 2015

Uh oh!

jtimon commented Sep 19, 2015

Uh oh!