[FreshEyes] Fee Estimation: Ignore all transactions with an in-block child

src/policy/fees.cpp

@@ -662,6 +662,22 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const Remo
     return true;
 }
 
+void CBlockPolicyEstimator::removeParentTxs(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block)
+{
+    std::set<Txid> seen_transactions;
+    for (const auto& tx : txs_removed_for_block) {
+        seen_transactions.insert(tx.info.m_tx->GetHash());
+        for (const auto& input : tx.info.m_tx->vin) {
+            const Txid& parentId = input.prevout.hash;
+            if (seen_transactions.count(parentId)) {
+                // Ignore any transaction with a child in the received block
+                // It may be fee bumped by the child.
+                _removeTx(parentId, /*inblock=*/true);
+                seen_transactions.erase(parentId);
+            }
+        }
+    }
+}
 void CBlockPolicyEstimator::processBlock(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block,
                                          unsigned int nBlockHeight)
 {
@@ -690,6 +706,8 @@ void CBlockPolicyEstimator::processBlock(const std::vector<RemovedMempoolTransac
     shortStats->UpdateMovingAverages();
     longStats->UpdateMovingAverages();
 
+    removeParentTxs(txs_removed_for_block);
+
     unsigned int countedTxs = 0;
     // Update averages with data points from current block
     for (const auto& tx : txs_removed_for_block) {

src/policy/fees.h

@@ -303,6 +303,9 @@ class CBlockPolicyEstimator : public CValidationInterface
     /** Process a transaction confirmed in a block*/
     bool processBlockTx(unsigned int nBlockHeight, const RemovedMempoolTransactionInfo& tx) EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator);
 
+    /* Remove transactions with child from fee estimation tracking stats */
+    void removeParentTxs(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block) EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator);
+
     /** Helper for estimateSmartFee */
     double estimateCombinedFee(unsigned int confTarget, double successThreshold, bool checkShorterHorizon, EstimationResult *result) const EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator);
     /** Helper for estimateSmartFee */

test/functional/feature_fee_estimation.py

@@ -129,6 +129,15 @@ def make_tx(wallet, utxo, feerate):
     )
 
 
+def send_tx(wallet, node, utxo, feerate):
+    """Broadcast a 1in-1out transaction with a specific input and feerate (sat/vb)."""
+    return wallet.send_self_transfer(
+        from_node=node,
+        utxo_to_spend=utxo,
+        fee_rate=Decimal(feerate * 1000) / COIN,
+    )
+
+
 class EstimateFeeTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 3
@@ -265,6 +274,11 @@ def sanity_check_rbf_estimates(self, utxos):
             # Broadcast 5 low fee transaction which don't need to
             for _ in range(5):
                 tx = make_tx(self.wallet, utxos.pop(0), low_feerate)
+                self.confutxo.append({
+                    "txid": tx["txid"],
+                    "vout": 0,
+                    "value": Decimal(tx["tx"].vout[0].nValue) / COIN
+                })
                 txs.append(tx)
             batch_send_tx = [node.sendrawtransaction.get_request(tx["hex"]) for tx in txs]
             for n in self.nodes:
@@ -278,12 +292,16 @@ def sanity_check_rbf_estimates(self, utxos):
             while len(utxos_to_respend) > 0:
                 u = utxos_to_respend.pop(0)
                 tx = make_tx(self.wallet, u, high_feerate)
+                self.confutxo.append({
+                    "txid": tx["txid"],
+                    "vout": 0,
+                    "value": Decimal(tx["tx"].vout[0].nValue) / COIN
+                })
                 node.sendrawtransaction(tx["hex"])
                 txs.append(tx)
             dec_txs = [res["result"] for res in node.batch([node.decoderawtransaction.get_request(tx["hex"]) for tx in txs])]
             self.wallet.scan_txs(dec_txs)
 
-
         # Mine the last replacement txs
         self.sync_mempools(wait=0.1, nodes=[node, miner])
         self.generate(miner, 1)
@@ -383,6 +401,79 @@ def test_acceptstalefeeestimates_option(self):
         assert_equal(self.nodes[0].estimatesmartfee(1)["feerate"], fee_rate)
 
 
