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)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31953.
<!--021abf342d371248e50ceaed478a90ca-->
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.
BumpFee(transactionView, txid_non_bip125_replaceable, /*expectDisabled=*/false, /*expectError=*/{}, /*cancel=*/true);
BumpFee(transactionView, txid_bip125_replaceable, /*expectDisabled=*/false, /*expectError=*/{}, /*cancel=*/true);
thx, but leaving as-is for now
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},
Looking forward to seeing this argument going away in future PRs.
I'll open the follow-up when (and if) this is merged
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])
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.
thx, but leaving as-is for now
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.
- // Call bumpfee. Test disabled, canceled, enabled, then failing cases.
+ // Call bumpfee. Test canceled non replaceable, canceled replaceable, enabled replaceable, and then failing cases.
Or something like this.
thx, done. Should be trivial to re-ack with 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.
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.
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.
<details> <summary> psbtbumpfee test in regtest </summary>
bitcoinclireg walletcreatefundedpsbt "[]" "[{\"bcrt1q8gyv2uj2deac6g3msq6esak8fvapdvdz465s3k\": 80}]" 0 "{\"add_inputs\":true,\"fee_rate\":2,\"replaceable\":false}"
bitcoinclireg walletprocesspsbt "cHNidP8BAJoCAAAAAj5AVYuhSrt0xZaYRApBxQR0MVjS0YhzDiXEwOj9PsEAAAAAAAD+////aX6Bb7n4MpjHNZkEPcuI5K1eBaYQ9HzwelwWaD3WjBAAAAAAAP7///8CRrA/cQAAAAAWABTowrnMHK5C7HFLlBkyGnK54zd9EwBQ1twBAAAAFgAUOgjFckpue40iO4A1mHbHSzoWsaIAAAAAAAEAgwIAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD/////AlcA/////wIA8gUqAQAAABYAFDoIxXJKbnuNIjuANZh2x0s6FrGiAAAAAAAAAAAmaiSqIant4vYcP3HR3v0/qZnfo2lTdVxpBol5mWK0i+vYNpdOjPkAAAAAAQEfAPIFKgEAAAAWABQ6CMVySm57jSI7gDWYdsdLOhaxoiIGA5gUzq3yP8MvYPpPYHxIgvIOXLEvdmxmQH28YfDMUeMNGL8RyWdUAACAAQAAgAAAAIAAAAAAAAAAAAABAHECAAAAAY5hvqofhdq6SKRua+9OWNEvMnaNNHGFywd8mIzFr5drAAAAAAD9////AuYPECQBAAAAFgAUjWsi9uTAxdyWUpyl68atDN5ClmgA4fUFAAAAABYAFGjJPkju7W/altbSNNpcPMteBShWAAAAAAEBH+YPECQBAAAAFgAUjWsi9uTAxdyWUpyl68atDN5ClmgiBgITtyEbyfuStNjTxpdGILJeE+09b6x4K92k/8mmYDSpjRi/EclnVAAAgAEAAIAAAACAAQAAAAEAAAAAIgIDVbpaaZcZKgCGZSIfNeYvacQlMwZac0sDxQAnJSrg7q8YvxHJZ1QAAIABAACAAAAAgAEAAAADAAAAACICA5gUzq3yP8MvYPpPYHxIgvIOXLEvdmxmQH28YfDMUeMNGL8RyWdUAACAAQAAgAAAAIAAAAAAAAAAAAA="
bitcoinclireg sendrawtransaction 020000000001023e40558ba14abb74c59698440a41c504743158d2d188730e25c4c0e8fd3ec1000000000000feffffff697e816fb9f83298c73599043dcb88e4ad5e05a610f47cf07a5c16683dd68c100000000000feffffff0246b03f7100000000160014e8c2b9cc1cae42ec714b9419321a72b9e3377d130050d6dc010000001600143a08c5724a6e7b8d223b80359876c74b3a16b1a20247304402207d628207bacbf876218e52ff27028c0d5a0e66f4f7a8c6f1904c225b48686dec022062c0c050cb1b2320bf2f4eff25ff2756a096477e66fb8cf6a2edfafb7e8090110121039814ceadf23fc32f60fa4f607c4882f20e5cb12f766c66407dbc61f0cc51e30d02473044022073866e1db996bd450246c580c20c25a348b8040103166c43ae6a134fc568e06002201691dbe242fcb94e5502ea495a63fe89ded00ee11e67958669e422519f208f9e01210213b7211bc9fb92b4d8d3c6974620b25e13ed3d6fac782bdda4ffc9a66034a98d00000000c9ca0a6675ab35439a4fcc49f2810a1eee16f95d477f8b86e4981b71057e03aa
bitcoinclireg psbtbumpfee "c9ca0a6675ab35439a4fcc49f2810a1eee16f95d477f8b86e4981b71057e03aa" "{\"fee_rate\":5,\"replaceable\":true}"
bitcoinclireg psbtbumpfee "c9ca0a6675ab35439a4fcc49f2810a1eee16f95d477f8b86e4981b71057e03aa" "{\"fee_rate\":5,\"replaceable\":false}"
bitcoinclireg walletprocesspsbt "cHNidP8BAJoCAAAAAj5AVYuhSrt0xZaYRApBxQR0MVjS0YhzDiXEwOj9PsEAAAAAAAD+////aX6Bb7n4MpjHNZkEPcuI5K1eBaYQ9HzwelwWaD3WjBAAAAAAAP7///8C1q0/cQAAAAAWABTowrnMHK5C7HFLlBkyGnK54zd9EwBQ1twBAAAAFgAUOgjFckpue40iO4A1mHbHSzoWsaIAAAAAAAEAgwIAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD/////AlcA/////wIA8gUqAQAAABYAFDoIxXJKbnuNIjuANZh2x0s6FrGiAAAAAAAAAAAmaiSqIant4vYcP3HR3v0/qZnfo2lTdVxpBol5mWK0i+vYNpdOjPkAAAAAAQEfAPIFKgEAAAAWABQ6CMVySm57jSI7gDWYdsdLOhaxoiIGA5gUzq3yP8MvYPpPYHxIgvIOXLEvdmxmQH28YfDMUeMNGL8RyWdUAACAAQAAgAAAAIAAAAAAAAAAAAABAHECAAAAAY5hvqofhdq6SKRua+9OWNEvMnaNNHGFywd8mIzFr5drAAAAAAD9////AuYPECQBAAAAFgAUjWsi9uTAxdyWUpyl68atDN5ClmgA4fUFAAAAABYAFGjJPkju7W/altbSNNpcPMteBShWAAAAAAEBH+YPECQBAAAAFgAUjWsi9uTAxdyWUpyl68atDN5ClmgiBgITtyEbyfuStNjTxpdGILJeE+09b6x4K92k/8mmYDSpjRi/EclnVAAAgAEAAIAAAACAAQAAAAEAAAAAIgIDVbpaaZcZKgCGZSIfNeYvacQlMwZac0sDxQAnJSrg7q8YvxHJZ1QAAIABAACAAAAAgAEAAAADAAAAACICA5gUzq3yP8MvYPpPYHxIgvIOXLEvdmxmQH28YfDMUeMNGL8RyWdUAACAAQAAgAAAAIAAAAAAAAAAAAA="
bitcoinclireg sendrawtransaction "020000000001023e40558ba14abb74c59698440a41c504743158d2d188730e25c4c0e8fd3ec1000000000000feffffff697e816fb9f83298c73599043dcb88e4ad5e05a610f47cf07a5c16683dd68c100000000000feffffff02d6ad3f7100000000160014e8c2b9cc1cae42ec714b9419321a72b9e3377d130050d6dc010000001600143a08c5724a6e7b8d223b80359876c74b3a16b1a20247304402201d3248d7d33bbd7c02c8c5cc13ade38ab54e129841d21fd63a6c523aedf79cf402204194566553ac18d5c92ecce9af065df9ee2c386de07cd7944ff253a829e28fd80121039814ceadf23fc32f60fa4f607c4882f20e5cb12f766c66407dbc61f0cc51e30d0247304402207f7206fc76f367f51727514cd57dc78a92db75b4aeec3cb541c50aee18747f2c0220546e8cc14fa309fafed30ec930e2a264c238819233dd8626563582461bba9cf401210213b7211bc9fb92b4d8d3c6974620b25e13ed3d6fac782bdda4ffc9a66034a98d00000000"e282cd82b55d7a01b741c75fa88de05460cb3241816a83e0f8a34a703277569a"
</details>
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
thx, done
reACK fa16051
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.
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.
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.
thx, done
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.
thx, done
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.
Force pushed some doc fixups. Should be trivial to re-review.
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()
Nit: I missed this in my previous review, but now that the error is not being asserted, this particular test doesn't have any assertions, and understanding it relies on few internals such as the 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
git range-diff fa16051...fa86190
New changes: Updating the commit message, the release docs, functional test name & logs, and unit test comment.
ACK fa86190e6ed2aeda7bcceaf96f52403816bcd751
ACK fa86190e6ed2aeda7bcceaf96f52403816bcd751