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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | optout21, sedited, instagibbs |
| Concept ACK | ismaelsadeeq |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
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);
Request for clarification: for mempool transactions that did not signal RBF through their sequence (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.)
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
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:
$ 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.
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.
RPCs: getrawmempool, getmempoolancestors, getmempooldescendants, getmempoolentry
This key has been marked deprecated since v29, it can be
removed from the RPC response now unless the user requests it via the
-deprecatedrpc=bip125 node argument.
27 | @@ -28,6 +28,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): 28 | def set_test_params(self): 29 | self.num_nodes = 2 30 | self.uses_wallet = None 31 | + self.extra_args = [["-deprecatedrpc=fullrbf"], []]
f89d18c rpc, mempool: rpcdeprecate
fullrbfkey in getmempoolinfo RPC response:
The code now has two behaviors, depending on the config, but only one is tested. It would be complete if both versions are tested. Currently only the deprecated version is tested. Specifically, if I replace the added IsDeprecatedRPCEnabled conditions with false, the tests fail. However, if I change them to true, the tests pass. There should be failures in both cases.
27 | @@ -28,7 +28,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): 28 | def set_test_params(self): 29 | self.num_nodes = 2 30 | self.uses_wallet = None 31 | - self.extra_args = [["-deprecatedrpc=fullrbf"], []] 32 | + self.extra_args = [["-deprecatedrpc=fullrbf", "-deprecatedrpc=bip125"], []]
ACK 8deed0df066ef2cbb6f51de4075c891a48b8debd
Reviewed and tested.
Left comments about lack of test coverage -- the tests have been adapted, but they only test the variant when deprecatedrpc flag is supplied, and not the variant without. I suggest adding a basic test for non-presence of the removed fields.
I have tested with different flag combinations, works as expected.
| extra flags | fullrbf in getmempoolinfo | bip125-replaceable in getrawmempool |
|---|---|---|
| (none) | N | N |
-deprecatedrpc=fullrbf |
Y | N |
-deprecatedrpc=bip125 |
N | Y |
-deprecatedrpc=dummy |
N | N |
-deprecatedrpc=fullrbf -deprecatedrpc=bip125 |
Y | Y |
ACK 8deed0df066ef2cbb6f51de4075c891a48b8debd
ACK 8deed0df066ef2cbb6f51de4075c891a48b8debd
Manually tested that the keys don't exist in getmempoolinfo and getmempoolentry on regtest, since I don't think adding mechanical tests that will be quickly removed later is high value.
Milestone
32.0