From f8ad2a57c61d1e817e2445226688e03080fc8688 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 2 Sep 2021 15:27:37 +0100 Subject: [PATCH 1/7] Make GetEntriesForConflicts return std::optional Avoids reusing err_string. --- src/policy/rbf.cpp | 10 ++++------ src/policy/rbf.h | 11 +++++------ src/validation.cpp | 6 +++--- 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 43624c7993f..3ff4c1f9c2b 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -47,11 +47,10 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx) return SignalsOptInRBF(tx) ? RBFTransactionState::REPLACEABLE_BIP125 : RBFTransactionState::UNKNOWN; } -bool GetEntriesForConflicts(const CTransaction& tx, +std::optional GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& m_pool, const CTxMemPool::setEntries& setIterConflicting, - CTxMemPool::setEntries& allConflicting, - std::string& err_string) + CTxMemPool::setEntries& allConflicting) { AssertLockHeld(m_pool.cs); const uint256 hash = tx.GetHash(); @@ -62,11 +61,10 @@ bool GetEntriesForConflicts(const CTransaction& tx, // 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", + return 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 @@ -74,6 +72,6 @@ bool GetEntriesForConflicts(const CTransaction& tx, for (CTxMemPool::txiter it : setIterConflicting) { m_pool.CalculateDescendants(it, allConflicting); } - return true; + return std::nullopt; } diff --git a/src/policy/rbf.h b/src/policy/rbf.h index a67e9058df7..2a41ca88924 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -40,14 +40,13 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx); * 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. + * @returns an error message if Rule #5 is broken, otherwise a std::nullopt. */ -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); +std::optional GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& m_pool, + const CTxMemPool::setEntries& setIterConflicting, + CTxMemPool::setEntries& allConflicting) + EXCLUSIVE_LOCKS_REQUIRED(m_pool.cs); #endif // BITCOIN_POLICY_RBF_H diff --git a/src/validation.cpp b/src/validation.cpp index 753b8241673..6f28c42db35 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -789,7 +789,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) fReplacementTransaction = setConflicts.size(); if (fReplacementTransaction) { - std::string err_string; CFeeRate newFeeRate(nModifiedFees, nSize); for (const auto& mi : setIterConflicting) { // Don't allow the replacement to reduce the feerate of the @@ -818,8 +817,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } // 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); + if (const auto err_string{GetEntriesForConflicts(tx, m_pool, setIterConflicting, allConflicting)}) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, + "too many potential replacements", *err_string); } // Check if it's economically rational to mine this transaction rather From 7b60c02b7d5e2ab12288393d2258873ebb26d811 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 3 Aug 2021 13:13:43 +0100 Subject: [PATCH 2/7] MOVEONLY: BIP125 Rule 2 to policy/rbf --- src/policy/rbf.cpp | 37 +++++++++++++++++++++++++++++++++++++ src/policy/rbf.h | 7 +++++++ src/validation.cpp | 37 +++++-------------------------------- 3 files changed, 49 insertions(+), 32 deletions(-) diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 3ff4c1f9c2b..f6b3bc783a7 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -75,3 +75,40 @@ std::optional GetEntriesForConflicts(const CTransaction& tx, return std::nullopt; } +std::optional HasNoNewUnconfirmed(const CTransaction& tx, + const CTxMemPool& m_pool, + const CTxMemPool::setEntries& setIterConflicting) +{ + AssertLockHeld(m_pool.cs); + std::set setConflictsParents; + for (const auto& mi : setIterConflicting) { + for (const CTxIn &txin : mi->GetTx().vin) + { + setConflictsParents.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. + // + // 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. + if (!setConflictsParents.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. + if (m_pool.exists(tx.vin[j].prevout.hash)) { + return strprintf("replacement %s adds unconfirmed input, idx %d", + tx.GetHash().ToString(), j); + } + } + } + return std::nullopt; +} diff --git a/src/policy/rbf.h b/src/policy/rbf.h index 2a41ca88924..0f9a3d98565 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -49,4 +49,11 @@ std::optional GetEntriesForConflicts(const CTransaction& tx, CTxMem const CTxMemPool::setEntries& setIterConflicting, CTxMemPool::setEntries& allConflicting) EXCLUSIVE_LOCKS_REQUIRED(m_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& m_pool, + const CTxMemPool::setEntries& setIterConflicting) + EXCLUSIVE_LOCKS_REQUIRED(m_pool.cs); #endif // BITCOIN_POLICY_RBF_H diff --git a/src/validation.cpp b/src/validation.cpp index 6f28c42db35..20970bceb8f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -821,6 +821,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too many potential replacements", *err_string); } + // Enforce 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. @@ -829,38 +834,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) nConflictingSize += it->GetTxSize(); } - std::set setConflictsParents; - for (const auto& mi : setIterConflicting) { - for (const CTxIn &txin : mi->GetTx().vin) - { - setConflictsParents.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. - // - // 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. - if (!setConflictsParents.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. - if (m_pool.exists(tx.vin[j].prevout.hash)) { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "replacement-adds-unconfirmed", - strprintf("replacement %s adds unconfirmed input, idx %d", - hash.ToString(), j)); - } - } - } - // 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. From 3f033f01a6b0f7772ae1b21044903b8f4249ad08 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 27 Jul 2021 15:55:25 +0100 Subject: [PATCH 3/7] MOVEONLY: check for disjoint conflicts and ancestors to policy/rbf This checks that a transaction isn't trying to replace something it supposedly depends on. --- src/policy/rbf.cpp | 17 +++++++++++++++++ src/policy/rbf.h | 13 +++++++++++++ src/validation.cpp | 12 ++---------- 3 files changed, 32 insertions(+), 10 deletions(-) diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index f6b3bc783a7..95b74123c94 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -112,3 +112,20 @@ std::optional HasNoNewUnconfirmed(const CTransaction& tx, } return std::nullopt; } + +std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& setAncestors, + const std::set& setConflicts, + const uint256& txid) +{ + for (CTxMemPool::txiter ancestorIt : setAncestors) + { + const uint256 &hashAncestor = ancestorIt->GetTx().GetHash(); + if (setConflicts.count(hashAncestor)) + { + return strprintf("%s spends conflicting transaction %s", + txid.ToString(), + hashAncestor.ToString()); + } + } + return std::nullopt; +} diff --git a/src/policy/rbf.h b/src/policy/rbf.h index 0f9a3d98565..6c4e2189591 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -56,4 +56,17 @@ std::optional GetEntriesForConflicts(const CTransaction& tx, CTxMem std::optional HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& m_pool, const CTxMemPool::setEntries& setIterConflicting) EXCLUSIVE_LOCKS_REQUIRED(m_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. + * @param[in] setAncestors Set of mempool entries corresponding to ancestors of the + * replacement transactions. + * @param[in] setConflicts Set of txids corresponding to the mempool conflicts + * (candidates to be replaced). + * @param[in] txid Transaction ID, included in the error message if violation occurs. + * @returns error message if the sets intersect, std::nullopt if they are disjoint. + */ +std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& setAncestors, + const std::set& setConflicts, + const uint256& txid); #endif // BITCOIN_POLICY_RBF_H diff --git a/src/validation.cpp b/src/validation.cpp index 20970bceb8f..710fe2d8732 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -770,16 +770,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // that we have the set of all ancestors we can detect this // pathological case by making sure setConflicts and setAncestors don't // intersect. - for (CTxMemPool::txiter ancestorIt : setAncestors) - { - const uint256 &hashAncestor = ancestorIt->GetTx().GetHash(); - if (setConflicts.count(hashAncestor)) - { - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", - strprintf("%s spends conflicting transaction %s", - hash.ToString(), - hashAncestor.ToString())); - } + if (const auto err_string{EntriesAndTxidsDisjoint(setAncestors, setConflicts, hash)}) { + return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", *err_string); } From 9c2f9f89846264b503d5573341bb78cf609cbc5e Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 11 Aug 2021 15:51:27 +0100 Subject: [PATCH 4/7] MOVEONLY: check that fees > direct conflicts to policy/rbf --- src/policy/rbf.cpp | 32 ++++++++++++++++++++++++++++++++ src/policy/rbf.h | 9 +++++++++ src/validation.cpp | 26 ++------------------------ 3 files changed, 43 insertions(+), 24 deletions(-) diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 95b74123c94..5d1db6b58d5 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -129,3 +129,35 @@ std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& } return std::nullopt; } + +std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& setIterConflicting, + CFeeRate newFeeRate, + const uint256& hash) +{ + for (const auto& mi : setIterConflicting) { + // 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. + // + // 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->GetModifiedFee(), mi->GetTxSize()); + if (newFeeRate <= oldFeeRate) + { + return strprintf("rejecting replacement %s; new feerate %s <= old feerate %s", + hash.ToString(), + newFeeRate.ToString(), + oldFeeRate.ToString()); + } + } + return std::nullopt; +} + diff --git a/src/policy/rbf.h b/src/policy/rbf.h index 6c4e2189591..2c548152b59 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -69,4 +69,13 @@ std::optional HasNoNewUnconfirmed(const CTransaction& tx, const CTx std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& setAncestors, const std::set& setConflicts, const uint256& txid); + +/** Check that the feerate of the replacement transaction(s) is higher than the feerate of each + * of the transactions in setIterConflicting. + * @param[in] setIterConflicting The set of mempool entries. + * @returns error message if fees insufficient, otherwise std::nullopt. + */ +std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& setIterConflicting, + CFeeRate newFeeRate, const uint256& hash); + #endif // BITCOIN_POLICY_RBF_H diff --git a/src/validation.cpp b/src/validation.cpp index 710fe2d8732..b0403bffd26 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -782,30 +782,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) if (fReplacementTransaction) { CFeeRate newFeeRate(nModifiedFees, nSize); - for (const auto& mi : setIterConflicting) { - // 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. - // - // 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->GetModifiedFee(), mi->GetTxSize()); - if (newFeeRate <= oldFeeRate) - { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", - strprintf("rejecting replacement %s; new feerate %s <= old feerate %s", - hash.ToString(), - newFeeRate.ToString(), - oldFeeRate.ToString())); - } + 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. From ac761f0a23c9c469fa00885edf3d5c9ae7c6a2b3 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 11 Aug 2021 15:51:41 +0100 Subject: [PATCH 5/7] MOVEONLY: fee checks (Rules 3 and 4) to policy/rbf --- src/policy/rbf.cpp | 26 ++++++++++++++++++++++++++ src/policy/rbf.h | 14 ++++++++++++++ src/validation.cpp | 25 +++---------------------- 3 files changed, 43 insertions(+), 22 deletions(-) diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 5d1db6b58d5..1e03e2331aa 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -161,3 +161,29 @@ std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& s return std::nullopt; } +std::optional PaysForRBF(CAmount nConflictingFees, + CAmount nModifiedFees, + size_t nSize, + const uint256& hash) +{ + // 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 (nModifiedFees < nConflictingFees) + { + return strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s", + hash.ToString(), FormatMoney(nModifiedFees), FormatMoney(nConflictingFees)); + } + + // Finally in addition to paying more fees than the conflicts the + // new transaction must pay for its own bandwidth. + CAmount nDeltaFees = nModifiedFees - nConflictingFees; + if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize)) + { + return strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s", + hash.ToString(), + FormatMoney(nDeltaFees), + FormatMoney(::incrementalRelayFee.GetFee(nSize))); + } + return std::nullopt; +} diff --git a/src/policy/rbf.h b/src/policy/rbf.h index 2c548152b59..d32ba010fba 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -78,4 +78,18 @@ std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& setIterConflicting, CFeeRate newFeeRate, const uint256& hash); +/** Enforce BIP125 Rule #3 "The replacement transaction pays an absolute fee of at least the sum + * paid by the original transactions." Enforce BIP125 Rule #4 "The replacement transaction must also + * pay for its own bandwidth at or above the rate set by the node's minimum relay fee setting." + * @param[in] nConflictingFees Total modified fees of original transaction(s). + * @param[in] nModifiedFees Total modified fees of replacement transaction(s). + * @param[in] nSize Total virtual size of replacement transaction(s). + * @param[in] hash Transaction ID, included in the error message if violation occurs. + * @returns error string if fees are insufficient, otherwise std::nullopt. + */ +std::optional PaysForRBF(CAmount nConflictingFees, + CAmount nModifiedFees, + size_t nSize, + const uint256& hash); + #endif // BITCOIN_POLICY_RBF_H diff --git a/src/validation.cpp b/src/validation.cpp index b0403bffd26..e1d218e3589 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -798,32 +798,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } // Check if it's economically rational to mine this transaction rather - // than the ones it replaces. + // than the ones it replaces. Enforce Rules #3 and #4. for (CTxMemPool::txiter it : allConflicting) { nConflictingFees += it->GetModifiedFee(); nConflictingSize += it->GetTxSize(); } - - // 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 (nModifiedFees < nConflictingFees) - { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", - strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s", - hash.ToString(), FormatMoney(nModifiedFees), FormatMoney(nConflictingFees))); - } - - // Finally in addition to paying more fees than the conflicts the - // new transaction must pay for its own bandwidth. - CAmount nDeltaFees = nModifiedFees - nConflictingFees; - if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize)) - { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", - strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s", - hash.ToString(), - FormatMoney(nDeltaFees), - FormatMoney(::incrementalRelayFee.GetFee(nSize)))); + if (const auto err_string{PaysForRBF(nConflictingFees, nModifiedFees, nSize, hash)}) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string); } } return true; From fa47622e8dc66bec9ea690aec3f0999108d76dc9 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 16 Aug 2021 10:19:15 +0100 Subject: [PATCH 6/7] scripted-diff: rename variables in policy/rbf "Fee Delta" is already a term used for prioritizing transactions: modified = base fees + delta Here, delta also means the difference between original and modified replacement fees: nDeltaFees = (original_base + original_delta) - (replacement_base + replacement_delta) This is insanely confusing. Also, since mempool is no longer a member of a class (MemPoolAccept.m_pool), the "m" prefix is unnecessary. The rest are clarity/style-focused changes to already-touched lines. -BEGIN VERIFY SCRIPT- ren() { sed -i "s/\<$1\>/$2/g" src/policy/rbf* ; } ren nDeltaFees additional_fees ren m_pool pool ren nSize replacement_vsize ren nModifiedFees replacement_fees ren nConflictingFees original_fees ren oldFeeRate original_feerate ren newFeeRate replacement_feerate ren setAncestors ancestors ren setIterConflicting iters_conflicting ren setConflictsParents parents_of_conflicts ren setConflicts direct_conflicts ren allConflicting all_conflicts sed -i "s/ hash\b/ txid/g" src/policy/rbf* -END VERIFY SCRIPT- --- src/policy/rbf.cpp | 88 +++++++++++++++++++++++----------------------- src/policy/rbf.h | 54 ++++++++++++++-------------- 2 files changed, 71 insertions(+), 71 deletions(-) diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 1e03e2331aa..8fe897dcea9 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -13,7 +13,7 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) { AssertLockHeld(pool.cs); - CTxMemPool::setEntries setAncestors; + CTxMemPool::setEntries ancestors; // First check the transaction itself. if (SignalsOptInRBF(tx)) { @@ -31,9 +31,9 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) uint64_t noLimit = std::numeric_limits::max(); std::string dummy; CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash()); - pool.CalculateMemPoolAncestors(entry, setAncestors, noLimit, noLimit, noLimit, noLimit, dummy, false); + pool.CalculateMemPoolAncestors(entry, ancestors, noLimit, noLimit, noLimit, noLimit, dummy, false); - for (CTxMemPool::txiter it : setAncestors) { + for (CTxMemPool::txiter it : ancestors) { if (SignalsOptInRBF(it->GetTx())) { return RBFTransactionState::REPLACEABLE_BIP125; } @@ -48,43 +48,43 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx) } std::optional GetEntriesForConflicts(const CTransaction& tx, - CTxMemPool& m_pool, - const CTxMemPool::setEntries& setIterConflicting, - CTxMemPool::setEntries& allConflicting) + CTxMemPool& pool, + const CTxMemPool::setEntries& iters_conflicting, + CTxMemPool::setEntries& all_conflicts) { - AssertLockHeld(m_pool.cs); - const uint256 hash = tx.GetHash(); + AssertLockHeld(pool.cs); + const uint256 txid = tx.GetHash(); uint64_t nConflictingCount = 0; - for (const auto& mi : setIterConflicting) { + 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. if (nConflictingCount > MAX_BIP125_REPLACEMENT_CANDIDATES) { return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", - hash.ToString(), + txid.ToString(), nConflictingCount, MAX_BIP125_REPLACEMENT_CANDIDATES); } } // 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 : iters_conflicting) { + pool.CalculateDescendants(it, all_conflicts); } return std::nullopt; } std::optional HasNoNewUnconfirmed(const CTransaction& tx, - const CTxMemPool& m_pool, - const CTxMemPool::setEntries& setIterConflicting) + const CTxMemPool& pool, + const CTxMemPool::setEntries& iters_conflicting) { - AssertLockHeld(m_pool.cs); - std::set setConflictsParents; - for (const auto& mi : setIterConflicting) { + AssertLockHeld(pool.cs); + std::set parents_of_conflicts; + for (const auto& mi : iters_conflicting) { for (const CTxIn &txin : mi->GetTx().vin) { - setConflictsParents.insert(txin.prevout.hash); + parents_of_conflicts.insert(txin.prevout.hash); } } @@ -99,12 +99,12 @@ std::optional HasNoNewUnconfirmed(const CTransaction& tx, // this may break the CalculateMempoolAncestors RBF relaxation, // above. See the comment above the first CalculateMempoolAncestors // call for more info. - if (!setConflictsParents.count(tx.vin[j].prevout.hash)) + 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. - if (m_pool.exists(tx.vin[j].prevout.hash)) { + if (pool.exists(tx.vin[j].prevout.hash)) { return strprintf("replacement %s adds unconfirmed input, idx %d", tx.GetHash().ToString(), j); } @@ -113,14 +113,14 @@ std::optional HasNoNewUnconfirmed(const CTransaction& tx, return std::nullopt; } -std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& setAncestors, - const std::set& setConflicts, +std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors, + const std::set& direct_conflicts, const uint256& txid) { - for (CTxMemPool::txiter ancestorIt : setAncestors) + for (CTxMemPool::txiter ancestorIt : ancestors) { const uint256 &hashAncestor = ancestorIt->GetTx().GetHash(); - if (setConflicts.count(hashAncestor)) + if (direct_conflicts.count(hashAncestor)) { return strprintf("%s spends conflicting transaction %s", txid.ToString(), @@ -130,11 +130,11 @@ std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& return std::nullopt; } -std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& setIterConflicting, - CFeeRate newFeeRate, - const uint256& hash) +std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting, + CFeeRate replacement_feerate, + const uint256& txid) { - for (const auto& mi : setIterConflicting) { + for (const auto& mi : iters_conflicting) { // Don't allow the replacement to reduce the feerate of the // mempool. // @@ -149,41 +149,41 @@ std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& s // 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->GetModifiedFee(), mi->GetTxSize()); - if (newFeeRate <= oldFeeRate) + CFeeRate original_feerate(mi->GetModifiedFee(), mi->GetTxSize()); + if (replacement_feerate <= original_feerate) { return strprintf("rejecting replacement %s; new feerate %s <= old feerate %s", - hash.ToString(), - newFeeRate.ToString(), - oldFeeRate.ToString()); + txid.ToString(), + replacement_feerate.ToString(), + original_feerate.ToString()); } } return std::nullopt; } -std::optional PaysForRBF(CAmount nConflictingFees, - CAmount nModifiedFees, - size_t nSize, - const uint256& hash) +std::optional PaysForRBF(CAmount original_fees, + CAmount replacement_fees, + size_t replacement_vsize, + 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. - if (nModifiedFees < nConflictingFees) + if (replacement_fees < original_fees) { return strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s", - hash.ToString(), FormatMoney(nModifiedFees), FormatMoney(nConflictingFees)); + 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. - CAmount nDeltaFees = nModifiedFees - nConflictingFees; - if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize)) + CAmount additional_fees = replacement_fees - original_fees; + if (additional_fees < ::incrementalRelayFee.GetFee(replacement_vsize)) { return strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s", - hash.ToString(), - FormatMoney(nDeltaFees), - FormatMoney(::incrementalRelayFee.GetFee(nSize))); + txid.ToString(), + FormatMoney(additional_fees), + FormatMoney(::incrementalRelayFee.GetFee(replacement_vsize))); } return std::nullopt; } diff --git a/src/policy/rbf.h b/src/policy/rbf.h index d32ba010fba..55baae0fa2c 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -35,61 +35,61 @@ 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 +/** Get all descendants of iters_conflicting. 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] allConflicting Populated with all the mempool entries that would be replaced, - * which includes descendants of setIterConflicting. Not cleared at + * @param[in] iters_conflicting The set of iterators to mempool entries. + * @param[out] all_conflicts Populated with all the mempool entries that would be replaced, + * which includes descendants of iters_conflicting. Not cleared at * the start; any existing mempool entries will remain in the set. * @returns an error message if Rule #5 is broken, otherwise a std::nullopt. */ -std::optional GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& m_pool, - const CTxMemPool::setEntries& setIterConflicting, - CTxMemPool::setEntries& allConflicting) - EXCLUSIVE_LOCKS_REQUIRED(m_pool.cs); +std::optional GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& pool, + const CTxMemPool::setEntries& iters_conflicting, + CTxMemPool::setEntries& all_conflicts) + 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& m_pool, - const CTxMemPool::setEntries& setIterConflicting) - EXCLUSIVE_LOCKS_REQUIRED(m_pool.cs); +std::optional HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& pool, + const CTxMemPool::setEntries& iters_conflicting) + 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. - * @param[in] setAncestors Set of mempool entries corresponding to ancestors of the + * @param[in] ancestors Set of mempool entries corresponding to ancestors of the * replacement transactions. - * @param[in] setConflicts Set of txids corresponding to the mempool conflicts + * @param[in] direct_conflicts Set of txids corresponding to the mempool conflicts * (candidates to be replaced). * @param[in] txid Transaction ID, included in the error message if violation occurs. * @returns error message if the sets intersect, std::nullopt if they are disjoint. */ -std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& setAncestors, - const std::set& setConflicts, +std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors, + const std::set& direct_conflicts, const uint256& txid); /** Check that the feerate of the replacement transaction(s) is higher than the feerate of each - * of the transactions in setIterConflicting. - * @param[in] setIterConflicting The set of mempool entries. + * of the transactions in iters_conflicting. + * @param[in] iters_conflicting The set of mempool entries. * @returns error message if fees insufficient, otherwise std::nullopt. */ -std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& setIterConflicting, - CFeeRate newFeeRate, const uint256& hash); +std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting, + CFeeRate replacement_feerate, const uint256& txid); /** Enforce BIP125 Rule #3 "The replacement transaction pays an absolute fee of at least the sum * paid by the original transactions." Enforce BIP125 Rule #4 "The replacement transaction must also * pay for its own bandwidth at or above the rate set by the node's minimum relay fee setting." - * @param[in] nConflictingFees Total modified fees of original transaction(s). - * @param[in] nModifiedFees Total modified fees of replacement transaction(s). - * @param[in] nSize Total virtual size of replacement transaction(s). - * @param[in] hash Transaction ID, included in the error message if violation occurs. + * @param[in] original_fees Total modified fees of original transaction(s). + * @param[in] replacement_fees Total modified fees of replacement transaction(s). + * @param[in] replacement_vsize Total virtual size of replacement transaction(s). + * @param[in] txid Transaction ID, included in the error message if violation occurs. * @returns error string if fees are insufficient, otherwise std::nullopt. */ -std::optional PaysForRBF(CAmount nConflictingFees, - CAmount nModifiedFees, - size_t nSize, - const uint256& hash); +std::optional PaysForRBF(CAmount original_fees, + CAmount replacement_fees, + size_t replacement_vsize, + const uint256& txid); #endif // BITCOIN_POLICY_RBF_H From 32748da0f47f7aa9fba78dfb29aa426b14f15624 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 1 Sep 2021 09:54:04 +0100 Subject: [PATCH 7/7] whitespace fixups after move and scripted-diff --- src/policy/rbf.cpp | 74 ++++++++++++++++++---------------------------- src/policy/rbf.h | 18 +++++------ src/util/rbf.h | 14 ++++----- 3 files changed, 44 insertions(+), 62 deletions(-) diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 8fe897dcea9..15527afb8ac 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -57,14 +57,13 @@ std::optional GetEntriesForConflicts(const CTransaction& tx, 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. + // 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) { return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", - txid.ToString(), - nConflictingCount, - MAX_BIP125_REPLACEMENT_CANDIDATES); + txid.ToString(), + nConflictingCount, + MAX_BIP125_REPLACEMENT_CANDIDATES); } } // If not too many to replace, then calculate the set of @@ -82,28 +81,22 @@ 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. + 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. // - // 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. - 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. + // 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. + 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. if (pool.exists(tx.vin[j].prevout.hash)) { return strprintf("replacement %s adds unconfirmed input, idx %d", tx.GetHash().ToString(), j); @@ -117,11 +110,9 @@ std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& const std::set& direct_conflicts, const uint256& txid) { - for (CTxMemPool::txiter ancestorIt : ancestors) - { + for (CTxMemPool::txiter ancestorIt : ancestors) { const uint256 &hashAncestor = ancestorIt->GetTx().GetHash(); - if (direct_conflicts.count(hashAncestor)) - { + if (direct_conflicts.count(hashAncestor)) { return strprintf("%s spends conflicting transaction %s", txid.ToString(), hashAncestor.ToString()); @@ -135,23 +126,18 @@ std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& i const uint256& txid) { for (const auto& mi : iters_conflicting) { - // Don't allow the replacement to reduce the feerate of the - // mempool. + // 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. + // 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. // - // 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. + // 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 original_feerate(mi->GetModifiedFee(), mi->GetTxSize()); - if (replacement_feerate <= original_feerate) - { + if (replacement_feerate <= original_feerate) { return strprintf("rejecting replacement %s; new feerate %s <= old feerate %s", txid.ToString(), replacement_feerate.ToString(), @@ -169,8 +155,7 @@ std::optional PaysForRBF(CAmount original_fees, // 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 (replacement_fees < original_fees) - { + 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)); } @@ -178,8 +163,7 @@ std::optional PaysForRBF(CAmount original_fees, // Finally in addition to paying more fees than the conflicts the // new transaction must pay for its own bandwidth. CAmount additional_fees = replacement_fees - original_fees; - if (additional_fees < ::incrementalRelayFee.GetFee(replacement_vsize)) - { + if (additional_fees < ::incrementalRelayFee.GetFee(replacement_vsize)) { return strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s", txid.ToString(), FormatMoney(additional_fees), diff --git a/src/policy/rbf.h b/src/policy/rbf.h index 55baae0fa2c..56468a09b21 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -39,8 +39,8 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx); * 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] iters_conflicting The set of iterators to mempool entries. - * @param[out] all_conflicts Populated with all the mempool entries that would be replaced, + * @param[in] iters_conflicting The set of iterators to mempool entries. + * @param[out] all_conflicts Populated with all the mempool entries that would be replaced, * which includes descendants of iters_conflicting. Not cleared at * the start; any existing mempool entries will remain in the set. * @returns an error message if Rule #5 is broken, otherwise a std::nullopt. @@ -59,11 +59,11 @@ std::optional HasNoNewUnconfirmed(const CTransaction& tx, const CTx /** Check the intersection between two sets of transactions (a set of mempool entries and a set of * txids) to make sure they are disjoint. - * @param[in] ancestors Set of mempool entries corresponding to ancestors of the - * replacement transactions. + * @param[in] ancestors Set of mempool entries corresponding to ancestors of the + * replacement transactions. * @param[in] direct_conflicts Set of txids corresponding to the mempool conflicts - * (candidates to be replaced). - * @param[in] txid Transaction ID, included in the error message if violation occurs. + * (candidates to be replaced). + * @param[in] txid Transaction ID, included in the error message if violation occurs. * @returns error message if the sets intersect, std::nullopt if they are disjoint. */ std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors, @@ -81,9 +81,9 @@ std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& i /** Enforce BIP125 Rule #3 "The replacement transaction pays an absolute fee of at least the sum * paid by the original transactions." Enforce BIP125 Rule #4 "The replacement transaction must also * pay for its own bandwidth at or above the rate set by the node's minimum relay fee setting." - * @param[in] original_fees Total modified fees of original transaction(s). - * @param[in] replacement_fees Total modified fees of replacement transaction(s). - * @param[in] replacement_vsize Total virtual size of replacement transaction(s). + * @param[in] original_fees Total modified fees of original transaction(s). + * @param[in] replacement_fees Total modified fees of replacement transaction(s). + * @param[in] replacement_vsize Total virtual size of replacement transaction(s). * @param[in] txid Transaction ID, included in the error message if violation occurs. * @returns error string if fees are insufficient, otherwise std::nullopt. */ diff --git a/src/util/rbf.h b/src/util/rbf.h index 4eb44b904f5..6d44a2cb83c 100644 --- a/src/util/rbf.h +++ b/src/util/rbf.h @@ -11,15 +11,13 @@ 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. -* Allow opt-out of transaction replacement by setting -* nSequence > MAX_BIP125_RBF_SEQUENCE (SEQUENCE_FINAL-2) on all inputs. +/** 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. */ +* 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