init, doc: Replace datacarrier(size) deprecation with non-recommendation text #32714
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32714. ReviewsSee 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. |
|
lgtm ACK 4f06bf3 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.] |
| 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); | ||
| 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); | ||
| argsman.AddArg("-datacarrier", strprintf("(DEPRECATED) Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); | ||
| 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sure, any suggested wording?
|
ACK 4f06bf3 Deprecating seemed premature to me, let's do it in smaller steps to be more aligned with the users. |
|
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. |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
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:
|
|
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. |
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.
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:
Taking that approach would mean not deprecating the option until after a release with the new default has been out for some time. |
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
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.
| 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); | ||
| 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); | ||
| argsman.AddArg("-datacarrier", strprintf("(DEPRECATED) Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); | ||
| 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); |
There was a problem hiding this comment.
Prefer dropping the extra recommendation sentence for each of these.
| 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); | |
| argsman.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u).", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); |
|
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
left a comment
glozow
left a comment
There was a problem hiding this comment.
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.
| @@ -1,3 +1,3 @@ | |||
| - `-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) | |||
| - `-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) | |||
There was a problem hiding this comment.
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."
…guration options 451ba9a datacarrier: Undeprecate configuration option (Anthony Towns) Pull request description: Removes the deprecation for the `datacarrier` and `datacarriersize` options by reverting commit 0b4048c from #32406 **Many current Bitcoin Core users want to continue using this option** This statement is based on public postings from many Bitcoin Core users and not a formal survey. AJ Towns’ observation from [#32406](0b4048c#r2084024874) that “_for now there seem to be a bunch of users who like the option_” has only become more apparent in the months since. **The deprecation intent is unclear to users** This echo’s Ava Chow’s comment from #32714 that “_IMO we should not have removal warnings if there is no current plan to actually remove them._” In months since that comment, partially due to increased feedback from Bitcoin Core users wanting to keep this option, there is even less likelihood of a near term plan to remove these options. That leaves Bitcoin Core users in an unclear situation: the option could be removed in the next version or perhaps never. Removing the deprecation gives clarity for their planning purposes. Deprecating the option in the future, preferably with a removal schedule to better inform users, would still be possible. **Minimal downsides to removing deprecation** As a best practice, Bitcoin Core has avoided an option when the developers cannot articulate when they should be used. There is non-zero maintenance cost to keeping this code around (although leaving the options deprecated for a long time has the same effect). “Don’t offer users footguns” is also a good principle, but with this option, there seems to be only small impacts that can quickly be remedied by changing the option value by Bitcoin Core users. There already exist in Bitcoin Core more potentially-user-harmful options/values than what datacarrier might cause. ACKs for top commit: ajtowns: ACK 451ba9a darosior: That said, certain users care strongly about using those options. In these conditions, i do not see the project removing the option anytime soon. Therefore i think it's technically incorrect (and confusing) to mark it as deprecated. utACK 451ba9a on removing the deprecation. instagibbs: crACK 451ba9a Raimo33: ACK 451ba9a Ademan: utACK 451ba9a ryanofsky: Code review ACK 451ba9a marcofleon: ACK 451ba9a achow101: ACK 451ba9a moonsettler: ACK 451ba9a ismaelsadeeq: utACK 451ba9a 🛰️ jonatack: ACK 451ba9a Zero-1729: crACK 451ba9a vasild: ACK 451ba9a Tree-SHA512: b83fc509f5dd820976596e1ae9fb69a22ada567e0e0ac88da5fc5e940a46d8894b40cc70c3eff2cbdabd4da5ec913f0d18c1632fc906f210b308855868410699
14 participants
Instead of marking
-datacarrierand-datacarriersizeoptions 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.