Fee Estimation: Ignore all transactions that are CPFP'd

ismaelsadeeq commented at 9:04 am on May 10, 2024: member

Another attempt at #25380 with an alternate approach

This PR updates CBlockPolicyEstimator to ignore all transactions that are CPFP’d by some child when a new block is received. This fixes the assumption that all transactions are confirmed solely because of their fee rate. As some transactions are confirmed due to a CPFP by some child.

This approach linearize all transactions removed from the mempool because of the new block, and only ignore transactions whose mining score is not the same with their the fee rate.

All transaction with in-mempool parent are already not tracked for fee estimation, so the child that CPFP’d the parent is also ignored.

Upon implementing the cluster mempool, we will just track chunks and make the fee estimator package aware.

DrahtBot commented at 9:04 am on May 10, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30079.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	instagibbs, josibake
Stale ACK	rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

glozow commented at 1:54 pm on May 10, 2024: member

cc @josibake @instagibbs @naumenkogs @darosior @murchandamus since you reviewed the last time

instagibbs commented at 1:58 pm on May 10, 2024: member

concept ACK

strikes me as the right idea, keep it simple for now, focusing on correctness. Do we have charts anywhere tracking % of transactions that are in a cluster size of 1?

ismaelsadeeq commented at 2:19 pm on May 10, 2024: member

Do we have charts anywhere tracking % of transactions that are in a cluster size of 1?

I will analyze the percentage of cluster size 1 transactions mined in previous blocks.

josibake commented at 5:41 pm on May 10, 2024: member

Concept ACK

in test/functional/feature_fee_estimation.py:526 in c12a677cc2 outdated

528 
529         self.log.info("Testing estimates with RBF.")
530-        self.sanity_check_rbf_estimates(self.confutxo + self.memutxo)
531+        self.sanity_check_rbf_estimates(self.confutxo)
532+
533+        self.log.info("Restarting node with fresh estimation")

rkrux commented at 7:49 am on May 14, 2024:

This log and the same one on line 520 can go inside clear_first_node_estimates().

ismaelsadeeq commented at 5:05 pm on May 14, 2024:

Fixed, thanks

in test/functional/feature_fee_estimation.py:415 in c12a677cc2 outdated

410+        broadcaster = self.nodes[0]
411+        miner = self.nodes[1]
412+        # In sat/vb
413+        low_feerate = Decimal(2)
414+        med_feerate = Decimal(15)
415+        high_feerate = Decimal(20)

rkrux commented at 7:52 am on May 14, 2024:

Nit:

0[low_feerate, med_feerate, high_feerate] = [Decimal(2), Decimal(15), Decimal(20)]

ismaelsadeeq commented at 5:06 pm on May 14, 2024:

fixed

in test/functional/feature_fee_estimation.py:441 in c12a677cc2 outdated

436+        tx = send_tx(wallet=self.wallet, node=broadcaster, utxo=None, feerate=high_feerate)
437+        u = {"txid": tx["txid"], "vout": 0, "value": Decimal(tx["tx"].vout[0].nValue) / COIN}
438+        send_tx(wallet=self.wallet, node=broadcaster, utxo=u, feerate=high_feerate)
439+        self.sync_mempools(wait=0.1, nodes=[broadcaster, miner])
440+        self.generate(miner, 1)
441+        assert_equal(broadcaster.estimaterawfee(1)["short"]["fail"]["totalconfirmed"], 0)

rkrux commented at 8:12 am on May 14, 2024:

This is an easy candidate to remove code duplication by extracting this out in an internal function, and by accepting tx and feeRate as function args.

ismaelsadeeq commented at 5:06 pm on May 14, 2024:

Yes, this is fixed

in test/functional/feature_fee_estimation.py:465 in c12a677cc2 outdated

462+                u = utxos.pop(0)
463+                tx = make_tx(wallet=self.wallet, utxo=u, feerate=med_feerate)
464+                txs.append(tx)
465+            batch_send_tx = (broadcaster.sendrawtransaction.get_request(tx["hex"]) for tx in txs)
466+            broadcaster.batch(batch_send_tx)
467+            self.sync_mempools(wait=0.1, nodes=[broadcaster, miner])

