Skip to content

Commit 2079b80

Browse files
instagibbssdaftuar
andcommitted
Implement ImprovesFeerateDiagram
This new function takes the populated sets of direct and all conflicts computed in the current mempool, assuming the replacements are a single chunk, and computes a diagram check. The diagram check only works against cluster sizes of 2 or less, and fails if it encounters a different topology. Co-authored-by: Suhas Daftuar <[email protected]>
1 parent 66d966d commit 2079b80

File tree

4 files changed

+201
-0
lines changed

4 files changed

+201
-0
lines changed

src/policy/rbf.cpp

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919
#include <limits>
2020
#include <vector>
2121

22+
#include <compare>
23+
2224
RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
2325
{
2426
AssertLockHeld(pool.cs);
@@ -181,3 +183,24 @@ std::optional<std::string> PaysForRBF(CAmount original_fees,
181183
}
182184
return std::nullopt;
183185
}
186+
187+
std::optional<std::pair<DiagramCheckError, std::string>> ImprovesFeerateDiagram(CTxMemPool& pool,
188+
const CTxMemPool::setEntries& direct_conflicts,
189+
const CTxMemPool::setEntries& all_conflicts,
190+
CAmount replacement_fees,
191+
int64_t replacement_vsize)
192+
{
193+
// Require that the replacement strictly improve the mempool's feerate diagram.
194+
std::vector<FeeFrac> old_diagram, new_diagram;
195+
196+
const auto diagram_results{pool.CalculateFeerateDiagramsForRBF(replacement_fees, replacement_vsize, direct_conflicts, all_conflicts)};
197+
198+
if (!diagram_results.has_value()) {
199+
return std::make_pair(DiagramCheckError::UNCALCULABLE, util::ErrorString(diagram_results).original);
200+
}
201+
202+
if (!std::is_gt(CompareFeerateDiagram(diagram_results.value().second, diagram_results.value().first))) {
203+
return std::make_pair(DiagramCheckError::FAILURE, "insufficient feerate: does not improve feerate diagram");
204+
}
205+
return std::nullopt;
206+
}

src/policy/rbf.h

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@
99
#include <primitives/transaction.h>
1010
#include <threadsafety.h>
1111
#include <txmempool.h>
12+
#include <util/feefrac.h>
1213

14+
#include <compare>
1315
#include <cstddef>
1416
#include <cstdint>
1517
#include <optional>
@@ -33,6 +35,13 @@ enum class RBFTransactionState {
3335
FINAL,
3436
};
3537

38+
enum class DiagramCheckError {
39+
/** Unable to calculate due to topology or other reason */
40+
UNCALCULABLE,
41+
/** New diagram wasn't strictly superior */
42+
FAILURE,
43+
};
44+
3645
/**
3746
* Determine whether an unconfirmed transaction is signaling opt-in to RBF
3847
* according to BIP 125
@@ -106,4 +115,21 @@ std::optional<std::string> PaysForRBF(CAmount original_fees,
106115
CFeeRate relay_fee,
107116
const uint256& txid);
108117

118+
/**
119+
* The replacement transaction must improve the feerate diagram of the mempool.
120+
* @param[in] pool The mempool.
121+
* @param[in] direct_conflicts Set of in-mempool txids corresponding to the direct conflicts i.e.
122+
* input double-spends with the proposed transaction
123+
* @param[in] all_conflicts Set of mempool entries corresponding to all transactions to be evicted
124+
* @param[in] replacement_fees Fees of proposed replacement package
125+
* @param[in] replacement_vsize Size of proposed replacement package
126+
* @returns error type and string if mempool diagram doesn't improve, otherwise std::nullopt.
127+
*/
128+
std::optional<std::pair<DiagramCheckError, std::string>> ImprovesFeerateDiagram(CTxMemPool& pool,
129+
const CTxMemPool::setEntries& direct_conflicts,
130+
const CTxMemPool::setEntries& all_conflicts,
131+
CAmount replacement_fees,
132+
int64_t replacement_vsize)
133+
EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
134+
109135
#endif // BITCOIN_POLICY_RBF_H

