rpc, mempool: -deprecatedrpc fullrbf and bip125-replaceable from mempool RPCs #34911

mempoolfullrbf=1 behaviour has been the default since v28 and the argument has been removed since v29 subsequently.

The getmempoolinfo RPC need not return the deprecated fullrbf key in the response anymore unless the user requests it via -deprecatedrpc=fullrbf node argument.

Also, remove the bip125-replaceable key from the mempool RPCs responses (because it, too, has been deprecated since v29) unless the user requests it via -deprecatedrpc=bip125 node argument.

<!-- *** Please remove the following help text before submitting: *** Pull requests without a rationale and clear improvement may be closed immediately. GUI-related pull requests should be opened against https://github.com/bitcoin-core/gui first. See CONTRIBUTING.md -->

<!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. -->

<!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. -->

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Concept ACK

See https://github.com/bitcoin/bitcoin/issues/32661

Thanks for sharing the issue, I didn't know about it.

I did raise #34917 some time back that I guess does the first step mentioned in the issue, this one:

Remove the -walletrbf configuration option.

Concept ACK c504fd6b82c1deffa9f9d563305c828237ec7a29

As described, this change removed the following RPC response fields:

• getmempoolinfo RPC fullrbf field -- currently always set to true
• getmempoolentry RPC bip125-replaceable field.

Note that raw transaction and transaction RPCs are not modified (createrawtransaction RPC replaceable argument, gettransaction RPC bip125-replaceable field, etc.)

Usually this goes through an rpcdeprecated cycle, not sure if this is needed here?

I agree with eliminating the fullrbf key in the response as this was marked as deprecated.

Usually this goes through an rpcdeprecated cycle, not sure if this is needed here?

I think this should be discussed for the bip125-replaceable key. If my logic is correct, if we eliminate this key from getmempoolentry without marking it first as deprecated, then would it also be okay to directly eliminate the bip125-replaceable field from transaction info without deprecating it first? Or since this field has had no use due to the mempoolfullrbf=1 for a long time, is it okay to remove it directly?

In both cases, getmempoolentry and the transaction info field, there might be users who still use or expect this field.

I agree with eliminating the fullrbf key in the response as this was marked as deprecated.

The description of getmempoolentry.bip125-replaceable contains "(DEPRECATED)" for 17 months (since Oct. 31, 2024, v29.0). Is that sufficient deprecation warning? (honest question)

I don't think clients read the docs when calling the RPC (and there are no machine-readable RPC docs yet). I can certainly see a client using the bip125 field today. No strong opinion, it is just what the dev notes say:

$ git grep deprecatedrpc doc/developer-notes.md
doc/developer-notes.md:- It's preferable to avoid changing an RPC in a backward-incompatible manner, but in that case, add an associated `-deprecatedrpc=` option to retain previous RPC behavior during the deprecation period. Backward-incompatible changes include: data type changes (e.g. from `{"warnings":""}` to `{"warnings":[]}`, changing a value from a string to a number, etc.), logical meaning changes of a value, key name changes (e.g. `{"warning":""}` to `{"warnings":""}`), or removing a key from an object. Adding a key to an object is generally considered backward-compatible. Include a release note that refers the user to the RPC help for details of feature deprecation and re-enabling previous behavior. [Example RPC help](https://github.com/bitcoin/bitcoin/blob/94f0adcc/src/rpc/blockchain.cpp#L1316-L1323).
doc/developer-notes.md:  - *Rationale*: Changes in RPC JSON structure can break downstream application compatibility. Implementation of `deprecatedrpc` provides a grace period for downstream applications to migrate. Release notes provide notification to downstream users.

I can see how the process can appear annoying and tedious, but if we don't want to follow the process, it should be removed from the dev notes and from doc/JSON-RPC-interface.md. It just seems inconsistent to come up with a process, document it, but then do some other process.

it is just what the dev notes say:

I can make it go through the rpcdeprecated cycle.

I don't think clients read the docs when calling the RPC (and there are no machine-readable RPC docs yet).

However, if this is the case, then the practice of marking a field (DEPRECATED) seems redundant? I guess the PRs can straightaway remove the field and make it go through the rpcdeprecated cycle.

I think we should follow the rpcdeprecated cycle here.

However, if this is the case, then the practice of marking a field (DEPRECATED) seems redundant? I guess the PRs can straightaway remove the field and make it go through the rpcdeprecated cycle.

Maybe, but if there are plans to remove things, there is no harm in giving notice through multiple avenues.

I will add the rpcdeprecated changes here.

I think marking with DEPRECATED in the string mostly means: I put this here, so that the meaning is out there, but it is harmless to keep and I don't want to bother with the deprecatedrpc cycle myself, or at least there is no immediate need or rush to deal with the removal right now, it can be done later.

I will add the rpcdeprecated changes here.

Can you push these changes @rkrux?

I will add the rpcdeprecated changes here.

Can you push these changes @rkrux?

I have pushed the changes to make these deprecated keys go through the -deprecatedrpc cycle.

Pushed to fix lint failure here.