wallet, test: optinrbf deprecation followups #35381

rkrux commented at 1:06 PM on May 26, 2026: contributor

Partially fixes #32661.

Prerequisite to #35404 and #35405.

All these changes address the points raised in the review of PR #34917 here: #34917#pullrequestreview-4362148900.

Essentially updating the existing wallet functional tests without using the -deprecatedrpc=bip125 and -walletrbf startup options. Instead, these two are added and tested via a singular new wallet_deprecated_rbf.py test that can be removed easily later when these startup options are completely removed from the wallet post deprecation.

DrahtBot commented at 1:06 PM on May 26, 2026: 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/35381.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, achow101
Stale ACK	polespinasa

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35302 (Silent Payments: Sending (take 2) by Eunovo)
#33034 (wallet: Store transactions in a separate sqlite table by achow101)

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.

rkrux renamed this:
~~Wallet optinrbf deprecation followups~~
wallet optinrbf deprecation followups
on May 26, 2026

wallet: refactor to read -walletrbf only once instead of twice a3fe455a95

rkrux force-pushed on May 26, 2026

rkrux commented at 1:51 PM on May 26, 2026: contributor

Forced pushed from c0f55ef to 695b1fe in order to fix the failing "No wallet" job here: https://github.com/bitcoin/bitcoin/actions/runs/26449764159/job/77866906419?pr=35381

DrahtBot added the label CI failed on May 26, 2026

rkrux renamed this:
~~wallet optinrbf deprecation followups~~
wallet, test: optinrbf deprecation followups
on May 26, 2026

DrahtBot removed the label CI failed on May 26, 2026

fanquake added this to the milestone 32.0 on May 27, 2026

fanquake commented at 8:29 AM on May 27, 2026: member

cc @w0xlt @polespinasa @achow101

in test/functional/rpc_psbt.py:1292 in 739a38b255

1278 | @@ -1287,9 +1279,6 @@ def test_psbt_input_keys(psbt_input, keys):
1279 |          parsed_psbt = PSBT.from_base64(psbt)
1280 |          assert_greater_than(len(parsed_psbt.o[vout].map[PSBT_OUT_TAP_TREE]), 0)
1281 |          assert "taproot_tree" in self.nodes[0].decodepsbt(psbt)["outputs"][vout]
1282 | -        parsed_psbt.make_blank()
1283 | -        comb_psbt = self.nodes[0].combinepsbt([psbt, parsed_psbt.to_base64()])
1284 | -        assert_equal(comb_psbt, psbt)

maflcko commented at 10:10 AM on May 27, 2026:

this was added in 22c051ca70bae73e0430b05fb9d879591df27699. How is removing this related to rbf??

rkrux commented at 12:32 PM on May 27, 2026:

Thanks for raising this question, yes it's not related to RBF. This portion was failing after removing the -walletrbf option from the nodes in this test. I was not able to figure out a solution for it at that moment, so I had removed it temporarily in order to continue with other removals - but I forgot to revisit it before raising the PR.

I have added it back now in the latest push after fixing this failing portion. The reason for failure was related to how the V2 PSBTs were made blank here in this portion due to which combinepsbt RPC was throwing a PSBTs not compatible (different transactions) error. I have fixed it by ensuring the PSBT_GLOBAL_FALLBACK_LOCKTIME key is also included when making a V2 PSBT blank because that was the source of discrepancy here.

in test/functional/rpc_deprecated.py:25 in ecb96d9554 outdated

  19 | @@ -20,9 +20,9 @@ def run_test(self):
  20 |          # self.log.info("Test generate RPC")
  21 |          # assert_raises_rpc_error(-32, 'The wallet generate rpc method is deprecated', self.nodes[0].generate, 1)
  22 |          #
  23 | -        # Please ensure that for all the RPC methods tested here, there is
  24 | -        # at least one other functional test that still tests the RPCs
  25 | -        # functionality using the respective -deprecatedrpc flag.
  26 | +        # This test can also be used to test the functionalities of the
  27 | +        # currently deprecated RPC methods or deprecated keys in the RPC
  28 | +        # responses (with the -deprecatedrpc flag).

