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

jtimon commented at 2:13 pm on July 12, 2015: contributor

This is a rebased and modified version of #5079. If @petertodd accepts my suggestions (specially the one about taking to opportunity of decoupling Solver() from nMaxDatacarrierBytes), I can close this PR. If he doesn’t and #5079 gets merged first, this can be rebased to do the decoupling.

in src/policy/policy.cpp: in df698329c3 outdated

48@@ -49,7 +49,9 @@ bool IsStandard(const CScript& scriptPubKey, txnouttype& whichType)
49             return false;
50         if (m < 1 || m > n)
51             return false;
52-    }
53+    } else if (whichType == TX_NULL_DATA &&
54+               (!GetBoolArg("-datacarrier", true) || scriptPubKey.size() > nMaxDatacarrierBytes))

dexX7 commented at 2:44 pm on July 12, 2015:

If nMaxDatacarrierBytes is 80, wouldn’t the effective size only be 79 77 byte now, due to the op code, which is part of scriptPubKey?

jtimon commented at 4:23 pm on July 12, 2015:

It is clearer if you read commit by commit: https://github.com/jtimon/bitcoin/commit/dbc241dfd51623da770725909bb329f2188a5cb1 https://github.com/jtimon/bitcoin/commit/904d59368899c9e522325e1925b500b3f9e34ee9 https://github.com/jtimon/bitcoin/commit/984f7b4796f59b79da7a9074245b435007fb16a9

The simplest way to maintain the current “effective size” of 80 is to simply add 3 to MAX_OP_RETURN_RELAY. But the 3 is quite arbitrary: the number of pushes depends on the size of the data. So I think it’s simpler to count the op_return and the pushes as part of the data: simply reduce the “effective size” from 80 to 77 and change the tests instead of the constant (as in the last commit).

dexX7 commented at 5:38 pm on July 12, 2015:

Thanks, I understand your reasoning, and even though it’s a bit arbitrary, this may have an impact on applications. For example, a block header doesn’t fit in nicely anymore. (I don’t really care about those few bytes, but I recall reading about this use case somewhere..)

jtimon commented at 6:02 pm on July 12, 2015:

Yes, I know the last commit will probably be discarded (that’s why I put it the last), but I just want it to be considered as an option. I’m totally fine with 83 in the constant.

jgarzik commented at 1:19 pm on September 16, 2015:

It is a bit disappointing that GetBoolArg() use from prior code is continued. Need a state-and-options struct.

jtimon commented at 6:32 pm on September 17, 2015:

It still needs to be a global. Later it can be an attribute of a global object, like in https://github.com/bitcoin/bitcoin/commit/4c412c994b9eccc0fdb9df3bfaea464afa0f90f5 but that global object can be “moved up” as a paramter (see https://github.com/bitcoin/bitcoin/commit/ddcb6a131791fc848036db450b0a75a55aae8010 ).

jtimon force-pushed on Jul 12, 2015

laanwj commented at 7:06 am on July 13, 2015: member

Concept ACK - I’ve always thought policy doesn’t belong in the Solver, which would be expected to determine what kind of script something is, then the policy to judge based on its output.

laanwj added the label Refactoring on Jul 13, 2015

petertodd commented at 3:25 pm on July 17, 2015: contributor

utACK

petertodd commented at 3:27 pm on July 17, 2015: contributor

If you want to go ahead and rebase, that’d be great - I don’t have the time/funding to do anything with this.

jtimon force-pushed on Jul 17, 2015

jtimon commented at 7:35 pm on July 17, 2015: contributor

Squashed everything but the last commit. @petertodd Thanks for the review. As long as there are reviewers with time to re-review, I’m happy to re-rebase.

Thoughts on the last commit again?

petertodd commented at 5:15 pm on July 18, 2015: contributor

@jtimon I would strongly recommend you change the constant rather than the unit tests - people end up designing stuff based on things like “most miners will mine 80 byte op_return” so changing it can be highly disruptive.

jtimon force-pushed on Jul 21, 2015

jtimon force-pushed on Aug 21, 2015

jtimon commented at 2:21 am on August 21, 2015: contributor

No input from anyone else besides @petertodd for a while, so I’ve just removed the last commit “fixup! Better yet, change the tests instead of the default constant” like he suggested.

jtimon force-pushed on Aug 25, 2015

in src/test/transaction_tests.cpp: in 5ca12eec66 outdated

349@@ -350,12 +350,29 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
350     t.vout[0].scriptPubKey = CScript() << OP_1;
351     BOOST_CHECK(!IsStandardTx(t, reason));
352 
353-    // 80-byte TX_NULL_DATA (standard)
354+    // MAX_OP_RETURN_RELAY-byte TX_NULL_DATA (standard)

