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

Reviews

See the guideline for information on the review process.

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.

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

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.

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?

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.

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.

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:

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

Maybe change it to:

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

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

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.

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

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

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

bare OP_RETURN outputs are not data carrying outputs

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

@Sjors Added a test.

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

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.

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

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

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

Probably in da894ab, 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.

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 da894ab

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

... config ... will only see a subtle log message.

Someone should probably fix #15021 :)

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?

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:

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

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

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

Closing in favor of #28130