-
Notifications
You must be signed in to change notification settings - Fork 35.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fee Estimation: Ignore all transactions that are CPFP'd #30079
base: master
Are you sure you want to change the base?
Fee Estimation: Ignore all transactions that are CPFP'd #30079
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
cc @josibake @instagibbs @naumenkogs @darosior @murchandamus since you reviewed the last time |
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? |
I will analyze the percentage of cluster size 1 transactions mined in previous blocks. |
Concept ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
c12a677
to
2563305
Compare
I tracked recent 1000 blocks from block ~61% of transactions in the last 1000 blocks were confirmed in a cluster size > 1. Transactions: 3974143 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 |
Thanks for review @rkrux
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reACK 2563305
Thanks for the quick turnaround on this @ismaelsadeeq!
@@ -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} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to modify if there is need to retouch, else this is a good idea for a follow-up @rkrux
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can pick this up in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/policy/fees.cpp
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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()); |
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the comment, thanks.
@@ -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} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
2563305
to
57556e5
Compare
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
|
57556e5
to
55d615c
Compare
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
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>
55d615c
to
055d9e6
Compare
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.