init, doc: Replace datacarrier(size) deprecation with non-recommendation text

achow101 commented at 4:41 am on June 10, 2025: member

Instead of marking -datacarrier and -datacarriersize options as deprecated, only say that these options are not recommended for use.

While deprecation obviously means that the options are not recommended for use, many users seem to believe that it also means that the options will be imminently removed. My understanding is that there is currently no plan to remove these options in the near future, and I don’t think it would be wise to open a PR to remove the options for a few more years. While deprecation certainly does not give a timeline for removal, and the current text is still technically correct, I think that it would be clearer to users to remove the word “deprecated” from the documentation of this option while preserving the non-recommendation.

One thing that could be bikeshed is whether to warn on init if these options are set. IMO we should not have removal warnings if there is no current plan to actually remove them.

init: Replace datacarrier(size) deprecation with non-recommendation text 4f06bf3451

DrahtBot commented at 4:41 am on June 10, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32714.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, maflcko
Concept NACK	Sjors, waketraindev, hodlinator, fjahr, instagibbs
Concept ACK	jonatack

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

maflcko added the label Docs on Jun 10, 2025

maflcko commented at 5:42 am on June 10, 2025: member

lgtm ACK 4f06bf3451c7800ccc54853b354285fb159ef382

Makes sense to go through a normal and proper deprecate-remove cycle over two releases, once, and if there is need to. Otherwise, this may be a situation similar to #29240, where the deprecation warning is meaningless and confusing (because removal is permanently postponed).

[Edit: I assigned the milestone, because this needs to be closed/merged before the milestone.]

maflcko added this to the milestone 30.0 on Jun 10, 2025

in src/init.cpp:633 in 4f06bf3451

