Some cleanup work for mempool and policy estimator #7556

@jtimon Let me know if you think this would be acceptable. I got tired of passing in arguments to GetMinFee() so I took my fix for that but then added rebased versions of your two commits from #7115.

Concept ACK

I still disagree that storing the last sizelimit passed to CTxMemPool::TrimToSize() are the right semantics. It strikes me as hackish, unclean and not very reusable. I would prefer any of the multiple solutions I offered in #7115. But at that point none of the solutions were acceptable to you, in fact, you were strongly opposed to https://github.com/morcos/bitcoin/commit/cf95aa1225df59139c774ca98d6a24fba88a5d4f unless I was open to "concede" your https://github.com/morcos/bitcoin/commit/a8261aea4a67d3482893b01c6583b58048b10b95 . May I ask what has made you change your mind about decoupling CBlockPolicyEstimator from CTxMemPool ? At the same time, that change of opinion seems in direct contradiction with #7557.

If anyone else prefers the solution in #7115, I'm happy to rebase and reopen (or open a new one replacing it, whatever is preferred).

Also, morcos, last time we discussed, you didn't had time to collaborate with me on policy encapsulation. If that's not the case anymore, would you mind to share your thoughts on #7145 and #6423 (based on the apparently-impossible-to-merge #6068) now?

That would probably help me understand the differences in #7557 much better.

@jtimon Actually the only piece of this I care about is the first commit, storing the sizeLimit in TrimToSize and not having to pass it into GetMinFee. In my opinion that is the design of the code:

• The mempool is limited (there is no reason for that limit to be static).
• The size of that limit is set with TrimToSize. (unchanged in this PR)
• GetMinFee does some calculations based on the mempool's current limit (oddly passed in from the outside before this PR)

I added your two commits because I thought you wanted them and they were made easier after my commit. I am still hesitant to think that the CBlockPolicyEstimator won't need to depend on the mempool, but as I'd said before, I'm willing to cross that bridge when we get to it.

I'm happy to help with your other refactor commits, but my hesitation has been not wanting to review the code without knowing that is the direction other contributors want to go. If you could get a couple concept ACK's, I'd be happy to dive in more. I tried to limit this and #7557 to the parts that I understand well and trusted my own opinion on direction.

The mempool is limited (there is no reason for that limit to be static).

True, but is currently static. Are there any plans to make it dynamic? Can you share more about that?

The size of that limit is set with TrimToSize. (unchanged in this PR)

No, before this PR, it is set in GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE), not in TrimToSize (apart from the unittests). I disagree that https://github.com/morcos/bitcoin/commit/a8261aea4a67d3482893b01c6583b58048b10b95#diff-ca74c4b28865382863b8fe7633a85cd6R958 is not change. It is a change that I'm still not able to understand. I also think it is a very counter-intuitive design and will cause confusion no matter how well is documented (by the way, you haven't updated CTxMemPool::TrimToSize's documentation when making this change).

GetMinFee does some calculations based on the mempool's current limit (oddly passed in from the outside before this PR)

Oddly indeed, I'm glad that you are not opposed to changing it anymore like you were in the past when you nacked https://github.com/bitcoin/bitcoin/commit/30ac1d378c71fcf84999d119f341e8e46b4022bf in #7115 (comment) I solved this in multiple alternative ways (none of them including your weird [at least to me] design choice of setting the value every time) and none of them was satisfactory to you at the time. Setting the max size in TrimToSize like you do here is NOT a requirement for cleaning up GetMinFee.

I strongly dislike that design choice. As said, if there's interest, I'm happy to rebase/rewrite an alternative without that design choice. In the meantime, NACK from me on that particular change (although if other people understand and like that decision there's no point for me to work on an alternative without it).

Btw, @laanwj shouldn't this have the "refactoring" label just like #7115 had?

@jtimon I don't mean to be difficult but I would like to get a8261ae merged. Will it be easier for me to achieve that if I keep your commits in this PR or if I remove them? I am indifferent. I'll add the commenting you suggest either way.

@jtimon I don't mean to be difficult but I would like to get a8261ae merged.

I know, you've been wanting it since at least #7115. I don't want to be difficult myself, but I still don't understand https://github.com/morcos/bitcoin/commit/a8261aea4a67d3482893b01c6583b58048b10b95#diff-ca74c4b28865382863b8fe7633a85cd6R958 and I highly dislike it as previously explained.

Will it be easier for me to achieve that if I keep your commits in this PR or if I remove them? I am indifferent.

I don't see how adding other things that I like to a PR can make me like the one particular line I still highly dislike. I thank you for rebasing my commits on top of your desired change, because that makes it clearer where our disagreement is, but I think we already discovered this disagreement when discussing #7115. The only thing that could change my mind on this is an explanation that I can understand and agree with for https://github.com/morcos/bitcoin/commit/a8261aea4a67d3482893b01c6583b58048b10b95#diff-ca74c4b28865382863b8fe7633a85cd6R958 . Bargaining with other changes won't help. Telling me that "it is the current design" or "it is the most clean design" or something vague of the sort, or "to solve X" (X being something that #7115 also solves) won't help. If you ever provided a more reasonable explanation, I'm very sorry but I must confess that I missed it.

Maybe someone else understanding and liking your choice can help me understand, assuming this is preferred to a rebased version of #7115.

I'll add the commenting you suggest either way.

Thank you, hopefully I'm wrong about the "counter-intuitive-ness" of your choice and documentation is enough to avoid confusing people reading the code.

Closing. I'd still really like a8261ae but it doesn't seem to be generally approved.