Skip to content

Commit a33fdd0

Browse files
glozowglozow
committed
[doc] improve RBF documentation + style cleanups after refactors
Document a few non-obvious things and delete no-longer-relevant comments (e.g. about taking a lock that we're already holding). No change in behavior.
1 parent 0d93e6c commit a33fdd0

File tree

3 files changed

+64
-68
lines changed

3 files changed

+64
-68
lines changed

‎src/policy/rbf.cpp‎

Lines changed: 47 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -58,19 +58,19 @@ bool GetEntriesForConflicts(const CTransaction& tx,
5858
uint64_t nConflictingCount = 0;
5959
for (const auto& mi : iters_conflicting) {
6060
nConflictingCount += mi->GetCountWithDescendants();
61-
// This potentially overestimates the number of actual descendants
62-
// but we just want to be conservative to avoid doing too much
63-
// work.
61+
// BIP125 Rule #5: don't consider replacing more than MAX_BIP125_REPLACEMENT_CANDIDATES
62+
// entries from the mempool. This potentially overestimates the number of actual
63+
// descendants (i.e. if multiple conflicts share a descendant, it will be counted multiple
64+
// times), but we just want to be conservative to avoid doing too much work.
6465
if (nConflictingCount > MAX_BIP125_REPLACEMENT_CANDIDATES) {
6566
err_string = strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n",
66-
hash.ToString(),
67-
nConflictingCount,
68-
MAX_BIP125_REPLACEMENT_CANDIDATES);
67+
hash.ToString(),
68+
nConflictingCount,
69+
MAX_BIP125_REPLACEMENT_CANDIDATES);
6970
return false;
7071
}
7172
}
72-
// If not too many to replace, then calculate the set of
73-
// transactions that would have to be evicted
73+
// Calculate the set of all transactions that would have to be evicted.
7474
for (CTxMemPool::txiter it : iters_conflicting) {
7575
pool.CalculateDescendants(it, all_conflicts);
7676
}
@@ -84,31 +84,25 @@ bool HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& pool,
8484
AssertLockHeld(pool.cs);
8585
std::set<uint256> parents_of_conflicts;
8686
for (const auto& mi : iters_conflicting) {
87-
for (const CTxIn &txin : mi->GetTx().vin)
88-
{
87+
for (const CTxIn &txin : mi->GetTx().vin) {
8988
parents_of_conflicts.insert(txin.prevout.hash);
9089
}
9190
}
9291

93-
for (unsigned int j = 0; j < tx.vin.size(); j++)
94-
{
95-
// We don't want to accept replacements that require low
96-
// feerate junk to be mined first. Ideally we'd keep track of
97-
// the ancestor feerates and make the decision based on that,
98-
// but for now requiring all new inputs to be confirmed works.
92+
for (unsigned int j = 0; j < tx.vin.size(); j++) {
93+
// BIP125 Rule #2: We don't want to accept replacements that require low feerate junk to be
94+
// mined first. Ideally we'd keep track of the ancestor feerates and make the decision
95+
// based on that, but for now requiring all new inputs to be confirmed works.
9996
//
100-
// Note that if you relax this to make RBF a little more useful,
101-
// this may break the CalculateMempoolAncestors RBF relaxation,
102-
// above. See the comment above the first CalculateMempoolAncestors
103-
// call for more info.
104-
if (!parents_of_conflicts.count(tx.vin[j].prevout.hash))
105-
{
106-
// Rather than check the UTXO set - potentially expensive -
107-
// it's cheaper to just check if the new input refers to a
108-
// tx that's in the mempool.
97+
// Note that if you relax this to make RBF a little more useful, this may break the
98+
// CalculateMempoolAncestors RBF relaxation which subtracts the conflict count/size from the
99+
// descendant limit.
100+
if (!parents_of_conflicts.count(tx.vin[j].prevout.hash)) {
101+
// Rather than check the UTXO set - potentially expensive - it's cheaper to just check
102+
// if the new input refers to a tx that's in the mempool.
109103
if (pool.exists(tx.vin[j].prevout.hash)) {
110104
err_string = strprintf("replacement %s adds unconfirmed input, idx %d",
111-
tx.GetHash().ToString(), j);
105+
tx.GetHash().ToString(), j);
112106
return false;
113107
}
114108
}
@@ -120,14 +114,12 @@ bool EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors,
120114
const std::set<uint256>& direct_conflicts,
121115
const uint256& txid, std::string& err_string)
122116
{
123-
for (CTxMemPool::txiter ancestorIt : ancestors)
124-
{
117+
for (CTxMemPool::txiter ancestorIt : ancestors) {
125118
const uint256 &hashAncestor = ancestorIt->GetTx().GetHash();
126-
if (direct_conflicts.count(hashAncestor))
127-
{
119+
if (direct_conflicts.count(hashAncestor)) {
128120
err_string = strprintf("%s spends conflicting transaction %s",
129-
txid.ToString(),
130-
hashAncestor.ToString());
121+
txid.ToString(),
122+
hashAncestor.ToString());
131123
return false;
132124
}
133125
}
@@ -138,27 +130,23 @@ bool PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting, CFee
138130
const uint256& hash, std::string& err_string)
139131
{
140132
for (const auto& mi : iters_conflicting) {
141-
// Don't allow the replacement to reduce the feerate of the
142-
// mempool.
133+
// Don't allow the replacement to reduce the feerate of the mempool.
143134
//
144-
// We usually don't want to accept replacements with lower
145-
// feerates than what they replaced as that would lower the
146-
// feerate of the next block. Requiring that the feerate always
147-
// be increased is also an easy-to-reason about way to prevent
148-
// DoS attacks via replacements.
135+
// We usually don't want to accept replacements with lower feerates than what they replaced
136+
// as that would lower the feerate of the next block. Requiring that the feerate always be
137+
// increased is also an easy-to-reason about way to prevent DoS attacks via replacements.
149138
//
150-
// We only consider the feerates of transactions being directly
151-
// replaced, not their indirect descendants. While that does
152-
// mean high feerate children are ignored when deciding whether
153-
// or not to replace, we do require the replacement to pay more
154-
// overall fees too, mitigating most cases.
139+
// We only consider the feerates of transactions being directly replaced, not their indirect
140+
// descendants. While that does mean high feerate children are ignored when deciding whether
141+
// or not to replace, we do require the replacement to pay more overall fees too, mitigating
142+
// most cases.
155143
CFeeRate original_feerate(mi->GetModifiedFee(), mi->GetTxSize());
156144
if (replacement_feerate <= original_feerate)
157145
{
158146
err_string = strprintf("rejecting replacement %s; new feerate %s <= old feerate %s",
159-
hash.ToString(),
160-
replacement_feerate.ToString(),
161-
original_feerate.ToString());
147+
hash.ToString(),
148+
replacement_feerate.ToString(),
149+
original_feerate.ToString());
162150
return false;
163151
}
164152
}
@@ -168,25 +156,28 @@ bool PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting, CFee
168156
bool PaysForRBF(CAmount original_fees, CAmount replacement_fees, size_t replacement_vsize,
169157
const uint256& hash, std::string& err_string)
170158
{
171-
// The replacement must pay greater fees than the transactions it
172-
// replaces - if we did the bandwidth used by those conflicting
173-
// transactions would not be paid for.
159+
// BIP125 Rule #3: The replacement fees must be greater than or equal to fees of the
160+
// transactions it replaces, otherwise the bandwidth used by those conflicting transactions
161+
// would not be paid for.
174162
if (replacement_fees < original_fees)
175163
{
176164
err_string = strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s",
177-
hash.ToString(), FormatMoney(replacement_fees), FormatMoney(original_fees));
165+
hash.ToString(),
166+
FormatMoney(replacement_fees),
167+
FormatMoney(original_fees));
178168
return false;
179169
}
180170

181-
// Finally in addition to paying more fees than the conflicts the
182-
// new transaction must pay for its own bandwidth.
171+
// BIP125 Rule #4: The new transaction must pay for its own bandwidth. Otherwise, we have a DoS
172+
// vector where attackers can cause a transaction to be replaced (and relayed) repeatedly by
173+
// increasing the fee by tiny amounts.
183174
CAmount additional_fees = replacement_fees - original_fees;
184175
if (additional_fees < ::incrementalRelayFee.GetFee(replacement_vsize))
185176
{
186177
err_string = strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s",
187-
hash.ToString(),
188-
FormatMoney(additional_fees),
189-
FormatMoney(::incrementalRelayFee.GetFee(replacement_vsize)));
178+
hash.ToString(),
179+
FormatMoney(additional_fees),
180+
FormatMoney(::incrementalRelayFee.GetFee(replacement_vsize)));
190181
return false;
191182
}
192183
return true;

