Remove -mempoolfullrbf option

instagibbs · instagibbs · commit 111a23d9b361 · 2024-10-28T11:53:20.000-04:00
diff --git a/src/init.cpp b/src/init.cpp
@@ -633,7 +633,6 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
                              "is of this size or less (default: %u)",
                              MAX_OP_RETURN_RELAY),
                    ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
-    argsman.AddArg("-mempoolfullrbf", strprintf("(DEPRECATED) Accept transaction replace-by-fee without requiring replaceability signaling (default: %u)", DEFAULT_MEMPOOL_FULL_RBF), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
     argsman.AddArg("-permitbaremultisig", strprintf("Relay transactions creating non-P2SH multisig outputs (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY,
                    OptionsCategory::NODE_RELAY);
     argsman.AddArg("-minrelaytxfee=<amt>", strprintf("Fees (in %s/kvB) smaller than this are considered zero fee for relaying, mining and transaction creation (default: %s)",
diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h
@@ -21,8 +21,6 @@ static constexpr unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB{300};
 static constexpr unsigned int DEFAULT_BLOCKSONLY_MAX_MEMPOOL_SIZE_MB{5};
 /** Default for -mempoolexpiry, expiration time for mempool transactions in hours */
 static constexpr unsigned int DEFAULT_MEMPOOL_EXPIRY_HOURS{336};
-/** Default for -mempoolfullrbf, if the transaction replaceability signaling is ignored */
-static constexpr bool DEFAULT_MEMPOOL_FULL_RBF{true};
 /** Whether to fall back to legacy V1 serialization when writing mempool.dat */
 static constexpr bool DEFAULT_PERSIST_V1_DAT{false};
 /** Default for -acceptnonstdtxn */
@@ -55,7 +53,6 @@ struct MemPoolOptions {
     std::optional<unsigned> max_datacarrier_bytes{DEFAULT_ACCEPT_DATACARRIER ? std::optional{MAX_OP_RETURN_RELAY} : std::nullopt};
     bool permit_bare_multisig{DEFAULT_PERMIT_BAREMULTISIG};
     bool require_standard{true};
-    bool full_rbf{DEFAULT_MEMPOOL_FULL_RBF};
     bool persist_v1_dat{DEFAULT_PERSIST_V1_DAT};
     MemPoolLimits limits{};
 
diff --git a/src/node/mempool_args.cpp b/src/node/mempool_args.cpp
@@ -92,11 +92,6 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& argsman, const CChainP
         return util::Error{strprintf(Untranslated("acceptnonstdtxn is not currently supported for %s chain"), chainparams.GetChainTypeString())};
     }
 
-    mempool_opts.full_rbf = argsman.GetBoolArg("-mempoolfullrbf", mempool_opts.full_rbf);
-    if (!mempool_opts.full_rbf) {
-        LogInfo("Warning: mempoolfullrbf=0 set but deprecated and will be removed in a future release\n");
-    }
-
     mempool_opts.persist_v1_dat = argsman.GetBoolArg("-persistmempoolv1", mempool_opts.persist_v1_dat);
 
     ApplyArgsManOptions(argsman, mempool_opts.limits);
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
@@ -679,7 +679,7 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
     ret.pushKV("minrelaytxfee", ValueFromAmount(pool.m_opts.min_relay_feerate.GetFeePerK()));
     ret.pushKV("incrementalrelayfee", ValueFromAmount(pool.m_opts.incremental_relay_feerate.GetFeePerK()));
     ret.pushKV("unbroadcastcount", uint64_t{pool.GetUnbroadcastTxs().size()});
-    ret.pushKV("fullrbf", pool.m_opts.full_rbf);
+    ret.pushKV("fullrbf", true);
     return ret;
 }
 
diff --git a/src/validation.cpp b/src/validation.cpp
@@ -820,30 +820,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
         const CTransaction* ptxConflicting = m_pool.GetConflictTx(txin.prevout);
         if (ptxConflicting) {
             if (!args.m_allow_replacement) {
-                // Transaction conflicts with a mempool tx, but we're not allowing replacements.
+                // Transaction conflicts with a mempool tx, but we're not allowing replacements in this context.
                 return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "bip125-replacement-disallowed");
             }
-            if (!ws.m_conflicts.count(ptxConflicting->GetHash()))
-            {
-                // Transactions that don't explicitly signal replaceability are
-                // *not* replaceable with the current logic, even if one of their
-                // unconfirmed ancestors signals replaceability. This diverges
-                // from BIP125's inherited signaling description (see CVE-2021-31876).
-                // Applications relying on first-seen mempool behavior should
-                // check all unconfirmed ancestors; otherwise an opt-in ancestor
-                // might be replaced, causing removal of this descendant.
-                //
-                // All TRUC transactions are considered replaceable.
-                //
-                // Replaceability signaling of the original transactions may be
-                // ignored due to node setting.
-                const bool allow_rbf{m_pool.m_opts.full_rbf || SignalsOptInRBF(*ptxConflicting) || ptxConflicting->version == TRUC_VERSION};
-                if (!allow_rbf) {
-                    return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict");
-                }
-
-                ws.m_conflicts.insert(ptxConflicting->GetHash());
-            }
+            ws.m_conflicts.insert(ptxConflicting->GetHash());
         }
     }
 
diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
@@ -9,7 +9,6 @@
 from test_framework.messages import (
     MAX_BIP125_RBF_SEQUENCE,
     COIN,
-    SEQUENCE_FINAL,
 )
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
@@ -26,18 +25,15 @@ def add_options(self, parser):
 
     def set_test_params(self):
         self.num_nodes = 2
-        # both nodes disable full-rbf to test BIP125 signaling
         self.extra_args = [
             [
-                "-mempoolfullrbf=0",
                 "-limitancestorcount=50",
                 "-limitancestorsize=101",
                 "-limitdescendantcount=200",
                 "-limitdescendantsize=101",
             ],
-            # second node has default mempool parameters, besides mempoolfullrbf being disabled
+            # second node has default mempool parameters
             [
-                "-mempoolfullrbf=0",
             ],
         ]
         self.supports_cli = False
@@ -69,18 +65,12 @@ def run_test(self):
         self.log.info("Running test too many replacements using default mempool params...")
         self.test_too_many_replacements_with_default_mempool_params()
 
-        self.log.info("Running test opt-in...")
-        self.test_opt_in()
-
         self.log.info("Running test RPC...")
         self.test_rpc()
 
         self.log.info("Running test prioritised transactions...")
         self.test_prioritised_transactions()
 
-        self.log.info("Running test no inherited signaling...")
-        self.test_no_inherited_signaling()
-
         self.log.info("Running test replacement relay fee...")
         self.test_replacement_relay_fee()
 
@@ -426,14 +416,12 @@ def test_too_many_replacements_with_default_mempool_params(self):
             for graph_num in range(num_tx_graphs):
                 root_utxos.append(wallet.get_utxo())
 
-                optin_parent_tx = wallet.send_self_transfer_multi(
+                parent_tx = wallet.send_self_transfer_multi(
                     from_node=normal_node,
-                    sequence=MAX_BIP125_RBF_SEQUENCE,
                     utxos_to_spend=[root_utxos[graph_num]],
                     num_outputs=txs_per_graph,
                 )
-                assert_equal(True, normal_node.getmempoolentry(optin_parent_tx['txid'])['bip125-replaceable'])
-                new_utxos = optin_parent_tx['new_utxos']
+                new_utxos = parent_tx['new_utxos']
 
                 for utxo in new_utxos:
                     # Create spends for each output from the "root" of this graph.
@@ -470,81 +458,6 @@ def test_too_many_replacements_with_default_mempool_params(self):
         self.generate(normal_node, 1)
         self.wallet.rescan_utxos()
 
-    def test_opt_in(self):
-        """Replacing should only work if orig tx opted in"""
-        tx0_outpoint = self.make_utxo(self.nodes[0], int(1.1 * COIN))
-
-        # Create a non-opting in transaction
-        tx1a_utxo = self.wallet.send_self_transfer(
-            from_node=self.nodes[0],
-            utxo_to_spend=tx0_outpoint,
-            sequence=SEQUENCE_FINAL,
-            fee=Decimal("0.1"),
-        )["new_utxo"]
-
-        # This transaction isn't shown as replaceable
-        assert_equal(self.nodes[0].getmempoolentry(tx1a_utxo["txid"])['bip125-replaceable'], False)
-
-        # Shouldn't be able to double-spend
-        tx1b_hex = self.wallet.create_self_transfer(
-            utxo_to_spend=tx0_outpoint,
-            sequence=0,
-            fee=Decimal("0.2"),
-        )["hex"]
-
-        # This will raise an exception
-        assert_raises_rpc_error(-26, "txn-mempool-conflict", self.nodes[0].sendrawtransaction, tx1b_hex, 0)
-
-        tx1_outpoint = self.make_utxo(self.nodes[0], int(1.1 * COIN))
-
-        # Create a different non-opting in transaction
-        tx2a_utxo = self.wallet.send_self_transfer(
-            from_node=self.nodes[0],
-            utxo_to_spend=tx1_outpoint,
-            sequence=0xfffffffe,
-            fee=Decimal("0.1"),
-        )["new_utxo"]
-
-        # Still shouldn't be able to double-spend
-        tx2b_hex = self.wallet.create_self_transfer(
-            utxo_to_spend=tx1_outpoint,
-            sequence=0,
-            fee=Decimal("0.2"),
-        )["hex"]
-
-        # This will raise an exception
-        assert_raises_rpc_error(-26, "txn-mempool-conflict", self.nodes[0].sendrawtransaction, tx2b_hex, 0)
-
-        # Now create a new transaction that spends from tx1a and tx2a
-        # opt-in on one of the inputs
-        # Transaction should be replaceable on either input
-
-        tx3a_txid = self.wallet.send_self_transfer_multi(
-            from_node=self.nodes[0],
-            utxos_to_spend=[tx1a_utxo, tx2a_utxo],
-            sequence=[SEQUENCE_FINAL, 0xfffffffd],
-            fee_per_output=int(0.1 * COIN),
-        )["txid"]
-
-        # This transaction is shown as replaceable
-        assert_equal(self.nodes[0].getmempoolentry(tx3a_txid)['bip125-replaceable'], True)
-
-        self.wallet.send_self_transfer(
-            from_node=self.nodes[0],
-            utxo_to_spend=tx1a_utxo,
-            sequence=0,
-            fee=Decimal("0.4"),
-        )
-
-        # If tx3b was accepted, tx3c won't look like a replacement,
-        # but make sure it is accepted anyway
-        self.wallet.send_self_transfer(
-            from_node=self.nodes[0],
-            utxo_to_spend=tx2a_utxo,
-            sequence=0,
-            fee=Decimal("0.4"),
-        )
-
     def test_prioritised_transactions(self):
         # Ensure that fee deltas used via prioritisetransaction are
         # correctly used by replacement logic
@@ -629,69 +542,6 @@ def test_rpc(self):
             assert_equal(json0["vin"][0]["sequence"], 4294967293)
             assert_equal(json1["vin"][0]["sequence"], 4294967294)
 
-    def test_no_inherited_signaling(self):
-        confirmed_utxo = self.wallet.get_utxo()
-
-        # Create an explicitly opt-in parent transaction
-        optin_parent_tx = self.wallet.send_self_transfer(
-            from_node=self.nodes[0],
-            utxo_to_spend=confirmed_utxo,
-            sequence=MAX_BIP125_RBF_SEQUENCE,
-            fee_rate=Decimal('0.01'),
-        )
-        assert_equal(True, self.nodes[0].getmempoolentry(optin_parent_tx['txid'])['bip125-replaceable'])
-
-        replacement_parent_tx = self.wallet.create_self_transfer(
-            utxo_to_spend=confirmed_utxo,
-            sequence=MAX_BIP125_RBF_SEQUENCE,
-            fee_rate=Decimal('0.02'),
-        )
-
-        # Test if parent tx can be replaced.
-        res = self.nodes[0].testmempoolaccept(rawtxs=[replacement_parent_tx['hex']])[0]
-
-        # Parent can be replaced.
-        assert_equal(res['allowed'], True)
-
-        # Create an opt-out child tx spending the opt-in parent
-        parent_utxo = self.wallet.get_utxo(txid=optin_parent_tx['txid'])
-        optout_child_tx = self.wallet.send_self_transfer(
-            from_node=self.nodes[0],
-            utxo_to_spend=parent_utxo,
-            sequence=SEQUENCE_FINAL,
-            fee_rate=Decimal('0.01'),
-        )
-
-        # Reports true due to inheritance
-        assert_equal(True, self.nodes[0].getmempoolentry(optout_child_tx['txid'])['bip125-replaceable'])
-
-        replacement_child_tx = self.wallet.create_self_transfer(
-            utxo_to_spend=parent_utxo,
-            sequence=SEQUENCE_FINAL,
-            fee_rate=Decimal('0.02'),
-        )
-
-        # Broadcast replacement child tx
-        # BIP 125 :
-        # 1. The original transactions signal replaceability explicitly or through inheritance as described in the above
-        # Summary section.
-        # The original transaction (`optout_child_tx`) doesn't signal RBF but its parent (`optin_parent_tx`) does.
-        # The replacement transaction (`replacement_child_tx`) should be able to replace the original transaction.
-        # See CVE-2021-31876 for further explanations.
-        assert_equal(True, self.nodes[0].getmempoolentry(optin_parent_tx['txid'])['bip125-replaceable'])
-        assert_raises_rpc_error(-26, 'txn-mempool-conflict', self.nodes[0].sendrawtransaction, replacement_child_tx["hex"], 0)
-
-        self.log.info('Check that the child tx can still be replaced (via a tx that also replaces the parent)')
-        replacement_parent_tx = self.wallet.send_self_transfer(
-            from_node=self.nodes[0],
-            utxo_to_spend=confirmed_utxo,
-            sequence=SEQUENCE_FINAL,
-            fee_rate=Decimal('0.03'),
-        )
-        # Check that child is removed and update wallet utxo state
-        assert_raises_rpc_error(-5, 'Transaction not in mempool', self.nodes[0].getmempoolentry, optout_child_tx['txid'])
-        self.wallet.get_utxo(txid=optout_child_tx['txid'])
-
     def test_replacement_relay_fee(self):
         tx = self.wallet.send_self_transfer(from_node=self.nodes[0])['tx']
 
@@ -702,12 +552,12 @@ def test_replacement_relay_fee(self):
         assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx.serialize().hex())
 
     def test_fullrbf(self):
+        # BIP125 signaling is not respected
 
         confirmed_utxo = self.make_utxo(self.nodes[0], int(2 * COIN))
-        self.restart_node(0, extra_args=["-mempoolfullrbf=1"])
         assert self.nodes[0].getmempoolinfo()["fullrbf"]
 
-        # Create an explicitly opt-out transaction
+        # Create an explicitly opt-out BIP125 transaction, which will be ignored
         optout_tx = self.wallet.send_self_transfer(
             from_node=self.nodes[0],
             utxo_to_spend=confirmed_utxo,
@@ -718,7 +568,6 @@ def test_fullrbf(self):
 
         conflicting_tx = self.wallet.create_self_transfer(
                 utxo_to_spend=confirmed_utxo,
-                sequence=SEQUENCE_FINAL,
                 fee_rate=Decimal('0.02'),
         )
 
diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
@@ -55,7 +55,6 @@ def set_test_params(self):
         self.num_nodes = 1
         self.extra_args = [[
             '-txindex','-permitbaremultisig=0',
-            '-mempoolfullrbf=0',
         ]] * self.num_nodes
         self.supports_cli = False
 
@@ -155,25 +154,13 @@ def run_test(self):
         self.log.info('A transaction that replaces a mempool transaction')
         tx = tx_from_hex(raw_tx_0)
         tx.vout[0].nValue -= int(fee * COIN)  # Double the fee
-        tx.vin[0].nSequence = MAX_BIP125_RBF_SEQUENCE + 1  # Now, opt out of RBF
         raw_tx_0 = tx.serialize().hex()
         txid_0 = tx.rehash()
         self.check_mempool_result(
             result_expected=[{'txid': txid_0, 'allowed': True, 'vsize': tx.get_vsize(), 'fees': {'base': (2 * fee)}}],
             rawtxs=[raw_tx_0],
         )
-
-        self.log.info('A transaction that conflicts with an unconfirmed tx')
-        # Send the transaction that replaces the mempool transaction and opts out of replaceability
         node.sendrawtransaction(hexstring=tx.serialize().hex(), maxfeerate=0)
-        # take original raw_tx_0
-        tx = tx_from_hex(raw_tx_0)
-        tx.vout[0].nValue -= int(4 * fee * COIN)  # Set more fee
-        self.check_mempool_result(
-            result_expected=[{'txid': tx.rehash(), 'allowed': False, 'reject-reason': 'txn-mempool-conflict'}],
-            rawtxs=[tx.serialize().hex()],
-            maxfeerate=0,
-        )
 
         self.log.info('A transaction with missing inputs, that never existed')
         tx = tx_from_hex(raw_tx_0)
diff --git a/test/functional/mempool_truc.py b/test/functional/mempool_truc.py

Original file line number	Diff line number	Diff line change
`@@ -679,7 +679,7 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)`
`679`	`679`	`ret.pushKV("minrelaytxfee", ValueFromAmount(pool.m_opts.min_relay_feerate.GetFeePerK()));`
`680`	`680`	`ret.pushKV("incrementalrelayfee", ValueFromAmount(pool.m_opts.incremental_relay_feerate.GetFeePerK()));`
`681`	`681`	`ret.pushKV("unbroadcastcount", uint64_t{pool.GetUnbroadcastTxs().size()});`
`682`		`- ret.pushKV("fullrbf", pool.m_opts.full_rbf);`
	`682`	`+ ret.pushKV("fullrbf", true);`
`683`	`683`	`return ret;`
`684`	`684`	`}`
`685`	`685`