From b001b9f6de7a039a468cf0f9645f3f0a430fa889 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 27 Jul 2021 10:03:49 +0100 Subject: [PATCH 1/5] MOVEONLY: BIP125 max conflicts limit to policy/rbf.h A circular dependency is added because policy now depends on txmempool and txmempool depends on validation. It is natural for [mempool] policy to rely on mempool; the problem is caused by txmempool depending on validation. #22677 will resolve this. --- src/policy/rbf.h | 4 ++++ src/validation.cpp | 6 +++--- test/lint/lint-circular-dependencies.sh | 1 + 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/policy/rbf.h b/src/policy/rbf.h index e078070c1c..d61390361b 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 */ diff --git a/src/validation.cpp b/src/validation.cpp index ec457da5cc..29f82c2de0 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -810,7 +811,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) { 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. @@ -846,7 +846,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // 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 (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) { @@ -861,7 +861,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", hash.ToString(), nConflictingCount, - maxDescendantsToVisit)); + MAX_BIP125_REPLACEMENT_CANDIDATES)); } 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" From e0df41d7d584b854c2914d4afe7b21e0af3fbf69 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 20 Aug 2021 15:17:49 +0100 Subject: [PATCH 2/5] [validation] default conflicting fees and size to 0 This should have no effect in practice, since we only ever call PreChecks once per transaction. --- src/validation.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 29f82c2de0..cc24c79e13 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -475,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; @@ -799,8 +801,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 From badb9b11a6f7e1e693cecc8cd5aae55a197d70e2 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 20 Aug 2021 13:01:07 +0100 Subject: [PATCH 3/5] call SignalsOptInRBF instead of checking all inputs --- src/util/rbf.h | 11 +++++++++-- src/validation.cpp | 19 +------------------ 2 files changed, 10 insertions(+), 20 deletions(-) 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 cc24c79e13..0dade35ab2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -605,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 @@ -620,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"); } From 8d7179633552f58ca0d23305196dcb4249b6dce7 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 27 Jul 2021 11:49:34 +0100 Subject: [PATCH 4/5] [validation] quit RBF logic earlier and separate loops No behavior change. While we're looking through the descendants and calculating how many transactions we might replace, quit early, as soon as we hit 100. Since we're failing faster, we can also separate the loops - yes, we loop through more times, but this helps us detangle the different BIP125 rules later. --- src/validation.cpp | 55 ++++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 0dade35ab2..fdcf128924 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -782,9 +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. - uint64_t nConflictingCount = 0; // If we don't hold the lock allConflicting might be incomplete; the // subsequent RemoveStaged() and addUnchecked() calls don't guarantee @@ -793,7 +790,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) if (fReplacementTransaction) { CFeeRate newFeeRate(nModifiedFees, nSize); - std::set setConflictsParents; for (const auto& mi : setIterConflicting) { // Don't allow the replacement to reduce the feerate of the // mempool. @@ -818,33 +814,40 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) newFeeRate.ToString(), oldFeeRate.ToString())); } + } + 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) { + 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, + 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); + } + // 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 <= 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 : 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, - MAX_BIP125_REPLACEMENT_CANDIDATES)); } for (unsigned int j = 0; j < tx.vin.size(); j++) From f293c68be0469894c988711559f5528020c0ff71 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 27 Jul 2021 14:23:40 +0100 Subject: [PATCH 5/5] MOVEONLY: getting mempool conflicts to policy/rbf --- src/policy/rbf.cpp | 35 +++++++++++++++++++++++++++++++++++ src/policy/rbf.h | 15 +++++++++++++++ src/validation.cpp | 23 +++++------------------ 3 files changed, 55 insertions(+), 18 deletions(-) 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 d61390361b..a67e9058df 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -35,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/validation.cpp b/src/validation.cpp index fdcf128924..5baa9f5fd8 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -789,6 +789,7 @@ 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 @@ -816,25 +817,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } } - 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) { - 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, - 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); + // 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) {