From 111a23d9b3615094fbfdf6cc8c996adc3db2782c Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Thu, 1 Aug 2024 12:26:47 -0400 Subject: [PATCH] Remove -mempoolfullrbf option --- src/init.cpp | 1 - src/kernel/mempool_options.h | 3 - src/node/mempool_args.cpp | 5 - src/rpc/mempool.cpp | 2 +- src/validation.cpp | 24 +---- test/functional/feature_rbf.py | 161 +----------------------------- test/functional/mempool_accept.py | 13 --- test/functional/mempool_truc.py | 29 ------ 8 files changed, 8 insertions(+), 230 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 3b8474dd469..8d89222829b 100644 --- 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=", 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 index 4e1e24a11df..d57dbb393f3 100644 --- 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 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 index a488c1b1498..0b6725ee1be 100644 --- a/src/node/mempool_args.cpp +++ b/src/node/mempool_args.cpp @@ -92,11 +92,6 @@ util::Result 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 index d61898260bf..aacdbe4e8b7 100644 --- 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 index 5da3387ad29..93336019210 100644 --- 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 index b660b96935d..d9700e2ee2d 100755 --- a/test/functional/feature_rbf.py +++ b/test/functional/feature_rbf.py @@ -9,7 +9,6 @@ from decimal import Decimal 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 @@ class ReplaceByFeeTest(BitcoinTestFramework): 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 @@ class ReplaceByFeeTest(BitcoinTestFramework): 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 @@ class ReplaceByFeeTest(BitcoinTestFramework): 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 @@ class ReplaceByFeeTest(BitcoinTestFramework): 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 @@ class ReplaceByFeeTest(BitcoinTestFramework): 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 @@ class ReplaceByFeeTest(BitcoinTestFramework): 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 @@ class ReplaceByFeeTest(BitcoinTestFramework): 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 index 4d08575255b..ea609bbb343 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -55,7 +55,6 @@ class MempoolAcceptanceTest(BitcoinTestFramework): self.num_nodes = 1 self.extra_args = [[ '-txindex','-permitbaremultisig=0', - '-mempoolfullrbf=0', ]] * self.num_nodes self.supports_cli = False @@ -155,25 +154,13 @@ class MempoolAcceptanceTest(BitcoinTestFramework): 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 index 54a258215dc..ff754283269 100755 --- a/test/functional/mempool_truc.py +++ b/test/functional/mempool_truc.py @@ -4,9 +4,6 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. from decimal import Decimal -from test_framework.messages import ( - MAX_BIP125_RBF_SEQUENCE, -) from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, @@ -115,7 +112,6 @@ class MempoolTRUC(BitcoinTestFramework): from_node=node, fee_rate=DEFAULT_FEE, utxo_to_spend=utxo_v3_bip125, - sequence=MAX_BIP125_RBF_SEQUENCE, version=3 ) self.check_mempool([tx_v3_bip125["txid"]]) @@ -163,30 +159,6 @@ class MempoolTRUC(BitcoinTestFramework): self.check_mempool([tx_v3_bip125_rbf_v2["txid"], tx_v3_parent["txid"], tx_v3_child["txid"]]) - @cleanup(extra_args=["-mempoolfullrbf=0"]) - def test_truc_bip125(self): - node = self.nodes[0] - self.log.info("Test TRUC transactions that don't signal BIP125 are replaceable") - assert_equal(node.getmempoolinfo()["fullrbf"], False) - utxo_v3_no_bip125 = self.wallet.get_utxo() - tx_v3_no_bip125 = self.wallet.send_self_transfer( - from_node=node, - fee_rate=DEFAULT_FEE, - utxo_to_spend=utxo_v3_no_bip125, - sequence=MAX_BIP125_RBF_SEQUENCE + 1, - version=3 - ) - - self.check_mempool([tx_v3_no_bip125["txid"]]) - assert not node.getmempoolentry(tx_v3_no_bip125["txid"])["bip125-replaceable"] - tx_v3_no_bip125_rbf = self.wallet.send_self_transfer( - from_node=node, - fee_rate=DEFAULT_FEE * 2, - utxo_to_spend=utxo_v3_no_bip125, - version=3 - ) - self.check_mempool([tx_v3_no_bip125_rbf["txid"]]) - @cleanup(extra_args=["-datacarriersize=40000"]) def test_truc_reorg(self): node = self.nodes[0] @@ -631,7 +603,6 @@ class MempoolTRUC(BitcoinTestFramework): self.test_truc_max_vsize() self.test_truc_acceptance() self.test_truc_replacement() - self.test_truc_bip125() self.test_truc_reorg() self.test_nondefault_package_limits() self.test_truc_ancestors_package()