diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 8125b41c41..43624c7993 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -3,6 +3,10 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include + +#include +#include +#include #include RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) @@ -42,3 +46,34 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx) // If we don't have a local mempool we can only check the transaction itself. return SignalsOptInRBF(tx) ? RBFTransactionState::REPLACEABLE_BIP125 : RBFTransactionState::UNKNOWN; } + +bool GetEntriesForConflicts(const CTransaction& tx, + CTxMemPool& m_pool, + const CTxMemPool::setEntries& setIterConflicting, + CTxMemPool::setEntries& allConflicting, + std::string& err_string) +{ + AssertLockHeld(m_pool.cs); + const uint256 hash = tx.GetHash(); + uint64_t nConflictingCount = 0; + for (const auto& mi : setIterConflicting) { + nConflictingCount += mi->GetCountWithDescendants(); + // This potentially overestimates the number of actual descendants + // but we just want to be conservative to avoid doing too much + // work. + if (nConflictingCount > MAX_BIP125_REPLACEMENT_CANDIDATES) { + err_string = strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", + hash.ToString(), + nConflictingCount, + MAX_BIP125_REPLACEMENT_CANDIDATES); + return false; + } + } + // If not too many to replace, then calculate the set of + // transactions that would have to be evicted + for (CTxMemPool::txiter it : setIterConflicting) { + m_pool.CalculateDescendants(it, allConflicting); + } + return true; +} + diff --git a/src/policy/rbf.h b/src/policy/rbf.h index e078070c1c..a67e9058df 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -7,6 +7,10 @@ #include +/** Maximum number of transactions that can be replaced by BIP125 RBF (Rule #5). This includes all + * mempool conflicts and their descendants. */ +static constexpr uint32_t MAX_BIP125_REPLACEMENT_CANDIDATES{100}; + /** The rbf state of unconfirmed transactions */ enum class RBFTransactionState { /** Unconfirmed tx that does not signal rbf and is not in the mempool */ @@ -31,4 +35,19 @@ enum class RBFTransactionState { RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(pool.cs); RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx); +/** Get all descendants of setIterConflicting. Also enforce BIP125 Rule #5, "The number of original + * transactions to be replaced and their descendant transactions which will be evicted from the + * mempool must not exceed a total of 100 transactions." Quit as early as possible. There cannot be + * more than MAX_BIP125_REPLACEMENT_CANDIDATES potential entries. + * @param[in] setIterConflicting The set of iterators to mempool entries. + * @param[out] err_string Used to return errors, if any. + * @param[out] allConflicting Populated with all the mempool entries that would be replaced, + * which includes descendants of setIterConflicting. Not cleared at + * the start; any existing mempool entries will remain in the set. + * @returns false if Rule 5 is broken. + */ +bool GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& m_pool, + const CTxMemPool::setEntries& setIterConflicting, + CTxMemPool::setEntries& allConflicting, + std::string& err_string) EXCLUSIVE_LOCKS_REQUIRED(m_pool.cs); #endif // BITCOIN_POLICY_RBF_H diff --git a/src/util/rbf.h b/src/util/rbf.h index 6a20b37de5..4eb44b904f 100644 --- a/src/util/rbf.h +++ b/src/util/rbf.h @@ -11,8 +11,15 @@ class CTransaction; static const uint32_t MAX_BIP125_RBF_SEQUENCE = 0xfffffffd; -// Check whether the sequence numbers on this transaction are signaling -// opt-in to replace-by-fee, according to BIP 125 +/** Check whether the sequence numbers on this transaction are signaling +* opt-in to replace-by-fee, according to BIP 125. +* Allow opt-out of transaction replacement by setting +* nSequence > MAX_BIP125_RBF_SEQUENCE (SEQUENCE_FINAL-2) on all inputs. +* +* SEQUENCE_FINAL-1 is picked to still allow use of nLockTime by +* non-replaceable transactions. All inputs rather than just one +* is for the sake of multi-party protocols, where we don't +* want a single party to be able to disable replacement. */ bool SignalsOptInRBF(const CTransaction &tx); #endif // BITCOIN_UTIL_RBF_H diff --git a/src/validation.cpp b/src/validation.cpp index 9944e31557..753b824167 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -474,8 +475,10 @@ private: bool m_replacement_transaction; CAmount m_base_fees; CAmount m_modified_fees; - CAmount m_conflicting_fees; - size_t m_conflicting_size; + /** Total modified fees of all transactions being replaced. */ + CAmount m_conflicting_fees{0}; + /** Total virtual size of all transactions being replaced. */ + size_t m_conflicting_size{0}; const CTransactionRef& m_ptx; const uint256& m_hash; @@ -602,14 +605,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } if (!setConflicts.count(ptxConflicting->GetHash())) { - // Allow opt-out of transaction replacement by setting - // nSequence > MAX_BIP125_RBF_SEQUENCE (SEQUENCE_FINAL-2) on all inputs. - // - // SEQUENCE_FINAL-1 is picked to still allow use of nLockTime by - // non-replaceable transactions. All inputs rather than just one - // is for the sake of multi-party protocols, where we don't - // want a single party to be able to disable replacement. - // // 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 @@ -617,16 +612,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // 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. - bool fReplacementOptOut = true; - for (const CTxIn &_txin : ptxConflicting->vin) - { - if (_txin.nSequence <= MAX_BIP125_RBF_SEQUENCE) - { - fReplacementOptOut = false; - break; - } - } - if (fReplacementOptOut) { + if (!SignalsOptInRBF(*ptxConflicting)) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict"); } @@ -796,11 +782,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } } - // Check if it's economically rational to mine this transaction rather - // than the ones it replaces. - nConflictingFees = 0; - nConflictingSize = 0; - uint64_t nConflictingCount = 0; // If we don't hold the lock allConflicting might be incomplete; the // subsequent RemoveStaged() and addUnchecked() calls don't guarantee @@ -808,9 +789,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) fReplacementTransaction = setConflicts.size(); if (fReplacementTransaction) { + std::string err_string; CFeeRate newFeeRate(nModifiedFees, nSize); - std::set setConflictsParents; - const int maxDescendantsToVisit = 100; for (const auto& mi : setIterConflicting) { // Don't allow the replacement to reduce the feerate of the // mempool. @@ -835,33 +815,26 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) newFeeRate.ToString(), oldFeeRate.ToString())); } + } + // Calculate all conflicting entries and enforce Rule #5. + if (!GetEntriesForConflicts(tx, m_pool, setIterConflicting, allConflicting, err_string)) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too many potential replacements", err_string); + } + + // Check if it's economically rational to mine this transaction rather + // than the ones it replaces. + for (CTxMemPool::txiter it : allConflicting) { + nConflictingFees += it->GetModifiedFee(); + nConflictingSize += it->GetTxSize(); + } + + std::set setConflictsParents; + for (const auto& mi : setIterConflicting) { for (const CTxIn &txin : mi->GetTx().vin) { setConflictsParents.insert(txin.prevout.hash); } - - nConflictingCount += mi->GetCountWithDescendants(); - } - // This potentially overestimates the number of actual descendants - // but we just want to be conservative to avoid doing too much - // work. - if (nConflictingCount <= maxDescendantsToVisit) { - // If not too many to replace, then calculate the set of - // transactions that would have to be evicted - for (CTxMemPool::txiter it : setIterConflicting) { - m_pool.CalculateDescendants(it, allConflicting); - } - for (CTxMemPool::txiter it : allConflicting) { - nConflictingFees += it->GetModifiedFee(); - nConflictingSize += it->GetTxSize(); - } - } else { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too many potential replacements", - strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", - hash.ToString(), - nConflictingCount, - maxDescendantsToVisit)); } for (unsigned int j = 0; j < tx.vin.size(); j++) diff --git a/test/lint/lint-circular-dependencies.sh b/test/lint/lint-circular-dependencies.sh index df5051720b..233381f2d9 100755 --- a/test/lint/lint-circular-dependencies.sh +++ b/test/lint/lint-circular-dependencies.sh @@ -15,6 +15,7 @@ EXPECTED_CIRCULAR_DEPENDENCIES=( "index/base -> validation -> index/blockfilterindex -> index/base" "index/coinstatsindex -> node/coinstats -> index/coinstatsindex" "policy/fees -> txmempool -> policy/fees" + "policy/rbf -> txmempool -> validation -> policy/rbf" "qt/addresstablemodel -> qt/walletmodel -> qt/addresstablemodel" "qt/recentrequeststablemodel -> qt/walletmodel -> qt/recentrequeststablemodel" "qt/sendcoinsdialog -> qt/walletmodel -> qt/sendcoinsdialog"