Current test coverage doesn’t ensure that mempool trimming doesn’t appear prior to the entire package, and not just the subpackage, is finished being submitted.
Add a scenario that covers this case, where package ancestors can make it in individually, but would be immadiately evicted if not for the package CPFP.
in response to #31122 (review) where if applied onto that PR’s old commit, the test fails due to package failure.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See 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.
Reviewers, 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.
116+ package_hex = []
117+ # UTXOs to be spent by the ultimate child transaction
118+ parent_utxos = []
119+
120+ # Series of parents that don't need CPFP and are submitted individually. Each one is large and
121+ # belo, which means they should trigger eviction, but child submission should result
142+
143+ # Create a child spending everything with an insane fee, bumping the package above mempool_entry_minrate
144+ child = self.wallet.create_self_transfer_multi(utxos_to_spend=parent_utxos, fee_per_output=10000000)
145+ package_hex.append(child["hex"])
146+
147+ # Package should be submitted, temporarily exceeding maxmempool, and then evicted.
103+
104+ mempool_txids = node.getrawmempool()
105+ mempool_entries = [node.getmempoolentry(entry) for entry in mempool_txids]
106+ fees_btc_per_kvb = [entry["fees"]["base"] / (Decimal(entry["vsize"]) / 1000) for entry in mempool_entries]
107+ #mempool_maxrate = max(fees_btc_per_kvb)
108+ #mempool_maxrate = mempool_maxrate.quantize(Decimal("0.00000000"))
128+
129+ big_parent_txids = []
130+ big_parent_wtxids = []
131+ for i in range(num_big_parents):
132+ # Last parent is higher feerate causing other parents to be possibly
133+ # be evicted if trimming was allowed, which would cause the package to end up failing
nit: to be possibly be
0 # Last parent is higher feerate causing other parents to possibly
1 # be evicted if trimming was allowed, which would cause the package to end up failing
168+ assert txid in resulting_mempool_txids
169+
170+ # Check every evicted tx was higher feerate than parents which evicted it
171+ eviction_set = set(mempool_txids) - set(resulting_mempool_txids) - set(big_parent_txids)
172+ parent_entries = [node.getmempoolentry(entry) for entry in big_parent_txids]
173+ max_parent_feerate = max([entry["fees"]["base"] / (Decimal(entry["vsize"]) / 1000) for entry in parent_entries])
nit: strictly speaking…
0 max_parent_feerate = max([entry["fees"]["modified"] / (Decimal(entry["vsize"]) / 1000) for entry in parent_entries])
91+
92+ # Clear mempool so it can be filled with minrelay txns
93+ self.restart_node(0, extra_args=self.extra_args[0] + ["-persistmempool=0"])
94+ assert_equal(node.getrawmempool(), [])
95+
96+ # Restarting the node resets mempool minimum feerate
149+ with node.assert_debug_log(expected_msgs=["rolling minimum fee bumped"]):
150+ package_res = node.submitpackage(package=package_hex, maxfeerate=0)
151+
152+ assert_equal(package_res["package_msg"], "success")
153+
154+ # If intra-packing trimming ends up happening anyways, we would
166+ assert child["txid"] in resulting_mempool_txids
167+ for txid in big_parent_txids:
168+ assert txid in resulting_mempool_txids
169+
170+ # Check every evicted tx was higher feerate than parents which evicted it
171+ eviction_set = set(mempool_txids) - set(resulting_mempool_txids) - set(big_parent_txids)
- set(big_parent_txids)
I suppose this can be dropped based on the assertion above on lines 167/8?big_parent_txids is already in resulting_mempool_txids .
big_parent_txids is only 3 - I guess it won’t be an issue perf wise.
Current test coverage doesn't ensure that mempool trimming
doesn't appear prior to the entire package, and not just
the subpackage, is finished being submitted.
Add a scenario that covers this case, where package
ancestors can make it in individually, but would be
immadiately evicted if not for the package CPFP.
119+ # which means in aggregate they could trigger eviction, but child submission should result
120+ # in them not being evicted
121+ parent_vsize = 25000
122+ num_big_parents = 3
123+ # Need to be large enough to trigger eviction
124+ # (note that the mempool usage of a tx is about three times its vsize)
120+ # in them not being evicted
121+ parent_vsize = 25000
122+ num_big_parents = 3
123+ # Need to be large enough to trigger eviction
124+ # (note that the mempool usage of a tx is about three times its vsize)
125+ assert_greater_than(parent_vsize * num_big_parents * 3, current_info["maxmempool"] - current_info["bytes"])
usage be used here instead of bytes because the first argument of assert uses memory usage?
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mempool.cpp#L699-L700
151+
152+ # Ensure that intra-package trimming is not happening.
153+ # Each transaction separately satisfies the current
154+ # minfee and shouldn't need package evaluation to
155+ # be included. If trimming of a parent were to happen,
156+ # package evaluation would happen to reintrodce the evicted
reintrodce
“If trimming of a parent were to happen, # package evaluation would happen to reintrodce the evicted parent”
Out of curiosity: Is the only way for us to know if trimming happened is through debug logs?
Typo reintrodce
Will fix if I touch again
There’s no direct way to query that a trimming happened, no. Note that even if it was directly reported, the results can’t really be cached since we’re evicting things.
107+ mempool_entry_minrate = min(fees_btc_per_kvb)
108+ mempool_entry_minrate = mempool_entry_minrate.quantize(Decimal("0.00000000"))
109+
110+ # There is a gap, our parents will be minrate, with child bringing up descendant fee sufficiently to avoid
111+ # eviction even though parents cause eviction on their own
112+ assert_greater_than(mempool_entry_minrate, mempoolmin_feerate)
mempool_entry_minrate and mempoolmin_feerate currently as mempool_entry_minrate is calculated based on the current mempool entries that don’t include the to be added parents and child?
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mempool.cpp#L703
A gap can/does happen naturally in that the highest descendant feerate evicted, plus minrelay(1 sat/vbyte) becomes the new floating fee.
I guess fill_mempool does it in such a way that all the leftover transactions are higher than the new minrelay?
I’ll take a longer look into seeing if we can guarantee this more directly somehow
tACK f32c34d0c3d4041a301822b27e88d6db4cbf631e
Successful make and functional tests. In my system, 1 out of the 72 txs in the mempool was evicted when this package of 3 parents and 1 child was submitted.
Thanks for adding this one, it is a fantastic functional test to have that tests different pieces, I enjoyed going through it. Asked couple questions for my understanding.