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

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

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 · 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
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

Reviewers, this pull request conflicts with the following ones:

#30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

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

test/functional/feature_fee_estimation.py

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?

Added the comment, thanks.

src/policy/fees.h

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.

ismaelsadeeq · 2024-06-03T13:04:17Z

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 2563305 to 055d9e6 Compare diff
Addressed all comments.

DrahtBot · 2024-06-03T14:57:29Z

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

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

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

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

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

rkrux approved these changes May 14, 2024

View reviewed changes

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

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

bitcoin deleted a comment from laib200 Jun 3, 2024

ismaelsadeeq force-pushed the 05-2023-ignore-transactions-with-parents branch from 2563305 to 57556e5 Compare June 3, 2024 12:55

ismaelsadeeq changed the title ~~Fee Estimation: Ignore all transactions with an in-block child~~ Fee Estimation: Ignore all transactions that are CPFP'd Jun 3, 2024

ismaelsadeeq force-pushed the 05-2023-ignore-transactions-with-parents branch from 57556e5 to 55d615c Compare June 3, 2024 14:57

DrahtBot added the CI failed label Jun 3, 2024

ismaelsadeeq and others added 6 commits June 3, 2024 16:02

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

80bc40d

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

[fees]: add method that linearize transaction removed from mempool

134ec3d

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

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

470917b

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

bca1e81

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

055d9e6

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

ismaelsadeeq force-pushed the 05-2023-ignore-transactions-with-parents branch from 55d615c to 055d9e6 Compare June 3, 2024 15:08

DrahtBot mentioned this pull request Jun 3, 2024

Fee Estimation via Fee rate Forecasters #30157

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Fee Estimation: Ignore all transactions that are CPFP'd #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

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

ismaelsadeeq Jun 3, 2024

josibake May 16, 2024

ismaelsadeeq commented Jun 3, 2024 •

edited

DrahtBot commented Jun 3, 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 that are CPFP'd #30079

Are you sure you want to change the base?

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

Conversation

ismaelsadeeq commented May 10, 2024 • edited

DrahtBot commented May 10, 2024 • edited

Code Coverage

Reviews

Conflicts

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

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 Jun 3, 2024 • edited

DrahtBot commented Jun 3, 2024

ismaelsadeeq commented May 10, 2024 •

edited

DrahtBot commented May 10, 2024 •

edited

rkrux left a comment •

edited

ismaelsadeeq May 16, 2024 •

edited

glozow May 16, 2024 •

edited

ismaelsadeeq commented Jun 3, 2024 •

edited