Reluctant ACK.

Code ACK, agree about avoiding the circular dependency, and agree about avoiding GetArg calls to determine the mempool max size the whole time.

I dislike moving more policy knowledge into the mempool itself (its size limit), though. It will complicate a more generic memory limiting approach, where there is not a single memory size limit, but one that depends on the UTXO size and perhaps other buffers.

Complicate it by putting it into a single attribute in the class that makes more sense?
Although I'm not sure how what you have in mind would interact with the current mempool limiting, can't we just create a simple setter when we need it?
I mean, putting it as an attribute there is not the only solution. We can also make it a global in main that the mempool takes as parameter.
About having policy code in the mempool, now that #6871 has been merged I'm eager to also decouple the CTxMemPool from CBlockPolicyEstimator (making them completely independent of each other). I expect it to be harder than last time I did it though.
Unless we don't want anything else related to policy encapsulation in 0.12, in that case I can just leave that for Bitcoin JT 0.12...

@jtimon It's a philosophical point. The CTxMempool data structure IMHO should not make policy decisions at all. It shouldn't even know there is something like a size limit, it's just a data structure. It should provide methods to query its state and indexes and make mutations to it, but not be involved in what data should be in it. That's the caller's responsibility. That's just separation of concerns, and is easier to reason about if you know what responsibility is located where in the code. Just because moving a variable somewhere makes the code easier doesn't mean it's the right place for it.

As I said, ACK. I don't know a better solution right now, and I agree that the actual problem you're after (the circular dependency) needs to be solved.

@jtimon It's a philosophical point. The CTxMempool data structure IMHO should not make policy decisions at all.

And I agree. But we're very far from that. Right now CTxMempool has a CBlockPolicyEstimator, which is a policy class.
But I would like to understand your point about "a more generic memory limiting approach" better.

Just because moving a variable somewhere makes the code easier doesn't mean it's the right place for it.

The main goal is not simplifying the code but removing the circular dependency, and this is not the only way to do it. As said another option is to make it a global instead of an attribute of CTxMempool.
I'm fine doing that instead if that is preferred.

