Sorry for this little change, but...

@Thuni was concerned about introducing globals in policy.h
fIsBareMultisigStd can be easily turned into an attribute later as shown in jtimon/bitcoin@policy_new...jtimon:policy_moveonly (in fact it could be done more easily not moving AreInputsStandard at the same time as IsStandardTx)
But minRelayTxFee is in many more places, and after proposing some changes to #5159, I am no longer convinced that I want to move things like IsDust to policy directly. One possibility would be to put all the fee policy stuff in the estimator. Then an interface of the estimator can be returned by CPolicy, for example, making a CPolicy::GetFeePolicy() analogous to CChainParams::GetConsensus().
The minimum fee can still be configured from the constructor of CStandardPolicy but it can pass it directly to the estimator without storing it redundantly.
In that case some files could be dependent on policy/fees but not in policy/policy (just like some functions depend on Consensus::Params but not on CChainParams), then some of the 5 includes I just removed would have to be changed for policy/fees later.

For all these reasons I'm keeping minRelayTxFee in main for now.

Moving minRelayTxFee to policy/policy could still prevent it from depending on main temporarily when IsStandardTx is moveonly, but it doesn't seem to be worth it.

Nodes can do random things, but why encourage them to adopt diverse relay policies, create custom policies, and define custom parameters for others to create more custom relay policies?

Shouldn't there just be baked-in policy parameters that have predictable effects when deployed with different values, like resource constraints and settings that influence the strength of preference for higher fees?

I'm not concerned about miner policies, but something more like CokePolicy and PepsiPolicy disrupting the network with divergent relay rules. What is the argument in favor?

@dgenr8 Non-centralisation. Authority to decide policy lies with the node operators and miners, and it's not a responsibility/power developers should have. Too much policy monoculture also creates incentives to use bad practices (trusting unconfirmed transactions) and make trouble for the outliers. Also, since the current policy monoculture is mostly reference policy, it also causes spam problems since there are no effective reference-suitable spam filters; and this is in partly out of necessity due to policy being intimately mixed in with the reference codebase.

I think there are different issues here.

One is that this is a step towards separating out policy code from consensus code. This PR is part of that. Eventually, we should end up with a situation where consensus is clearly separated in the source code (so we know when to be extremely careful in review and tests when someone wants to change it), and likewise have policy separated out (so people changing/forking the code know what can be safely changed).

And yes, I think that the ability to change policy is essential, for the reasons that @luke-jr brings up. For one, relay (and other) policy is constantly changing anyway (pretty much every Bitcoin Core release has changed what is being relayed). You should not rely on it being uniform on the network.

Of course, I think that we, as maintainers of a particular piece of Bitcoin network software, have a responsibility to make the code do what we think is best for the network - and possibly expose only part of the involved parameters to users. But that doesn't mean that others can't disagree. And separating out policy code makes it explicit exactly which part of the code is a choice.

@luke-jr The blockchain is the world's most consistent distributed system. It makes no sense to promote chaos right up to the assembly point. If we have CokePolicy and PepsiPolicy, reorgs too become less safe.

dgenr8 differences in consensus code can cause reorgs, differences in policy code can't. That's the main reason why we want to separate them. There are people already maintaining alternative policies and there's nothing wrong about it. Let's just make their implementations safer and cleaner. We will safe A LOT of review work by clearly separating policy proposals, which tend to be numerous, controversial and are also often uninteresting to many reviewers

@jtimon He means that when there are different mempool acceptance policies present, a reorg can reduce the (apparent) safety of transactions already in the blockchain.

And that is true - but that's just a sign that you shouldn't be relying on having a consistent policy in the first place, and why you should use confirmations to gauge risk, not mere acceptance.

@dgenr8 And the assembly point matters. There is a significant economic difference before and after. Miners get paid for what happens after, but not before.

@jtimon Nothing against code separation, but this mixes in a multi-relay-policy framework.

