Continuously limit the memory pool memory consumption

[sipa]

This pull request contains a squashed version of #6331 and #6410, and replaces #6281.

Whenever a new transaction arrives, we check which of the lowest-feerate transactions in the mempool (including their dependencies) would have to be removed to make the memory consumption go below a configurable limit. The DoS protection relay rules then operate on the size of both the new transaction and the removed ones (as pointed out by @lapp0 in #6281).

Note that:

• Even though it adds a feerate index to the mempool, it does not use this index for block construction.
• The mempool eviction code does not take priority into account. It strictly aims at maximizing mempool fee rate.
• Untested.

[jonasschnelli]

Needs rebase because #6410 is merged now.

[petertodd]

Need to update this error msg.

[sipa]

Suggestion?

[petertodd]

Oh, actually I misread it; no changes needed.

[petertodd]

Can you update this comment to clarify if we're talking about txs or ram?

[sipa]

Fixed.

[jtimon]

Nits from #6331 rebased at sipa/bitcoin@limitpool...jtimon:pr-6421-0.11.99 (will force push to jtimon/pr-6421-0.11.99 as this gets rebased).

[sipa]

@lapp0 You suggested using "the new transaction's fees should pay for the size of the new plus removed transaction". That doesn't help, as you can create a sequence of transactions that each replace the previous one, and each have enough fees to pay for both. This would give you infinite relay bandwidth at fixed cost.

The solution is perhaps to remember for each mempool transaction what the size of everything it has replaced is, but that's a bit more complex than I'm willing to do now. I've chosen the conservative approach here which is to look at the fee difference instead.

[sipa]

@jtimon It's probably a bit more complicated than just a score function, I now realize. The mempool code is trying to optimize for fee/byte (currently), independently of what sorting is implemented by the index. I think we'll need a policy-controlled "cost" (as a generalization of size, perhaps corrected for UTXO differences) and policy-controlled "revenue" (as a generalization of fee). The reason is that you can't compute the "score" of a collection of transactions - you need (revenue1+revenue2)/(cost1+cost2) rather than just score1+score2.

[jonasschnelli]

Slightly tested. Running this code (git commit tip 3c638fc0ba82a9d9c235f428d098a098fc0b6b16, not the latest tip) since some hours with -maxmempool=100. Since 1h i have a stable dynamic memory size of ~100MB. Graph: http://bitcoin.jonasschnelli.ch/charts/mempool6410/

Log filtered after the "stored orphan txs": https://gist.githubusercontent.com/jonasschnelli/1f2e89d64887710f6c5b/raw/dba3d68d79cc649cd7e01c992d40da8d46073431/gistfile1.txt

[jtimon]

@sipa answered in #6331: bike-shed the name of the variable, the getter and the comparator, but please don't make the type CFeeRate so that we have to fix it later. int64_t should work perfectly fine for these changes.

[jtimon]

I believe this could be much simpler (and the end result better) after #5709 (is 10 commits but ready to be squashed into the first one), but I doubt people want to read step by step to be sure behavior is not being changed. At least not more now than when it was opened...

[sipa]

@jtimon It's a bit more complicated. The replacement code needs to have a way to know whether replacing a set of transactions with another transaction is a good idea. Contrary to what I first thought, just having a score to compare is not enough - if the index order doesn't match the feerate well, its search for sets to remove will degrade or fail.

One way to generalize this to something policy-controllable is to have a "general reward" (typically fee) and a "general cost" (typically bytes) determined by the policy at mempool entry time, and then compare reward/cost ratios (typically feerates), both in the index, and in the replacement code (the limiting code, but also for example CPFP combination code and RBF code). But it's really not as simple as making the index order configurable - sorry for saying so at first.

[jtimon]

Even in that case, both the "general reward" and the "general cost" indexes can use int64_t instead of CFeeRate and size_t respectively. Can we agree on that first?

I still don't understand why this needs transaction replacement. We can add it or not as normal and, after adding, trim to the desired size. with this, we could have a unified index that it's just reward/cost instead of two separate ones.
But for transaction replacement, #6416 is what I had in mind. Something more flexible that is independent from the index or the mempool entries themselves. I just realized that ApproveTxReplacement needs a CCoinsViewCache parameter and it would be a good idea to call it later. Even a "general reward" and "general cost" index in the mempool may not be enough for certain replacement policies, for example, zero-conf-safer-RBF (also known as FSS-RBF, but it seems to me that everything is "first seen safe").
So I don't think we need or can generally solve replacements here: expiring old transactions before adding a new transaction and forcing the mempool to a given size just after that (simply by dropping until it is enough from the bottom of the unified index) should be enough.
In my opinion adding transaction replacement will unnecessarily complicate things, not only for this PR, but also for later changes in replacement policies (for example adding an option to use ZCS-RBF instead of FS as replacement policy) and with later stages of your own plan (#6331 (comment) ):

• Generalize the feerate here to a unified policy-dependent score (effectively removing the priority as it exists today, as in Remove TX priority and free transaction area from mempool, block creator. #6405).
• Implement block creation using this score-based index.

[sipa]

@jtimon There is a DoS attack possible by mempool limiting, where someone sends a transaction that ends up at the bottom of the mempool, and then sends another transaction with slightly higher feerate, causing the previous one to be evicted later on. This leads to network broadcast bandwidth at much lower cost than the actual network relay fee, as discovered by @lapp0.

The solution is to treat block size limiting as transaction replacement with the mempool bottom (sorted by feerate/score), and require that the new transaction pays in fee for the relay of the old transactions that we kicked out.

[dgenr8]

@sipa You don't need to optimize a ratio, if you can represent both the rewards and costs in comparable units. Then you could optimize the difference. I wonder if unit costs could be represented in BTC/byte ...

[sipa]

@dgenr8 Optimizing feerate is what you expect miners to do in their mempool, as it maximizes income given a constrained (by rule or propagation economics) block size.

@jtimon Yes, I agree that instead of feerate and size we can use int64_t. Or double even. But the logic is already complicated enough here. I really don't think it's wise to spend more mental power of maintainers and reviewers to understand how this code will at some point generalize to a configurable policy.

[dgenr8]

@sipa Miners don't care about relay cost, which you are now trying to include.

[sipa]

@dgenr8 Of course. This is DoS protection code for relaying nodes, not for miners. Its primary purpose is preventing people from being able to spam the network, in various ways. It aims to build a mempool which is as aligned with miner's incentives as possible, but is restricted to prevent network actors from causing too high memory consumption, get massive flooding bandwidth or consume too much CPU power on the nodes traversed by it.

[sipa]

Pushed a new version which tries more than just the bottom transactions and their dependencies in the mempool, is more efficient, and is better documented.

[jtimon]

@jtimon Yes, I agree that instead of feerate and size we can use int64_t. Or double even. But the logic is already complicated enough here. I really don't think it's wise to spend more mental power of maintainers and reviewers to understand how this code will at some point generalize to a configurable policy.

Whatever, If CFeeRate(nFee, nTxSize) is more readable than nFee / nTxSize and jtimon@00baf3d makes things more complicated (I strongly disagree), let's not waste the time of today's reviewers and let's waste the time of future maintainers instead. This doesn't generalize anything (it is functionally equivalent!), it's just avoids introducing unnecessary barriers to generalization at this point. But if we're going "spend too much mental power" by thinking about a cleaner history, let's not do it and let's do things wrong instead for the shake of preserving such a vaguely defined resource. Since this is urgent, let's not make perfectly reasonable nits that will "waste" our time, let's write now what we already know we will have to erase tomorrow. I think we've already wasted enough time discussing this already and since you've been arguing against the little nit, I'm sure @ashleyholman will not want to incorporate it to #6331.
I'll just write something down to remember to fix up what you're doing wrong now, that's fine.

[dgenr8]

@morcos In what I described, every child "pays" for its parents up-front in reduced mempool/relay attractiveness. Multiple children pay again for the same parent, and there is a recursive effect.

Unconfirmed chains are expensive to process, have huge DoS risk, and limited usefulness. Replacement complicates everything. I mentioned in a mailing list post an overall approach I took.

[jtimon]

It seems everybody is happy with sipa@8adacf1
Can we merge that first (after rebase) while we discuss the last commit?

@morcos I'm not sure I understand your complains, but if the mempool is capped there must be some replacement criteria, even if it's the dumb "never replace" we have now (that's why I think the last commit would be clearer and more forward compatible with sipa@44d29ff ).

In fact, capping the mempool with the current first seen replacement policy (that is, all replacements forbidden policy) would be the simplest way to cap the mempool (although not precisely the best way to cap it). Anything beyond that (always rejecting new transactions when the mempool is full) must necessarily be more complicated, but also hopefully better than never replacing.

[jtimon]

Btw, there's slightly related optimizations in #6445. The most relevant parts for this PR being in AcceptToMemoryPool:

• Don't calculate nValueOut 5 times
• Don't calculate nValueIn 3 times
• Don't call CCoinsViewCache::HaveInputs 3 times

[jtimon]

Rebased version (with my suggestions on top) in https://github.com/jtimon/bitcoin/commits/post_limitpool

[Diapolo]

Can this be rebased, the Qt keyword pull sneaked in here ;).

[jtimon]

Shouldn't this be mapTx.erase(hash); ?

[morcos]

This test is better placed after we've randomly skipped some entries below. Otherwise we might end up evicting something that actually has a better fee rate than the tx being considered.

[sipa]

Unless you're talking about an outer-loop transaction being hit which has an earlier (lower feerate) skipped transaction as dependency, I think the odds are small. But it won't hurt, both are very cheap checks.

[sipa]

Superseded by a dozen other PRs.