diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index cefc25a2251..7ac2e22006f 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -48,17 +48,19 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx) } std::optional GetEntriesForConflicts(const CTransaction& tx, - CTxMemPool& pool, - const CTxMemPool::setEntries& iters_conflicting, - CTxMemPool::setEntries& all_conflicts) + CTxMemPool& pool, + const CTxMemPool::setEntries& iters_conflicting, + CTxMemPool::setEntries& all_conflicts) { AssertLockHeld(pool.cs); const uint256 txid = tx.GetHash(); uint64_t nConflictingCount = 0; for (const auto& mi : iters_conflicting) { nConflictingCount += mi->GetCountWithDescendants(); - // This potentially overestimates the number of actual descendants but we just want to be - // conservative to avoid doing too much work. + // BIP125 Rule #5: don't consider replacing more than MAX_BIP125_REPLACEMENT_CANDIDATES + // entries from the mempool. This potentially overestimates the number of actual + // descendants (i.e. if multiple conflicts share a descendant, it will be counted multiple + // times), but we just want to be conservative to avoid doing too much work. if (nConflictingCount > MAX_BIP125_REPLACEMENT_CANDIDATES) { return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", txid.ToString(), @@ -66,8 +68,7 @@ std::optional GetEntriesForConflicts(const CTransaction& tx, MAX_BIP125_REPLACEMENT_CANDIDATES); } } - // If not too many to replace, then calculate the set of - // transactions that would have to be evicted + // Calculate the set of all transactions that would have to be evicted. for (CTxMemPool::txiter it : iters_conflicting) { pool.CalculateDescendants(it, all_conflicts); } @@ -81,19 +82,19 @@ std::optional HasNoNewUnconfirmed(const CTransaction& tx, AssertLockHeld(pool.cs); std::set parents_of_conflicts; for (const auto& mi : iters_conflicting) { - for (const CTxIn &txin : mi->GetTx().vin) { + for (const CTxIn& txin : mi->GetTx().vin) { parents_of_conflicts.insert(txin.prevout.hash); } } for (unsigned int j = 0; j < tx.vin.size(); j++) { - // We don't want to accept replacements that require low feerate junk to be mined first. - // Ideally we'd keep track of the ancestor feerates and make the decision based on that, but - // for now requiring all new inputs to be confirmed works. + // BIP125 Rule #2: We don't want to accept replacements that require low feerate junk to be + // mined first. Ideally we'd keep track of the ancestor feerates and make the decision + // based on that, but for now requiring all new inputs to be confirmed works. // // Note that if you relax this to make RBF a little more useful, this may break the - // CalculateMempoolAncestors RBF relaxation, above. See the comment above the first - // CalculateMempoolAncestors call for more info. + // CalculateMempoolAncestors RBF relaxation which subtracts the conflict count/size from the + // descendant limit. if (!parents_of_conflicts.count(tx.vin[j].prevout.hash)) { // Rather than check the UTXO set - potentially expensive - it's cheaper to just check // if the new input refers to a tx that's in the mempool. @@ -111,7 +112,7 @@ std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& const uint256& txid) { for (CTxMemPool::txiter ancestorIt : ancestors) { - const uint256 &hashAncestor = ancestorIt->GetTx().GetHash(); + const uint256& hashAncestor = ancestorIt->GetTx().GetHash(); if (direct_conflicts.count(hashAncestor)) { return strprintf("%s spends conflicting transaction %s", txid.ToString(), @@ -153,16 +154,17 @@ std::optional PaysForRBF(CAmount original_fees, CFeeRate relay_fee, const uint256& txid) { - // The replacement must pay greater fees than the transactions it - // replaces - if we did the bandwidth used by those conflicting - // transactions would not be paid for. + // BIP125 Rule #3: The replacement fees must be greater than or equal to fees of the + // transactions it replaces, otherwise the bandwidth used by those conflicting transactions + // would not be paid for. if (replacement_fees < original_fees) { return strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s", txid.ToString(), FormatMoney(replacement_fees), FormatMoney(original_fees)); } - // Finally in addition to paying more fees than the conflicts the - // new transaction must pay for its own bandwidth. + // BIP125 Rule #4: The new transaction must pay for its own bandwidth. Otherwise, we have a DoS + // vector where attackers can cause a transaction to be replaced (and relayed) repeatedly by + // increasing the fee by tiny amounts. CAmount additional_fees = replacement_fees - original_fees; if (additional_fees < relay_fee.GetFee(replacement_vsize)) { return strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s", diff --git a/src/policy/rbf.h b/src/policy/rbf.h index e8cfb2e6438..6031745d8fd 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -48,14 +48,14 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx); std::optional GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& pool, const CTxMemPool::setEntries& iters_conflicting, CTxMemPool::setEntries& all_conflicts) - EXCLUSIVE_LOCKS_REQUIRED(pool.cs); + EXCLUSIVE_LOCKS_REQUIRED(pool.cs); /** BIP125 Rule #2: "The replacement transaction may only include an unconfirmed input if that input * was included in one of the original transactions." * @returns error message if Rule #2 is broken, otherwise std::nullopt. */ std::optional HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& pool, const CTxMemPool::setEntries& iters_conflicting) - EXCLUSIVE_LOCKS_REQUIRED(pool.cs); + EXCLUSIVE_LOCKS_REQUIRED(pool.cs); /** Check the intersection between two sets of transactions (a set of mempool entries and a set of * txids) to make sure they are disjoint. diff --git a/src/util/rbf.h b/src/util/rbf.h index 6d44a2cb83c..135817d6698 100644 --- a/src/util/rbf.h +++ b/src/util/rbf.h @@ -17,7 +17,7 @@ static const uint32_t MAX_BIP125_RBF_SEQUENCE = 0xfffffffd; * * 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); +* party to be able to disable replacement by opting out in their own input. */ +bool SignalsOptInRBF(const CTransaction& tx); #endif // BITCOIN_UTIL_RBF_H diff --git a/src/validation.cpp b/src/validation.cpp index e4b8b62c5a6..44b2f8e298f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -771,34 +771,38 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // pathological case by making sure setConflicts and setAncestors don't // intersect. if (const auto err_string{EntriesAndTxidsDisjoint(setAncestors, setConflicts, hash)}) { + // We classify this as a consensus error because a transaction depending on something it + // conflicts with would be inconsistent. return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", *err_string); } - // If we don't hold the lock allConflicting might be incomplete; the - // subsequent RemoveStaged() and addUnchecked() calls don't guarantee - // mempool consistency for us. fReplacementTransaction = setConflicts.size(); - if (fReplacementTransaction) - { + if (fReplacementTransaction) { CFeeRate newFeeRate(nModifiedFees, nSize); + // It's possible that the replacement pays more fees than its direct conflicts but not more + // than all conflicts (i.e. the direct conflicts have high-fee descendants). However, if the + // replacement doesn't pay more fees than its direct conflicts, then we can be sure it's not + // more economically rational to mine. Before we go digging through the mempool for all + // transactions that would need to be removed (direct conflicts and all descendants), check + // that the replacement transaction pays more than its direct conflicts. if (const auto err_string{PaysMoreThanConflicts(setIterConflicting, newFeeRate, hash)}) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string); } - // Calculate all conflicting entries and enforce Rule #5. + // Calculate all conflicting entries and enforce BIP125 Rule #5. if (const auto err_string{GetEntriesForConflicts(tx, m_pool, setIterConflicting, allConflicting)}) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too many potential replacements", *err_string); } - // Enforce Rule #2. + // Enforce BIP125 Rule #2. if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, setIterConflicting)}) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "replacement-adds-unconfirmed", *err_string); } - // Check if it's economically rational to mine this transaction rather - // than the ones it replaces. Enforce Rules #3 and #4. + // Check if it's economically rational to mine this transaction rather than the ones it + // replaces and pays for its own relay fees. Enforce BIP125 Rules #3 and #4. for (CTxMemPool::txiter it : allConflicting) { nConflictingFees += it->GetModifiedFee(); nConflictingSize += it->GetTxSize();