@sipa Use confirmations to gauge risk, but the system should try to reduce risk at all confirmation levels. Nobody has shown that lower confirmation levels, including 0-conf, cannot be made safer with easily acceptable tradeoffs. A multi-relay-policy framework is a step in the other direction.

@dgenr8 "Unconfirmed" is not a confirmation level at all. It is the absence of any confirmation. You cannot make it "safe" except through attempted centralisation and control over others.

@luke-jr That's a claim, not a proof, and rather than trying to prove a negative, why not think about how to accomplish the positive?

@dgenr8 feel free to spend your time on how to accomplish the positive.
In the meantime, policy diversity is what you get with a decentralized system. Multiple relay policies are already deployed and I don't think we can or should fight that.
In the meantime there's a lot of noise because we don't have support for different policies and because everybody wants their own favorite policy to be everyone else's policy too. Everyone wants his policy to be the standard one, but that's simply not possible.

Built, confirmed -policy option exposed, and checked msig tests still pass - ACK

Note: Had to pull in https://github.com/bitcoin/bitcoin/pull/6114/files, which should be OK as long as you're building with 1.58<

Needed trivial rebase after policy/fees got into the makefile.

Needed rebase.
Also, after #5669 minRelayTxFee will be the only dependency from main, so I feel more comfortable moving more code.
Added a couple of squashme commits and a last commit that removes the global @theuni was concerned about.
If it is ok to extend the scope of the PR (mostly with moveonly stuff), I will squash all the moveonly/rename commits into luke's commit (Luke-Jr was moving all 3 methods in other branches so I'm sure he will be ok with putting that on his name).

Needed rebase again.
I didn't squased the fixup commits introduced in the last push though since I was hoping to get some approval on that, but I will do it in the next forced rebased if nobody complains.
Also if people want me to s/"main"/Policy::STANDARD or something similar I'm happy to do so, I should have done it from the beginning. If I get silence about this as well I may do it at any point I feel like it.

Needed rebase.

ACK and well done with the switch to pure abstract class.
Current merge conflicts seem trivial, hope @laanwj has time to look at this and merge it.

No objection from me since this is now just encapsulation. Expire and TrimToSize are candidates for future inclusion too.

This is not just encapsulation, is is the creation of an API. Until this is resolved, it creates practical problems for compatibility. A policy written for 0.12 shouldn't quietly do something different in 0.13, so either the method name should be changed to AcceptTx_WIP, the class names changed with _WIP, or not merged until the desired interface for AcceptTx is implemented.

@sipa Overall, I agree. The parts I was thinking were fee policies, replacement decisions, rate limiting of gratis txns, ancestor/descendent limits, etc. I'm okay with waiting until mempool code stabilises more, but I don't think mempool code should be placing any limitations on what policy code does, and would consider such limitations to be a bug...

For example, the current mempool code has a feerate index which the size limiting relies on for correctness. It doesn't require that the feerate exactly represents fee per byte, but it does represent an ordering that has to be respected. Policy can reasonably override what a transaction's "fee" means and what its "size" means, but can't bypass or change the need for such an index in the first place. So no, I think it's unreasonable to say that the mempool can't place any limitation of what the policy can do. Policy is inherently just something that plugs into the other code, and I don't think there can be any expectation of a stable API. If the mempool code changes, what can be configured in it can change too.

It seems we have different ideas of what the policy class will do, and I'm afraid we can only decide that in incremental changes. If I understood @luke-jr correctly, he is not going to utACK unless I rename some of the methods, for example s/AcceptTxInputs/AcceptTxInputs_Incomplete. He agrees that we can AcceptTxInputsV2, AcceptTxInputsV3, etc when/if we change the method (for example, to include fee validation), but he would prefer to "not start counting until a reasonable baseline" (although didn't provide a definition for what a "reasonable baseline" is). We will have to move on without his utACK because I think it is too late for bike-shedding and he already got me to s/Validate/Accept in all methods.

