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 from the upcoming release
(v32) imho.
Also, remove the bip125-replaceable key from the getmempoolentry RPC
response because it, too, has been deprecated since v29.
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 from the upcoming release
(v32) imho.
BREAKING CHANGE: removes deprecated `fullrbf` key from `getmempoolinfo` RPC
response
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | ismaelsadeeq, optout21 |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
BREAKING CHANGE: This key has been marked deprecated since v29, it can be
removed from the RPC response now.
Concept ACK
561- throw JSONRPCError(RPC_MISC_ERROR, "Transaction is not in mempool");
562- } else if (rbfState == RBFTransactionState::REPLACEABLE_BIP125) {
563- rbfStatus = true;
564- }
565-
566- info.pushKV("bip125-replaceable", rbfStatus);
IsRBFOptIn() returns RBFTransactionState::FINAL) the getmempoolentry RPC returned bip125-replaceable=false, but the behavior was replaceable. With this change the bip125-replaceable field is not returned, and the replaceable behavior is unchanged.
Concept ACK c504fd6b82c1deffa9f9d563305c828237ec7a29
As described, this change removed the following RPC response fields:
getmempoolinfo RPC fullrbf field – currently always set to truegetmempoolentry RPC bip125-replaceable field.Note that raw transaction and transaction RPCs are not modified (createrawtransaction RPC replaceable argument, gettransaction RPC bip125-replaceable field, etc.)
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
fullrbfkey 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:
0$ git grep deprecatedrpc doc/developer-notes.md
1doc/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).
2doc/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.
rpcdeprecated changes here.
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.