maflcko commented at 10:36 AM on May 27, 2026:

Ok, I see you correctly followed the docs in rpc_deprecated.py and my claim that such a test must sit in this file was wrong, sorry.

So I think an alternative to this commit could be to just keep a single exhaustive test (e.g. run_rbf_opt_in_test) in one file and just reword the docstring of the run_rbf_opt_in_test test to say: "This test exercises the deprecatedrpc=bip125 flag and depreacted -walletrbf option and can be removed when the options are removed".

(unrelated: Not ideal that the docs here are now split in three places: dev-notes section RPC interface guidelines and the next section Feature deprecation, as well as this test. But this is unrelated.)

rkrux commented at 12:38 PM on May 27, 2026:

So I think an alternative to this commit could be to just keep a single exhaustive test (e.g. run_rbf_opt_in_test) in one file

While updating the rpc_deprecated test, I realised that the documentation was quite restrictive in what could go in here and thus, I updated it and put the test in here. I feel it just makes it easier to find the testing of all the deprecated stuff in just one file that touches deprecated stuff.

maflcko commented at 1:04 PM on May 27, 2026:

ok, but you'll have to use the is_wallet_compiled() check. Otherwise, it is not possible to add other non-wallet checks to the file. Ref the last commit that touched this file:

commit 331a5279d2775fb701a0bf4607436ec05e476df3
Author: Pol Espinasa 
Date:   Tue Mar 25 19:24:43 2025 +0100

    wallet, rpc:remove settxfee and paytxfee

