phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-01-11 12:22:39 -03:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

Merge bitcoin/bitcoin#22855: RBF move 3/3: move followups + improve RBF documentation

Browse source 
0ef08f8bed add missing includes in policy/rbf (glozow)
c6abeb76fb make MAX_BIP125_RBF_SEQUENCE constexpr (glozow)
3cf46f6055 [doc] improve RBF documentation (glozow)
c78eb8651b [policy/refactor] pass in relay fee instead of using global (glozow)

Pull request description:

  Followups to #22675 and documentation-only changes intended to clarify the code/logic concerning mempool Replace-by-Fee.

ACKs for top commit:
  jnewbery:
    utACK 0ef08f8bed
  fanquake:
    ACK 0ef08f8bed

Tree-SHA512: 6797ae758beca0c9673cb00ce85da48e9a4ac5cb5100074ca93e004cdb31d24d91a1a7721b57fc2f619addfeb4950d8caf45fee0f5b7528defbbd121eb4d271f
This commit is contained in:
fanquake 2021-09-23 16:27:25 +08:00
commit 8bda5e0988
No known key found for this signature in database
GPG key ID: 2EEB9F5CC09526C1
4 changed files with 50 additions and 36 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

39
src/policy/rbf.cpp
View file

@ -57,8 +57,10 @@ std::optional<std::string> 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.
// 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(),
@ -150,24 +151,26 @@ std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& i
std::optional<std::string> PaysForRBF(CAmount original_fees,
CAmount replacement_fees,
size_t replacement_vsize,
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 < ::incrementalRelayFee.GetFee(replacement_vsize)) {
if (additional_fees < relay_fee.GetFee(replacement_vsize)) {
return strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s",
txid.ToString(),
FormatMoney(additional_fees),
FormatMoney(::incrementalRelayFee.GetFee(replacement_vsize)));
FormatMoney(relay_fee.GetFee(replacement_vsize)));
}
return std::nullopt;
}

7
src/policy/rbf.h
View file

@ -5,7 +5,12 @@
#ifndef BITCOIN_POLICY_RBF_H
#define BITCOIN_POLICY_RBF_H
#include <primitives/transaction.h>
#include <txmempool.h>
#include <uint256.h>
#include <optional>
#include <string>
/** Maximum number of transactions that can be replaced by BIP125 RBF (Rule #5). This includes all
* mempool conflicts and their descendants. */
@ -84,12 +89,14 @@ std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& i
* @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] relay_fee The node's minimum feerate for transaction relay.
* @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<std::string> PaysForRBF(CAmount original_fees,
CAmount replacement_fees,
size_t replacement_vsize,
CFeeRate relay_fee,
const uint256& txid);
#endif // BITCOIN_POLICY_RBF_H

6
src/util/rbf.h
View file

@ -9,7 +9,7 @@
class CTransaction;
static const uint32_t MAX_BIP125_RBF_SEQUENCE = 0xfffffffd;
static constexpr 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 >
@ -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

24
src/validation.cpp
View file

@ -772,39 +772,43 @@ 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();
}
if (const auto err_string{PaysForRBF(nConflictingFees, nModifiedFees, nSize, hash)}) {
if (const auto err_string{PaysForRBF(nConflictingFees, nModifiedFees, nSize, ::incrementalRelayFee, hash)}) {
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string);
}
}