629@@ -630,10 +630,10 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
630     argsman.AddArg("-dustrelayfee=<amt>", strprintf("Fee rate (in %s/kvB) used to define dust, the value of an output such that it will cost more than its value in fees at this fee rate to spend it. (default: %s)", CURRENCY_UNIT, FormatMoney(DUST_RELAY_TX_FEE)), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::NODE_RELAY);
631     argsman.AddArg("-acceptstalefeeestimates", strprintf("Read fee estimates even if they are stale (%sdefault: %u) fee estimates are considered stale if they are %s hours old", "regtest only; ", DEFAULT_ACCEPT_STALE_FEE_ESTIMATES, Ticks<std::chrono::hours>(MAX_FILE_AGE)), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
632     argsman.AddArg("-bytespersigop", strprintf("Equivalent bytes per sigop in transactions for relay and mining (default: %u)", DEFAULT_BYTES_PER_SIGOP), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
633-    argsman.AddArg("-datacarrier", strprintf("(DEPRECATED) Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
634+    argsman.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions. This option is not recommended for use. (default: %u)", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);

TheCharlatan commented at 6:00 am on June 10, 2025:

I think it would be easy enough to enumerate the potential degredations of performance here explicitly, no? Should have said this on the other PR, but the notice in general feels weirdly inconsistent to me. Basically all the other policy options have similar effects if they are tightened beyond their defaults too. What makes this one different?

achow101 commented at 6:07 am on June 10, 2025:

Sure, any suggested wording?

jonatack commented at 4:33 pm on June 10, 2025:

Prefer dropping the extra recommendation sentence for each of these.

0    argsman.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u).", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);

l0rinc commented at 6:36 am on June 10, 2025: contributor

ACK 4f06bf3451c7800ccc54853b354285fb159ef382

Deprecating seemed premature to me, let’s do it in smaller steps to be more aligned with the users.

Sjors commented at 7:33 am on June 10, 2025: member

Concept NACK.

It’s fine to add a non-recommendation text, but we should keep the deprecation warning. If by the time we would normally drop a depreceated feature (v31 or v32) there’s still a strong risk of brigading, then that’s perhaps a political argument for delaying it further, but it’s still deprecated.

I don’t think the comparison with #29240 is correct here. There the deprecation itself was a mistake, because there were good technical reasons for users to keep using username and password. That’s simply not the case here, as evidenced by the need to add non-recommendation text.

Removing the deprecation warning now also means having to repeat the drama when bringing the deprecation back.

waketraindev commented at 9:20 am on June 10, 2025: none

Concept NACK

Using datacarrier for mempool/relay filtering appears to be driven more by ideological or social coercion motivations than technical merit. It’s better to just remove it.

As long as there’s enough fee attached, these transactions will likely be mined anyway. That means nodes that previously saw and rejected them will end up redownloading them. Also note that vExtraTxnForCompact has a default size of 100 entries, so this kind of filtering is not well-suited for real-world conditions.

hodlinator commented at 10:25 am on June 10, 2025: contributor

Concept NACK for v30

Noting that this PR is opened on the day after 32406 is merged.

Keeping the settings as deprecated for now means the delta to full removal is smaller, while removing the deprecation increases that delta. I think enough boats have been burned already, let’s not re-build them only to burn them again. People implying maliciousness behind the first PR will not suddenly have their confidence restored by this one.

What would change my mind:

New/better arguments for using them with non-default values.
Enough releases having passed without a push for complete removal.

fjahr commented at 1:22 pm on June 10, 2025: contributor

Concept NACK

I personally prefer to keep the deprecation warning as I think all the rationale given (among many other places) here and here points to the fact that it should be removed some time in the future. The fact that outsiders don’t understand deprecation and how it is handled in Bitcoin Core is not a strong argument. I am also surprised by this because the original PR that just removed the option outright seemed to have broad support among contributors. I was one of the few reviewers to ask for a deprecation period as far as I can remember.

I have also seen contributors express aversion to revisit this discussion again in the future. This was given as a rationale to not increase the limit in steps as needed by layer 2 protocols but to remove the limit completely, for example. This PR adds another step in the future when we actually want to remove the option. We’ll have drama for the deprecation PR and the removal, rather than just the removal as it stand currently.

As others have noted before the deprecation can be walked back any time in the future if there are better reasons for keeping the option in place long term.

ajtowns commented at 2:39 pm on June 10, 2025: contributor

While deprecation obviously means that the options are not recommended for use, many users seem to believe that it also means that the options will be imminently removed.

Given “deprecation obviously means that the options are not recommended for use” I think that trying to differentiate “not recommended for use” and “deprecated” will only lead to more confusion, so I think we should either (a) recommend the option not be used and mark it deprecated; or (b) not mark it deprecated, and leave it up to the user to decide whether to use the option, without making any particular recommendation; and not try (c) some other option to split the non-existent difference. If it were just up to me, I’d choose (b) over (a), but it’s not a terribly strong opinion.

My understanding is that there is currently no plan to remove these options in the near future, and I don’t think it would be wise to open a PR to remove the options for a few more years.

Both -datacarrier and -datacarriersize options have been marked as deprecated and are expected to be removed in a future release. (#32406)`

InitWarning(_("Options '-datacarrier' or '-datacarriersize' are set but are marked as deprecated. They will be removed in a future version."));

I think various contributors would like to remove the options in the near future, and both marking the option as deprecated and having explicit warnings about the feature being removed on startup and in the release notes lends support to those opinions. I also think it’s hard to justify keeping around an option that we recommend nobody use for a long period of time.

Personally, I think it would make more sense to take the following approach:

decide what we think the default behaviour should be (ie, relay/mine txs with an arbitrary number of OP_RETURN outputs, of arbitrary length)
determine if enough users seem interested in alternative behaviours to justify providing/maintaining a runtime configuration option (sure seems like there are)
observe if that configuration option is useful in practice on the network (ie, some level of hashpower actually changes from the default behaviour – not observable at the moment because the new default behaviour isn’t even available in released software)
- if 99.9% of hashpower (or some other number) seems satisfied with the default behaviour, remove the option, possibly with a deprecation period
- on the other hand, if a majority of hashpower (or plurality?) is using some different behaviour to our default, even after a release with the new default has been out for some time, seriously consider changing our defaults to match that behaviour

Taking that approach would mean not deprecating the option until after a release with the new default has been out for some time.

jonatack commented at 4:38 pm on June 10, 2025: member

Concept ACK, modulo the following

I think we should either (a) recommend the option not be used and mark it deprecated; or (b) not mark it deprecated, and leave it up to the user to decide whether to use the option, without making any particular recommendation; and not try (c) some other option to split the non-existent difference. If it were just up to me, I’d choose (b) over (a), but it’s not a terribly strong opinion.

Agree with @ajtowns.

glozow requested review from instagibbs on Jun 10, 2025

in doc/release-notes-32406.md:1 in 4f06bf3451

0@@ -1,3 +1,3 @@
1-- `-datacarriersize` is increased to 100,000 which effectively uncaps the limit (as the maximum transaction size limit will be hit first). It can be overridden with -datacarriersize=83 to revert to the limit enforced in previous versions. Both `-datacarrier` and `-datacarriersize` options have been marked as deprecated and are expected to be removed in a future release. (#32406)
2+- `-datacarriersize` is increased to 100,000 which effectively uncaps the limit (as the maximum transaction size limit will be hit first). It can be overridden with -datacarriersize=83 to revert to the limit enforced in previous versions. Both `-datacarrier` and `-datacarriersize` options are not recommended to be used, and may be removed in a future release, after a deprecation warning. (#32406)

glozow commented at 6:30 pm on June 10, 2025:

I do prefer “are not recommended to be used” more than “have been marked as deprecated” as it explains it more clearly to the user. However, discouragement is deprecation, it’s weird to say “after a deprecation warning.”

instagibbs commented at 6:30 pm on June 10, 2025: member

concept NACK, seems needless churn to me to backtrack on something a number of original reviewers(and author himself) disagree with

As per @ajtowns if miners end up over-riding the value a lot, we can rethink deprecation. I don’t think removing deprecation now makes anything clearer.

glozow commented at 6:36 pm on June 10, 2025: member

I’m all for explaining more clearly what the status of these config options it. But it seems like we’re removing the “vegetarian” label from a menu item because people misinterpreted it and are upset that it contains eggs. We should keep the vegetarian label and just elaborate “this has no meat, but does contain eggs.”

Also, I think it’d be fine to leave the option for more than 1-2 release cycles before removal.

achow101 closed this on Jun 10, 2025

init, doc: Replace datacarrier(size) deprecation with non-recommendation text #32714

Code Coverage & Benchmarks

Reviews