Skip to content

Commit b8336b2

Browse files
fanquakefanquake
committed
Merge bitcoin/bitcoin#22675: RBF move 2/3: extract RBF logic into policy/rbf
32748da whitespace fixups after move and scripted-diff (glozow) fa47622 scripted-diff: rename variables in policy/rbf (glozow) ac761f0 MOVEONLY: fee checks (Rules 3 and 4) to policy/rbf (glozow) 9c2f9f8 MOVEONLY: check that fees > direct conflicts to policy/rbf (glozow) 3f033f0 MOVEONLY: check for disjoint conflicts and ancestors to policy/rbf (glozow) 7b60c02 MOVEONLY: BIP125 Rule 2 to policy/rbf (glozow) f8ad2a5 Make GetEntriesForConflicts return std::optional (glozow) Pull request description: This PR does not change behavior. It extracts the BIP125 logic into helper functions (and puts them in the policy/rbf* files). This enables three things - I think each one individually is pretty good: - Implementation of package RBF (see #22290). I want it to be as close to BIP125 as possible so that it doesn't become a distinct fee-bumping mechanism. Doing these move-only commits first means the diff is mostly mechanical to review, and I just need to create a function that mirrors the single transaction validation. - We will be able to isolate and test our RBF logic alone. Recently, there have been some discussions on discrepancies between our code and BIP125, as well as proposals for improving it. Generally, I think making this code more modular and de-bloating validation.cpp is probably a good idea. - Witness Replacement (replacing same-txid-different-wtxid when the witness is significantly smaller and therefore higher feerate) in a BIP125-similar way. Hopefully it can just be implemented with calls to the rbf functions (i.e. `PaysForRBF`) and an edit to the relevant mempool entries. ACKs for top commit: mjdietzx: ACK 32748da theStack: Code-review ACK 32748da 📐 MarcoFalke: review ACK 32748da 🦇 Tree-SHA512: d89985c8b4b42b54861018deb89468e04968c85a3fb1113bbcb2eb2609577bc4fd9bf254593b5bd0e7ab059a0fa8192d1a903b00f77e6f120c7a80488ffcbfc0
2 parents 5446070 + 32748da commit b8336b2

File tree

4 files changed

+189
-131
lines changed

4 files changed

+189
-131
lines changed

‎src/policy/rbf.cpp

Lines changed: 116 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
1313
{
1414
AssertLockHeld(pool.cs);
1515

16-
CTxMemPool::setEntries setAncestors;
16+
CTxMemPool::setEntries ancestors;
1717

1818
// First check the transaction itself.
1919
if (SignalsOptInRBF(tx)) {
@@ -31,9 +31,9 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
3131
uint64_t noLimit = std::numeric_limits<uint64_t>::max();
3232
std::string dummy;
3333
CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
34-
pool.CalculateMemPoolAncestors(entry, setAncestors, noLimit, noLimit, noLimit, noLimit, dummy, false);
34+
pool.CalculateMemPoolAncestors(entry, ancestors, noLimit, noLimit, noLimit, noLimit, dummy, false);
3535

36-
for (CTxMemPool::txiter it : setAncestors) {
36+
for (CTxMemPool::txiter it : ancestors) {
3737
if (SignalsOptInRBF(it->GetTx())) {
3838
return RBFTransactionState::REPLACEABLE_BIP125;
3939
}
@@ -47,33 +47,127 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx)
4747
return SignalsOptInRBF(tx) ? RBFTransactionState::REPLACEABLE_BIP125 : RBFTransactionState::UNKNOWN;
4848
}
4949

50-
bool GetEntriesForConflicts(const CTransaction& tx,
51-
CTxMemPool& m_pool,
52-
const CTxMemPool::setEntries& setIterConflicting,
53-
CTxMemPool::setEntries& allConflicting,
54-
std::string& err_string)
50+
std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx,
51+
CTxMemPool& pool,
52+
const CTxMemPool::setEntries& iters_conflicting,
53+
CTxMemPool::setEntries& all_conflicts)
5554
{
56-
AssertLockHeld(m_pool.cs);
57-
const uint256 hash = tx.GetHash();
55+
AssertLockHeld(pool.cs);
56+
const uint256 txid = tx.GetHash();
5857
uint64_t nConflictingCount = 0;
59-
for (const auto& mi : setIterConflicting) {
58+
for (const auto& mi : iters_conflicting) {
6059
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.
60+
// This potentially overestimates the number of actual descendants but we just want to be
61+
// conservative to avoid doing too much work.
6462
if (nConflictingCount > MAX_BIP125_REPLACEMENT_CANDIDATES) {
65-
err_string = strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n",
66-
hash.ToString(),
67-
nConflictingCount,
68-
MAX_BIP125_REPLACEMENT_CANDIDATES);
69-
return false;
63+
return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n",
64+
txid.ToString(),
65+
nConflictingCount,
66+
MAX_BIP125_REPLACEMENT_CANDIDATES);
7067
}
7168
}
7269
// If not too many to replace, then calculate the set of
7370
// transactions that would have to be evicted
74-
for (CTxMemPool::txiter it : setIterConflicting) {
75-
m_pool.CalculateDescendants(it, allConflicting);
71+
for (CTxMemPool::txiter it : iters_conflicting) {
72+
pool.CalculateDescendants(it, all_conflicts);
7673
}
77-
return true;
74+
return std::nullopt;
7875
}
7976

77+
std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx,
78+
const CTxMemPool& pool,
79+
const CTxMemPool::setEntries& iters_conflicting)
80+
{
81+
AssertLockHeld(pool.cs);
82+
std::set<uint256> parents_of_conflicts;
83+
for (const auto& mi : iters_conflicting) {
84+
for (const CTxIn &txin : mi->GetTx().vin) {
85+
parents_of_conflicts.insert(txin.prevout.hash);
86+
}
87+
}
88+
89+
for (unsigned int j = 0; j < tx.vin.size(); j++) {
90+
// We don't want to accept replacements that require low feerate junk to be mined first.
91+
// Ideally we'd keep track of the ancestor feerates and make the decision based on that, but
92+
// for now requiring all new inputs to be confirmed works.
93+
//
94+
// Note that if you relax this to make RBF a little more useful, this may break the
95+
// CalculateMempoolAncestors RBF relaxation, above. See the comment above the first
96+
// CalculateMempoolAncestors call for more info.
97+
if (!parents_of_conflicts.count(tx.vin[j].prevout.hash)) {
98+
// Rather than check the UTXO set - potentially expensive - it's cheaper to just check
99+
// if the new input refers to a tx that's in the mempool.
100+
if (pool.exists(tx.vin[j].prevout.hash)) {
101+
return strprintf("replacement %s adds unconfirmed input, idx %d",
102+
tx.GetHash().ToString(), j);
103+
}
104+
}
105+
}
106+
return std::nullopt;
107+
}
108+
109+
std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors,
110+
const std::set<uint256>& direct_conflicts,
111+
const uint256& txid)
112+
{
113+
for (CTxMemPool::txiter ancestorIt : ancestors) {
114+
const uint256 &hashAncestor = ancestorIt->GetTx().GetHash();
115+
if (direct_conflicts.count(hashAncestor)) {
116+
return strprintf("%s spends conflicting transaction %s",
117+
txid.ToString(),
118+
hashAncestor.ToString());
119+
}
120+
}
121+
return std::nullopt;
122+
}
123+
124+
std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting,
125+
CFeeRate replacement_feerate,
126+
const uint256& txid)
127+
{
128+
for (const auto& mi : iters_conflicting) {
129+
// Don't allow the replacement to reduce the feerate of the mempool.
130+
//
131+
// We usually don't want to accept replacements with lower feerates than what they replaced
132+
// as that would lower the feerate of the next block. Requiring that the feerate always be
133+
// increased is also an easy-to-reason about way to prevent DoS attacks via replacements.
134+
//
135+
// We only consider the feerates of transactions being directly replaced, not their indirect
136+
// descendants. While that does mean high feerate children are ignored when deciding whether
137+
// or not to replace, we do require the replacement to pay more overall fees too, mitigating
138+
// most cases.
139+
CFeeRate original_feerate(mi->GetModifiedFee(), mi->GetTxSize());
140+
if (replacement_feerate <= original_feerate) {
141+
return strprintf("rejecting replacement %s; new feerate %s <= old feerate %s",
142+
txid.ToString(),
143+
replacement_feerate.ToString(),
144+
original_feerate.ToString());
145+
}
146+
}
147+
return std::nullopt;
148+
}
149+
150+
std::optional<std::string> PaysForRBF(CAmount original_fees,
151+
CAmount replacement_fees,
152+
size_t replacement_vsize,
153+
const uint256& txid)
154+
{
155+
// The replacement must pay greater fees than the transactions it
156+
// replaces - if we did the bandwidth used by those conflicting
157+
// transactions would not be paid for.
158+
if (replacement_fees < original_fees) {
159+
return strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s",
160+
txid.ToString(), FormatMoney(replacement_fees), FormatMoney(original_fees));
161+
}
162+
163+
// Finally in addition to paying more fees than the conflicts the
164+
// new transaction must pay for its own bandwidth.
165+
CAmount additional_fees = replacement_fees - original_fees;
166+
if (additional_fees < ::incrementalRelayFee.GetFee(replacement_vsize)) {
167+
return strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s",
168+
txid.ToString(),
169+
FormatMoney(additional_fees),
170+
FormatMoney(::incrementalRelayFee.GetFee(replacement_vsize)));
171+
}
172+
return std::nullopt;
173+
}

