(TL;DR your suggestion is best but i’m leaving the response to the first section for context)
Yeah it’s a best effort. I could make sure that the descendant was also part of the block. Is it worth it though? If an ancestor-less transaction with mempool child that pay a higher feerate than itself got mined, the child probably got included as well (not necessarily, but i assumed very probably).
Do you think that assumption isn’t reasonable? My doubt is more with more convoluted packages. For instance:
0A ------ D
1 __/ /
2B / /
3 _ /
4C /
With A 1sat/vb, B 10sat/vb, C 10sat/vb and D 1sat/vb. It would be a false positive for A. To prevent this we could simply condition the check on the descendants feerate of A:
0diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp
1index 029dd108c3..5d5dfb2efe 100644
2--- a/src/policy/fees.cpp
3+++ b/src/policy/fees.cpp
4@@ -609,9 +609,12 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM
5 // Note this check is only best-effort, as the child might not be part of the block. Or
6 // another child might be part of the block that we didn't have in our mempool.
7 CFeeRate feerate_entry{entry->GetFee(), (uint32_t)entry->GetTxSize()};
8- for (const CTxMemPoolEntry& child : entry->GetMemPoolChildren()) {
9- CFeeRate ancestor_score{child.GetModFeesWithAncestors(), (uint32_t)child.GetSizeWithAncestors()};
10- if (ancestor_score > feerate_entry) return false;
11+ CFeeRate feerate_desc{entry->GetModFeesWithDescendants(), (uint32_t)entry->GetSizeWithDescendants()};
12+ if (feerate_desc > feerate_entry) {
13+ for (const CTxMemPoolEntry& child : entry->GetMemPoolChildren()) {
14+ CFeeRate ancestor_score{child.GetModFeesWithAncestors(), (uint32_t)child.GetSizeWithAncestors()};
15+ if (ancestor_score > feerate_entry) return false;
16+ }
17 }
18
19 // How many blocks did it take for miners to include this transaction?
Regarding your suggestion. do you mean we should iterate over all the mempool entries that were part of the block to detect CPFPs beforehand? I agree it’s much better. It makes sure that the child was part of the same block. And your suggestion to only drop the ancestors whose feerates are lower than this transaction’s covers the case i detailed above. Nice! Is that roughly what you were thinking about:
0diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp
1index 029dd108c3..8c0e71ea18 100644
2--- a/src/policy/fees.cpp
3+++ b/src/policy/fees.cpp
4@@ -604,16 +604,6 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM
5 return false;
6 }
7
8- // Do not consider this transaction's feerate if it might have been mined due to one of
9- // its children (CPFP).
10- // Note this check is only best-effort, as the child might not be part of the block. Or
11- // another child might be part of the block that we didn't have in our mempool.
12- CFeeRate feerate_entry{entry->GetFee(), (uint32_t)entry->GetTxSize()};
13- for (const CTxMemPoolEntry& child : entry->GetMemPoolChildren()) {
14- CFeeRate ancestor_score{child.GetModFeesWithAncestors(), (uint32_t)child.GetSizeWithAncestors()};
15- if (ancestor_score > feerate_entry) return false;
16- }
17-
18 // How many blocks did it take for miners to include this transaction?
19 // blocksToConfirm is 1-based, so a transaction included in the earliest
20 // possible block has confirmation count of 1
21@@ -625,6 +615,7 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM
22 return false;
23 }
24
25+ CFeeRate feerate_entry{entry->GetFee(), (uint32_t)entry->GetTxSize()};
26 feeStats->Record(blocksToConfirm, (double)feerate_entry.GetFeePerK());
27 shortStats->Record(blocksToConfirm, (double)feerate_entry.GetFeePerK());
28 longStats->Record(blocksToConfirm, (double)feerate_entry.GetFeePerK());
29@@ -659,6 +650,18 @@ void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight,
30 shortStats->UpdateMovingAverages();
31 longStats->UpdateMovingAverages();
32
33+ for (const auto& entry : entries) {
34+ CFeeRate individual_feerate{entry->GetFee(), (uint32_t)entry->GetTxSize()};
35+ CFeeRate ancestor_score{entry->GetModFeesWithAncestors(), (uint32_t)entry->GetSizeWithAncestors()};
36+
37+ if (individual_feerate > ancestor_score) {
38+ for (const CTxMemPoolEntry& parent : entry->GetMemPoolParents()) {
39+ CFeeRate parent_feerate{parent.GetFee(), (uint32_t)parent.GetTxSize()};
40+ if (parent_feerate < individual_feerate) _removeTx(parent.GetTx().GetHash(), true);
41+ }
42+ }
43+ }
44+
45 unsigned int countedTxs = 0;
46 // Update averages with data points from current block
47 for (const auto& entry : entries) {
