From a6e81336897bfa472b79c82899584b2763af99e4 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Fri, 10 May 2024 07:51:56 +0100 Subject: [PATCH 1/3] [fees]: ignore all transaction with in block child --- src/policy/fees.cpp | 18 ++++++++++++++++++ src/policy/fees.h | 3 +++ 2 files changed, 21 insertions(+) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 5f1d15c5f27a5..73bac53f4c451 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -662,6 +662,22 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const Remo return true; } +void CBlockPolicyEstimator::removeParentTxs(const std::vector& txs_removed_for_block) +{ + std::set 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& txs_removed_for_block, unsigned int nBlockHeight) { @@ -690,6 +706,8 @@ void CBlockPolicyEstimator::processBlock(const std::vectorUpdateMovingAverages(); 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) { diff --git a/src/policy/fees.h b/src/policy/fees.h index f34f66d3f0d6b..b35bed71ca3e1 100644 --- a/src/policy/fees.h +++ b/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& 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 */ From 3e50d1342912467833d1c0bcbff2424b693f46b8 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Wed, 15 Jun 2022 16:03:24 +0200 Subject: [PATCH 2/3] [test]: keep the list of confirmed utxos up to date in RBF test --- test/functional/feature_fee_estimation.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/functional/feature_fee_estimation.py b/test/functional/feature_fee_estimation.py index ffc87f8b8bad0..976c11419672c 100755 --- a/test/functional/feature_fee_estimation.py +++ b/test/functional/feature_fee_estimation.py @@ -265,6 +265,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 +283,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) From 2563305c0aef3975a6321911db7e0f2a245486de Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Fri, 10 May 2024 08:21:16 +0100 Subject: [PATCH 3/3] [test]: test we ignore parent transactions in a CPFP Co-authored-by: Antoine Poinsot --- test/functional/feature_fee_estimation.py | 97 +++++++++++++++++++++-- 1 file changed, 89 insertions(+), 8 deletions(-) diff --git a/test/functional/feature_fee_estimation.py b/test/functional/feature_fee_estimation.py index 976c11419672c..8a32ff8594b24 100755 --- a/test/functional/feature_fee_estimation.py +++ b/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 @@ -392,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") @@ -429,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"])