Well, ideally (but that would require significant changes, which I'm not suggesting now) is that the mempool has generic hooks that inform listeners about changes, and that the policyestimator registrers itself there. The code that manages the mempool (currently main) would tell the policyestimator what the memory limit is, if any. The -maxmempool option would only be parsed there.

Here's another option without the new attribute in CTxMemPool: master...jtimon:mempool-circular-dependency

If that is preferred, I'm happy with that too.

I believe I disagree with your ideal solution with generic hooks.
As said I would prefer to just make CTxMempool and CBlockPolicyEstimator completely independent, and only parse -maxmempool in CBlockPolicyEstimator's or CStandardPolicy's constructor (it would become an attribute instead of a global).
But that also requires more changes. Last time I did it, it included things as disruptive as #6068, so it's almost certainly too much for 0.12. Jokes aside, this is the less outdated preparations for the complete decoupling I have at hand in case anyone is interested https://github.com/jtimon/bitcoin/commits/policy-minrelayfee-0.12.99

NACK for now.

@sipa Why does this circular dependency have to be solved right now?

I'm all for cleaning up the code design, but I think the proper next step is to make the mempool unaware of the policy estimator not the other way around. I think it would make sense for the mempool to be aware of it's current state, so when TrimToSize is called with an argument, the mempool should remember what size it has now been trimmed to, thus GetMinFee would no longer need an argument. I think thats cleaner and makes more sense than having two TrimToSize functions just for testing purposes as in this pull. I'm happy to do that as soon as I'm in front of a computer again.

More importantly, the bigger problem with this pull is it has now put logic inside of the CTxMemPool::estimate functions which were previously just dumb pass throughs. When those functions go away (because the block policy estimator is used directly), then that logic will have to be repeated at all calling sites. I don't see why calling code should need to understand that you have to ask the mempool for its min fee in order to call estimate fee. I think all the logic thats required to do smart fee estimation should be contained in one place, and the policy estimator makes sense to me as that place.

Even further, future developments in the policy estimator are surely going to require it to be aware of the mempool, I just don't see a way around that. The most obvious next direct step is to asses how much current back log there is in the mempool and make sure a fee is not returned which is lower than could be reached in the target number of blocks.

These concerns have been raised with @jtimon previously, and it would been nice if this pull would have referenced them.

@morcos Ok, I hadn't considered that the estimator likely will need more access to the mempool anyway.

Seeing circular dependencies in the code still makes me cringe, though...

@sipa Why does this circular dependency have to be solved right now?

Because you just introduced it without the need to do so?
The question could be rather, why this circular dependency needed to be introduced in #6134?

I'm all for cleaning up the code design, but I think the proper next step is to make the mempool unaware of the policy estimator not the other way around.

I agree with that next step, but "the other way around" was already done until #6134. You don't have to chose one or the other, they can be completely independent.

I think it would make sense for the mempool to be aware of it's current state, so when TrimToSize is called with an argument, the mempool should remember what size it has now been trimmed to, thus GetMinFee would no longer need an argument

I highly dislike that "solution" (what is this solving exactly?), the default if not provided being "the same that was used in the last call" is a twisted interface.

More importantly, the bigger problem with this pull is it has now put logic inside of the CTxMemPool::estimate functions which were previously just dumb pass throughs. When those functions go away (because the block policy estimator is used directly), then that logic will have to be repeated at all calling sites

It is really dumb logic, but as you say that can be solved when those functions go away. For example, if GetMinFee() was part of the estimator rather than the mempool, it could be called internally CBlockPolicyEstimator::estimateSmartFee()

I don't see why calling code should need to understand that you have to ask the mempool for its min fee in order to call estimate fee. I think all the logic thats required to do smart fee estimation should be contained in one place, and the policy estimator makes sense to me as that place.

Agreed, but this has nothing to do with CBlockPolicyEstimator depending on CTxMemPool.

Even further, future developments in the policy estimator are surely going to require it to be aware of the mempool, I just don't see a way around that.

I've done it once, I will do it again, and I'm convinced it is completely possible to make the mempool and the policy fully independent no matter the functionality.
If you are skeptic, that's fine. But please don't put obstacles in the way just because you don't think is possible. I'll gladly rewrite all that whenever "is time" for #6068 and friends again (a signal I'm waiting to hear from @sipa, btw, since he insisted that policy encapsulation would get in the way of mempool work).

These concerns have been raised with @jtimon previously, and it would been nice if this pull would have referenced them.

I very explicitly and repeatedly referenced #6134 where those discussions where happening.
But for better reference: #6134 (comment)
#6134 (comment)

I should have been more explicit and just nacked #6134 instead of coding a nit to be ignored...

@sipa did you looked at the option without moving anything to mempool.o ?

I highly dislike that "solution" (what is this solving exactly?), the default if not provided being "the same that was used in the last call" is a twisted interface.

@jtimon but it's not a default, that's exactly what that attribute is meant to represent. It's used only for GetMinFee which only cares about how much the mempool has shrunk since the last time you trimmed it. If we ever changed to some algorithm with dynamic mempool sizes and we had an attribute for the new mempool size, we would also need a separate variable tracking the last size we were trimmed to. Would be nice if that variable were perhaps limited to the GetMinFee code itself I suppose.

@sipa I really disagree re: CTxMemPool having knowledge of its size limit, etc. Bitcoin Core has a number of places where we've made similar decisions and the result is a simple data structure that has almost no encapsulation with callers left to do all the work. The mempool limiting stuff as written already suffers from this and it adds a number of lines to main that could otherwise be hidden away from the already-monolithic code there.

On November 27, 2015 9:28:13 AM EST, Pieter Wuille notifications@github.com wrote:

Reluctant ACK.

Code ACK, agree about avoiding the circular dependency, and agree about
avoiding GetArg calls to determine the mempool max size the whole time.

I dislike moving more policy knowledge into the mempool itself (its
size limit), though. It will complicate a more generic memory limiting
approach, where there is not a single memory size limit, but one that
depends on the UTXO size and perhaps other buffers.

---

Reply to this email directly or view it on GitHub:
#7115 (comment)

Sure, I was commenting more generally, not really in response to this specific pull.

On November 29, 2015 2:28:12 PM EST, Pieter Wuille notifications@github.com wrote:

Heh, fully agree about all that. But I don't think we should have let a
circular dependency in, so absent a better plan to fix that, I'd be
fine
with at least temporarily localizing that information to the mempool.

It seems @morcos does have a better plan though, so let's go for that.

---

Reply to this email directly or view it on GitHub:
#7115 (comment)

So which of the two branches is preferred? The attribute or the global?
On Nov 29, 2015 8:30 PM, "Matt Corallo" notifications@github.com wrote:

Sure, I was commenting more generally, not really in response to this
specific pull.

On November 29, 2015 2:28:12 PM EST, Pieter Wuille <
notifications@github.com> wrote:

Heh, fully agree about all that. But I don't think we should have let a
circular dependency in, so absent a better plan to fix that, I'd be
fine
with at least temporarily localizing that information to the mempool.

It seems @morcos does have a better plan though, so let's go for that.

---

Reply to this email directly or view it on GitHub:
#7115 (comment)

—
Reply to this email directly or view it on GitHub
#7115 (comment).

Unfortunately we're talking about two separate issues addressed in this pull.

1. I have a different idea about how the size attribute should be stored in that I think the attribute should be called lastTrimmedSize and set from TrimToSize. This preserves a bit more flexibility for how we do mempool trimming in the future, but both my approach and Jorge's approach just eliminate repeated calls to GetArg("-maxmempool"). They don't solve the circular dependency.

2. Jorge and I both agree that CTxMemPool should not depend on the policy estimation code. This is a larger change not addressed in this pull. The other half of the circular dependency is where we disagree in that I actually think the policy estimation code should depend on the mempool. I don't particularly understand why its bad to temporarily have introduced this circular dependency if the side that was recently added is the side that we think will persist. But if everyone feels it has to be removed immediately, then the solution Jorge has for that in this pull (putting a tiny bit of the logic in the pass-through estimation functions) is probably as good as it gets. When we eliminate the other side of the dependency, then we can figure out how to fix that up.

@morcos Yes, it is two separate things, that's why I proposed master...jtimon:mempool-circular-dependency because what we're discussing is where to put the maxsize command-line option (not dynamic).
Right now it's in the global https://github.com/bitcoin/bitcoin/blob/master/src/util.h#L43

I initially moved it to a CTxMemPool attribute but @sipa complained so I proposed it to just move it to main.o like most of the other globals in that other branch. I could have left the GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000 all around, but I had already grep that...
I think we can all agree that doing it in init like most command line options is a reasonable and simple refactor.
It seems we disagree on whether this is passed too much as a parameter or it should be part of CTxMemPool or CBlockPolicyEstimator or CDefaultPolicy.
I think going from mapArgs to its own independent global is clearly a step forward.
But if people disagree, I can easily write a third patch that removes the circular dependency while maintaining all the GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000 verbosity.

2. Jorge and I both agree that CTxMemPool should not depend on the policy estimation code.

Yes, but it seems that your plan to do so is by making CBlockPolicyEstimator (and policy/policy.o?) dependent on CTxMemPool instead. This is not necessary.
And moving in that direction at the cost of creating a new circular dependency was clearly not necessary in #6134, and that's all what this PR is about.

We can discuss the future of relay/fee/mining policy code encapsulation. Although I won't be able to rebase/rewrite a branch completely decoupling CBlockPolicyEstimator and CTxMemPool anytime before 0.12 relase, but I assure you that you can always place the code wherever you want without affecting what the code itself does. In the meantime I'm happy to review any fee estimation/policy encapsulation specific PR you may have (but I warn you, I will always be adverse to gratuitously make CBlockPolicyEstimator dependend on CTxMemPool, and I firmly believe I can always proof that it is being done "gratuitously", just by providing a similar alternative that doesn't, like I did in #6134).
Policy and fee encapsulation is something of interest for both of us and that's great.
Finding out where we agree and disagree is, I believe, in our mutual interest.
But please let's find a solution for this PR first.

In bitcoin JT-0.12 CTxMemPool and CBlockPolicyEstimator will be completely independent, and I believe this may come with a performance improvement that I'm not sure will ever be closer than 6 commits away from bitcoin/master.
Closing, @morcos you won whatever we were fighting for.
Long live circular dependencies!

You're arguing that because it's possible to remove both dependencies, that
is what should be done.

No I'm arguing that because it can't be done and we disagree, I don't like that we're deciding a priori in going towards morcos preferred direction when it is clearly not necessary at this point.

And yes I disagree on making the estimator dependent on the mempool being a good design decision.
But if I'm alone there, let’s just move in favor of morcos without ever seeing the two competing options.

Sorry for the confusion, although @morcos disagrees with removing the circular dependency, he wasn't nacking the first commit in concept, he was just nacking it because I left some code on qt/coincontroldialog.cpp by mistake.
Updated the PR with the first commit properly separated from the others. I have left some more commits to comment on before definitely discarding them or squashing the uncontroversial ones.

e2a9957 - Alex was here
30ac1d3 - NACK
2e32923 - NACK
0dc47a0 - NACK

But if you would please consider 2b455fb then you could accomplish what you're trying to do with 30ac1d3 and 2e32923.

e2a9957 - ACK
Wow, spending a lot of code to avoid just letting the mempool have a "always limit to X" field. Unless someone wants to write code to make it actually clean, just add one. Making CTxMemPool a "dumb datastructure" with absolutely 0 knowledge of anything is a really poor goal IMO anyway.

Making CTxMemPool a "dumb datastructure" with absolutely 0 knowledge of anything is a really poor goal IMO anyway.

At the same time, unlike the max size of the mempool, nothing seems to be wrong about minReasonableRelayFee, lastRollingFeeUpdate, blockSinceLastRollingFeeBump and rollingMinimumFeeRate being created as attributes of CTxMemPool for some reason I'm still not able to understand.

But as said this PR was never about turning the global to an attribute, that was just one way to remove the circular dependency. But as shown removing the verbosity around "-maxmempool" is not strictly necessary to reach that goal: the first commit is enough.

I'm not interested in maintaing a PR to remove the circular dependency if it's not going to be a fast fix of the recently-merged #6134. This has proven not to be a fast fix PR and thus I have closed it. But the code is there quite fresh if anybody else wants to continue this.

utACK on e2a9957

Undecided about the others.