rpc: Allow fullrbf fee bump in (psbt)bumpfee

maflcko commented at 4:12 pm on February 25, 2025: member

The RPCs (psbt)bumpfee, and the GUI, reject fee bumps when BIP 125 signalling is absent in the transaction even when the mempool and other RPCs allow them. Fix the confusion by allowing the fee bump.

This is done after fullrbf is always on (https://github.com/bitcoin/bitcoin/pull/30592)

DrahtBot commented at 4:13 pm on February 25, 2025: 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/31953.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, 1440000bytes, ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label RPC/REST/ZMQ on Feb 25, 2025

maflcko force-pushed on Feb 25, 2025

in src/qt/test/wallettests.cpp:305 in fa2068c589 outdated

300@@ -301,7 +301,7 @@ void TestGUI(interfaces::Node& node, const std::shared_ptr<CWallet>& wallet)
301     QVERIFY(FindTx(*transactionTableModel, txid2).isValid());
302 
303     // Call bumpfee. Test disabled, canceled, enabled, then failing cases.
304-    BumpFee(transactionView, txid1, /*expectDisabled=*/true, /*expectError=*/"not BIP 125 replaceable", /*cancel=*/false);
305+    BumpFee(transactionView, txid1, /*expectDisabled=*/false, /*expectError=*/{}, /*cancel=*/true);
306     BumpFee(transactionView, txid2, /*expectDisabled=*/false, /*expectError=*/{}, /*cancel=*/true);

rkrux commented at 9:00 am on February 27, 2025:

Nit: Not introduced in this PR instead older code but in case you retouch and agree, this is to improve readability. Otherwise it makes one wonder what’s the difference b/w the 2 tests.

0BumpFee(transactionView, txid_non_bip125_replaceable, /*expectDisabled=*/false, /*expectError=*/{}, /*cancel=*/true);
1BumpFee(transactionView, txid_bip125_replaceable, /*expectDisabled=*/false, /*expectError=*/{}, /*cancel=*/true);

maflcko commented at 10:19 am on February 27, 2025:

thx, but leaving as-is for now

in src/wallet/rpc/spend.cpp:1015 in fa2068c589 outdated

1011@@ -1012,10 +1012,11 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
1012                              "\nSpecify a fee rate in " + CURRENCY_ATOM + "/vB instead of relying on the built-in fee estimator.\n"
1013                              "Must be at least " + incremental_fee + " higher than the current transaction fee rate.\n"
1014                              "WARNING: before version 0.21, fee_rate was in " + CURRENCY_UNIT + "/kvB. As of 0.21, fee_rate is in " + CURRENCY_ATOM + "/vB.\n"},
1015-                    {"replaceable", RPCArg::Type::BOOL, RPCArg::Default{true}, "Whether the new transaction should still be\n"
1016+                    {"replaceable", RPCArg::Type::BOOL, RPCArg::Default{true},

rkrux commented at 9:46 am on February 27, 2025:

Looking forward to seeing this argument going away in future PRs.

maflcko commented at 10:18 am on February 27, 2025:

I’ll open the follow-up when (and if) this is merged

in test/functional/wallet_bumpfee.py:688 in fa2068c589 outdated

684+
685+    def check_sequence(tx, seq_in):
686+        tx = rbf_node.getrawtransaction(tx["txid"])
687+        tx = rbf_node.decoderawtransaction(tx)
688+        seq = [i["sequence"] for i in tx["vin"]]
689+        assert_equal(seq, [seq_in])

rkrux commented at 9:50 am on February 27, 2025:

Not a fan of looping here and explicitly creating an array of just 1 element [seq_in] only for assertion because the test explicitly spends 1 input, but okay I guess.

maflcko commented at 10:20 am on February 27, 2025:

thx, but leaving as-is for now

in src/qt/test/wallettests.cpp:303 in fa2068c589 outdated

300@@ -301,7 +301,7 @@ void TestGUI(interfaces::Node& node, const std::shared_ptr<CWallet>& wallet)
301     QVERIFY(FindTx(*transactionTableModel, txid2).isValid());
302 
303     // Call bumpfee. Test disabled, canceled, enabled, then failing cases.

rkrux commented at 10:11 am on February 27, 2025:

0- // Call bumpfee. Test disabled, canceled, enabled, then failing cases.
1+ // Call bumpfee. Test canceled non replaceable, canceled replaceable, enabled replaceable, and then failing cases.

Or something like this.

maflcko commented at 10:24 am on February 27, 2025:

thx, done. Should be trivial to re-ack with git range-diff bitcoin-core/master A B

rkrux approved

rkrux commented at 10:12 am on February 27, 2025: contributor

tACK fa2068c58905d027918e2d3917deeeec020681f3

Definitely in favour of this because it cleans up remnants of the recently outdated BIP 125 signalling flag in transaction creation and propagation. The PR description can be updated though to mention the case of BIP 125 signalling.

0The RPCs (psbt)bumpfee, and the GUI, reject fee bumps when BIP 125 signalling is absent in the transaction even when the mempool and other RPCs allow them.

Besides the tests, I tested the scenario in my regtest like below. Bumped the PSBT twice each with replaceable true and false, and verified the sequence numbers.

0bitcoinclireg walletcreatefundedpsbt "[]" "[{\"bcrt1q8gyv2uj2deac6g3msq6esak8fvapdvdz465s3k\": 80}]" 0 "{\"add_inputs\":true,\"fee_rate\":2,\"replaceable\":false}"
1bitcoinclireg walletprocesspsbt "cHNidP8BAJoCAAAAAj5AVYuhSrt0xZaYRApBxQR0MVjS0YhzDiXEwOj9PsEAAAAAAAD+////aX6Bb7n4MpjHNZkEPcuI5K1eBaYQ9HzwelwWaD3WjBAAAAAAAP7///8CRrA/cQAAAAAWABTowrnMHK5C7HFLlBkyGnK54zd9EwBQ1twBAAAAFgAUOgjFckpue40iO4A1mHbHSzoWsaIAAAAAAAEAgwIAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD/////AlcA/////wIA8gUqAQAAABYAFDoIxXJKbnuNIjuANZh2x0s6FrGiAAAAAAAAAAAmaiSqIant4vYcP3HR3v0/qZnfo2lTdVxpBol5mWK0i+vYNpdOjPkAAAAAAQEfAPIFKgEAAAAWABQ6CMVySm57jSI7gDWYdsdLOhaxoiIGA5gUzq3yP8MvYPpPYHxIgvIOXLEvdmxmQH28YfDMUeMNGL8RyWdUAACAAQAAgAAAAIAAAAAAAAAAAAABAHECAAAAAY5hvqofhdq6SKRua+9OWNEvMnaNNHGFywd8mIzFr5drAAAAAAD9////AuYPECQBAAAAFgAUjWsi9uTAxdyWUpyl68atDN5ClmgA4fUFAAAAABYAFGjJPkju7W/altbSNNpcPMteBShWAAAAAAEBH+YPECQBAAAAFgAUjWsi9uTAxdyWUpyl68atDN5ClmgiBgITtyEbyfuStNjTxpdGILJeE+09b6x4K92k/8mmYDSpjRi/EclnVAAAgAEAAIAAAACAAQAAAAEAAAAAIgIDVbpaaZcZKgCGZSIfNeYvacQlMwZac0sDxQAnJSrg7q8YvxHJZ1QAAIABAACAAAAAgAEAAAADAAAAACICA5gUzq3yP8MvYPpPYHxIgvIOXLEvdmxmQH28YfDMUeMNGL8RyWdUAACAAQAAgAAAAIAAAAAAAAAAAAA="
2bitcoinclireg sendrawtransaction 020000000001023e40558ba14abb74c59698440a41c504743158d2d188730e25c4c0e8fd3ec1000000000000feffffff697e816fb9f83298c73599043dcb88e4ad5e05a610f47cf07a5c16683dd68c100000000000feffffff0246b03f7100000000160014e8c2b9cc1cae42ec714b9419321a72b9e3377d130050d6dc010000001600143a08c5724a6e7b8d223b80359876c74b3a16b1a20247304402207d628207bacbf876218e52ff27028c0d5a0e66f4f7a8c6f1904c225b48686dec022062c0c050cb1b2320bf2f4eff25ff2756a096477e66fb8cf6a2edfafb7e8090110121039814ceadf23fc32f60fa4f607c4882f20e5cb12f766c66407dbc61f0cc51e30d02473044022073866e1db996bd450246c580c20c25a348b8040103166c43ae6a134fc568e06002201691dbe242fcb94e5502ea495a63fe89ded00ee11e67958669e422519f208f9e01210213b7211bc9fb92b4d8d3c6974620b25e13ed3d6fac782bdda4ffc9a66034a98d00000000c9ca0a6675ab35439a4fcc49f2810a1eee16f95d477f8b86e4981b71057e03aa
3bitcoinclireg psbtbumpfee "c9ca0a6675ab35439a4fcc49f2810a1eee16f95d477f8b86e4981b71057e03aa" "{\"fee_rate\":5,\"replaceable\":true}"
4bitcoinclireg psbtbumpfee "c9ca0a6675ab35439a4fcc49f2810a1eee16f95d477f8b86e4981b71057e03aa" "{\"fee_rate\":5,\"replaceable\":false}"
5bitcoinclireg walletprocesspsbt "cHNidP8BAJoCAAAAAj5AVYuhSrt0xZaYRApBxQR0MVjS0YhzDiXEwOj9PsEAAAAAAAD+////aX6Bb7n4MpjHNZkEPcuI5K1eBaYQ9HzwelwWaD3WjBAAAAAAAP7///8C1q0/cQAAAAAWABTowrnMHK5C7HFLlBkyGnK54zd9EwBQ1twBAAAAFgAUOgjFckpue40iO4A1mHbHSzoWsaIAAAAAAAEAgwIAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD/////AlcA/////wIA8gUqAQAAABYAFDoIxXJKbnuNIjuANZh2x0s6FrGiAAAAAAAAAAAmaiSqIant4vYcP3HR3v0/qZnfo2lTdVxpBol5mWK0i+vYNpdOjPkAAAAAAQEfAPIFKgEAAAAWABQ6CMVySm57jSI7gDWYdsdLOhaxoiIGA5gUzq3yP8MvYPpPYHxIgvIOXLEvdmxmQH28YfDMUeMNGL8RyWdUAACAAQAAgAAAAIAAAAAAAAAAAAABAHECAAAAAY5hvqofhdq6SKRua+9OWNEvMnaNNHGFywd8mIzFr5drAAAAAAD9////AuYPECQBAAAAFgAUjWsi9uTAxdyWUpyl68atDN5ClmgA4fUFAAAAABYAFGjJPkju7W/altbSNNpcPMteBShWAAAAAAEBH+YPECQBAAAAFgAUjWsi9uTAxdyWUpyl68atDN5ClmgiBgITtyEbyfuStNjTxpdGILJeE+09b6x4K92k/8mmYDSpjRi/EclnVAAAgAEAAIAAAACAAQAAAAEAAAAAIgIDVbpaaZcZKgCGZSIfNeYvacQlMwZac0sDxQAnJSrg7q8YvxHJZ1QAAIABAACAAAAAgAEAAAADAAAAACICA5gUzq3yP8MvYPpPYHxIgvIOXLEvdmxmQH28YfDMUeMNGL8RyWdUAACAAQAAgAAAAIAAAAAAAAAAAAA="
6bitcoinclireg sendrawtransaction "020000000001023e40558ba14abb74c59698440a41c504743158d2d188730e25c4c0e8fd3ec1000000000000feffffff697e816fb9f83298c73599043dcb88e4ad5e05a610f47cf07a5c16683dd68c100000000000feffffff02d6ad3f7100000000160014e8c2b9cc1cae42ec714b9419321a72b9e3377d130050d6dc010000001600143a08c5724a6e7b8d223b80359876c74b3a16b1a20247304402201d3248d7d33bbd7c02c8c5cc13ade38ab54e129841d21fd63a6c523aedf79cf402204194566553ac18d5c92ecce9af065df9ee2c386de07cd7944ff253a829e28fd80121039814ceadf23fc32f60fa4f607c4882f20e5cb12f766c66407dbc61f0cc51e30d0247304402207f7206fc76f367f51727514cd57dc78a92db75b4aeec3cb541c50aee18747f2c0220546e8cc14fa309fafed30ec930e2a264c238819233dd8626563582461bba9cf401210213b7211bc9fb92b4d8d3c6974620b25e13ed3d6fac782bdda4ffc9a66034a98d00000000"e282cd82b55d7a01b741c75fa88de05460cb3241816a83e0f8a34a703277569a"

rpc: Allow fullrbf fee bump fa16051eac

maflcko force-pushed on Feb 27, 2025

in src/qt/test/wallettests.cpp:303 in fa16051eac

299@@ -300,8 +300,8 @@ void TestGUI(interfaces::Node& node, const std::shared_ptr<CWallet>& wallet)
300     QVERIFY(FindTx(*transactionTableModel, txid1).isValid());
301     QVERIFY(FindTx(*transactionTableModel, txid2).isValid());
302 
303-    // Call bumpfee. Test disabled, canceled, enabled, then failing cases.
304-    BumpFee(transactionView, txid1, /*expectDisabled=*/true, /*expectError=*/"not BIP 125 replaceable", /*cancel=*/false);
305+    // Call bumpfee. Test canceled fullrb bump, canceled bip-125-rbf bump, passing bump, and then failing bump.

rkrux commented at 10:41 am on February 27, 2025:

Typo in case retouched, fine otherwise.

canceled fullrbf bump

rkrux approved

rkrux commented at 10:42 am on February 27, 2025: contributor

reACK fa16051

glozow added this to the milestone 30.0 on Mar 3, 2025

1440000bytes commented at 4:23 pm on March 3, 2025: none

utACK https://github.com/bitcoin/bitcoin/pull/31953/commits/fa16051eac9a4bc1a7e0234b65d615e655ff7cf0

in src/wallet/rpc/spend.cpp:1018 in fa16051eac

1011@@ -1012,10 +1012,11 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
1012                              "\nSpecify a fee rate in " + CURRENCY_ATOM + "/vB instead of relying on the built-in fee estimator.\n"
1013                              "Must be at least " + incremental_fee + " higher than the current transaction fee rate.\n"
1014                              "WARNING: before version 0.21, fee_rate was in " + CURRENCY_UNIT + "/kvB. As of 0.21, fee_rate is in " + CURRENCY_ATOM + "/vB.\n"},
1015-                    {"replaceable", RPCArg::Type::BOOL, RPCArg::Default{true}, "Whether the new transaction should still be\n"
1016+                    {"replaceable", RPCArg::Type::BOOL, RPCArg::Default{true},
1017+                             "Whether the new transaction should be\n"
1018                              "marked bip-125 replaceable. If true, the sequence numbers in the transaction will\n"
1019-                             "be left unchanged from the original. If false, any input sequence numbers in the\n"
1020-                             "original transaction that were less than 0xfffffffe will be increased to 0xfffffffe\n"

ryanofsky commented at 2:02 pm on March 23, 2025:

In commit “rpc: Allow fullrbf fee bump” (fa16051eac9a4bc1a7e0234b65d615e655ff7cf0)

New comment here seems right, and old comment here seems wrong, but I’m confused about how code got in the current state. It looks like maybe the documentation here was written before #15557 and #15557 tried to preserve the behavior that’s described but accidentally changed it because the code added to do that was dead and didn’t work. Then the dead code was removed in #24562 and the documentation was not updated then either. Now the documentation is being updated in this PR, even though this PR is not changing that behavior? Is that correct?

I think would recommend moving this documentation change into a separate commit because it’s pretty confusing and doesn’t have any explanation right now. Also it seems not directly related to the other changes in this commit.

ryanofsky approved

ryanofsky commented at 2:06 pm on March 23, 2025: contributor

Code review ACK fa16051eac9a4bc1a7e0234b65d615e655ff7cf0, but I was confused by a documentation change here, and think it would be nice to update the PR to make that change clearer, and also fix the other typo that was pointed out.

rpc: Allow fullrbf fee bump in (psbt)bumpfee #31953

Code Coverage & Benchmarks

Reviews