rkrux commented at 8:13 am on May 14, 2024:

I’ve seen this function call in other functional tests as well, can you share why this call is required?

rkrux commented at 1:54 pm on May 14, 2024:

Oh nevermind, reviewing this PR gave me the answer: https://github.com/bitcoin/bitcoin/pull/29982

in test/functional/feature_fee_estimation.py:132 in c12a677cc2 outdated

128@@ -129,6 +129,15 @@ def make_tx(wallet, utxo, feerate):
129     )
130 
131 
132+def send_tx(wallet, node, utxo, feerate):

rkrux commented at 8:19 am on May 14, 2024:

Nit: The meaning would be more explicit if we called the last argument as feerate_satvb.

ismaelsadeeq commented at 5:07 pm on May 14, 2024:

Their is a description below that states that the feerate is in sat/vb so I will leave this as is.

in test/functional/feature_fee_estimation.py:137 in c12a677cc2 outdated

128@@ -129,6 +129,15 @@ def make_tx(wallet, utxo, feerate):
129     )
130 
131 
132+def send_tx(wallet, node, utxo, feerate):
133+    """Broadcast a 1in-1out transaction with a specific input and feerate (sat/vb)."""
134+    return wallet.send_self_transfer(
135+        from_node=node,
136+        utxo_to_spend=utxo,
137+        fee_rate=Decimal(feerate * 1000) / COIN,

rkrux commented at 8:21 am on May 14, 2024:

I dont fully understand this conversion, is the intention to convert it to feerate_satkvb?

ismaelsadeeq commented at 4:13 pm on May 14, 2024:

This is converting to feerate in sat/vb to (BTC/kvB)

in src/policy/fees.cpp:705 in c12a677cc2 outdated

700+                // It may be fee bumped by the child.
701+                _removeTx(parentId, /*inblock=*/true);
702+                seen_transactions.erase(parentId);
703+            }
704+        }
705+    }

rkrux commented at 8:25 am on May 14, 2024:

Nit: This code block can be extracted in a separate function because after reading the code above that’s mostly delegation consecutive function calls, it’d be nice to have something similar here as well - removeParentTxs. One more point being that seen_transactions variable is internal to this logic and doesn’t need to be exposed to processBlock.

ismaelsadeeq commented at 5:07 pm on May 14, 2024:

Fixed

in src/policy/fees.cpp:698 in c12a677cc2 outdated