‎src/policy/rbf.h

Lines changed: 52 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -35,19 +35,61 @@ enum class RBFTransactionState {
3535
RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
3636
RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx);
3737

38-
/** Get all descendants of setIterConflicting. Also enforce BIP125 Rule #5, "The number of original
38+
/** Get all descendants of iters_conflicting. Also enforce BIP125 Rule #5, "The number of original
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] setIterConflicting The set of iterators to mempool entries.
43-
* @param[out] err_string Used to return errors, if any.
44-
* @param[out] allConflicting Populated with all the mempool entries that would be replaced,
45-
* which includes descendants of setIterConflicting. Not cleared at
42+
* @param[in] iters_conflicting The set of iterators to mempool entries.
43+
* @param[out] all_conflicts Populated with all the mempool entries that would be replaced,
44+
* which includes descendants of iters_conflicting. Not cleared at
4645
* the start; any existing mempool entries will remain in the set.
47-
* @returns false if Rule 5 is broken.
46+
* @returns an error message if Rule #5 is broken, otherwise a std::nullopt.
4847
*/
49-
bool GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& m_pool,
50-
const CTxMemPool::setEntries& setIterConflicting,
51-
CTxMemPool::setEntries& allConflicting,
52-
std::string& err_string) EXCLUSIVE_LOCKS_REQUIRED(m_pool.cs);
48+
std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& pool,
49+
const CTxMemPool::setEntries& iters_conflicting,
50+
CTxMemPool::setEntries& all_conflicts)
51+
EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
52+
53+
/** BIP125 Rule #2: "The replacement transaction may only include an unconfirmed input if that input
54+
* was included in one of the original transactions."
55+
* @returns error message if Rule #2 is broken, otherwise std::nullopt. */
56+
std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& pool,
57+
const CTxMemPool::setEntries& iters_conflicting)
58+
EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
59+
60+
/** Check the intersection between two sets of transactions (a set of mempool entries and a set of
61+
* txids) to make sure they are disjoint.
62+
* @param[in] ancestors Set of mempool entries corresponding to ancestors of the
63+
* replacement transactions.
64+
* @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.
67+
* @returns error message if the sets intersect, std::nullopt if they are disjoint.
68+
*/
69+
std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors,
70+
const std::set<uint256>& direct_conflicts,
71+
const uint256& txid);
72+
73+
/** Check that the feerate of the replacement transaction(s) is higher than the feerate of each
74+
* of the transactions in iters_conflicting.
75+
* @param[in] iters_conflicting The set of mempool entries.
76+
* @returns error message if fees insufficient, otherwise std::nullopt.
77+
*/
78+
std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting,
79+
CFeeRate replacement_feerate, const uint256& txid);
80+
81+
/** Enforce BIP125 Rule #3 "The replacement transaction pays an absolute fee of at least the sum
82+
* paid by the original transactions." Enforce BIP125 Rule #4 "The replacement transaction must also
83+
* pay for its own bandwidth at or above the rate set by the node's minimum relay fee setting."
84+
* @param[in] original_fees Total modified fees of original transaction(s).
85+
* @param[in] replacement_fees Total modified fees of replacement transaction(s).
86+
* @param[in] replacement_vsize Total virtual size of replacement transaction(s).
87+
* @param[in] txid Transaction ID, included in the error message if violation occurs.
88+
* @returns error string if fees are insufficient, otherwise std::nullopt.
89+
*/
90+
std::optional<std::string> PaysForRBF(CAmount original_fees,
91+
CAmount replacement_fees,
92+
size_t replacement_vsize,
93+
const uint256& txid);
94+
5395
#endif // BITCOIN_POLICY_RBF_H