‎src/policy/rbf.h‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,9 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx);
3939
* transactions to be replaced and their descendant transactions which will be evicted from the
4040
* mempool must not exceed a total of 100 transactions." Quit as early as possible. There cannot be
4141
* more than MAX_BIP125_REPLACEMENT_CANDIDATES potential entries.
42-
* @param[in] iters_conflicting The set of iterators to mempool entries.
42+
* @param[in] iters_conflicting The set of iterators to mempool entries.
4343
* @param[out] err_string Used to return errors, if any.
44-
* @param[out] all_conflicts Populated with all the mempool entries that would be replaced,
44+
* @param[out] all_conflicts Populated with all the mempool entries that would be replaced,
4545
* which includes descendants of iters_conflicting. Not cleared at
4646
* the start; any existing mempool entries will remain in the set.
4747
* @returns false if Rule 5 is broken.
@@ -59,11 +59,11 @@ bool HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& pool,
5959

6060
/** Check the intersection between two sets of transactions (a set of mempool entries and a set of
6161
* txids) to make sure they are disjoint.
62-
* @param[in] ancestors Set of mempool entries corresponding to ancestors of the
63-
* replacement transactions.
62+
* @param[in] ancestors Set of mempool entries corresponding to ancestors of the
63+
* replacement transactions.
6464
* @param[in] direct_conflicts Set of txids corresponding to the mempool conflicts
65-
* (candidates to be replaced).
66-
* @param[in] txid Transaction ID, included in the error message if violation occurs.
65+
* (candidates to be replaced).
66+
* @param[in] txid Transaction ID, included in the error message if violation occurs.
6767
* returns true if the two sets are disjoint (i.e. intersection is empty), false if otherwise.
6868
*/
6969
bool EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors,
@@ -79,9 +79,9 @@ bool PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting, CFee
7979
/** Enforce BIP125 Rule #3 "The replacement transaction pays an absolute fee of at least the sum
8080
* paid by the original transactions." Enforce BIP125 Rule #4 "The replacement transaction must also
8181
* pay for its own bandwidth at or above the rate set by the node's minimum relay fee setting."
82-
* @param[in] original_fees Total modified fees of original transaction(s).
83-
* @param[in] replacement_fees Total modified fees of replacement transaction(s).
84-
* @param[in] replacement_vsize Total virtual size of replacement transaction(s).
82+
* @param[in] original_fees Total modified fees of original transaction(s).
83+
* @param[in] replacement_fees Total modified fees of replacement transaction(s).
84+
* @param[in] replacement_vsize Total virtual size of replacement transaction(s).
8585
* @param[in] hash Transaction ID, included in the error message if violation occurs.
8686
* returns true if fees are sufficient, false if otherwise.
8787
*/

