From 6773f92b302915b75db4ded9814563d42de3d489 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Tue, 9 Jan 2018 11:53:40 -0500 Subject: [PATCH 1/4] Refactor CompareTxMemPoolEntryByDescendantScore --- src/txmempool.h | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/src/txmempool.h b/src/txmempool.h index 512e70f8f..a0698387f 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -206,18 +206,14 @@ class CompareTxMemPoolEntryByDescendantScore public: bool operator()(const CTxMemPoolEntry& a, const CTxMemPoolEntry& b) const { - bool fUseADescendants = UseDescendantScore(a); - bool fUseBDescendants = UseDescendantScore(b); + double a_mod_fee, a_size, b_mod_fee, b_size; - double aModFee = fUseADescendants ? a.GetModFeesWithDescendants() : a.GetModifiedFee(); - double aSize = fUseADescendants ? a.GetSizeWithDescendants() : a.GetTxSize(); - - double bModFee = fUseBDescendants ? b.GetModFeesWithDescendants() : b.GetModifiedFee(); - double bSize = fUseBDescendants ? b.GetSizeWithDescendants() : b.GetTxSize(); + GetModFeeAndSize(a, a_mod_fee, a_size); + GetModFeeAndSize(b, b_mod_fee, b_size); // Avoid division by rewriting (a/b > c/d) as (a*d > c*b). - double f1 = aModFee * bSize; - double f2 = aSize * bModFee; + double f1 = a_mod_fee * b_size; + double f2 = a_size * b_mod_fee; if (f1 == f2) { return a.GetTime() >= b.GetTime(); @@ -225,12 +221,21 @@ public: return f1 < f2; } - // Calculate which score to use for an entry (avoiding division). - bool UseDescendantScore(const CTxMemPoolEntry &a) const + // Return the fee/size we're using for sorting this entry. + void GetModFeeAndSize(const CTxMemPoolEntry &a, double &mod_fee, double &size) const { + // Compare feerate with descendants to feerate of the transaction, and + // return the fee/size for the max. double f1 = (double)a.GetModifiedFee() * a.GetSizeWithDescendants(); double f2 = (double)a.GetModFeesWithDescendants() * a.GetTxSize(); - return f2 > f1; + + if (f2 > f1) { + mod_fee = a.GetModFeesWithDescendants(); + size = a.GetSizeWithDescendants(); + } else { + mod_fee = a.GetModifiedFee(); + size = a.GetTxSize(); + } } }; From 9a51319578091234fdd218a1eb144d517ea82b85 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Tue, 9 Jan 2018 12:27:57 -0500 Subject: [PATCH 2/4] Sort mempool by min(feerate, ancestor_feerate) This more closely approximates the desirability of a given transaction for mining. --- src/txmempool.h | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/src/txmempool.h b/src/txmempool.h index a0698387f..a5eba9787 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -266,27 +266,46 @@ public: } }; +/** \class CompareTxMemPoolEntryByAncestorScore + * + * Sort an entry by min(score/size of entry's tx, score/size with all ancestors). + */ class CompareTxMemPoolEntryByAncestorFee { public: bool operator()(const CTxMemPoolEntry& a, const CTxMemPoolEntry& b) const { - double aFees = a.GetModFeesWithAncestors(); - double aSize = a.GetSizeWithAncestors(); + double a_mod_fee, a_size, b_mod_fee, b_size; - double bFees = b.GetModFeesWithAncestors(); - double bSize = b.GetSizeWithAncestors(); + GetModFeeAndSize(a, a_mod_fee, a_size); + GetModFeeAndSize(b, b_mod_fee, b_size); // Avoid division by rewriting (a/b > c/d) as (a*d > c*b). - double f1 = aFees * bSize; - double f2 = aSize * bFees; + double f1 = a_mod_fee * b_size; + double f2 = a_size * b_mod_fee; if (f1 == f2) { return a.GetTx().GetHash() < b.GetTx().GetHash(); } - return f1 > f2; } + + // Return the fee/size we're using for sorting this entry. + void GetModFeeAndSize(const CTxMemPoolEntry &a, double &mod_fee, double &size) const + { + // Compare feerate with ancestors to feerate of the transaction, and + // return the fee/size for the min. + double f1 = (double)a.GetModifiedFee() * a.GetSizeWithAncestors(); + double f2 = (double)a.GetModFeesWithAncestors() * a.GetTxSize(); + + if (f1 > f2) { + mod_fee = a.GetModFeesWithAncestors(); + size = a.GetSizeWithAncestors(); + } else { + mod_fee = a.GetModifiedFee(); + size = a.GetTxSize(); + } + } }; // Multi_index tag names From 7abfa538b5a4508e0cf0589ae3ac0620b2188912 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Fri, 12 Jan 2018 12:40:55 -0500 Subject: [PATCH 3/4] Add test for new ancestor feerate sort behavior --- src/test/mempool_tests.cpp | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index f56f34149..e6234c5ab 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -427,6 +427,23 @@ BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest) sortedOrder.erase(sortedOrder.end()-2); sortedOrder.insert(sortedOrder.begin(), tx7.GetHash().ToString()); CheckSort(pool, sortedOrder); + + // High-fee parent, low-fee child + // tx7 -> tx8 + CMutableTransaction tx8 = CMutableTransaction(); + tx8.vin.resize(1); + tx8.vin[0].prevout = COutPoint(tx7.GetHash(), 0); + tx8.vin[0].scriptSig = CScript() << OP_11; + tx8.vout.resize(1); + tx8.vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL; + tx8.vout[0].nValue = 10*COIN; + + // Check that we sort by min(feerate, ancestor_feerate): + // set the fee so that the ancestor feerate is above tx1/5, + // but the transaction's own feerate is lower + pool.addUnchecked(tx8.GetHash(), entry.Fee(5000LL).FromTx(tx8)); + sortedOrder.insert(sortedOrder.end()-1, tx8.GetHash().ToString()); + CheckSort(pool, sortedOrder); } From 0a22a52918ad5af6d105b4f5ae9dd6c52199f0e8 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Sat, 13 Jan 2018 15:57:30 -0500 Subject: [PATCH 4/4] Use mempool's ancestor sort in transaction selection Transaction selection for mining tracks ancestor feerates that are modified based on transactions that have already been selected. This commit de-duplicates the code so that the ancestor feerate sorting used by the mempool can also be directly applied to the miner. --- src/miner.cpp | 2 +- src/miner.h | 23 +++++++---------------- src/txmempool.h | 6 ++++-- 3 files changed, 12 insertions(+), 19 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index 4e63ab4df..dda52790c 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -352,7 +352,7 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda // Try to compare the mapTx entry to the mapModifiedTx entry iter = mempool.mapTx.project<0>(mi); if (modit != mapModifiedTx.get().end() && - CompareModifiedEntry()(*modit, CTxMemPoolModifiedEntry(iter))) { + CompareTxMemPoolEntryByAncestorFee()(*modit, CTxMemPoolModifiedEntry(iter))) { // The best entry in mapModifiedTx has higher score // than the one from mapTx. // Switch which transaction (package) to consider diff --git a/src/miner.h b/src/miner.h index 698b4a478..9c086332d 100644 --- a/src/miner.h +++ b/src/miner.h @@ -41,6 +41,12 @@ struct CTxMemPoolModifiedEntry { nSigOpCostWithAncestors = entry->GetSigOpCostWithAncestors(); } + int64_t GetModifiedFee() const { return iter->GetModifiedFee(); } + uint64_t GetSizeWithAncestors() const { return nSizeWithAncestors; } + CAmount GetModFeesWithAncestors() const { return nModFeesWithAncestors; } + size_t GetTxSize() const { return iter->GetTxSize(); } + const CTransaction& GetTx() const { return iter->GetTx(); } + CTxMemPool::txiter iter; uint64_t nSizeWithAncestors; CAmount nModFeesWithAncestors; @@ -67,21 +73,6 @@ struct modifiedentry_iter { } }; -// This matches the calculation in CompareTxMemPoolEntryByAncestorFee, -// except operating on CTxMemPoolModifiedEntry. -// TODO: refactor to avoid duplication of this logic. -struct CompareModifiedEntry { - bool operator()(const CTxMemPoolModifiedEntry &a, const CTxMemPoolModifiedEntry &b) const - { - double f1 = (double)a.nModFeesWithAncestors * b.nSizeWithAncestors; - double f2 = (double)b.nModFeesWithAncestors * a.nSizeWithAncestors; - if (f1 == f2) { - return CTxMemPool::CompareIteratorByHash()(a.iter, b.iter); - } - return f1 > f2; - } -}; - // A comparator that sorts transactions based on number of ancestors. // This is sufficient to sort an ancestor package in an order that is valid // to appear in a block. @@ -106,7 +97,7 @@ typedef boost::multi_index_container< // Reuse same tag from CTxMemPool's similar index boost::multi_index::tag, boost::multi_index::identity, - CompareModifiedEntry + CompareTxMemPoolEntryByAncestorFee > > > indexed_modified_transaction_set; diff --git a/src/txmempool.h b/src/txmempool.h index a5eba9787..d32131d7a 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -273,7 +273,8 @@ public: class CompareTxMemPoolEntryByAncestorFee { public: - bool operator()(const CTxMemPoolEntry& a, const CTxMemPoolEntry& b) const + template + bool operator()(const T& a, const T& b) const { double a_mod_fee, a_size, b_mod_fee, b_size; @@ -291,7 +292,8 @@ public: } // Return the fee/size we're using for sorting this entry. - void GetModFeeAndSize(const CTxMemPoolEntry &a, double &mod_fee, double &size) const + template + void GetModFeeAndSize(const T &a, double &mod_fee, double &size) const { // Compare feerate with ancestors to feerate of the transaction, and // return the fee/size for the min.