‎src/util/rbf.h

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,15 +11,13 @@ class CTransaction;
1111

1212
static const uint32_t MAX_BIP125_RBF_SEQUENCE = 0xfffffffd;
1313

14-
/** Check whether the sequence numbers on this transaction are signaling
15-
* opt-in to replace-by-fee, according to BIP 125.
16-
* Allow opt-out of transaction replacement by setting
17-
* nSequence > MAX_BIP125_RBF_SEQUENCE (SEQUENCE_FINAL-2) on all inputs.
14+
/** Check whether the sequence numbers on this transaction are signaling opt-in to replace-by-fee,
15+
* according to BIP 125. Allow opt-out of transaction replacement by setting nSequence >
16+
* MAX_BIP125_RBF_SEQUENCE (SEQUENCE_FINAL-2) on all inputs.
1817
*
19-
* SEQUENCE_FINAL-1 is picked to still allow use of nLockTime by
20-
* non-replaceable transactions. All inputs rather than just one
21-
* is for the sake of multi-party protocols, where we don't
22-
* want a single party to be able to disable replacement. */
18+
* SEQUENCE_FINAL-1 is picked to still allow use of nLockTime by non-replaceable transactions. All
19+
* inputs rather than just one is for the sake of multi-party protocols, where we don't want a single
20+
* party to be able to disable replacement. */
2321
bool SignalsOptInRBF(const CTransaction &tx);
2422

2523
#endif // BITCOIN_UTIL_RBF_H

0 commit comments

Comments
 (0)