aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/policy/rbf.cpp40
-rw-r--r--src/policy/rbf.h4
-rw-r--r--src/util/rbf.h4
-rw-r--r--src/validation.cpp22
4 files changed, 38 insertions, 32 deletions
diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp
index cefc25a225..7ac2e22006 100644
--- a/src/policy/rbf.cpp
+++ b/src/policy/rbf.cpp
@@ -48,17 +48,19 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx)
}
std::optional<std::string> 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<std::string> 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<std::string> HasNoNewUnconfirmed(const CTransaction& tx,
AssertLockHeld(pool.cs);
std::set<uint256> 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<std::string> 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<std::string> 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 e8cfb2e643..6031745d8f 100644
--- a/src/policy/rbf.h
+++ b/src/policy/rbf.h
@@ -48,14 +48,14 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx);
std::optional<std::string> 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<std::string> 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 6d44a2cb83..135817d669 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 e4b8b62c5a..44b2f8e298 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();
generated by cgit v1.2.3 (git 2.26.2) at 2024-10-31 19:04:08 +0000