Ignore datacarrier limits for dataless OP

petertodd commented at 1:55 pm on March 15, 2023: contributor

They don’t carry any data, so the -datacarrier/-datacarriersize options should not apply to them. Previously a transaction containing an OP_RETURN without data would be rejected if datacarrier was set to false, or the datacarriersize was set to zero.

DrahtBot commented at 1:55 pm on March 15, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	ajtowns
Concept ACK	ghost, Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27832 (doc: Clarify -datacarriersize, add -datacarriersize=2 tests by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

instagibbs commented at 2:04 pm on March 15, 2023: member

Is the use-case idea to be able to a little more compactly burn a dusty output to OP_RETURN?

petertodd commented at 2:10 pm on March 15, 2023: contributor

Is the use-case idea to be able to a little more compactly burn a dusty output to OP_RETURN?

Yes. For example, my dust-b-gone script uses a bare OP_RETURN output without data: https://github.com/petertodd/dust-b-gone/blob/95020b4d0280d5ad2c8973b2e981e5dd966e2315/merge-dust-txs.py#L70

To be clear, bare OP_RETURN is currently standard with the default datacarrier settings. All this pull-req changes is it’ll allow a bare OP_RETURN in the (rare) case that people are disabling datacarrier outputs, because a bare OP_RETURN carries no data.

ghost commented at 5:25 pm on March 16, 2023: none

Concept ACK

Some questions:

Previously a transaction containing an OP_RETURN without data would be rejected if datacarrier was set to false, or the datacarriersize was set to zero.

Minimum for datacarriersize is 1 so tx shouldn’t be rejected if datacarrier is also 1(default)?

All this pull-req changes is it’ll allow a bare OP_RETURN in the (rare) case that people are disabling datacarrier outputs, because a bare OP_RETURN carries no data.

It is still OP_RETURN output, right?

instagibbs commented at 5:29 pm on March 16, 2023: member

Honestly I think it’s time to discuss removing the datacarrier option altogether. Less code and args being threaded through for something that no one sets(?), especially in light of tapscript witness sizes.

ajtowns commented at 5:30 am on March 17, 2023: contributor

NACK. If someone wants to allow a bare OP_RETURN and nothing more they can set -datacarriersize=1. There’s no reason to ignore someone setting -nodatacarrier to indicate they don’t want OP_RETURN outputs in their mempool.

Sjors commented at 12:16 pm on March 17, 2023: member

I’m not sure how this is being used in practice, so perhaps the safest thing to do is what @ajtowns suggests: document clearly that -datacarriersize=1 means “OP_RETURN allowed, but no data” whereas -nodatacarrier means “no OP_RETURN allowed at all”.

The current doc for -datacarriersize= says:

0Maximum size of data in data carrier transactions we relay and mine (default: %u)

Maybe change it to:

0Maximum size of data in data carrier transactions we relay and mine (default: %u).
1Set to 1 to allow OP_RETURN transactions that create an unspendable output with no data.

glozow added the label TX fees and policy on Mar 23, 2023

Ayms commented at 5:32 pm on March 24, 2023: none

Well, reading this 10 times, I am not sur to see what it has to do with https://github.com/bitcoin/bitcoin/issues/27043

ghost commented at 7:30 pm on March 24, 2023: none

Well, reading this 10 times, I am not sur to see what it has to do with #27043

I assume this was a test run for real pull request which will be controversial for sure.

petertodd commented at 4:51 am on March 25, 2023: contributor

@ajtowns

NACK. If someone wants to allow a bare OP_RETURN and nothing more they can set -datacarriersize=1. There’s no reason to ignore someone setting -nodatacarrier to indicate they don’t want OP_RETURN outputs in their mempool.

Like I said, bare OP_RETURN outputs are not data carrying outputs. Treating them as such is misleading.

Second, if you are going to take that attitude, why do we even have the -nodatacarrier option? Setting -datacarriersize=0 will also prevent bare OP_RETURN outputs. Which actually suggests we should fix that redundancy if we want both options. @instagibbs Last time I checked a few months ago I could find examples of every single active mining pool mining data carrying OP_RETURN outputs. So yes, the -nodatacarrier option is likely entirely unused by ~100% of all hashing power. Secondly, since -nodatacarrier acts to censor transactions otherwise supported by ~100% of nodes, individual relay nodes setting it makes absolutely no difference to whether or not transactions reach miners. So I’m ok with just removing it.

Ayms commented at 1:17 pm on March 25, 2023: none

@1440000bytes

dont get mad else some people from this repo might write things about you. Getting mad is only reserved for a few people here

What do you mean and what is your problem? Write things about me?

ghost commented at 1:22 pm on March 25, 2023: none

@Ayms Edited. Sorry it wasn’t for you.

Sjors commented at 10:26 am on March 30, 2023: member

bare OP_RETURN outputs are not data carrying outputs

Agreed. But if you stick to this approach, please write a test.

Ignore datacarrier limits for dataless OP_RETURN outputs

They don't carry any data, so the -datacarrier/-datacarriersize options should
not apply to them. Previously a transaction containing an OP_RETURN without
data would be rejected if datacarrier was set to false, or the datacarriersize
was set to zero.

baa28315e7

petertodd force-pushed on Apr 27, 2023

petertodd commented at 0:43 am on April 27, 2023: contributor

@Sjors Added a test.

DrahtBot added the label CI failed on Apr 27, 2023

MarcoFalke commented at 11:21 am on May 2, 2023: member

Would you mind submitting the test to test current master behavior for -nodatacarrier, or would you mind if someone else did that?

petertodd commented at 2:47 pm on June 2, 2023: contributor

Would you mind submitting the test to test current master behavior for -nodatacarrier, or would you mind if someone else did that?

I certainly don’t mind if someone else does that.

Ayms commented at 12:56 pm on June 3, 2023: none

I must be brain damaged or something like this, still I don’t get what it has to do with #27043 (and when it will happen)

in src/policy/policy.cpp:86 in baa28315e7

82@@ -83,7 +83,9 @@ bool IsStandard(const CScript& scriptPubKey, const std::optional<unsigned>& max_
83         if (m < 1 || m > n)
84             return false;
85     } else if (whichType == TxoutType::NULL_DATA) {
86-        if (!max_datacarrier_bytes || scriptPubKey.size() > *max_datacarrier_bytes) {

Sjors commented at 5:30 pm on June 5, 2023:

Note to other reviewers: max_datacarrier_bytes is nullptr when DEFAULT_ACCEPT_DATACARRIER is (patched to) false.

Sjors approved

Sjors commented at 5:43 pm on June 5, 2023: member

Concept ACK, unless there’s clear historical context somewhere (that I missed) showing that the folks who introduced -nodatacarrier intended to prevent burning of coins (as distinct from “spam”).

As far as I can tell the current behavior was implemented by accident in #5077. The goal of that PR was to make the limit configurable for miners. Specifically the limit at the time was 40 and some miners wanted 80. A side-effect of the change is that if you set it to 0 it would also stop an OP_RETURN burn, but that’s not even mentioned in the PR.

In other words, imo this PR fixes a bug.

utACK baa28315e70e52c4f42abbb0bd183a244801f55f

MarcoFalke commented at 3:25 pm on June 7, 2023: member

In other words, imo this PR fixes a bug.

Not sure. I don’t think it makes sense to change the behavior of the -datacarrier setting. There is no reason for it to exist in the first place and it should just be removed. It is redundant with the -datacarriersize setting, because setting -datacarriersize=0 is equivalent. Having two ways to achieve the same is just confusing and can lead to issues. Also, unrelated to removing the option, the std::optional<unsigned> should just be flattened to unsigned, with nullopt being mapped to 0U. Or, ideally along with removing the setting, nullopt is also removed in one go with no mapping required.

Sjors commented at 8:01 pm on June 14, 2023: member

@MarcoFalke the -datacarrier help says Relay and mine data carrier transactions. At the time of #5077 (before and after) an OP_RETURN-only transaction was standard regardless of the setting:

https://github.com/bitcoin/bitcoin/blob/2ffdf21ce39fc3133fc028fb51d49cd7479eaa43/src/script/standard.cpp#LL55C7-L55C7

I didn’t check when that was changed and why though.

MarcoFalke commented at 6:57 am on June 15, 2023: member

I didn’t check when that was changed and why though.

Probably in da894ab5da222ad317039eb008ec6443fb9113d9, which added the IsStandard code logic that is still in place in today’s master?

Though, I still think -datacarrier should just be removed for my reasons mentioned previously.

Sjors commented at 7:48 am on June 15, 2023: member

Though, I still think -datacarrier should just be removed for my reasons mentioned previously.

That’s fine by me as well (as an extra commit here or a followup). It would need a breaking change release note, because otherwise someone setting datacarrier=0 in their config without also setting datacarriersize=0 will only see a subtle log message.

Probably in https://github.com/bitcoin/bitcoin/commit/da894ab5da222ad317039eb008ec6443fb9113d9

Indeed, #6424 lost the distinction. That PR was a cleaner alternative to #5079. The latter did preserve the distinction, but didn’t include a test for the just-an-OP_RETURN case (these tests were copied into the new PR).

MarcoFalke commented at 7:59 am on June 15, 2023: member

… config … will only see a subtle log message.

Someone should probably fix #15021 :)

luke-jr commented at 9:16 pm on June 22, 2023: member

The presence of the output is itself data (debatable if 1-bit or more). If an exception is being made, perhaps it should at least require a value burned or absence of other outputs?

petertodd commented at 9:24 pm on June 22, 2023: contributor

On Thu, Jun 22, 2023 at 02:16:34PM -0700, Luke Dashjr wrote:

The presence of the output is itself data (debatable if 1-bit or more). If an exception is being made, perhaps it should at least require a value burned or absence of other outputs?

We already allow transactions to pick nSequence and nLocktime, allowing for far more bits of data than the data embedded by the presence or absence of an OP_Return. There is no reason to create such convoluted rules to prevent the off chance of someone embedding a bit or two.

ajtowns commented at 7:26 am on June 23, 2023: contributor

Not sure. I don’t think it makes sense to change the behavior of the -datacarrier setting. There is no reason for it to exist in the first place and it should just be removed. It is redundant with the -datacarriersize setting, because setting -datacarriersize=0 is equivalent. Having two ways to achieve the same is just confusing and can lead to issues. Also, unrelated to removing the option, the std::optional<unsigned> should just be flattened to unsigned, with nullopt being mapped to 0U. Or, ideally along with removing the setting, nullopt is also removed in one go with no mapping required.

Agreed on removing the optional wrapping. IMO, having two ways of specifying the same thing can be fine if it makes things more usable or provides better backwards compatability, in the same way that we support both -testnet and -chain=test eg.

I could perhaps see an argument for moving all the -datacarrier logic to InitParameterInteraction along the lines of:

0const auto dc = args.GetBoolArg("-datacarrier");
1if (dc.has_value()) {
2    if (args.IsArgSet("-datacarriersize")) {
3        LogPrintf("%s: parameter interaction: -datacarriersize set -> ignoring -datacarrier=%d\n", __func__,  *dc);
4    } else if (!*dc) {
5        args.SoftSetArg("-datacarriersize", 1); // allow a single push-less OP_RETURN
6        LogPrintf("%s: parameter interaction: -datacarrier=%d -> setting -datacarriersize=%d\n", __func__, *dc, 1);
7    }
8}

in order to have an easy way of saying “no data thanks, but a dummy output for burning to fees is fine” other than the not at all obvious -datacarriersize=1. But I can only find a single twitter user recommending setting -nodatacarrier or -datacarrier=0 or -datacarriersize=0 though (knots sets -datacarriersize=42 by default), so even then I’m not really seeing the benefit of worrying about the edge case here.

Even more so given we don’t actually support creating a pushless OP_RETURN via createrawtransaction, createpsbt, send etc, and since spending a single input to an OP_RETURN without 3 or more bytes of data is likely to be rejected as too small anyway. Better to just sign the input you want to burn with ANYONECANPAY|NONE, spending to 0x464545 (“FEE”) and let an enterprising miner combine/coinjoin them as desired (and if that ever becomes common, make it a standard part of mempool management).

luke-jr commented at 2:12 am on June 24, 2023: member

We already allow transactions to pick nSequence and nLocktime,

It’s not possible to omit them…

allowing for far more bits of data than the data embedded by the presence or absence of an OP_Return.

As far as actual data usage, the additional output requires at least 10 bytes (64-bit amount, length of script, and the actual OP_RETURN).

petertodd commented at 1:33 am on July 23, 2023: contributor

Closing in favor of https://github.com/bitcoin/bitcoin/pull/28130

petertodd closed this on Jul 23, 2023

Ignore datacarrier limits for dataless OP_RETURN outputs #27261

Reviews

Conflicts