As suggested in the main cluster mempool PR (https://github.com/bitcoin/bitcoin/pull/28676#pullrequestreview-3177119367), I’ve pulled out some of the non-essential optimizations and cleanups into this separate PR.
Will continue to add more commits here to address non-blocking suggestions/improvements as they come up.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33591.
See the guideline for information on the review process. A summary of reviews will appear here.
Reviewers, this pull request conflicts with the following ones:
src/init and src/policy by hebasto)BlockTemplateCache 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.
Possible typos and grammar issues:
to to -> to [duplicate “to” makes the sentence awkward/confusing] Context: “for singleton transactions (that are in clusters by themselves) it’s sufficient for a replacement to to have a higher fee and feerate than the original.” (release notes)
5). -> 5. or (5) [extra closing parenthesis; may confuse the reference to rule 5] Context: “analogous to regular replacement rule 5).”
with more -> with more [double space between words; minor but unnecessary and can interrupt reading] Context: “Check Package RBF cannot conflict with more than MAX_REPLACEMENT_CANDIDATES clusters” (test text)
No other added lines contain typographic/grammatical errors that render the English invalid or incomprehensible.
drahtbot_id_5_m
This takes the same amount of space as CTransaction pointers, and saves a map
lookup in many common uses.
Effectively this is treating all transactions in txgraph as being in a cluster
of size 1.
Preparatory commit to the rbf functional test, before changes are made to the
rbf rules as part of cluster mempool.
Include an adjustment to mempool_tests.cpp due to the additional memory used by
txgraph.
Includes a temporary change to the mempool_ephemeral_dust.py functional test,
due to validation checks being reordered. This change will revert once the RBF
rules are changed in a later commit.
Rather than evicting the transactions with the lowest descendant feerate,
instead evict transactions that have the lowest chunk feerate.
Once mining is implemented based on choosing transactions with highest chunk
feerate (see next commit), mining and eviction will be opposites, so that we
will evict the transactions that would be mined last.
Co-Authored-By: Gregory Sanders <gsanders87@gmail.com>
The addition of a cluster size limit makes the CPFP carveout rule useless,
because carveout cannot be used to bypass the cluster size limit. Remove this
policy rule and update tests to no longer rely on the behavior.
With a total ordering on mempool transactions, we are now able to calculate a
transaction's mining score at all times. Use this to improve the RBF logic:
- we no longer enforce a "no new unconfirmed parents" rule
- we now require that the mempool's feerate diagram must improve in order
to accept a replacement
- the topology restrictions for conflicts in the package rbf setting have been
eliminated
Revert the temporary change to mempool_ephemeral_dust.py that were previously
made due to RBF validation checks being reordered.
Co-authored-by: Gregory Sanders <gsanders87@gmail.com>, glozow <gloriajzhao@gmail.com>
Previously, transaction batches were first sorted by ancestor count and then
feerate, to ensure transactions are announced in a topologically valid order,
while prioritizing higher feerate transactions. Ancestor count is a crude
topological sort criteria, so replace this with linearization order so that the
highest feerate transactions (as would be observed by the mining algorithm) are
relayed before lower feerate ones, in a topologically valid way.
This also fixes a test that only worked due to the ancestor-count-based sort
order.
The mempool clusters and linearization permit sorting the mempool topologically
without making use of ancestor counts (as long as the graph is not oversized).
Co-authored-by: Pieter Wuille <pieter@wuille.net>
In preparation for removing ancestor data from CTxMemPoolEntry, recalculate the
ancestor statistics on demand wherever needed.
The cluster limits should be sufficient.
Co-Authored-By: Gregory Sanders <gsanders87@gmail.com>
Remove a reference to GetCountWithDescendants() in preparation for removing
this function and the associated cached state from the mempool.
This is in preparation for removing the cached descendant state from the
mempool.
Cluster size limits should be enough.
The new cluster mempool RBF rules take into account clusters sizes exactly, so
with the removal of descendant count enforcement this idea is obsolete.
With the descendant size limits removed, replace the concept of "max number of
descendants of any ancestor of a given tx" with the cluster count of the cluster
that the transaction belongs to.
Now that ancestor calculation never fails (due to ancestor/descendant limits
being eliminated), we can eliminate the error handling from
CalculateMemPoolAncestors.
Calculating mempool ancestors for a new transaction should not be done until
after cluster size limits have been enforced, to limit CPU DoS potential.
Achieve this by reworking TRUC and RBF validation logic:
- TRUC policy enforcement is now done using only mempool parents of
new transactions, not all mempool ancestors (note that it's fine to calculate
ancestors of in-mempool transactions, if the number of such calls is
reasonably bounded).
- RBF replacement checks are performed earlier (which allows for checking
cluster size limits earlier, because cluster size checks cannot happen until
after all conflicts are staged for removal).
- Verifying that a new transaction doesn't conflict with an ancestor now
happens later, in AcceptSingleTransaction() rather than in PreChecks(). This
means that the test is not performed at all in AcceptMultipleTransactions(),
but in package acceptance we already disallow RBF in situations where a
package transaction has in-mempool parents.
Also to ensure that all RBF validation logic is applied in both the single
transaction and multiple transaction cases, remove the optimization that skips
the PackageMempoolChecks() in the case of a single transaction being validated
in AcceptMultipleTransactions().
Changes AddToMempool() helper to only apply changes if the mempool limits are
respected.
Fix package_rbf fuzz target to handle mempool policy violations
Add benchmarks for:
- adding a transaction
- calculating mempool ancestors/descendants
Including test coverage for mempool eviction and expiry
Co-authored-by: glozow <gloriajzhao@gmail.com>
Co-authored-by: glozow <gloriajzhao@gmail.com>
After cluster mempool, the mini_miner will no longer match the miner's block
construction. Eventually mini_miner should be reworked to directly use
linearizations done in the mempool.
We use CompareMiningScoreWithTopology() for sorting transaction announcements
during tx relay, and we use GetSortedScoreWithTopology() in
CTxMemPool::check().
Co-Authored-By: Gregory Sanders <gsanders87@gmail.com>
Previously we would sanity check the -maxmempool configuration based on a
multiple of the descendant size limit, but with cluster mempool the maximum
evicted size is now the cluster size limit, so use that instead.
Also allow -maxmempool=0 in general (and not just if
-limitdescendantsize/-limitclustersize is set to 0).
-BEGIN VERIFY SCRIPT-
find src/test -type f -exec sed -i 's/AddToMempool/TryAddToMempool/g' {} +
find src/bench -type f -exec sed -i 's/AddToMempool/TryAddToMempool/g' {} +
-END VERIFY SCRIPT-
This makes the terminology consistent with other parts of the codebase, as part
of the cluster mempool implementation.
For now, just link to the long writeups on delvingbitcoin.org
Now that the mempool no longer keeps any feerate-based indices, we can modify
feerates in mempool entries directly.
Labels
Mempool
Needs rebase