src/txmempool.cpp

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include <random.h>
1818
#include <reverse_iterator.h>
1919
#include <util/check.h>
20+
#include <util/feefrac.h>
2021
#include <util/moneystr.h>
2122
#include <util/overflow.h>
2223
#include <util/result.h>
@@ -1238,3 +1239,131 @@ std::vector<CTxMemPool::txiter> CTxMemPool::GatherClusters(const std::vector<uin
12381239
}
12391240
return clustered_txs;
12401241
}
1242+
1243+
std::optional<std::string> CTxMemPool::CheckConflictTopology(const setEntries& direct_conflicts)
1244+
{
1245+
for (const auto& direct_conflict : direct_conflicts) {
1246+
// Ancestor and descendant counts are inclusive of the tx itself.
1247+
const auto ancestor_count{direct_conflict->GetCountWithAncestors()};
1248+
const auto descendant_count{direct_conflict->GetCountWithDescendants()};
1249+
const bool has_ancestor{ancestor_count > 1};
1250+
const bool has_descendant{descendant_count > 1};
1251+
const auto& txid_string{direct_conflict->GetSharedTx()->GetHash().ToString()};
1252+
// The only allowed configurations are:
1253+
// 1 ancestor and 0 descendant
1254+
// 0 ancestor and 1 descendant
1255+
// 0 ancestor and 0 descendant
1256+
if (ancestor_count > 2) {
1257+
return strprintf("%s has %u ancestors, max 1 allowed", txid_string, ancestor_count - 1);
1258+
} else if (descendant_count > 2) {
1259+
return strprintf("%s has %u descendants, max 1 allowed", txid_string, descendant_count - 1);
1260+
} else if (has_ancestor && has_descendant) {
1261+
return strprintf("%s has both ancestor and descendant, exceeding cluster limit of 2", txid_string);
1262+
}
1263+
// Additionally enforce that:
1264+
// If we have a child, we are its only parent.
1265+
// If we have a parent, we are its only child.
1266+
if (has_descendant) {
1267+
const auto& our_child = direct_conflict->GetMemPoolChildrenConst().begin();
1268+
if (our_child->get().GetCountWithAncestors() > 2) {
1269+
return strprintf("%s is not the only parent of child %s",
1270+
txid_string, our_child->get().GetSharedTx()->GetHash().ToString());
1271+
}
1272+
} else if (has_ancestor) {
1273+
const auto& our_parent = direct_conflict->GetMemPoolParentsConst().begin();
1274+
if (our_parent->get().GetCountWithDescendants() > 2) {
1275+
return strprintf("%s is not the only child of parent %s",
1276+
txid_string, our_parent->get().GetSharedTx()->GetHash().ToString());
1277+
}
1278+
}
1279+
}
1280+
return std::nullopt;
1281+
}
1282+
1283+
util::Result<std::pair<std::vector<FeeFrac>, std::vector<FeeFrac>>> CTxMemPool::CalculateFeerateDiagramsForRBF(CAmount replacement_fees, int64_t replacement_vsize, const setEntries& direct_conflicts, const setEntries& all_conflicts)
1284+
{
1285+
Assume(replacement_vsize > 0);
1286+
1287+
auto err_string{CheckConflictTopology(direct_conflicts)};
1288+
if (err_string.has_value()) {
1289+
// Unsupported topology for calculating a feerate diagram
1290+
return util::Error{Untranslated(err_string.value())};
1291+
}
1292+
1293+
// new diagram will have chunks that consist of each ancestor of
1294+
// direct_conflicts that is at its own fee/size, along with the replacement
1295+
// tx/package at its own fee/size
1296+
1297+
// old diagram will consist of each element of all_conflicts either at
1298+
// its own feerate (followed by any descendant at its own feerate) or as a
1299+
// single chunk at its descendant's ancestor feerate.
1300+
1301+
std::vector<FeeFrac> old_chunks;
1302+
// Step 1: build the old diagram.
1303+
1304+
// The above clusters are all trivially linearized;
1305+
// they have a strict topology of 1 or two connected transactions.
1306+
1307+
// OLD: Compute existing chunks from all affected clusters
1308+
for (auto txiter : all_conflicts) {
1309+
// Does this transaction have descendants?
1310+
if (txiter->GetCountWithDescendants() > 1) {
1311+
// Consider this tx when we consider the descendant.
1312+
continue;
1313+
}
1314+
// Does this transaction have ancestors?
1315+
FeeFrac individual{txiter->GetModifiedFee(), txiter->GetTxSize()};
1316+
if (txiter->GetCountWithAncestors() > 1) {
1317+
// We'll add chunks for either the ancestor by itself and this tx
1318+
// by itself, or for a combined package.
1319+
FeeFrac package{txiter->GetModFeesWithAncestors(), static_cast<int32_t>(txiter->GetSizeWithAncestors())};
1320+
if (individual > package) {
1321+
// The individual feerate is higher than the package, and
1322+
// therefore higher than the parent's fee. Chunk these
1323+
// together.
1324+
old_chunks.emplace_back(package);
1325+
} else {
1326+
// Add two points, one for the parent and one for this child.
1327+
old_chunks.emplace_back(package - individual);
1328+
old_chunks.emplace_back(individual);
1329+
}
1330+
} else {
1331+
old_chunks.emplace_back(individual);
1332+
}
1333+
}
1334+
1335+
// No topology restrictions post-chunking; sort
1336+
std::sort(old_chunks.begin(), old_chunks.end(), std::greater());
1337+
std::vector<FeeFrac> old_diagram = BuildDiagramFromChunks(old_chunks);
1338+
1339+
std::vector<FeeFrac> new_chunks;
1340+
1341+
/* Step 2: build the NEW diagram
1342+
* CON = Conflicts of proposed chunk
1343+
* CNK = Proposed chunk
1344+
* NEW = OLD - CON + CNK: New diagram includes all chunks in OLD, minus
1345+
* the conflicts, plus the proposed chunk
1346+
*/
1347+
1348+
// OLD - CON: Add any parents of direct conflicts that are not conflicted themselves
1349+
for (auto direct_conflict : direct_conflicts) {
1350+
// If a direct conflict has an ancestor that is not in all_conflicts,
1351+
// it can be affected by the replacement of the child.
1352+
if (direct_conflict->GetMemPoolParentsConst().size() > 0) {
1353+
// Grab the parent.
1354+
const CTxMemPoolEntry& parent = direct_conflict->GetMemPoolParentsConst().begin()->get();
1355+
if (!all_conflicts.count(mapTx.iterator_to(parent))) {
1356+
// This transaction would be left over, so add to the NEW
1357+
// diagram.
1358+
new_chunks.emplace_back(parent.GetModifiedFee(), parent.GetTxSize());
1359+
}
1360+
}
1361+
}
1362+
// + CNK: Add the proposed chunk itself
1363+
new_chunks.emplace_back(replacement_fees, int32_t(replacement_vsize));
1364+
1365+
// No topology restrictions post-chunking; sort
1366+
std::sort(new_chunks.begin(), new_chunks.end(), std::greater());
1367+
std::vector<FeeFrac> new_diagram = BuildDiagramFromChunks(new_chunks);
1368+
return std::make_pair(old_diagram, new_diagram);
1369+
}