689@@ -690,6 +690,20 @@ void CBlockPolicyEstimator::processBlock(const std::vector<RemovedMempoolTransac
690     shortStats->UpdateMovingAverages();
691     longStats->UpdateMovingAverages();
692 
693+    std::set<Txid> seen_transactions;
694+    for (const auto& tx : txs_removed_for_block) {
695+        seen_transactions.insert(tx.info.m_tx->GetHash());
696+        for (const auto& input : tx.info.m_tx->vin) {
697+            const Txid& parentId = input.prevout.hash;
698+            if (seen_transactions.count(parentId)) {

rkrux commented at 8:26 am on May 14, 2024:

I’m assuming the transactions are sorted from parents to children. So in the case A -> B -> C -> D, all except D would be removed, right?

ismaelsadeeq commented at 8:58 am on May 14, 2024:

I’m assuming the transactions are sorted from parents to children.

Yes, they are.

The fee estimator does not take all transaction with parents in the mempool into account already see https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ece11dba3/src/policy/fees.cpp#L615

Assuming A->B->C->D are topologically sorted from parent to last descendant.

In this case B, C, D are already not considered for fee estimation, After this PR A will also be ignored.

rkrux commented at 2:01 pm on May 14, 2024:

Hmm I see, as per this comment it says “not dependent on any other transactions in the mempool” - does this also apply to other transactions in the block?

https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ece11dba3/src/policy/fees.cpp#L613

ismaelsadeeq commented at 5:28 pm on May 14, 2024:

does this also apply to other transactions in the block?

I don’t fully understand your question. The comment you highlighted refers to all incoming transactions into the mempool.

After a new block arrives, the fee estimator listens for notifications of all transactions removed from mempool because of the new block, and updates their tracking stats. All transactions with mempool dependencies that are mined are not tracked initially, they are just going to be skipped.

in test/functional/feature_fee_estimation.py:307 in 37b7ce3c1a outdated

282@@ -278,12 +283,16 @@ def sanity_check_rbf_estimates(self, utxos):
283             while len(utxos_to_respend) > 0:
284                 u = utxos_to_respend.pop(0)
285                 tx = make_tx(self.wallet, u, high_feerate)
286+                self.confutxo.append({
287+                    "txid": tx["txid"],
288+                    "vout": 0,
289+                    "value": Decimal(tx["tx"].vout[0].nValue) / COIN
290+                })

rkrux commented at 8:29 am on May 14, 2024:

Wondering why these calls are not required after every make_tx call?

ismaelsadeeq commented at 5:22 pm on May 14, 2024:

Because make_tx is only constructing the transaction, the transaction created might not be confirmed, so Its inaccurate to add it to confirmed utxo’s after constructing the transaction.

Transactions added to the confirmed utxo’s in the diff (sanity_check_rbf_estimates) are all going to be confirmed eventually so we are accounting for them.

in test/functional/feature_fee_estimation.py:425 in c12a677cc2 outdated

406+        sanity checks its behaviour when receiving transactions that were confirmed because
407+        of their child's feerate.
408+        """
409+        # The broadcaster and block producer
410+        broadcaster = self.nodes[0]
411+        miner = self.nodes[1]

rkrux commented at 8:37 am on May 14, 2024:

+1 for expressive node names!

rkrux approved

rkrux commented at 8:38 am on May 14, 2024: none

tACK c12a677

Make successful, all functional tests successful. Mentioned few nits and asked few questions for my clarity.

The downside of this approach is that transactions that are not CPFP’d will aslo be ignored.

Related to this^, one specific case I can think of that will be affected is of layered transactions that are meant to be confirmed together, example: fanout transactions to increase the utxo count in the wallet and make the utxo value distribution more even, but Idk how frequently these transactions happen.

DrahtBot requested review from instagibbs on May 14, 2024

DrahtBot requested review from josibake on May 14, 2024

ismaelsadeeq force-pushed on May 14, 2024

ismaelsadeeq commented at 4:43 pm on May 15, 2024: member

Do we have charts anywhere tracking % of transactions that are in a cluster size of 1?

I will analyze the percentage of cluster size 1 transactions mined in previous blocks.

I tracked recent 1000 blocks from block 842457 to 843457

~61% of transactions in the last 1000 blocks were confirmed in a cluster size > 1. ~38% of transactions in the last 1000 blocks were confirmed in a cluster size 1

Transactions: 3974143 Cluster size 1 transactions: 1516505 Approximate percentage of Cluster size 1 transactions: ~38 Transactions in a cluster size > 1 transactions: 2457638 Approximate percentage of transactions in a cluster size > 1: ~61

The data is available here here https://docs.google.com/spreadsheets/d/1QTE2EQeQlamA123pIn0SY4F-9jtnzS7e7CV1etT6ytM/edit?usp=sharing

I used this script to collect the data https://gist.github.com/ismaelsadeeq/e2767040b720f65986976b3f8c6b7b20

ismaelsadeeq commented at 4:53 pm on May 15, 2024: member

Thanks for review @rkrux

I addressed all your comments
Force pushed from c12a677cc250608171bc4f6311095b60ba24abab to 2563305c0aef3975a6321911db7e0f2a245486de compare diff

in test/functional/feature_fee_estimation.py:413 in 2563305c0a outdated

400@@ -383,6 +401,79 @@ def test_acceptstalefeeestimates_option(self):
401         assert_equal(self.nodes[0].estimatesmartfee(1)["feerate"], fee_rate)
402 
403 
404+    def send_and_mine_child_tx(self, broadcaster, miner, parent_tx, feerate):
405+        u = {"txid": parent_tx["txid"], "vout": 0, "value": Decimal(parent_tx["tx"].vout[0].nValue) / COIN}

rkrux commented at 9:20 am on May 16, 2024:

Decimal(parent_tx["tx"].vout[0].nValue) / COIN

Is there not a satToBtc() already? WDYT about introducing one? I’ve noticed this conversion in this diff thrice.

ismaelsadeeq commented at 9:33 am on May 16, 2024:

Happy to modify if there is need to retouch, else this is a good idea for a follow-up @rkrux

josibake commented at 10:00 am on May 16, 2024:

yeah, would recommend this be a separate PR, since there are likely multiple instances in other tests where we could replace the satToBtc conversion with a utility function in the test framework.

rkrux commented at 10:57 am on May 16, 2024:

Yeah, I can pick this up in a separate PR.

rkrux commented at 12:03 pm on November 22, 2024:

Quite a late reply here but I don’t think I will get to this anytime soon, created an issue here for anyone else to pick up: https://github.com/bitcoin/bitcoin/issues/31345

rkrux approved

rkrux commented at 9:21 am on May 16, 2024: none

reACK 2563305

Thanks for the quick turnaround on this @ismaelsadeeq!

in src/policy/fees.cpp:665 in a6e8133689 outdated

661@@ -662,6 +662,22 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const Remo
662     return true;
663 }
664 
665+void CBlockPolicyEstimator::removeParentTxs(const std::vector<RemovedMempoolTransactionInfo>& txs_removed_for_block)

josibake commented at 9:56 am on May 16, 2024:

in https://github.com/bitcoin/bitcoin/pull/30079/commits/a6e81336897bfa472b79c82899584b2763af99e4 (“ignore all transaction with in block child”):

Shouldn’t we also remove the child? Otherwise we will overestimate fees, unless I’m missing something? Example:

Parent pays 1 sat/vb
10 sat/vb per tx is required to be competitive for the next block
Child pays 20 sat/vb to try and get them + parent to 10 sat/vb
Now in fee estimation, we see 20 sat/vb for a child that could have only paid 10 sat/vb

glozow commented at 10:32 am on May 16, 2024:

although unclear to me why we don’t also remove the child transaction from fee estimation to avoid overestimating

A child is already ignored, new mempool txns with unconfirmed parents are never added

https://github.com/bitcoin/bitcoin/blob/dd42a5ddea6a72e1e9cad54f8352c76b0b701973/src/validation.cpp#L1274-L1279 https://github.com/bitcoin/bitcoin/blob/dd42a5ddea6a72e1e9cad54f8352c76b0b701973/src/policy/fees.cpp#L610-L615

ismaelsadeeq commented at 10:39 am on May 16, 2024:

Thanks for your review, Josie. I think you are missing something.

When the child arrives, it will have a mempool parent, and we currently do not consider all transactions with mempool parent for fee estimation.

So when processing a transaction in processBlockTx all transactions with mempool parent, will be missing and , and we will skip them.

Child transactions whose parents aren’t in our mempool will also be missing, and we will skip them because orphan transactions aren’t added to the mempool and hence won’t be tracked. They are skipped here: https://github.com/bitcoin/bitcoin/blob/dd42a5ddea6a72e1e9cad54f8352c76b0b701973/src/policy/fees.cpp#L640

See the discussion with @rkrux: #30079 (review)

I recently made a delving on how fix this limitation and track chunks post cluster mempool. Package-aware fee estimator post

josibake commented at 8:28 am on May 17, 2024:

ah, my bad. I had even read that comment that @glozow linked to earlier but it didn’t click for me that by the time we get to the block we’ve already processed transactions and excluded the child transaction. I think a comment explaining what you just explained here would help a lot, maybe at the call site of removeParentTxs?

ismaelsadeeq commented at 1:00 pm on June 3, 2024:

Added the comment, thanks.

in src/policy/fees.h:306 in a6e8133689 outdated

302@@ -303,6 +303,9 @@ class CBlockPolicyEstimator : public CValidationInterface
303     /** Process a transaction confirmed in a block*/
304     bool processBlockTx(unsigned int nBlockHeight, const RemovedMempoolTransactionInfo& tx) EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator);
305 
306+    /* Remove transactions with child from fee estimation tracking stats */

josibake commented at 9:58 am on May 16, 2024:

in https://github.com/bitcoin/bitcoin/pull/30079/commits/a6e81336897bfa472b79c82899584b2763af99e4 (“ignore all transaction with in block child”):

nit: /** comment */ to match the styling of the surrounding comments. Might also be worth mention this is only for in-block children (CPFP).

ismaelsadeeq commented at 1:00 pm on June 3, 2024:

Fixed

josibake commented at 10:02 am on May 16, 2024: member

Looks good, although unclear to me why we don’t also remove the child transaction from fee estimation to avoid overestimating. Would recommend also removing the children, or at least documenting why it doesn’t matter in a comment.

DrahtBot requested review from josibake on May 16, 2024

laanwj added the label Mempool on May 16, 2024

bitcoin deleted a comment on Jun 3, 2024

ismaelsadeeq force-pushed on Jun 3, 2024

ismaelsadeeq renamed this:
~~Fee Estimation: Ignore all transactions with an in-block child~~
Fee Estimation: Ignore all transactions that are CPFP'd
on Jun 3, 2024

ismaelsadeeq commented at 1:04 pm on June 3, 2024: member

Do we have charts anywhere tracking % of transactions that are in a cluster size of 1?

I will analyze the percentage of cluster size 1 transactions mined in previous blocks.

I tracked recent 1000 blocks from block 842457 to 843457

~61% of transactions in the last 1000 blocks were confirmed in a cluster size > 1. ~38% of transactions in the last 1000 blocks were confirmed in a cluster size 1

Transactions: 3974143 Cluster size 1 transactions: 1516505 Approximate percentage of Cluster size 1 transactions: ~38 Transactions in a cluster size > 1 transactions: 2457638 Approximate percentage of transactions in a cluster size > 1: ~61

The data is available here here https://docs.google.com/spreadsheets/d/1QTE2EQeQlamA123pIn0SY4F-9jtnzS7e7CV1etT6ytM/edit?usp=sharing

I used this script to collect the data https://gist.github.com/ismaelsadeeq/e2767040b720f65986976b3f8c6b7b20

This data shows that most transactions are in cluster size > 1.

So I switched to ignoring transaction that are CPFP’d only.

Thanks for your reviews @rkrux @josibake

Forced pushed from 2563305c0aef3975a6321911db7e0f2a245486de to 055d9e6752dc87b25cdd5ea0138bfca7e4322dcb Compare diff
Addressed all comments.

ismaelsadeeq force-pushed on Jun 3, 2024

DrahtBot added the label CI failed on Jun 3, 2024

DrahtBot commented at 2:57 pm on June 3, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/25734218532

ismaelsadeeq force-pushed on Jun 3, 2024

josibake commented at 9:51 am on June 10, 2024: member

I didn’t look too closely into the CI failure reason, but seems like a relatively simple fix, i.e. fixing an signed int overflow:

 0SUMMARY: UndefinedBehaviorSanitizer: signed-integer-overflow node/mini_miner.cpp:196:33 
 1MS: 0 ; base unit: 0000000000000000000000000000000000000000
 2artifact_prefix='./'; Test unit written to ./crash-2315c89458602360eb93362328da5c0ef21f9864
 3
 4Traceback (most recent call last):
 5  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 411, in <module>
 6    main()
 7  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 199, in main
 8    run_once(
 9  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 376, in run_once
10    assert len(done_stat) == 1
11           ^^^^^^^^^^^^^^^^^^^
12AssertionError

ismaelsadeeq marked this as a draft on Jun 10, 2024

ismaelsadeeq commented at 10:13 am on June 10, 2024: member

Marking as draft, as I attempt to fix the fuzzing overflow error.

ismaelsadeeq force-pushed on Jun 13, 2024

ismaelsadeeq commented at 7:45 pm on June 13, 2024: member

I didn’t look too closely into the CI failure reason, but seems like a relatively simple fix, i.e. fixing an signed int overflow: @josibake overflow is from mini miner AncestorFeerateComparator, added a fixup commit here and opened an issue for it. #30284

DrahtBot removed the label CI failed on Jun 13, 2024

ismaelsadeeq marked this as ready for review on Jun 13, 2024

ismaelsadeeq force-pushed on Jun 25, 2024

DrahtBot added the label Needs rebase on Jul 2, 2024

ismaelsadeeq force-pushed on Jul 3, 2024

DrahtBot removed the label Needs rebase on Jul 3, 2024

in src/test/feesutil_tests.cpp:102 in 5e6ff0f80e outdated

 97+        // Create cluster C child ---> J
 98+        outpoints = {COutPoint(transactions[2].info.m_tx->GetHash(), 0)};
 99+        const CTransactionRef txJ = make_tx(outpoints);
100+        transactions.emplace_back(entry.FromTx(txJ));
101+
102+        // Create cluster B child ---> K

rkrux commented at 9:04 am on July 8, 2024:

0// Create cluster D child ---> K

ismaelsadeeq commented at 5:34 pm on July 8, 2024:

fixed

in src/test/feesutil_tests.cpp:136 in 5e6ff0f80e outdated

118+            BOOST_CHECK(descendants.find(txA_Id) != descendants.end());
119+            for (auto i = 4; i <= 6; i++) {
120+                auto curr_tx = transactions[i];
121+                BOOST_CHECK(descendants.find(curr_tx.info.m_tx->GetHash()) != descendants.end());
122+            }
123+        }

rkrux commented at 9:20 am on July 8, 2024:

There are 9 such assertion blocks with almost similar validations on ancestors and descendants. If the author feels it’s worth it, IMO this duplication can be eliminated by accepting few variables as the input in a function and calling it with different arguments.

0validateTxRelations(txToValidate, ancestorTxs, descendantTxs)

ismaelsadeeq commented at 5:35 pm on July 8, 2024:

thanks accepted your suggestion with a slight modification.

rkrux commented at 8:23 am on July 12, 2024:

Great, thank you!

in src/policy/fees_util.cpp:11 in 5e6ff0f80e outdated

0@@ -0,0 +1,31 @@
1+// Copyright (c) 2024 The Bitcoin Core developers
2+// Distributed under the MIT software license. See the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
6+#include <policy/fees_util.h>
7+#include <primitives/transaction.h>
8+
9+TxAncestorsAndDescendants GetTxAncestorsAndDescendants(const std::vector<RemovedMempoolTransactionInfo>& transactions)

rkrux commented at 9:21 am on July 8, 2024:

+1 on separating this function out in a file and adding focussed unit tests for it, I feel this is generic enough to be used in several places later.

in src/policy/fees_util.cpp:15 in 5e6ff0f80e outdated

10+{
11+    TxAncestorsAndDescendants visited_txs;
12+    for (const auto& tx_info : transactions) {
13+        const auto& txid = tx_info.info.m_tx->GetHash();
14+        std::set<Txid> tx_set{txid};
15+        visited_txs.emplace(txid, std::make_tuple(tx_set, tx_set));

rkrux commented at 9:29 am on July 8, 2024:

Interesting that tx_set is passed by value here and 2 separate copies are made at the time of calling, which is the need here for separate ancestors and descendants copies.

ismaelsadeeq commented at 5:36 pm on July 8, 2024:

yes, this is gone now.

rkrux approved

rkrux commented at 9:35 am on July 8, 2024: none

tACK cb95f95

make, test/functional are successful.

Although, this round of review was focussed on 5e6ff0f

ismaelsadeeq force-pushed on Jul 8, 2024

in src/node/mini_miner.cpp:192 in b1b28a21e1 outdated

188@@ -189,11 +189,7 @@ struct AncestorFeerateComparator
189             const int64_t ancestor_size{e.GetSizeWithAncestors()};
190             const CAmount tx_fee{e.GetModifiedFee()};
191             const int64_t tx_size{e.GetTxSize()};
192-            // Comparing ancestor feerate with individual feerate:
193-            //     ancestor_fee / ancestor_size <= tx_fee / tx_size
194-            // Avoid division and possible loss of precision by
195-            // multiplying both sides by the sizes:
196-            return ancestor_fee * tx_size < tx_fee * ancestor_size ?
197+            return CFeeRate(ancestor_fee, ancestor_size) <= CFeeRate(tx_fee, tx_size) ?

glozow commented at 3:24 pm on July 9, 2024:

instead of b1b28a21e185dfbc0c75f92ac55401a9ba8a5b08, I’d recommend rebasing on #30412 I was able to reproduce the ub crash and verify that 6254c253e995e77196b853784cfd0f42619b324d fixes it.

ismaelsadeeq commented at 3:30 pm on July 9, 2024:

thanks I will rebase on it.

ismaelsadeeq commented at 3:44 pm on July 9, 2024:

done

ismaelsadeeq force-pushed on Jul 9, 2024

ismaelsadeeq force-pushed on Jul 15, 2024

DrahtBot added the label Needs rebase on Jul 25, 2024

ismaelsadeeq force-pushed on Jul 25, 2024

DrahtBot removed the label Needs rebase on Jul 25, 2024

hebasto added the label Needs CMake port on Aug 16, 2024

DrahtBot added the label CI failed on Aug 31, 2024

maflcko removed the label Needs CMake port on Sep 2, 2024

DrahtBot added the label Needs rebase on Sep 2, 2024

[mini miner]: Linearize should also return packages fees and sizes

Co-authored-by: willcl-ark <will@256k1.dev>

a305a64567

[fees]: add function that computes ancestors and descendants of txs

- GetTxAncestorsAndDescendants takes the vector of transactions removed from the mempool
  after a block is connected. The function assumed that the order the transactions were
  included in the block was maintained; that is, all transaction parents was be added
  into the vector first before descendants.

- GetTxAncestorsAndDescendants computes all the ancestors and descendants of the transactions.
  The transaction is also included as a descendant and ancestor of itself.

Co-authored-by: willcl-ark <will@256k1.dev>

2e95d126c2

[fees]: add `GetMiniMinerInput` to `fees_util`

- This method takes removed mempool transaction and generates
  an output that can be linearized by mini miner.

Co-authored-by: willcl-ark <will@256k1.dev>

4f3d399f87

[fees]: ignore all transactions that are CPFP'd fcf82d32b5

[test]: keep the list of confirmed utxos up to date in RBF test a9c413994b

[test]: test we ignore parent transactions in a CPFP

Co-authored-by: Antoine Poinsot <darosior@protonmail.com>

4c84f37ad9

ismaelsadeeq force-pushed on Sep 3, 2024

ismaelsadeeq commented at 10:04 am on September 3, 2024: member

Rebased and added newly introduced files to cmakelist file instead of makefile

DrahtBot removed the label Needs rebase on Sep 3, 2024

DrahtBot removed the label CI failed on Sep 3, 2024

DrahtBot added the label CI failed on Oct 19, 2024

DrahtBot removed the label CI failed on Oct 23, 2024

in src/test/miniminer_tests.cpp:693 in 4c84f37ad9

689@@ -690,7 +690,7 @@ BOOST_FIXTURE_TEST_CASE(manual_ctor, TestChain100Setup)
690 
691         node::MiniMiner miniminer_manual(miniminer_info, descendant_caches);
692         BOOST_CHECK(miniminer_manual.IsReadyToCalculate());
693-        const auto sequences{miniminer_manual.Linearize()};
694+        const auto sequences{miniminer_manual.Linearize().inclusion_order};

Bambibrown commented at 4:05 am on December 4, 2024:

0        const auto ### _**sequences{miniminer_manual.Linearize().inclusion_order};**_

Bambibrown approved

Fee Estimation: Ignore all transactions that are CPFP’d #30079

Code Coverage & Benchmarks

Reviews

Conflicts