From 03fa5a1b421be6c07bc5fc33118961d6da93fb65 Mon Sep 17 00:00:00 2001 From: Lawrence Nahum Date: Thu, 13 Jul 2017 00:00:57 +0200 Subject: [PATCH] Allow all mempool transactions to be replaced after a configurable timeout (default 6h) --- src/init.cpp | 8 ++++++++ src/interfaces/chain.cpp | 5 ++++- src/policy/rbf.cpp | 19 +++++++++++++------ src/policy/rbf.h | 9 ++++++--- src/rpc/blockchain.cpp | 5 ++++- src/txmempool.h | 3 ++- src/validation.cpp | 13 +++++-------- src/validation.h | 7 +++++++ src/wallet/feebumper.cpp | 4 +++- test/functional/wallet_bumpfee.py | 26 ++++++++++++++++++++++++++ 10 files changed, 78 insertions(+), 21 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 0898d0ff2582c..2624d09203205 100644 --- a/src/init.cpp +++ b/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=", 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=", 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; } diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 2eecea28d02e1..dda9f00936590 100644 --- a/src/interfaces/chain.cpp +++ b/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 { diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index c73a97fd7d9c3..b89cbeb60c774 100644 --- a/src/policy/rbf.cpp +++ b/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::max(); + const uint64_t noLimit = std::numeric_limits::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; } } diff --git a/src/policy/rbf.h b/src/policy/rbf.h index a4f877731094d..23e60f066f7cc 100644 --- a/src/policy/rbf.h +++ b/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 diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 1827aec637093..21c7b3fcdeff1 100644 --- a/src/rpc/blockchain.cpp +++ b/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) { diff --git a/src/txmempool.h b/src/txmempool.h index a8a0f7fa45459..38097bd8ed7d8 100644 --- a/src/txmempool.h +++ b/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: diff --git a/src/validation.cpp b/src/validation.cpp index a9b5bcf4a4502..67116d52503ff 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -613,6 +613,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // Check for conflicts with in-memory transactions std::set 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"); diff --git a/src/validation.h b/src/validation.h index 0acca013b57b2..d17d139ee7bad 100644 --- a/src/validation.h +++ b/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; diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index a1c3a21d4b186..583f7c613bc7b 100644 --- a/src/wallet/feebumper.cpp +++ b/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; } diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index fc752e5ac0da7..8e277cde0d2ed 100755 --- a/test/functional/wallet_bumpfee.py +++ b/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