diff --git a/test/functional/rpc_deprecated.py b/test/functional/rpc_deprecated.py
index acc3a05fea..2f86954f22 100755
--- a/test/functional/rpc_deprecated.py
+++ b/test/functional/rpc_deprecated.py
@@ -4,7 +4,6 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test deprecation of RPC calls."""
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_raises_rpc_error
 
 class DeprecatedRpcTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -27,12 +26,8 @@ class DeprecatedRpcTest(BitcoinTestFramework):
 
         # Please don't delete nor modify this comment
         self.log.info("Tests for deprecated RPC methods (if any)")
+        self.log.info("Currently no tests for deprecated RPC methods")
 
-        if self.is_wallet_compiled():
-            self.log.info("Tests for deprecated wallet-related RPC methods (if any)")
-            self.log.info("Test settxfee RPC deprecation")
-            self.nodes[0].createwallet("settxfeerpc")
-            assert_raises_rpc_error(-32, 'settxfee is deprecated and will be fully removed in v31.0.', self.nodes[0].settxfee, 0.01)
 
 if __name__ == '__main__':
     DeprecatedRpcTest(__file__).main()

rkrux commented at 1:49 PM on May 27, 2026:

but you'll have to use the is_wallet_compiled() check. Otherwise, it is not possible to add other non-wallet checks to the file

I think this file is rarely updated so it's not a big concern. But I have added it nevertheless.

polespinasa commented at 8:12 AM on May 28, 2026:

Not against having a new file wallet_deprecated_rbf.py file, but maybe we can just update the rpc_deprecated.py file to have two functions, one that does what is actually doing:

        # This test should be used to verify the errors of the currently
        # deprecated RPC methods (without the -deprecatedrpc flag) until
        # such RPCs are fully removed. For example:
        #
        # self.log.info("Test generate RPC")
        # assert_raises_rpc_error(-32, 'The wallet generate rpc method is deprecated', self.nodes[0].generate, 1)
        #
        # Please ensure that for all the RPC methods tested here, there is
        # at least one other functional test that still tests the RPCs
        # functionality using the respective -deprecatedrpc flag.

And add another one that tests that the deprecation behavior is correct (what wallet_deprecated_rbf.py does)?

We initially added that file for the settxfee case, but I think it is fine to generalize the usage for others. In any case I don't think that test file will grow to much.

rkrux commented at 9:35 AM on May 28, 2026:

I had this test in the rpc_deprecated.py file previously but the "No wallet" job failed after I added the "is_wallet_compiled" check. I was not able to find the right setting for the uses_wallet flag in the framework that passed for both the non-wallet-containing and wallet-containing CI jobs. So, I just moved it in a separate file as suggested in the first comment* of this thread.

maflcko approved

maflcko commented at 10:38 AM on May 27, 2026: member

lgtm, left some nits.

review ACK 695b1fe2e82baf0888bfb2a68cf9fba295473b44 🚆

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 695b1fe2e82baf0888bfb2a68cf9fba295473b44 🚆
4/KVUyN2rF/JuvVlUaEk5MXUuREW9r+YZojpdHIj35unF0E1Hoq/Li+44Hn1ON5HvFpKNbzfq3s9PDI7fGujDw==

</details>

sedited requested review from polespinasa on May 27, 2026

wallet, test: remove -walletrbf startup option from rpc_psbt.py

Also, include PSBT_GLOBAL_FALLBACK_LOCKTIME key/value while making the PSBT v2
blank for combinepsbt RPC.

a2a2b1745f

wallet, test: -walletrbf startup option from wallet_bumpfee.py 5e833e068d

wallet, test: remove -deprecatedrpc=bip125 from wallet_listtransactions.py 0ee94b2fef

wallet, test: remove -walletrbf startup option from wallet_listtransactions.py

The corresponding test case that tests for optin rbf variations can also
be removed.

8cb6e405d8

wallet, test: remove -walletrbf startup option from wallet_backwards_compatibility.py 42330922dd

rkrux force-pushed on May 27, 2026

DrahtBot added the label CI failed on May 27, 2026

rkrux force-pushed on May 27, 2026

DrahtBot removed the label CI failed on May 27, 2026

achow101 commented at 6:28 PM on May 27, 2026: member

The commit message in f0a8e4ee443670b810097f50db61198948aaf695 "wallet, test: remove -walletrbf startup option from wallet_backwards_compatibility.py" is incorrect. -nowallet is not the same as -disablewallet; rather it makes bitcoind start without a wallet, but wallets can still be loaded and created afterwards. So -walletrbf=1 is still doing something for the older nodes, although the test may not actually be checking for any rbf things.

maflcko commented at 7:57 AM on May 28, 2026: member

The commit message in f0a8e4e "wallet, test: remove -walletrbf startup option from wallet_backwards_compatibility.py" is incorrect. -nowallet is not the same as -disablewallet; rather it makes bitcoind start without a wallet, but wallets can still be loaded and created afterwards. So -walletrbf=1 is still doing something for the older nodes, although the test may not actually be checking for any rbf things.

Also noticed this, but didn't want to mention it, because it was a nit too small. The correct commit message would have been:

-walletrbf=1 can be removed from the previous releases, because the default for them is already 1.

in test/functional/wallet_listtransactions.py:100 in 8cb6e405d8 outdated

  91 | @@ -97,91 +92,12 @@ def run_test(self):
  92 |                              {"category": "receive", "amount": Decimal("0.44")},
  93 |                              {"txid": txid})
  94 |  
  95 | -        self.run_rbf_opt_in_test()

polespinasa commented at 8:01 AM on May 28, 2026:

Just a nit commit structure, obviously my personal view feel free to ignore, but seems like 0ee94b2fef0b246db50bdc1758f2dc80a2e77902 and 8cb6e405d8801eb86744b7477639d313cb630cf6 can be squashed?

I find a bit confusing to review some code that is modified in one commit and then completely removed in the following commit.

rkrux commented at 9:35 AM on May 28, 2026:

That modifications in one commit is also removal.

in test/functional/wallet_backwards_compatibility.py:42 in f0a8e4ee44 outdated

  38 | @@ -39,12 +39,12 @@ def set_test_params(self):
  39 |          self.extra_args = [
  40 |              ["-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # Pre-release: use to mine blocks. noban for immediate tx relay
  41 |              ["-nowallet", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # Pre-release: use to receive coins, swap wallets, etc
  42 | -            ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v25.0

polespinasa commented at 8:04 AM on May 28, 2026:

Same for 42330922dd8d5f96dbc0cc6a8e4092500029f89d and f0a8e4ee443670b810097f50db61198948aaf695

Why not just squash?

rkrux commented at 9:35 AM on May 28, 2026:

I hadn't removed this option from the previous releases in the previous PR and noticed it can be removed while working on this followup after I had removed the option from the pre-release.

It just so happened that my reasoning for its removal from the previous releases was not entirely correct that was noticed in the commit message by the reviewers. I might have not shared this reasoning earlier if I had squashed the commits.

I think it's fine to have separate commits here for future reference in the git history.

in doc/release-notes-34917.md:8 in a723947a6a outdated

   4 | @@ -5,5 +5,5 @@ as `listtransactions`, `listsinceblock`, and `gettransaction` is
   5 |  marked as deprecated. Users still have the option to retrieve this
   6 |  key by passing the `-deprecatedrpc=bip125` startup option. Also,
   7 |  the `-walletrbf` startup option has been marked as deprecated and
   8 | -will be fully removed in the next release. Using this options emits
   9 | +will be fully removed in the next release. Using this option emits

polespinasa commented at 8:14 AM on May 28, 2026:

I still think is good to mention the release number if we are convinced we are removing it in the next release. #34917 (review)

rkrux commented at 9:36 AM on May 28, 2026:

These release notes will go in v32.0, I find it slightly confusing to see a specific mention of any other future release in the notes.

polespinasa commented at 8:14 AM on May 28, 2026: member

lgtm ACK a723947a6ab816c8ebc861f6515dbca14f78a433

DrahtBot requested review from maflcko on May 28, 2026

wallet, test: remove -walletrbf startup option from wallet_backwards_compatibility.py

This option can be removed from the previous releases as well because the
default for them is already 1.

a52ea9bff9

wallet, test: remove -deprecatedrpc=bip125 from wallet_basic.py 3ec550d168

wallet, test: remove -deprecatedrpc=bip125 from wallet_migration.py 307134bd7e

wallet, test: remove -deprecatedrpc=bip125 from wallet_send.py 2cbbcb5659

wallet, test: add wallet_deprecated_rbf.py for walletrbf deprecated keys & options 7bc39e3d08

doc: fix typo in release notes of #34917 f701cd159a

rkrux force-pushed on May 28, 2026

rkrux commented at 3:01 PM on May 28, 2026: contributor

This PR is a prerequisite #35404 and #35405 that further removes optin rbf from wallet.

maflcko commented at 3:25 PM on May 28, 2026: member

review ACK f701cd159af60ae73275d01f41c45cd173e8f217 🌄

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK f701cd159af60ae73275d01f41c45cd173e8f217  🌄
tdQNhFFr+Tib/G1KIri2dkfPF5g87RGdOjkkYBoczxkr5AxLlUJCElUiRfCff0xmq/6EL8Qw/sxCjfdkFdgYDQ==

</details>

DrahtBot requested review from polespinasa on May 28, 2026

achow101 commented at 8:36 PM on May 28, 2026: member

because the default for them is already 1.

The default was only changed in 24.0, so the earlier versions would still need it. More pertinent is the fact that the old versions are never used to create transactions so the option doesn't matter.

achow101 commented at 8:37 PM on May 28, 2026: member

ACK f701cd159af60ae73275d01f41c45cd173e8f217

achow101 merged this on May 28, 2026

achow101 closed this on May 28, 2026