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

fa86190 rpc: Allow fullrbf fee bump (MarcoFalke)

Pull request description:

  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 (bitcoin/bitcoin#30592)

ACKs for top commit:
  1440000bytes:
    reACK bitcoin/bitcoin@fa86190
  achow101:
    ACK fa86190
  w0xlt:
    ACK bitcoin/bitcoin@fa86190
  rkrux:
    reACK fa86190
  glozow:
    ACK fa86190

Tree-SHA512: b2ffe8dcadbe71e9be767a16cf8aa0bf383c2de7aa1aee9438d125f444e24f3f7e4f02ddb28981bd3b8b645b6a24a407b4ad6bb0b21946ae637e78f6386e05bf

Author: achow101

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)

src/qt/test/wallettests.cpp

@@ -300,8 +300,8 @@ void TestGUI(interfaces::Node& node, const std::shared_ptr<CWallet>& 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);

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;

src/wallet/rpc/spend.cpp

@@ -995,9 +995,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"
@@ -1017,10 +1017,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"},

test/functional/test_framework/messages.py

@@ -42,6 +42,7 @@
 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

test/functional/wallet_bumpfee.py

@@ -20,6 +20,7 @@
 )
 from test_framework.messages import (
     MAX_BIP125_RBF_SEQUENCE,
+    MAX_SEQUENCE_NONFINAL,
 )
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
@@ -94,7 +95,7 @@ def run_test(self):
             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)
@@ -372,10 +373,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()
 
 
@@ -678,11 +679,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()