Fee Estimation: Ignore all transactions with an in-block child #30079

ismaelsadeeq · 2024-05-10T09:04:16Z

Another attempt at #25380 with an alternate approach

This PR updates CBlockPolicyEstimator to ignore all transactions with an in-block 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.

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

The correct approach is to 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.

I have a branch that implements this using a mini miner:

The implication of the approach in this branch is that the constructor used and the linearization code will be removed post-cluster mempool implementation:

Rework mini_miner to utilize cluster mempool features

Another approach I contemplated was to replicate the linearization code into policy/fees.cpp and use it but this means having duplicate code in policy/fees.cpp and node/mini_miner.cpp. Considering that post-cluster mempool, we will transition to tracking chunks rather than individual transactions and the specification for this is already under discussion here.

This PR is an interim solution, focusing on ignoring transactions with in-block children this is safe to use considering that the majority of transactions are individual transactions. Upon implementing the cluster mempool, we will just track chunks and make the fee estimator package aware.

DrahtBot · 2024-05-10T09:04:19Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux
Concept ACK	instagibbs, josibake

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

glozow · 2024-05-10T13:54:06Z

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

instagibbs · 2024-05-10T13:58:07Z

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 · 2024-05-10T14:19:12Z

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 · 2024-05-10T17:41:03Z

Concept ACK

rkrux

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.

test/functional/feature_fee_estimation.py

src/policy/fees.cpp

rkrux · 2024-05-14T08:26:41Z

src/policy/fees.cpp

+        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)) {


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?

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

bitcoin/src/policy/fees.cpp

Line 615 in 7fcf4e9

const bool validForFeeEstimation = !tx.m_mempool_limit_bypassed && !tx.m_submitted_in_package && tx.m_chainstate_is_current && tx.m_has_no_mempool_parents;

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.

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?

bitcoin/src/policy/fees.cpp

Line 613 in 7fcf4e9

// - the transaction is not dependent on any other transactions in the mempool

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.

test/functional/feature_fee_estimation.py

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

ismaelsadeeq · 2024-05-15T16:43:32Z

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 · 2024-05-15T16:53:15Z

Thanks for review @rkrux

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

rkrux

reACK 2563305

Thanks for the quick turnaround on this @ismaelsadeeq!

rkrux · 2024-05-16T09:20:33Z

test/functional/feature_fee_estimation.py

@@ -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}


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.

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

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.

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

josibake

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.

josibake · 2024-05-16T09:56:18Z

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)


in a6e8133 ("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

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

bitcoin/src/validation.cpp

Lines 1274 to 1279 in dd42a5d

const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees,

ws.m_vsize, ws.m_entry->GetHeight(),

args.m_bypass_limits, args.m_package_submission,

IsCurrentForFeeEstimation(m_active_chainstate),

m_pool.HasNoInputsOf(tx));

m_pool.m_opts.signals->TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence());

bitcoin/src/policy/fees.cpp

Lines 610 to 615 in dd42a5d

// This transaction should only count for fee estimation if:

// - it's not being re-added during a reorg which bypasses typical mempool fee limits

// - the node is not behind

// - the transaction is not dependent on any other transactions in the mempool

// - it's not part of a package.

const bool validForFeeEstimation = !tx.m_mempool_limit_bypassed && !tx.m_submitted_in_package && tx.m_chainstate_is_current && tx.m_has_no_mempool_parents;

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:

bitcoin/src/policy/fees.cpp

Line 640 in dd42a5d

if (!_removeTx(tx.info.m_tx->GetHash(), true)) {

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

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

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?

josibake · 2024-05-16T09:58:36Z

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 */


in a6e8133 ("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).

josibake · 2024-05-16T10:00:53Z

test/functional/feature_fee_estimation.py

@@ -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}


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 approved these changes May 14, 2024

View reviewed changes

DrahtBot requested review from instagibbs and josibake May 14, 2024 08:38

ismaelsadeeq and others added 3 commits May 14, 2024 10:21

[fees]: ignore all transaction with in block child

a6e8133

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

3e50d13

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

2563305

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

ismaelsadeeq force-pushed the 05-2023-ignore-transactions-with-parents branch from c12a677 to 2563305 Compare May 14, 2024 17:03

rkrux approved these changes May 16, 2024

View reviewed changes

josibake reviewed May 16, 2024

View reviewed changes

DrahtBot requested a review from josibake May 16, 2024 10:02

laanwj added the Mempool label May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fee Estimation: Ignore all transactions with an in-block child #30079

Fee Estimation: Ignore all transactions with an in-block child #30079

ismaelsadeeq commented May 10, 2024 •

edited

DrahtBot commented May 10, 2024 •

edited

glozow commented May 10, 2024

instagibbs commented May 10, 2024

ismaelsadeeq commented May 10, 2024

josibake commented May 10, 2024

rkrux left a comment •

edited

rkrux May 14, 2024

ismaelsadeeq May 14, 2024 •

edited

rkrux May 14, 2024

ismaelsadeeq May 14, 2024 •

edited

ismaelsadeeq commented May 15, 2024

ismaelsadeeq commented May 15, 2024

rkrux left a comment

rkrux May 16, 2024

ismaelsadeeq May 16, 2024 •

edited

josibake May 16, 2024

rkrux May 16, 2024

josibake left a comment

josibake May 16, 2024

glozow May 16, 2024 •

edited

ismaelsadeeq May 16, 2024

josibake May 17, 2024

josibake May 16, 2024

josibake May 16, 2024

	const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees,
	ws.m_vsize, ws.m_entry->GetHeight(),
	args.m_bypass_limits, args.m_package_submission,
	IsCurrentForFeeEstimation(m_active_chainstate),
	m_pool.HasNoInputsOf(tx));
	m_pool.m_opts.signals->TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence());

	// This transaction should only count for fee estimation if:
	// - it's not being re-added during a reorg which bypasses typical mempool fee limits
	// - the node is not behind
	// - the transaction is not dependent on any other transactions in the mempool
	// - it's not part of a package.
	const bool validForFeeEstimation = !tx.m_mempool_limit_bypassed && !tx.m_submitted_in_package && tx.m_chainstate_is_current && tx.m_has_no_mempool_parents;

Fee Estimation: Ignore all transactions with an in-block child #30079

Are you sure you want to change the base?

Fee Estimation: Ignore all transactions with an in-block child #30079

Conversation

ismaelsadeeq commented May 10, 2024 • edited

DrahtBot commented May 10, 2024 • edited

Code Coverage

Reviews

glozow commented May 10, 2024

instagibbs commented May 10, 2024

ismaelsadeeq commented May 10, 2024

josibake commented May 10, 2024

rkrux left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismaelsadeeq May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismaelsadeeq May 14, 2024 • edited

Choose a reason for hiding this comment

ismaelsadeeq commented May 15, 2024

ismaelsadeeq commented May 15, 2024

rkrux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismaelsadeeq May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josibake left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glozow May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismaelsadeeq commented May 10, 2024 •

edited

DrahtBot commented May 10, 2024 •

edited

rkrux left a comment •

edited

ismaelsadeeq May 14, 2024 •

edited

ismaelsadeeq May 14, 2024 •

edited

ismaelsadeeq May 16, 2024 •

edited

glozow May 16, 2024 •

edited