From fa86190e6ed2aeda7bcceaf96f52403816bcd751 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 25 Feb 2025 17:17:01 +0100 Subject: [PATCH] rpc: Allow fullrbf fee bump 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. --- doc/release-notes-31953.md | 9 ++++++++ src/qt/test/wallettests.cpp | 4 ++-- src/wallet/feebumper.cpp | 5 ----- src/wallet/rpc/spend.cpp | 11 +++++----- test/functional/test_framework/messages.py | 1 + test/functional/wallet_bumpfee.py | 24 +++++++++++++++------- 6 files changed, 35 insertions(+), 19 deletions(-) create mode 100644 doc/release-notes-31953.md diff --git a/doc/release-notes-31953.md b/doc/release-notes-31953.md new file mode 100644 index 00000000000..74d7ecd3f2c --- /dev/null +++ b/doc/release-notes-31953.md @@ -0,0 +1,9 @@ +# RPC + +- The RPCs psbtbumpfee and bumpfee allow a replacement under fullrbf and no + longer require BIP-125 signalling. (#31953) + +# GUI + +- A transaction's fee bump is allowed under fullrbf and no longer requires + BIP-125 signalling. (#31953) diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 98dfe12f084..53dfa2e6eaa 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -300,8 +300,8 @@ void TestGUI(interfaces::Node& node, const std::shared_ptr& wallet) QVERIFY(FindTx(*transactionTableModel, txid1).isValid()); QVERIFY(FindTx(*transactionTableModel, txid2).isValid()); - // Call bumpfee. Test disabled, canceled, enabled, then failing cases. - BumpFee(transactionView, txid1, /*expectDisabled=*/true, /*expectError=*/"not BIP 125 replaceable", /*cancel=*/false); + // Call bumpfee. Test canceled fullrbf bump, canceled bip-125-rbf bump, passing bump, and then failing bump. + BumpFee(transactionView, txid1, /*expectDisabled=*/false, /*expectError=*/{}, /*cancel=*/true); BumpFee(transactionView, txid2, /*expectDisabled=*/false, /*expectError=*/{}, /*cancel=*/true); BumpFee(transactionView, txid2, /*expectDisabled=*/false, /*expectError=*/{}, /*cancel=*/false); BumpFee(transactionView, txid2, /*expectDisabled=*/true, /*expectError=*/"already bumped", /*cancel=*/false); diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index b875461c9f2..f17d100e750 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -40,11 +40,6 @@ static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWallet return feebumper::Result::WALLET_ERROR; } - if (!SignalsOptInRBF(*wtx.tx)) { - errors.emplace_back(Untranslated("Transaction is not BIP 125 replaceable")); - return feebumper::Result::WALLET_ERROR; - } - if (wtx.mapValue.count("replaced_by_txid")) { errors.push_back(Untranslated(strprintf("Cannot bump transaction %s which was already bumped by transaction %s", wtx.GetHash().ToString(), wtx.mapValue.at("replaced_by_txid")))); return feebumper::Result::WALLET_ERROR; diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 0cba830e2ab..979be3e61c1 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -990,9 +990,9 @@ static RPCHelpMan bumpfee_helper(std::string method_name) const std::string incremental_fee{CFeeRate(DEFAULT_INCREMENTAL_RELAY_FEE).ToString(FeeEstimateMode::SAT_VB)}; return RPCHelpMan{method_name, - "\nBumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.\n" + "Bumps the fee of a transaction T, replacing it with a new transaction B.\n" + std::string(want_psbt ? "Returns a PSBT instead of creating and signing a new transaction.\n" : "") + - "An opt-in RBF transaction with the given txid must be in the wallet.\n" + "A transaction with the given txid must be in the wallet.\n" "The command will pay the additional fee by reducing change outputs or adding inputs when necessary.\n" "It may add a new change output if one does not already exist.\n" "All inputs in the original transaction will be included in the replacement transaction.\n" @@ -1012,10 +1012,11 @@ static RPCHelpMan bumpfee_helper(std::string method_name) "\nSpecify a fee rate in " + CURRENCY_ATOM + "/vB instead of relying on the built-in fee estimator.\n" "Must be at least " + incremental_fee + " higher than the current transaction fee rate.\n" "WARNING: before version 0.21, fee_rate was in " + CURRENCY_UNIT + "/kvB. As of 0.21, fee_rate is in " + CURRENCY_ATOM + "/vB.\n"}, - {"replaceable", RPCArg::Type::BOOL, RPCArg::Default{true}, "Whether the new transaction should still be\n" + {"replaceable", RPCArg::Type::BOOL, RPCArg::Default{true}, + "Whether the new transaction should be\n" "marked bip-125 replaceable. If true, the sequence numbers in the transaction will\n" - "be left unchanged from the original. If false, any input sequence numbers in the\n" - "original transaction that were less than 0xfffffffe will be increased to 0xfffffffe\n" + "be set to 0xfffffffd. If false, any input sequence numbers in the\n" + "transaction will be set to 0xfffffffe\n" "so the new transaction will not be explicitly bip-125 replaceable (though it may\n" "still be replaceable in practice, for example if it has unconfirmed ancestors which\n" "are replaceable).\n"}, diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index 9ebb683a9db..1ae9bbb8176 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -42,6 +42,7 @@ COIN = 100000000 # 1 btc in satoshis MAX_MONEY = 21000000 * COIN MAX_BIP125_RBF_SEQUENCE = 0xfffffffd # Sequence number that is rbf-opt-in (BIP 125) and csv-opt-out (BIP 68) +MAX_SEQUENCE_NONFINAL = 0xfffffffe # Sequence number that is csv-opt-out (BIP 68) SEQUENCE_FINAL = 0xffffffff # Sequence number that disables nLockTime if set for every input of a tx MAX_PROTOCOL_MESSAGE_LENGTH = 4000000 # Maximum length of incoming protocol messages diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index 061e9f2caa1..365a87d27f8 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -20,6 +20,7 @@ from test_framework.blocktools import ( ) from test_framework.messages import ( MAX_BIP125_RBF_SEQUENCE, + MAX_SEQUENCE_NONFINAL, ) from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( @@ -93,7 +94,7 @@ class BumpFeeTest(BitcoinTestFramework): test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address) self.test_invalid_parameters(rbf_node, peer_node, dest_address) test_segwit_bumpfee_succeeds(self, rbf_node, dest_address) - test_nonrbf_bumpfee_fails(self, peer_node, dest_address) + test_nonrbf_bumpfee_succeeds(self, peer_node, dest_address) test_notmine_bumpfee(self, rbf_node, peer_node, dest_address) test_bumpfee_with_descendant_fails(self, rbf_node, rbf_node_address, dest_address) test_bumpfee_with_abandoned_descendant_succeeds(self, rbf_node, rbf_node_address, dest_address) @@ -371,10 +372,10 @@ def test_segwit_bumpfee_succeeds(self, rbf_node, dest_address): self.clear_mempool() -def test_nonrbf_bumpfee_fails(self, peer_node, dest_address): - self.log.info('Test that we cannot replace a non RBF transaction') +def test_nonrbf_bumpfee_succeeds(self, peer_node, dest_address): + self.log.info("Test that we can replace a non RBF transaction") not_rbfid = peer_node.sendtoaddress(dest_address, Decimal("0.00090000")) - assert_raises_rpc_error(-4, "Transaction is not BIP 125 replaceable", peer_node.bumpfee, not_rbfid) + peer_node.bumpfee(not_rbfid) self.clear_mempool() @@ -677,11 +678,20 @@ def test_rebumping(self, rbf_node, dest_address): def test_rebumping_not_replaceable(self, rbf_node, dest_address): - self.log.info('Test that re-bumping non-replaceable fails') + self.log.info("Test that re-bumping non-replaceable passes") rbfid = spend_one_input(rbf_node, dest_address) + + def check_sequence(tx, seq_in): + tx = rbf_node.getrawtransaction(tx["txid"]) + tx = rbf_node.decoderawtransaction(tx) + seq = [i["sequence"] for i in tx["vin"]] + assert_equal(seq, [seq_in]) + bumped = rbf_node.bumpfee(rbfid, fee_rate=ECONOMICAL, replaceable=False) - assert_raises_rpc_error(-4, "Transaction is not BIP 125 replaceable", rbf_node.bumpfee, bumped["txid"], - {"fee_rate": NORMAL}) + check_sequence(bumped, MAX_SEQUENCE_NONFINAL) + bumped = rbf_node.bumpfee(bumped["txid"], {"fee_rate": NORMAL}) + check_sequence(bumped, MAX_BIP125_RBF_SEQUENCE) + self.clear_mempool()