@sipa It is true that we won't be able to finish the encapsulation while the mempool code is unstable, but we know that we want a policy interface and we know we want at least the code currently in policy/policy.cpp to be encapsulated behind this interface. We've known this for more than a year (and many more things like #5114 that are waiting for CPolicy to exist). This hardly conflicts with any of the work done in the mempool and if it does, it does it in a trivial-to-rebase way.

If it is decided to freeze policy encapsulation until "the mempool code is stable", fine. It is very frustrating to try to just with a simple class for policy encapsulation for more than one year (see #5071, #5180, #5595 among other related PRs...) with no success. In the meantime, my policy encapsulation research branches bitrot and I waste my time in the rebase mouse wheel. I could just stop wasting my time and stop any work on policy encapsulation (or at least, stop maintaining open PRs and start maintaining a per-major-release branch or something) until the "right time" arrives.

@laanwj should I rebase before 0.12 or just close it instead?

We will have to move on without his utACK because I think it is too late for bike-shedding and he already got me to s/Validate/Accept in all methods.

This is nonsense. I had made this point originally in #5071 (2014 Oct) which you based your work on initially. Even on this thread, I left the comment to that effect Aug 28, two days after you submitted the PR. Also, understandable API interfaces is not bike-shedding. Finally, it is never too late to address things in an unmerged PR, much less to "move on without...utACK" simply because you don't want to address them.

You are right, it is never too late to change things. But I've tried my best to understand how renaming a method is not bike-shedding and how AcceptTxInputs_Incomplete, AcceptTxInputs, AcceptTxInputsV2 is better than AcceptTxInputs, AcceptTxInputsV2, AcceptTxInputsV3 without success. I've been reducing the scope of the policy PRs (and adapting them from yours and other people's feedback) since I started and now suddenly "it is incomplete". If we cannot do this work incrementally I'm out, because I already know that my multi-commit branches will be considered to hard to review. #5071 was also incomplete: we can't do this perfectly in a simple one commit PR, that's simply not possible.
Take into account that I started implementing alternatives to #5071 because I didn't agree with everything that was there. Let's find out where we can agree easier first and then discuss the other more fundamental disagreements. But if our only disagreement and the only reason why you don't utACK this is the name of a method, then I'm fine not having your utACK (which otherwise I find very interesting because I know you've worked on policy encapsulation as well). Of course, I'm assuming that you won't nack this for a method name either, and if you do I assume your nack can be ignored.
No, this is not will be a final API, this is a tiny first step that will need more steps after it. That doesn't mean any of the current method names are bad choices for the first version of the API.

I disagree with the goal of making Policy a (stable) API. It's a way to separate configurable behaviour from non-configurable. What is configurable will depend on what the code calling it is doing.

@sipa Rather than a stable API, I just don't want the same policy to compile for both 0.15 and 0.16 without any changes and end up with wildly different behaviour because the meaning of the API changed without the method names reflecting it properly.

@luke-jr We can fix that by changing the names when appropriate (for example with V2, etc if no new name comes to mind).

@sipa I agree the goal it's not a stable API (and if it was we're far away from it anyway and that goal cannot be the scope of this PR). But do you anticipate any mempool PR to change any of the code currently in policy/policy any time soon?
I really believe this little change does not worryingly conflict with any work being done in the mempool (even if this changes AcceptToMemoryPool in a trivial way).

Since it doesn't seem like this will make it into 0.12, I'm closing it for now instead of rebasing it. Sigh.

re-ACK (was rebased since last ACK)

closed forever

@jtimon why?

For #6423 and more things on top of that.
Anyway, Bitcoin Core has been consistently rejecting this change (look at the previous PRs) for longer than a year, so I'll stop insisting. If you are interested more of this will be maintained in my own software fork, which will have 0.12 as its first release.

wooo. lets talk about that...

@laanwj are we interested in this? IMHO it makes the idea of node 'policy' clear and shows that IsStandard is simply just a default policy.
This definitely seems like something we should encourage.