From fc8c19a07c20ab63f6a69f7494f486204d8f2b7a Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Thu, 29 Oct 2015 22:55:48 -0400 Subject: [PATCH] Prevent low feerate txs from (directly) replacing high feerate txs Previously all conflicting transactions were evaluated as a whole to determine if the feerate was being increased. This meant that low feerate children pulled the feerate down, potentially allowing a high transaction with a high feerate to be replaced by one with a lower feerate. --- qa/replace-by-fee/rbf-tests.py | 2 +- src/main.cpp | 62 +++++++++++++++++++++------------- 2 files changed, 39 insertions(+), 25 deletions(-) diff --git a/qa/replace-by-fee/rbf-tests.py b/qa/replace-by-fee/rbf-tests.py index 391159a86..5173da641 100755 --- a/qa/replace-by-fee/rbf-tests.py +++ b/qa/replace-by-fee/rbf-tests.py @@ -219,7 +219,7 @@ class Test_ReplaceByFee(unittest.TestCase): self.proxy.getrawtransaction(tx.GetHash()) def test_replacement_feeperkb(self): - """Replacement requires overall fee-per-KB to be higher""" + """Replacement requires fee-per-KB to be higher""" tx0_outpoint = self.make_txout(1.1*COIN) tx1a = CTransaction([CTxIn(tx0_outpoint, nSequence=0)], diff --git a/src/main.cpp b/src/main.cpp index 274a336ee..10d661b2a 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1008,23 +1008,51 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa { LOCK(pool.cs); - // For efficiency we simply sum up the pre-calculated - // fees/size-with-descendants values from the mempool package - // tracking; this does mean the pathological case of diamond tx - // graphs will be overcounted. + CFeeRate newFeeRate(nFees, nSize); BOOST_FOREACH(const uint256 hashConflicting, setConflicts) { CTxMemPool::txiter mi = pool.mapTx.find(hashConflicting); if (mi == pool.mapTx.end()) continue; + + // Don't allow the replacement to reduce the feerate of the + // mempool. + // + // We usually don't want to accept replacements with lower + // feerates than what they replaced as that would lower the + // feerate of the next block. Requiring that the feerate always + // be increased is also an easy-to-reason about way to prevent + // DoS attacks via replacements. + // + // The mining code doesn't (currently) take children into + // account (CPFP) so we only consider the feerates of + // transactions being directly replaced, not their indirect + // descendants. While that does mean high feerate children are + // ignored when deciding whether or not to replace, we do + // require the replacement to pay more overall fees too, + // mitigating most cases. + CFeeRate oldFeeRate(mi->GetFee(), mi->GetTxSize()); + if (newFeeRate <= oldFeeRate) + { + return state.DoS(0, + error("AcceptToMemoryPool: rejecting replacement %s; new feerate %s <= old feerate %s", + hash.ToString(), + newFeeRate.ToString(), + oldFeeRate.ToString()), + REJECT_INSUFFICIENTFEE, "insufficient fee"); + } + + // For efficiency we simply sum up the pre-calculated + // fees/size-with-descendants values from the mempool package + // tracking; this does mean the pathological case of diamond tx + // graphs will be overcounted. nConflictingFees += mi->GetFeesWithDescendants(); nConflictingSize += mi->GetSizeWithDescendants(); } - // First of all we can't allow a replacement unless it pays greater - // fees than the transactions it conflicts with - if we did the - // bandwidth used by those conflicting transactions would not be - // paid for + // 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. if (nFees < nConflictingFees) { return state.DoS(0, error("AcceptToMemoryPool: rejecting replacement %s, less fees than conflicting txs; %s < %s", @@ -1032,8 +1060,8 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa REJECT_INSUFFICIENTFEE, "insufficient fee"); } - // Secondly in addition to paying more fees than the conflicts the - // new transaction must additionally pay for its own bandwidth. + // Finally in addition to paying more fees than the conflicts the + // new transaction must pay for its own bandwidth. CAmount nDeltaFees = nFees - nConflictingFees; if (nDeltaFees < ::minRelayTxFee.GetFee(nSize)) { @@ -1044,20 +1072,6 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa FormatMoney(::minRelayTxFee.GetFee(nSize))), REJECT_INSUFFICIENTFEE, "insufficient fee"); } - - // Finally replace only if we end up with a larger fees-per-kb than - // the replacements. - CFeeRate oldFeeRate(nConflictingFees, nConflictingSize); - CFeeRate newFeeRate(nFees, nSize); - if (newFeeRate <= oldFeeRate) - { - return state.DoS(0, - error("AcceptToMemoryPool: rejecting replacement %s; new feerate %s <= old feerate %s", - hash.ToString(), - newFeeRate.ToString(), - oldFeeRate.ToString()), - REJECT_INSUFFICIENTFEE, "insufficient fee"); - } } // Check against previous transactions