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)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31953.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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);
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);
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},
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])
[seq_in] only for assertion because the test explicitly spends 1 input, but okay I guess.
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.
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.
git range-diff bitcoin-core/master A B
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"
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.
Typo in case retouched, fine otherwise.
canceled fullrbf bump
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"
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.
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 actually did not recall 24562 when fixing the docs here. My understanding is that the docs were wrong since they were written in commit cc0243ad32cee1cc9faab317364b889beaf07647. Pulls 15557 and 24562 did not influence the docs being wrong.
it’s pretty confusing and doesn’t have any explanation right now.
Thx, added a note in the commit message body.
0@@ -0,0 +1,9 @@
1+# RPC
In fa16051eac9a4bc1a7e0234b65d615e655ff7cf0 “rpc: Allow fullrbf fee bump”
Please name the file following the expected release notes fragment naming convention.
374@@ -374,7 +375,7 @@ def test_segwit_bumpfee_succeeds(self, rbf_node, dest_address):
375 def test_nonrbf_bumpfee_fails(self, peer_node, dest_address):
376 self.log.info('Test that we cannot replace a non RBF transaction')
In fa16051eac9a4bc1a7e0234b65d615e655ff7cf0 “rpc: Allow fullrbf fee bump”
The test name and logging should be updated to reflect the new behavior of the test.
Also, fix the incorrect documention of the 'replaceable' RPC argument
with respect to sequence number handling. The docs were incorrect
before, so the fix could be extracted, but it seems fine to include here
as well.
ACK https://github.com/bitcoin/bitcoin/pull/31953/commits/fa86190e6ed2aeda7bcceaf96f52403816bcd751
CI error seems to be unrelated
377+def test_nonrbf_bumpfee_succeeds(self, peer_node, dest_address):
378+ self.log.info("Test that we can replace a non RBF transaction")
379 not_rbfid = peer_node.sendtoaddress(dest_address, Decimal("0.00090000"))
380- assert_raises_rpc_error(-4, "Transaction is not BIP 125 replaceable", peer_node.bumpfee, not_rbfid)
381+ peer_node.bumpfee(not_rbfid)
382 self.clear_mempool()
replaceable argument not being passed in bumpfee RPC and the transaction created on the peer_node has walletrbf=0 set, which ultimately leads to BIP 125 replaceability signalling not being set.
It’d be nice to assert for the transaction inputs sequence by using a more generalised version of the check_sequence function introduced in one of the below tests.
reACK fa86190e6ed2aeda7bcceaf96f52403816bcd751
0git range-diff fa16051...fa86190
New changes: Updating the commit message, the release docs, functional test name & logs, and unit test comment.