‎src/validation.cpp‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -771,18 +771,23 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
771771
// pathological case by making sure setConflicts and setAncestors don't
772772
// intersect.
773773
if (!EntriesAndTxidsDisjoint(setAncestors, setConflicts, hash, errString)) {
774+
// We classify this as a consensus error because a transaction depending on something it
775+
// conflicts with would be inconsistent.
774776
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", errString);
775777
}
776778

777779

778-
// If we don't hold the lock allConflicting might be incomplete; the
779-
// subsequent RemoveStaged() and addUnchecked() calls don't guarantee
780-
// mempool consistency for us.
781780
fReplacementTransaction = setConflicts.size();
782781
if (fReplacementTransaction)
783782
{
784783
std::string err_string;
785784
CFeeRate newFeeRate(nModifiedFees, nSize);
785+
// It's possible that the replacement pays more fees than its direct conflicts but not more
786+
// than all conflicts (i.e. the direct conflicts have high-fee descendants). However, if the
787+
// replacement doesn't pay more fees than its direct conflicts, then we can be sure it's not
788+
// more economically rational to mine. Before we go digging through the mempool for all
789+
// transactions that would need to be removed (direct conflicts and all descendants), check
790+
// that the replacement transaction pays more than its direct conflicts.
786791
if (!PaysMoreThanConflicts(setIterConflicting, newFeeRate, hash, err_string)) {
787792
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", err_string);
788793
}

0 commit comments

Comments
 (0)