src/txmempool.h

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include <util/epochguard.h>
2222
#include <util/hasher.h>
2323
#include <util/result.h>
24+
#include <util/feefrac.h>
2425

2526
#include <boost/multi_index/hashed_index.hpp>
2627
#include <boost/multi_index/identity.hpp>
@@ -736,6 +737,28 @@ class CTxMemPool
736737
return m_sequence_number;
737738
}
738739

740+
/**
741+
* Calculate the old and new mempool feerate diagrams relating to the
742+
* clusters that would be affected by a potential replacement transaction.
743+
* (replacement_fees, replacement_vsize) values are gathered from a
744+
* proposed set of replacement transactions that are considered as a single
745+
* chunk, and represent their complete cluster. In other words, they have no
746+
* in-mempool ancestors.
747+
*
748+
* @param[in] replacement_fees Package fees
749+
* @param[in] replacement_vsize Package size (must be greater than 0)
750+
* @param[in] direct_conflicts All transactions that would be removed directly by
751+
* having a conflicting input with a proposed transaction
752+
* @param[in] all_conflicts All transactions that would be removed
753+
* @return old and new diagram pair respectively, or an error string if the conflicts don't match a calculable topology
754+
*/
755+
util::Result<std::pair<std::vector<FeeFrac>, std::vector<FeeFrac>>> CalculateFeerateDiagramsForRBF(CAmount replacement_fees, int64_t replacement_vsize, const setEntries& direct_conflicts, const setEntries& all_conflicts) EXCLUSIVE_LOCKS_REQUIRED(cs);
756+
757+
/* Check that all direct conflicts are in a cluster size of two or less. Each
758+
* direct conflict may be in a separate cluster.
759+
*/
760+
std::optional<std::string> CheckConflictTopology(const setEntries& direct_conflicts);
761+
739762
private:
740763
/** UpdateForDescendants is used by UpdateTransactionsFromBlock to update
741764
* the descendants for a single transaction that has been added to the

0 commit comments

Comments
 (0)