+    def send_and_mine_child_tx(self, broadcaster, miner, parent_tx, feerate):
+        u = {"txid": parent_tx["txid"], "vout": 0, "value": Decimal(parent_tx["tx"].vout[0].nValue) / COIN}
+        send_tx(wallet=self.wallet, node=broadcaster, utxo=u, feerate=feerate)
+        self.sync_mempools(wait=0.1, nodes=[broadcaster, miner])
+        self.generate(miner, 1)
+        assert_equal(broadcaster.estimaterawfee(1)["short"]["fail"]["totalconfirmed"], 0)
+
+    def sanity_check_cpfp_estimates(self, utxos):
+        """The BlockPolicyEstimator currently does not take CPFP into account. This test
+        sanity checks its behaviour when receiving transactions that were confirmed because
+        of their child's feerate.
+        """
+        # The broadcaster and block producer
+        broadcaster = self.nodes[0]
+        miner = self.nodes[1]
+        # In sat/vb
+        [low_feerate, med_feerate, high_feerate] = [Decimal(2), Decimal(15), Decimal(20)]
+
+        self.log.info("Test that fee estimator will ignore all transaction with in block child")
+        # If a transaction got mined and has a child in the same block it was mined
+        # it does not get accounted in the fee estimator.
+        low_fee_parent = send_tx(wallet=self.wallet, node=broadcaster, utxo=None, feerate=low_feerate)
+        self.send_and_mine_child_tx(broadcaster=broadcaster, miner=miner, parent_tx=low_fee_parent, feerate=high_feerate)
+
+        # If it has descendants which have a lower ancestor score, it also does not.
+        high_fee_parent = send_tx(wallet=self.wallet, node=broadcaster, utxo=None, feerate=high_feerate)
+        self.send_and_mine_child_tx(broadcaster=broadcaster, miner=miner, parent_tx=high_fee_parent, feerate=low_feerate)
+
+        # Even if it's equal fee rate.
+        med_fee_parent = send_tx(wallet=self.wallet, node=broadcaster, utxo=None, feerate=med_feerate)
+        self.send_and_mine_child_tx(broadcaster=broadcaster, miner=miner, parent_tx=med_fee_parent, feerate=med_feerate)
+
+        # Generate and mine packages of transactions, 80% of them are a [low fee, high fee] package
+        # which get mined because of the child transaction. 20% are single-transaction packages with
+        # a medium-high feerate.
+        # Test that we don't give the low feerate as estimate, assuming the low fee transactions
+        # got mined on their own.
+        for _ in range(5):
+            txs = []  # Batch the RPCs calls.
+            for _ in range(20):
+                u = utxos.pop(0)
+                parent_tx = make_tx(wallet=self.wallet, utxo=u, feerate=low_feerate)
+                txs.append(parent_tx)
+                u = {
+                    "txid": parent_tx["txid"],
+                    "vout": 0,
+                    "value": Decimal(parent_tx["tx"].vout[0].nValue) / COIN
+                }
+                child_tx = make_tx(wallet=self.wallet, utxo=u, feerate=high_feerate)
+                txs.append(child_tx)
+            for _ in range(5):
+                u = utxos.pop(0)
+                tx = make_tx(wallet=self.wallet, utxo=u, feerate=med_feerate)
+                txs.append(tx)
+            batch_send_tx = (broadcaster.sendrawtransaction.get_request(tx["hex"]) for tx in txs)
+            broadcaster.batch(batch_send_tx)
+            self.sync_mempools(wait=0.1, nodes=[broadcaster, miner])
+            self.generate(miner, 1)
+        assert_equal(broadcaster.estimatesmartfee(2)["feerate"], med_feerate * 1000 / COIN)
+
+    def clear_first_node_estimates(self):
+        """Restart node 0 without a fee_estimates.dat."""
+        self.log.info("Restarting node with fresh estimation")
+        self.stop_node(0)
+        fee_dat = os.path.join(self.nodes[0].chain_path, "fee_estimates.dat")
+        os.remove(fee_dat)
+        self.start_node(0)
+        self.connect_nodes(0, 1)
+        self.connect_nodes(0, 2)
+        # Note: we need to get into the estimator's processBlock to set nBestSeenHeight or it
+        # will ignore all the txs of the first block we mine in the next test.
+        self.generate(self.nodes[0], 1)
+
     def run_test(self):
         self.log.info("This test is time consuming, please be patient")
         self.log.info("Splitting inputs so we can generate tx's")
@@ -420,16 +511,15 @@ def run_test(self):
         self.log.info("Test reading old fee_estimates.dat")
         self.test_old_fee_estimate_file()
 
-        self.log.info("Restarting node with fresh estimation")
-        self.stop_node(0)
-        fee_dat = os.path.join(self.nodes[0].chain_path, "fee_estimates.dat")
-        os.remove(fee_dat)
-        self.start_node(0)
-        self.connect_nodes(0, 1)
-        self.connect_nodes(0, 2)
+        self.clear_first_node_estimates()
 
         self.log.info("Testing estimates with RBF.")
-        self.sanity_check_rbf_estimates(self.confutxo + self.memutxo)
+        self.sanity_check_rbf_estimates(self.confutxo)
+
+        self.clear_first_node_estimates()
+
+        self.log.info("Testing estimates with CPFP.")
+        self.sanity_check_cpfp_estimates(self.confutxo)
 
         self.log.info("Testing that fee estimation is disabled in blocksonly.")
         self.restart_node(0, ["-blocksonly"])