dcousens commented at 2:36 pm on September 16, 2015:

MAX_OP_RETURN_RELAY length bytes, the hyphen seems odd here

dcousens commented at 2:38 pm on September 16, 2015: contributor

concept/utACK

jtimon commented at 12:47 pm on September 22, 2015: contributor

ping

sipa commented at 4:34 pm on September 22, 2015: member

Untested ACK.

laanwj commented at 4:04 pm on October 1, 2015: member

utACK, needs rebase

laanwj added the label TX fees and policy on Oct 1, 2015

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.

6a07eb676a

Add IsPushOnly(const_iterator pc)

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

5d8709c3b7

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

da894ab5da

jtimon force-pushed on Oct 1, 2015

jtimon commented at 4:42 pm on October 1, 2015: contributor

Rebased

laanwj merged this on Oct 1, 2015

laanwj closed this on Oct 1, 2015

laanwj referenced this in commit cd78c2a421 on Oct 1, 2015

laanwj commented at 9:45 am on October 2, 2015: member

Forgot this, but @petertodd sending a tweet remindede me of it: this needs mention in doc/release-notes.md.

petertodd referenced this in commit 2817dc9482 on Oct 3, 2015

petertodd commented at 1:00 pm on October 3, 2015: contributor

@laanwj Fixed: #6751

petertodd referenced this in commit c66472bf11 on Oct 3, 2015

petertodd referenced this in commit 9204930101 on Oct 5, 2015

laanwj referenced this in commit b6f3a4eb19 on Oct 6, 2015

luke-jr referenced this in commit 420e93c71f on Oct 15, 2015

NicolasDorier commented at 2:10 pm on November 9, 2015: contributor

What was the rational about allowing any push data in the OP_RETURN instead of just allowing any OP ? Why not accepting any operation ? After all, since the rest is never executed, we can save bytes by allowing any OP to be used, even invalid one. (Invalid OPs are standard if they are not executed)

jtimon commented at 1:54 pm on November 10, 2015: contributor

@NicolasDorier That’s a very good question. In the original PR #5079 @petertodd says “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.” So it seems that this additional limitation of in the standard policy of allowing only unspendable outputs that consist only on pushdata instructions (previously only one) seems to be related to bloom filters, presumable used in some protocol based on “proof of publication”. But I personally don’t know how these bloom filters are used or even for what. Maybe @petertodd can explain more.

NicolasDorier commented at 2:16 pm on November 10, 2015: contributor

bloom filter is a way to get notified about content in OP_RETURN by SPV wallets with configurable compromise on privacy.

So this is not the reason, accepting all OP do not prevent you to use PushData in your OP_RETURN instead of garbage if you really need SPV capabilities.

jtimon commented at 2:27 pm on November 10, 2015: contributor

Thanks for the clarification, I’m thinking that I should have moved that check to policy::IsStandard() as well. But I don’t really understand what is for, so I would like to hear an answer to your question as well.

petertodd commented at 7:58 pm on November 10, 2015: contributor

What was the rational about allowing any push data in the OP_RETURN instead of just allowing any OP ? Why not accepting any operation ? After all, since the rest is never executed, we can save bytes by allowing any OP to be used, even invalid one.

That’s an clever observation, and I had the same idea too!

But unfortunately sigops are counted in outputs, which makes it tricky to be sure we’re not opening up a some kind of DoS attack by allowing anything in OP_RETURN. Easiest was just to restrict this to only pushdata’s, which is pretty much just as good anyway.

NicolasDorier commented at 5:56 am on November 11, 2015: contributor

Ok, did not thought about sigops :( Will the SIG OP rule will change one day ? Something like a limit count per script ? I guess we can already be DoS by that now arbitrary redeem script are standard, and, most likely, miners does not even check the number of sig in a particular redeem.

sipa commented at 6:13 am on November 11, 2015: member

@NicolasDorier That would be hard fork. I don’t think anyone here has to authority to say whether or not that will change.

NicolasDorier commented at 6:15 am on November 11, 2015: contributor

So what I mean is more, is there proposal being written to fix the issue right now ? (or even some args that the user/miner can turn on if we get spammed attacked with that so it is possible to refuse script with too much sig, we’ll get some mitigation)

zkbot referenced this in commit 63c3d1ec94 on Dec 17, 2019

furszy referenced this in commit a5265a4db4 on Jun 27, 2020

DrahtBot locked this on Sep 8, 2021

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