So, you mean the scenario where we have a high fee-rate transaction that is spent by one or more low fee-rate transactions? For instance suppose we have two transactions in our mempool: tx1a, 1KB w/ 1mBTC fee, which is spent by tx2, 10KB w/ 1mBTC fee. We get tx1b, 10KB w/ 2.1mBTC fee. Since the overall feerate of tx1b is higher than tx1a+tx2, it’ll be accepted, even though a miner might have rather mined tx1a instead, ignoring tx2 (for now).
I agree that’s less than optimal. If we make the assumption that there’s always more demand for blockchain space than supply, it might be reasonable for the replacement logic criteria to be whether or not we’re increasing the fee-rate of any subset of the mempool. (while still paying more fees than the replaced transactions)
Without taking CPFP into account, you could simply use the same kind of priority heap logic as in CreateNewBlock() on the list of transactions that would be replaced by the new transaction. You’d iterate through the heap from highest priority to lowest, stopping once you had found as many bytes worth of transactions as the candidate replacement. If the fee-rate of the replacement is higher than the fee-rate of those transactions, accept the replacement into the mempool.
To take CPFP into account… Thinking about it more the mempool package tracking is probably backwards from what we want. Right now it tracks descendants, when really what we want to know is “what’s the total fee-rate if I mine this transaction, and all ancestors?” If we kept track of “packages” that way, we’d be able to do the comparison by determining if the total fee-rate of the new package created by the replacement is higher than the fee-rates of all the packages invalidated by it. I actually did some work on something along these lines a few years ago, though didn’t finish it - the implementation is a lot simpler now that we have strict ancestor limits. (when limiting, just throw away the tx associated with the lowest fee-rate package, which is guaranteed to have no descendants)
For now though I’d be inclined to merge this PR as-is, as all the above options are pretty complex. I also don’t see any way this code could be used in an DoS attack.