[FreshEyes] Fee Estimation: Ignore all transactions with an in-block child #16
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There were 37 issue comments left by 6 reviewers for the pull request |
![fresheyes-staging-bot[bot]](https://avatars.githubusercontent.com/in/855196?s=60&v=4){kind=small}
| 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]) |
There was a problem hiding this comment.
2 authors commented here with:
- comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599579906at 2024/05/14, 08:13:00 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600088583at 2024/05/14, 13:54:38 UTC.
| @@ -129,6 +129,15 @@ def make_tx(wallet, utxo, feerate): | |||
| ) | |||
|
|
|||
|
|
|||
| def send_tx(wallet, node, utxo, feerate): | |||
There was a problem hiding this comment.
2 authors commented here with:
- comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599589458at 2024/05/14, 08:19:59 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600383232at 2024/05/14, 17:07:24 UTC.
| return wallet.send_self_transfer( | ||
| from_node=node, | ||
| utxo_to_spend=utxo, | ||
| fee_rate=Decimal(feerate * 1000) / COIN, |
There was a problem hiding this comment.
2 authors commented here with:
- comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599591670at 2024/05/14, 08:21:34 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600311972at 2024/05/14, 16:13:46 UTC.
| "txid": tx["txid"], | ||
| "vout": 0, | ||
| "value": Decimal(tx["tx"].vout[0].nValue) / COIN | ||
| }) |
There was a problem hiding this comment.
2 authors commented here with:
- comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599602598at 2024/05/14, 08:29:20 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600400559at 2024/05/14, 17:22:12 UTC.
| """ | ||
| # The broadcaster and block producer | ||
| broadcaster = self.nodes[0] | ||
| miner = self.nodes[1] |
There was a problem hiding this comment.
An author commented here with:
- comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599613474at 2024/05/14, 08:37:13 UTC.
| @@ -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.
4 authors commented here with:
- comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1602965411at 2024/05/16, 09:20:33 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1602989484at 2024/05/16, 09:33:04 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1603035220at 2024/05/16, 10:00:53 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1603130595at 2024/05/16, 10:57:04 UTC.
| @@ -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.
3 authors commented here with:
- comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1603028373at 2024/05/16, 09:56:18 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1603098402at 2024/05/16, 10:32:57 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1603108416at 2024/05/16, 10:39:03 UTC.
| @@ -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 */ | |||
There was a problem hiding this comment.
An author commented here with:
- comment link
https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1603032016at 2024/05/16, 09:58:36 UTC.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The author ismaelsadeeq wrote the following PR called Fee Estimation: Ignore all transactions with an in-block child, issue number 30079 in bitcoin/bitcoin cloned by FreshEyes below:
Another attempt at
#25380with an alternate approachThis PR updates
CBlockPolicyEstimatorto 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:
mini miner: Linearize should also return package fees and sizes
fees: Add function computing ancestors and descendants of transactions
fees: Ignore transactions that are CPFP'd
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.cppand use it but this means having duplicate code inpolicy/fees.cppandnode/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.