As mentioned repeatedly in #6134, the PR request has introduced an unnecessary circular dependency:
Befere CTxMemPool depended on CBlockPolicyEstimator, but CBlockPolicyEstimator didn’t depend on CTxMemPool until #6134 was merged. The first commit would have been practically free diff-wise if it had been squashed in #6134. The second are additional simplifications that wouldn’t have been free, but include here for discussion.
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.
@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.
Here’s another option without the new attribute in CTxMemPool: https://github.com/bitcoin/bitcoin/compare/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.
@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 (review) #6134 (comment)
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)
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.
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.
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.
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 https://github.com/bitcoin/bitcoin/compare/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.
- 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.
@morcos was suggesting to reverse the dependency that existed from mempool on policy estimator, because it is more natural, and I agree. I don’t like that this (temporarily, apparently) resulted in a circular dependency.
You’re arguing that because it’s possible to remove both dependencies, that is what should be done. @morcos complained that it means moving the responsibility of knowing there is something like a mempool limit to every caller of the estimation code. I was fine with that approach as a simple solution for now, but do you not agree it’s natural for a mempool estimator to depend on the mempool, instead of making callers responsible for telling one about the other?
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.
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.
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 https://github.com/bitcoin/bitcoin/commit/e2a99575dff063471d033a0c18e1f8c0af0b53f9
Undecided about the others.
Labels
Refactoring