@jtimon this is another step I wanted to take in the direction of getting policy stuff more separated.
The first commit heavily borrows from your work to separate policy, but I was aiming for organizing more around fee/relay related stuff as opposed to transaction standardness. It wasn’t clear to me if those should all go together.
It touched a lot of files to get minRelayTxFee separated out, but now it should be easier to add other options.
This code is an adaption of jtimon's commit 3d03f15 for turning policy globals into attributes of a policy class.
This fails Travis because checkdoc.py doesn’t recognize the way I’m adding the help messages now. I used @jtimon’s code that builds a vector of string pairs. I’ll wait for some direction on whether that is a refactor thats wanted and we need to change the check script or whether I should just hack in the help messages the old fashioned way.
As a side note, it wasn’t clear to me how to easily find what had failed Travis.
AppendMessagesOpt. I do really like moving some of the policy configuration out of main and into policy code. I think there could be a lot of other command line options moved afterwards if we like this idea. I tried to follow the style @jtimon had used since he seems to be most involved in refactoring work. I’m happy to rewrite that part of this pull to be whatever is most preferred.
Would you consider using https://github.com/jtimon/bitcoin/commit/6de4f556884a4e362b20baae165271ce8ce7893f ?
Althought this has many similarities with #6423 and some with https://github.com/jtimon/bitcoin/commits/policy-minrelayfee-0.12.99 , I’m having a hard time undesrtanding the differences.
The reason for InitFromArgs and AppendMessagesOpt (apart from code encapsulation) is a feature that @luke-jr requested in #6068, #5595 or some other of the multiple failed attempts at creating a policy class, and it is exlained on the widely-ignored #6423:
“Also (although I’m not going to continue that GUI part), making policy options GUI-configurable (and hopefully, in the future all new existing and new options “for free”) would be very nice too. The way I’ve understood this from @luke-jr, with Policy::AppendHelpMessages() and CPolicy::InitFromArgs() it is relatively easy to create a generic GUI that doesn’t require any more work outside those methods to introduce new policy attributes.”
Since here the methods are modified to not allow such feature, it is indeed somewhat surprising that the methods have been maintained very similar nonetheless.
Other things I don’t understand are the elimination of the CPolicy abstract class from #6068 (I thought the whole point of encapsulating the policy code under an interface was to make it easier for people to maintain multiple implementations of that interface in parallel), and the rename to MempoolPolicy.
You say
It wasn’t clear to me if those should all go together.
What do you have in mind instead? Is there any longer branch using this that I can see so that I can see “the big picture” of how you intent to use this in the future? If #6423 was rebased, reviewed and merged before this PR, what would be the relationship between MempoolPolicy and CPolicy? Is the goal to have 2 different interfaces for policy, or that MempoolPolicy simply extends the implementation CStandardPolicy (I meant to rename that to CDefaultPolicy)?
What will be the relationship between MempoolPolicy, CBlockPolicyEstimator and CTxMemPool ?
@jtimon I need to do some work around minRelayTxFee because it is broken now in CTxMemPool (supposed to respect the command line option but is hard coded) and I was having same problem in the fee filter code. So I’d like to get something merged to do this policy encapsulation so we can fix that properly and probably add a second variable for the incremental relay rate.
I have no strong opinions about the way to do this encapsulation. I spent a little bit of time looking for updated versions of your encapsulation code and wasn’t sure where your latest thinking was. The reason I broke off mempool policy separately is I thought it was smaller, touched less thnigs and might be easier to get merged. I’m more than happy to follow your lead if you want to do this differently.
Edit: To answer your last question I was assuming both CTxMemPool and CBlockPolicyEstimator would depend on MempoolPolicy which would not depend on either of them.
The reason I broke off mempool policy separately…
Another option would be to start CPolicy and CDefaultPolicy like in #6423 , but just with the GetMinRelayFeeRate() method instead of ApproveTx [ from IsStandardTx ] and ApproveTxInputs [from AreInputsStandard ]. And doing it in policy/policy, no need to have two separate files and classes.
I’m assuming this minRelayFee will remain static for use in CDefaultPolicy::ApproveTx() and dust checks (/me remembers #5114, https://github.com/jtimon/bitcoin/commit/dc1b96d368d471ea5242ca021148f9f900f69661 , https://github.com/jtimon/bitcoin/commit/5f519c281c353b0bba8ed06421bf4b5b39b5ebbb and a long, etc, and sighs…).
If it’s something planned to be dynamic I believe the estimator would be a better place.
I would also maintain the previously mentioned feature of making it easy for the GUI layer to dynamically create forms to configure all the policy options. I assume you removed the parameter in InitFromArgs() because you didn’t knew what was about.
To answer your last question I was assuming both CTxMemPool and CBlockPolicyEstimator would depend on MempoolPolicy which would not depend on either of them.
IIRC you previously wanted the policy to depend on CTxMemPool. Great, this is much closer to what I want. It is important to make clear that this is completely a choice, nothing strictly needs to depend on anything else.
My ideal choice would be:
My preference is that CDefaultPolicy extends CBlockPolicyEstimator (see https://github.com/jtimon/bitcoin/commit/f64df9fcb210f43302c0d1242bf2fa221a77bd32 ), that is between the 2 they fully implement the CPolicy interface, but CBlockPolicyEstimator is, in principle, an abstract class as it doesn’t implement all the pure virtual methods in CPolicy.
Neither CBlockPolicyEstimator nor CDefaultPolicy would depend on CTxMempool, but CTxMempool would initially depend on the CPolicy interface.
Then decouple CTxMempool from CPolicy (see #7145 ).
That way implementors of alternatives to CDefaultPolicy don’t need to know or touch CTxMempool’s internals to implement their own policies. If I remember correctly, you were very skeptical of this even being possible or being compatible with clean, readable and extensible code, but that’s what I’ll do for Bitcoin JT (taking things from https://github.com/jtimon/bitcoin/commits/policy-tip-0.12.99 #7145 and https://github.com/jtimon/bitcoin/commits/policy-minrelayfee-0.12.99 which are the least-outdated most-complete branches I have on policy) anyway.
We really don’t need to decide that now, my fear was that you wanted to separate MempoolPolicy from CPolicy and CDefaultPolicy because you knew I didn’t want CDefaultPolicy to depend on CTxMemPool, but maybe I was ok with a separate MempoolPolicy depending on CTxMemPool. I prefer a single policy interface which takes CTxMemPool as a param to some of its methods (as said this is not necessary and I don’t like it), than 2 separated policy interfaces, even if one of them is fully decoupled from CTxMemPool. Anyway, that question has been answered. Summary of nits.
const CChainParams& chainparams).
Labels
Mempool