Allow all mempool txs to be replaced after a configurable timeout (default 6h)

src/init.cpp

@@ -516,6 +516,8 @@ void SetupServerArgs()
     gArgs.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), false, OptionsCategory::NODE_RELAY);
     gArgs.AddArg("-datacarriersize", strprintf("Maximum size of data in data carrier transactions we relay and mine (default: %u)", MAX_OP_RETURN_RELAY), false, OptionsCategory::NODE_RELAY);
     gArgs.AddArg("-mempoolreplacement", strprintf("Enable transaction replacement in the memory pool (default: %u)", DEFAULT_ENABLE_REPLACEMENT), false, OptionsCategory::NODE_RELAY);
+    gArgs.AddArg("-mempoolreplacementtimeout=<n>", strprintf("Number of seconds after which transactions in mempool can be replaced (default: %u)", DEFAULT_REPLACEMENT_TIMEOUT), false, OptionsCategory::NODE_RELAY);
+    gArgs.AddArg("-enablewalletreplacementtimeout", strprintf("Whether to enable wallet replacement timeout (default: %u)", DEFAULT_WALLET_REPLACEMENT_TIMEOUT), true, OptionsCategory::OPTIONS);
     gArgs.AddArg("-minrelaytxfee=<amt>", strprintf("Fees (in %s/kB) smaller than this are considered zero fee for relaying, mining and transaction creation (default: %s)",
         CURRENCY_UNIT, FormatMoney(DEFAULT_MIN_RELAY_TX_FEE)), false, OptionsCategory::NODE_RELAY);
     gArgs.AddArg("-whitelistforcerelay", strprintf("Force relay of transactions from whitelisted peers even if they violate local relay policy (default: %d)", DEFAULT_WHITELISTFORCERELAY), false, OptionsCategory::NODE_RELAY);
@@ -1171,6 +1173,12 @@ bool AppInitParameterInteraction()
         fEnableReplacement = (std::find(vstrReplacementModes.begin(), vstrReplacementModes.end(), "fee") != vstrReplacementModes.end());
     }
 
+    const int64_t replacement_timeout = gArgs.GetArg("-mempoolreplacementtimeout", DEFAULT_REPLACEMENT_TIMEOUT);
+    const int64_t expiry = gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60;
+    if (replacement_timeout < 0 || replacement_timeout > expiry) {
+        return InitError("mempoolreplacementtimeout has to be a non negative number below or equal to mempoolexpiry (in seconds)");
+    }
+
     return true;
 }
 

src/interfaces/chain.cpp

@@ -201,8 +201,11 @@ class ChainImpl : public Chain
     }
     RBFTransactionState isRBFOptIn(const CTransaction& tx) override
     {
+        const int64_t replacement_timeout = gArgs.GetArg("-mempoolreplacementtimeout", DEFAULT_REPLACEMENT_TIMEOUT);
+        const bool enabled_replacement_timeout = gArgs.GetArg("-enablewalletreplacementtimeout", DEFAULT_WALLET_REPLACEMENT_TIMEOUT);
+
         LOCK(::mempool.cs);
-        return IsRBFOptIn(tx, ::mempool);
+        return IsRBFOptIn(tx, ::mempool, GetTime(), replacement_timeout, enabled_replacement_timeout);
     }
     bool hasDescendantsInMempool(const uint256& txid) override
     {

src/policy/rbf.cpp

@@ -14,14 +14,20 @@ bool SignalsOptInRBF(const CTransaction &tx)
     return false;
 }
 
-RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
+bool ExpiredOptInRBFPolicy(const int64_t now, const int64_t accepted, const int64_t timeout)
+{
+    return now - accepted >= timeout;
+}
+
+RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool, const int64_t now, const int64_t timeout, const bool enabled_replacement_timeout)
 {
     AssertLockHeld(pool.cs);
 
     CTxMemPool::setEntries setAncestors;
 
     // First check the transaction itself.
-    if (SignalsOptInRBF(tx)) {
+    const int64_t conflicting_time = pool.info(tx.GetHash()).nTime;
+    if ((enabled_replacement_timeout && ExpiredOptInRBFPolicy(now, conflicting_time, timeout)) || SignalsOptInRBF(tx)) {
         return RBFTransactionState::REPLACEABLE_BIP125;
     }
 
@@ -33,13 +39,14 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
 
     // If all the inputs have nSequence >= maxint-1, it still might be
     // signaled for RBF if any unconfirmed parents have signaled.
-    uint64_t noLimit = std::numeric_limits<uint64_t>::max();
+    const uint64_t noLimit = std::numeric_limits<uint64_t>::max();
     std::string dummy;
-    CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
+    const CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
     pool.CalculateMemPoolAncestors(entry, setAncestors, noLimit, noLimit, noLimit, noLimit, dummy, false);
 
-    for (CTxMemPool::txiter it : setAncestors) {
-        if (SignalsOptInRBF(it->GetTx())) {
+    for (const CTxMemPool::txiter it : setAncestors) {
+        const int64_t ancestor_time = pool.info(it->GetTx().GetHash()).nTime;
+        if ((enabled_replacement_timeout && ExpiredOptInRBFPolicy(now, ancestor_time, timeout)) || SignalsOptInRBF(it->GetTx())) {
             return RBFTransactionState::REPLACEABLE_BIP125;
         }
     }

src/policy/rbf.h

@@ -19,10 +19,13 @@ enum class RBFTransactionState {
 // opt-in to replace-by-fee, according to BIP 125
 bool SignalsOptInRBF(const CTransaction &tx);
 
-// Determine whether an in-mempool transaction is signaling opt-in to RBF
-// according to BIP 125
+// Check whether the replacement policy has expired
+bool ExpiredOptInRBFPolicy(const int64_t now, const int64_t accepted, const int64_t timeout);
+
+// Determine whether an in-mempool transaction has hit the expired replacement policy or
+// is signaling opt-in to RBF according to BIP 125
 // This involves checking sequence numbers of the transaction, as well
 // as the sequence numbers of all in-mempool ancestors.
-RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
+RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool, const int64_t now, const int64_t timeout, const bool enabled_replacement_timeout) EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
 
 #endif // BITCOIN_POLICY_RBF_H

src/rpc/blockchain.cpp

@@ -455,7 +455,10 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool
 
     // Add opt-in RBF status
     bool rbfStatus = false;
-    RBFTransactionState rbfState = IsRBFOptIn(tx, pool);
+    const int64_t replacement_timeout = gArgs.GetArg("-mempoolreplacementtimeout", DEFAULT_REPLACEMENT_TIMEOUT);
+    const bool enabled_replacement_timeout = gArgs.GetArg("-enablewalletreplacementtimeout", DEFAULT_WALLET_REPLACEMENT_TIMEOUT);
+
+    RBFTransactionState rbfState = IsRBFOptIn(tx, pool, GetTime(), replacement_timeout, enabled_replacement_timeout);
     if (rbfState == RBFTransactionState::UNKNOWN) {
         throw JSONRPCError(RPC_MISC_ERROR, "Transaction is not in mempool");
     } else if (rbfState == RBFTransactionState::REPLACEABLE_BIP125) {

src/txmempool.h

@@ -377,7 +377,8 @@ class SaltedTxidHasher
  * - a transaction which doesn't meet the minimum fee requirements.
  * - a new transaction that double-spends an input of a transaction already in
  * the pool where the new transaction does not meet the Replace-By-Fee
- * requirements as defined in BIP 125.
+ * requirements as defined in BIP 125 and the transaction(s) being replaced
+ * has not been in mempool for over a timeout defaulting to 6 hours.
  * - a non-standard transaction.
  *
  * CTxMemPool::mapTx, and CTxMemPoolEntry bookkeeping:

src/validation.cpp

@@ -613,6 +613,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
 
     // Check for conflicts with in-memory transactions
     std::set<uint256> setConflicts;
+    const int64_t replacement_timeout = gArgs.GetArg("-mempoolreplacementtimeout", DEFAULT_REPLACEMENT_TIMEOUT);
     for (const CTxIn &txin : tx.vin)
     {
         const CTransaction* ptxConflicting = pool.GetConflictTx(txin.prevout);
@@ -631,17 +632,13 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
                 // first-seen mempool behavior should be checking all
                 // unconfirmed ancestors anyway; doing otherwise is hopelessly
                 // insecure.
+                // All transactions in mempool become replaceable after the timeout.
                 bool fReplacementOptOut = true;
                 if (fEnableReplacement)
                 {
-                    for (const CTxIn &_txin : ptxConflicting->vin)
-                    {
-                        if (_txin.nSequence <= MAX_BIP125_RBF_SEQUENCE)
-                        {
-                            fReplacementOptOut = false;
-                            break;
-                        }
-                    }
+                    const int64_t conflicting_time = pool.info(ptxConflicting->GetHash()).nTime;
+                    const bool conflicting_pretimeout = !ExpiredOptInRBFPolicy(nAcceptTime, conflicting_time, replacement_timeout);
+                    fReplacementOptOut = conflicting_pretimeout && !SignalsOptInRBF(*ptxConflicting);
                 }
                 if (fReplacementOptOut) {
                     return state.Invalid(false, REJECT_DUPLICATE, "txn-mempool-conflict");

src/validation.h

@@ -123,6 +123,13 @@ static const unsigned int DEFAULT_BANSCORE_THRESHOLD = 100;
 static const bool DEFAULT_PERSIST_MEMPOOL = true;
 /** Default for -mempoolreplacement */
 static const bool DEFAULT_ENABLE_REPLACEMENT = true;
+/** Default for -mempoolreplacementtimeout in seconds after which transactions in mempool are replaceable (i.e. 6 hours) */
+static const int64_t DEFAULT_REPLACEMENT_TIMEOUT = 6 * 60 * 60;
+/** Default for buffer in seconds on top of the timeout after which transactions in mempool are replaceable (i.e. 6 hours) in the GUI
+ * This buffer is needed because otherwise is more likely ours peers will reject the transaction in case they received it substantial time after us */
+static const int64_t REPLACEMENT_TIMEOUT_BUFFER = 5 * 60;
+/** Default to enable/disable wallet/rpc ability to replace timeout transactions */
+static const bool DEFAULT_WALLET_REPLACEMENT_TIMEOUT = false;
 /** Default for using fee filter */
 static const bool DEFAULT_FEEFILTER = true;
 

src/wallet/feebumper.cpp

@@ -38,7 +38,9 @@ static feebumper::Result PreconditionChecks(interfaces::Chain::Lock& locked_chai
         return feebumper::Result::WALLET_ERROR;
     }
 
-    if (!SignalsOptInRBF(*wtx.tx)) {
+    const int64_t replacement_timeout = gArgs.GetArg("-mempoolreplacementtimeout", DEFAULT_REPLACEMENT_TIMEOUT);
+    const bool replacement_timeout_enabled = gArgs.GetArg("-enablewalletreplacementtimeout", DEFAULT_WALLET_REPLACEMENT_TIMEOUT);
+    if (!(replacement_timeout_enabled && ExpiredOptInRBFPolicy(GetTime(), wtx.nTimeReceived, replacement_timeout + REPLACEMENT_TIMEOUT_BUFFER)) && !SignalsOptInRBF(*wtx.tx)) {
         errors.push_back("Transaction is not BIP 125 replaceable");
         return feebumper::Result::WALLET_ERROR;
     }

test/functional/wallet_bumpfee.py

@@ -15,6 +15,7 @@
 """
 from decimal import Decimal
 import io
+import time
 
 from test_framework.blocktools import add_witness_commitment, create_block, create_coinbase, send_to_witness
 from test_framework.messages import BIP125_SEQUENCE_NUMBER, CTransaction
@@ -63,6 +64,8 @@ def run_test(self):
         test_simple_bumpfee_succeeds(rbf_node, peer_node, dest_address)
         test_segwit_bumpfee_succeeds(rbf_node, dest_address)
         test_nonrbf_bumpfee_fails(peer_node, dest_address)
+        test_nonrbf_bumpfee_fails_then_succeeds(self, dest_address)
+        test_mempool_replacement_timeout(self)
         test_notmine_bumpfee_fails(rbf_node, peer_node, dest_address)
         test_bumpfee_with_descendant_fails(rbf_node, rbf_node_address, dest_address)
         test_small_output_fails(rbf_node, dest_address)
@@ -134,6 +137,29 @@ def test_nonrbf_bumpfee_fails(peer_node, dest_address):
     assert_raises_rpc_error(-4, "not BIP 125 replaceable", peer_node.bumpfee, not_rbfid)
 
 
+def test_mempool_replacement_timeout(self):
+    self.stop_node(0)
+    err_msg = "Error: mempoolreplacementtimeout has to be a non negative number below or equal to mempoolexpiry (in seconds)"
+    self.nodes[0].assert_start_raises_init_error(["-mempoolreplacementtimeout=-1"], err_msg)
+    beyond_expiry = 60 * 60 + 1
+    self.nodes[0].assert_start_raises_init_error(["-mempoolexpiry=1",
+                                                 "-mempoolreplacementtimeout=%d" % (beyond_expiry)], err_msg)
+    self.start_node(0)
+
+
+def test_nonrbf_bumpfee_fails_then_succeeds(self, dest_address):
+    self.restart_node(0, extra_args=["-mempoolreplacementtimeout=30", "-enablewalletreplacementtimeout=1"])
+    not_rbfid = self.nodes[0].sendtoaddress(dest_address, Decimal("0.00090000"))
+    assert_raises_rpc_error(-4, "not BIP 125 replaceable", self.nodes[0].bumpfee, not_rbfid)
+    self.nodes[0].setmocktime(int(time.time()) + 30 + 5 * 60)
+    bumped_tx = self.nodes[0].bumpfee(not_rbfid)
+    rawmempool = self.nodes[0].getrawmempool()
+    assert bumped_tx["txid"] in rawmempool
+    assert not_rbfid not in rawmempool
+    self.nodes[0].setmocktime(0)
+    self.restart_node(0)
+
+
 def test_notmine_bumpfee_fails(rbf_node, peer_node, dest_address):
     # cannot bump fee unless the tx has only inputs that we own.
     # here, the rbftx has a peer_node coin and then adds a rbf_node input