Policy: Create CPolicy interface and CStandardPolicy class implementing it

jtimon commented at 2:31 pm on April 26, 2015: contributor

Minimal changes to create a CPolicy interface and a CStandardPolicy implementation. No policy globals are encapsulated as CStandardPolicy attributes yet. CPolicy is not made a parameter of main::AcceptToMemoryPool() yet. No factory (to select another implementation other than CStandardPolicy) nor -policy= runtime argument are created yet.

Outdated:

This is a reopen of #5595. Even though I knew I had to reopen it before force-pushing to it, I forgot about it. I think I solved this problem once by pushing the old branch back, reopening and then pushing the new branch, but the first step doesn’t seem to be working. I’m happy to close this and reopen #5595 if somebody gives me a solution.

Text from the other PR:

First steps for encapsulating the policy code. An interface (abstract class) CPolicy and a concrete implementation CStandardPolicy are created. “Users” (people capable of modify and build Bitcoin core) can implement alternative policies and select them with the option -policy=<policy_name>. They can define new policy options and make their help messages be accesible to the users without having to touch init.cpp, only modifying policy.cpp is enough for all this. The help messages can also be accessed (per available policy) as a vector of string pairs to make it easier to implement a GUI to configure those options (although I don’t plan to do that myself). As more parts of the policy code move to policy.cpp, this encapsulation gets more useful.

To start using it. The function script/standard.o::IsStandard() is turned into a method: ApproveScript(). Many more policy-related improvements can be cleanly proposed after this first steps are merged.

jtimon force-pushed on Apr 27, 2015

jtimon commented at 10:55 pm on April 27, 2015: contributor

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 https://github.com/jtimon/bitcoin/compare/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.

jtimon force-pushed on Apr 27, 2015

laanwj added the label Improvement on Apr 29, 2015

dgenr8 commented at 3:50 pm on May 1, 2015: contributor

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?

luke-jr commented at 5:06 pm on May 1, 2015: member

@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.

sipa commented at 6:06 pm on May 1, 2015: member

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.

dgenr8 commented at 4:09 pm on May 2, 2015: contributor

@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.

jtimon commented at 4:21 pm on May 2, 2015: contributor

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

sipa commented at 4:27 pm on May 2, 2015: member

@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.

dgenr8 commented at 5:28 pm on May 2, 2015: contributor

@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.

luke-jr commented at 5:33 pm on May 2, 2015: member

@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.

dgenr8 commented at 5:45 pm on May 2, 2015: contributor

@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?

jtimon commented at 11:15 am on May 7, 2015: contributor

@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.

ghost commented at 5:48 pm on May 10, 2015: none

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<

jtimon force-pushed on May 13, 2015

jtimon commented at 10:49 pm on May 13, 2015: contributor

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

jtimon force-pushed on May 27, 2015

jtimon commented at 3:52 pm on May 27, 2015: contributor

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).

jtimon force-pushed on Jun 2, 2015

jtimon commented at 11:36 am on June 2, 2015: contributor

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.

in src/policy/policy.cpp: in d21542c710 outdated

53+
54+static CStandardPolicy standardPolicy;
55+
56+static CPolicy* pCurrentPolicy = 0;
57+
58+CPolicy& Policy(std::string policy)

Diapolo commented at 11:52 am on June 2, 2015:

Nit: Should be a constant reference.

jtimon commented at 12:05 pm on June 2, 2015:

Please, make it const and try to compile. I’m working on a solution (using a real factory and a container) to make it const, though. But I will propose that for chainparams first.

Diapolo commented at 12:14 pm on June 2, 2015:

This was just a read-only review of the code, what error is shown?

in src/policy/policy.cpp: in d21542c710 outdated

60+    if (policy == "standard")
61+        return standardPolicy;
62+    throw std::runtime_error(strprintf(_("Unknown policy '%s'"), policy));
63+}
64+
65+void SelectPolicy(std::string policy)

Diapolo commented at 11:53 am on June 2, 2015:

Nit: I guess this also could be a constant reference.

jtimon commented at 12:34 pm on June 2, 2015:

Nobody is doing it with strings, I believe the compiler can be smart enough by itself in this case but I’m not 100% sure. @sipa ?

Diapolo commented at 12:43 pm on June 2, 2015:

Take a look at our codebase or even at #6206. IMHO we shouldn’t work with copies where not needed.

in src/policy/policy.cpp: in d21542c710 outdated

76+std::string GetPolicyUsageStr(std::string selectedPolicy)
77+{
78+    CPolicy& policy = standardPolicy;
79+    try {
80+        policy = Policy(selectedPolicy);
81+    } catch(std::exception &e) {

Diapolo commented at 11:54 am on June 2, 2015:

Nit: I’m sure we use const here, too.

in src/policy/policy.cpp: in d21542c710 outdated

85+    strUsage += HelpMessageOpt("-policy", strprintf(_("Select a specific type of policy (default: %s)"), "standard"));
86+    strUsage += HelpMessagesOpt(policy.GetOptionsHelp());
87+    return strUsage;
88+}
89+
90+void InitPolicyFromArgs(const std::map<std::string, std::string>& mapArgs, std::string defaultPolicy)

Diapolo commented at 11:56 am on June 2, 2015:

Nit: defaultPolicy could also be const std::string&.

in src/init.cpp: in d21542c710 outdated

788@@ -787,6 +789,13 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
789     if (nConnectTimeout <= 0)
790         nConnectTimeout = DEFAULT_CONNECT_TIMEOUT;
791 
792+    // Init Policy
793+    try {
794+        InitPolicyFromArgs(mapArgs, "standard");
795+    } catch(std::exception &e) {

Diapolo commented at 12:00 pm on June 2, 2015:

Nit: catch(const std::exception& e)

in src/policy/policy.cpp: in d21542c710 outdated

81+    } catch(std::exception &e) {
82+        selectedPolicy = "standard";
83+    }
84+    std::string strUsage = HelpMessageGroup(strprintf(_("Policy options (for policy: %s):"), selectedPolicy));
85+    strUsage += HelpMessageOpt("-policy", strprintf(_("Select a specific type of policy (default: %s)"), "standard"));
86+    strUsage += HelpMessagesOpt(policy.GetOptionsHelp());

Diapolo commented at 12:02 pm on June 2, 2015:

Suggestion: The string “standard” is used quite often, I guess it could be nice to have some DEFAULT_ constant for it.

jtimon commented at 12:36 pm on June 2, 2015:

This is mentioned in the thread, in the last post actually.

Diapolo commented at 12:45 pm on June 2, 2015:

So you are asking for the way to got with this, do I get that right?

jtimon force-pushed on Jun 2, 2015

jtimon commented at 4:37 pm on June 2, 2015: contributor

Fixed most nits and squashed.

sipa commented at 5:51 pm on June 2, 2015: member

Passing a const reference to a string should be slightly more efficient. But std::string implements reference counting I believe, so copying a string does not require copying the entire data blob, only the (iirc) 24 or 32 byte header structure. Passing a cost reference only needs 8 bytes though.

jtimon commented at 7:39 am on June 3, 2015: contributor

Mhmm, I’m not able to reprduce https://travis-ci.org/bitcoin/bitcoin/jobs/65112154 (no wallet) locally… @laanwj do you know if that may e related to the travis cache issue or is it unrelated?

jtimon force-pushed on Jun 4, 2015

theuni commented at 11:50 pm on June 5, 2015: member

Could you outline the plan for next steps after this goes in? Standard flags/values are still hard-coded in several places, so I’m not sure that exposing the “-policy” option makes sense until it’s possible to actually run with a different policy.

jtimon force-pushed on Jun 6, 2015

jtimon commented at 6:58 pm on June 6, 2015: contributor

Could you outline the plan for next steps after this goes in?

Reopened #5180 as one of the things that I consider an important next step (although @luke-jr wasn’t very happy with it the first time I proposed it). Other “next steps” (like #5114 ) are related to fees.

Standard flags/values are still hard-coded in several places, so I’m not sure that exposing the “-policy” option makes sense until it’s possible to actually run with a different policy.

Well, it will take some time until all standard flags/values are encapsulated here (specially if we never take a first step), and some things will be harder to discuss than others. But I think the option is useful already. Advanced users (say, miners) can trivially implement their own CCustomPolicy and select it with the option (-policy=custom) without breaking any tests or risking consensus. As we encapsulate more policy code in CStandardPolicy, the option will become more useful as the custom policy can modify more things. I could always leave the -policy option for #5180 but as said I think it’s already useful to make it easy for the advanced user to implement and select their own policy.

jtimon force-pushed on Jun 8, 2015

jtimon commented at 3:22 am on June 8, 2015: contributor

Updated code:

But I think the option is useful already. Advanced users (say, miners) can trivially …

Thinking more about this…if we’re assuming advanced users and they want to preserve the standard policy in their build and not simply replace it, they can also trivially add the new option themselves, so there’s no reason to expose the new option until it has any use. So I starded reducing the scope of this PR.

Since I was touching the initialization parts, I also realized that @theuni was probably more concerned about was introducing globals in policy.cpp, something we’re trying to fix for chainparams. I previously said to @luke-jr that we shouldn’t rely on a policy global, but a global-returning-pseudo-factory function doesn’t solve that: only passing CPolicy as parameter to the functions that depend on it will solve that. My concern with luke’s approach wasn’t with the global itself, but with the fact that he was exposing and using a different for CStandardPolicy than for CPolicy. By hidding CStandardPolicy I’m sure I’m respecting advanced users and I’m happy to remove any additions to the “first step” (there were previous “first steps” before this “first step”). Actions:

Remove all globals from policy.cpp and the new -policy option, hopefully pleasing @theuni
Restore the simple global in main.cpp for the policy, just this time is compatible with CStandardPolicy being hidden by using a Factory. Hopefully this will please @luke-jr .

I think #5180 is a perfect example for motivating this PR. Even if it’s not accepted by itself, it’s a simple example of how an advanced user could write his own custom policy (although removing that CChainParams dependecy from AcceptToMemoryPool should even be nicer for an example).

jtimon force-pushed on Jun 17, 2015

jtimon commented at 9:30 am on June 18, 2015: contributor

Rebased

jtimon force-pushed on Jun 21, 2015

jtimon force-pushed on Jun 23, 2015

jtimon force-pushed on Jun 24, 2015

jtimon commented at 5:18 pm on June 24, 2015: contributor

#6335 has been separated from this PR. Please let’s focus on merging that first. An updated version of this is coming soon…

jtimon force-pushed on Jun 24, 2015

jtimon force-pushed on Jun 25, 2015

jtimon renamed this:
~~Policy: Create CPolicy interface and CStandardPolicy class implementing it~~
DEPENDENT: Policy: Create CPolicy interface and CStandardPolicy class implementing it
on Jun 25, 2015

jtimon commented at 5:00 pm on June 25, 2015: contributor

The scope has been reduced again. @faizkhan00 I’m sorry, but now the -policy option is left for #5180. Also, no globals are created since they’re not needed (yet, although I would happily put them back if nobody opposes). @theuni to discuss a solution for the globals that could be reused for chainparams, there’s a longer branch in https://github.com/jtimon/bitcoin/tree/policy-global-0.11.99

jtimon force-pushed on Jun 26, 2015

jtimon commented at 4:06 pm on June 26, 2015: contributor

Rebased

jtimon force-pushed on Jul 1, 2015

jtimon force-pushed on Jul 6, 2015

jtimon commented at 10:02 pm on July 6, 2015: contributor

Rebased

jtimon renamed this:
~~DEPENDENT: Policy: Create CPolicy interface and CStandardPolicy class implementing it~~
Policy: Create CPolicy interface and CStandardPolicy class implementing it
on Jul 6, 2015

jtimon force-pushed on Jul 7, 2015

jtimon commented at 11:31 am on July 7, 2015: contributor

Reduced the scope again to a single commit. This cannot get simpler, so hopefully both @luke-jr and @theuni will be satisfied with this version (we can discuss other things where it seems there’s still disagreements better after this has been merged).

jtimon commented at 1:08 pm on July 7, 2015: contributor

Well, I added back the example attribute commit since it was mostly reviewed.

jtimon force-pushed on Jul 7, 2015

jtimon commented at 9:00 pm on July 7, 2015: contributor

After a talk with @luke-jr on IRC, I think I understand his vision better. The “having a class named CStandardPolicy is a bug, because there shouldn’t be any standard policy” part is not clear to me yet though. If we want to make it simple for coders to implement and maintain custom policies (why would we have an interface and an implementation class otherwise?), then we need a name for the class implementing the interface. He doesn’t oppose to a -policy option either: he just wants to use “-acceptnonstdtxn” instead of “-policy=test”. Which is fine. I can always create a PR with “-policy=example_custom” or something (although “-policy=test” decouples policy/policy from chainparams, but let’s leave that for later).

About the prohibition of “-acceptnonstdtxn=0 -testnet=0 -regtest=0”, he doesn’t cares, but he feared that #6329 would be rejected otherwise. As discussed with @petertodd in #6329 (review) , I really believe that the whole point of exposing policy options is to allow users to select them, and I don’t think we want to maintain the ugly initialization complexity burden of knowing which policy options are prohibited for each particular chain. That’s why I added f2e8fbd1 at the end, but if we can’t arrive to a conclusion on this fast I will leave it for later instead of squashing it. 5c9c0bfc may seem bikeshedding but it’s just taking the fest opportunity to fix a little mistake in #6329. I’ll wait for review before squashing or discarding the fixup commits.

jtimon force-pushed on Jul 8, 2015

jtimon commented at 11:34 am on July 8, 2015: contributor

Updated with several minor improvements and squashed “fixup! RENAME: !fRequireStandard to fAcceptNonStdTxn (for “-acceptnonstdtxn”)”. So now there’s only one fixup commit “waiting for approval”.

jtimon force-pushed on Jul 11, 2015

jtimon commented at 6:39 pm on July 11, 2015: contributor

Policy::AppendHelpMessages() and CPolicy::InitFromArgs(), the removal of fIsBareMultisigStd and fRequireStandard globals from main.o, the removal of the unnecessary restriction on -acceptnonstdtxn and CStandardPolicy are left for #6423. It doesn’t seem like anyone is interested in acking or discussing those things here anymore (or for now) so I’m really minimizing the scope of this PR to something that it’s so trivial I could not imagine anyone having a nit for it.

jtimon force-pushed on Jul 11, 2015

jtimon force-pushed on Jul 12, 2015

jtimon force-pushed on Jul 22, 2015

jtimon commented at 11:23 pm on July 22, 2015: contributor

Even though I minimized the scope of this PR to make it easily-mergeable, trvially-reviewable/rebasable and nit-unfriendly, @theuni’s sharp review found something that forced me to update this:

IsStandard/CPolicy::ApproveScript only needs to expose the output parameter txnouttype& whichType internally for CPolicy::ApproveTx, so that can be a private/protected method. This improvement doesn’t make the PR less mergeable or reviewable, and since no acks/nacks where received since a minimized the scope I thought it was a no brainer to include it.

Now I’m 99% that further non-bike-shedding (and I already s/Validate/Approve) nits are possible at all.

The biggest subset of what has been moved out is still being kept rebased in #6423 (including CPolicy::GetOptionsHelp() which will supposedly help @luke-jr create qt forms for configuring these options dynamically [when you show me how to do that I will want to do the same for all the options, not just the policy ones]), but it will be closed (for depending on this one) as soon as I know what travis thinks about it and until this is merged.

theuni commented at 11:43 pm on July 22, 2015: member

ACK

in src/policy/policy.h: in d227be961a outdated

41@@ -42,17 +42,26 @@ static const unsigned int STANDARD_SCRIPT_VERIFY_FLAGS = MANDATORY_SCRIPT_VERIFY
42 /** For convenience, standard but not mandatory verify flags. */
43 static const unsigned int STANDARD_NOT_MANDATORY_VERIFY_FLAGS = STANDARD_SCRIPT_VERIFY_FLAGS & ~MANDATORY_SCRIPT_VERIFY_FLAGS;
44 
45-bool IsStandard(const CScript& scriptPubKey, txnouttype& whichType);
46+/**
47+ * \class CPolicy
48+ * Generic interface class for policy.

gavinandresen commented at 2:40 pm on July 23, 2015:

Bad comment. How about instead:

Non-consensus-critical policy logic, like whether or not a transaction should be relayed and/or included in blocks created.

jtimon commented at 0:04 am on July 24, 2015:

I really want to use the word “interface” (because the plan is to have a separated implementation class later and make this one a dumb interface that just always approves everything). But better documentation is always welcomed: I will certainly add your suggestion.

gavinandresen commented at 2:44 pm on July 23, 2015: contributor

Code review ACK; changes are trivial enough I trust the unit tests / Travis for testing correctness.

jtimon force-pushed on Jul 24, 2015

jtimon commented at 0:13 am on July 24, 2015: contributor

Updated with @gavinandresen ’s improvements to the doc.

in src/policy/policy.h: in 3ed911747a outdated

50+ */
51+class CPolicy
52+{
53+    bool ApproveScript(const CScript&, txnouttype&) const;
54+public:
55+    bool ApproveScript(const CScript&) const;

jtimon commented at 0:34 am on July 24, 2015:

By the way, documentation for the public version of CPolicy::ApproveScript is also welcomed. Specially with this PR where it’s a 100% merge-conflict-free to add it (nobody building on top of bitcoin/master will notice the difference).

jtimon commented at 4:08 pm on July 24, 2015: contributor

I’m sorry about suggesting this after getting some ACKs, but @theuni ’s nit made me realize that I went too far with the reduction of the scope. We don’t need to wait for having attributes (#6423) to separate the interface CPolicy from the reference implementation CStandardPolicy (but I wanted to leave that out because @luke-jr seems to have a problem with using the term “standard” in the reference implementation [I’m happy to paint it CReferencePolicy if that helps]). Now I’m looking at that private method in CPolicy and thinking that a purely-interface class like CPolicy is intended to be should never have private methods, call me purist if you want. By no means I want to delay this (I have plenty of code waiting for this [or that could become cleaner after this]), but if it’s not going to do it, I would be happy to squash the latest fixup commit I added (note that the delta to the total diff count is really small [even moving some documentation from the cpp to the .h, my mistake for thinking that we didn’t even wanted CStandardPolicy in policy.h, but hidden in the .cpp]). If it’s going to delay this, I’m happy leaving it for an independent PR, #6423 or just any other PR that depends on the separation, whatever is preferred.

theuni commented at 7:40 pm on July 24, 2015: member

@jtimon Grr, you already had ACKs and this was surely just a day or two from going in. This constant scope-changing is impossible to keep up with.

I don’t have a preference, with or without the new commit is fine. But please, pick one and leave it alone.

jtimon commented at 9:35 pm on July 25, 2015: contributor

But please, pick one and leave it alone.

I’ll let @laanwj pick between squashing it or taking it out and leave it alone. But yes, sorry for the changes in scope.

jtimon force-pushed on Jul 28, 2015

jtimon commented at 9:03 pm on July 28, 2015: contributor

Removed the fixup commit with @laanwj ’s nack and squashed the rest.

jtimon force-pushed on Aug 21, 2015

jtimon commented at 2:09 am on August 21, 2015: contributor

Needed rebase.

in src/main.cpp: in e743d04ced outdated

795@@ -794,7 +796,7 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
796 
797     // Rather not work on nonstandard transactions (unless -testnet/-regtest)
798     string reason;
799-    if (fRequireStandard && !IsStandardTx(tx, reason))
800+    if (!policy.ApproveTx(tx, reason))

luke-jr commented at 2:31 am on August 28, 2015:

IMO more of AcceptToMemoryPool should be included in ApproveTx. It doesn’t have to be immediately, but at the very least, the policy.h docs should mention that the API is not final yet, and further changes are planned. Ideally, the function should get a name like ApproveTx_Partial so that when it is completed, it can be renamed to break the API explicitly.

jtimon commented at 2:37 am on August 28, 2015:

Yes, this is a work in progress. I’m not sure what you have in mind for ApproveTx but I would certainly like to include more things in ApproveTxInputs. In any case, one step at a time.

I don’t think we need to mention that the API is not final because it may never be final. Other work in progress (for example, libconsensus) doesn’t clarify “the API is not final”. In any case, if more people think we should clarify that, I’m happy to include more things in the documentation.

luke-jr commented at 2:52 am on August 28, 2015:

Part of the point of libconsensus and policy abstraction is so that the API will some day be final and people can rely on their modifications working when they upgrade the main codebase…

jtimon commented at 2:56 am on August 28, 2015:

I almost agree for libconsensus (soft/hardforks may require a change in the API though). For policy changes are more likely to be expected (ie someone asks for a change in the API so that it’s possible to support some new policy).

luke-jr commented at 11:50 pm on September 3, 2015:

Such changes should be backward-compatible with old policies whenever possible, or at the very least cause a compile failure.

jtimon commented at 0:03 am on September 4, 2015:

I think encapsulating policy globals as CStandardPolicy attributes and being able to support 2 different policies with different configuration arguments (you didn’t like testpolicy so I will go with randomPolicy) as soon as possible is much more of a priority for me than being backward-compatible with “old policies” (I think you really meant “your branches” there). Anything else can be rebase on top of that and multiple parallel policies will allow implementators to not conflict with policies they don’t care about. If you are too lazy to do so, I can do it for you at any point in time: just point me to your longest branch and to the PR you are going to utACK if I do that work (for example, this one).

luke-jr commented at 0:18 am on September 4, 2015:

No, I don’t mean my branches. I mean that policies written today should not break or behave differently in subtle ways tomorrow.

jtimon commented at 3:43 pm on September 4, 2015:

It won’t break in subtle ways: they will break hard and require rebase or they won’t. More of AcceptToMemoryPool can be moved to policy later and changing the “API” will be unavoidable if we want to start with something simple like this. Another possibility is keep on complaianing about minor details of a trivial PR and wait another full year to have a policy class. Please, if you don’t like this PR exactly as it is you can code your particular nits instead of giving vague complains like this one.

in src/policy/interface.h: in e743d04ced outdated

24+    virtual bool ApproveScript(const CScript& scriptPubKey) const { return true; };
25+    /**
26+     * Check for standard transaction types
27+     * @return True if all outputs (scriptPubKeys) use only standard transaction forms
28+     */
29+    virtual bool ApproveTx(const CTransaction& tx, std::string& reason) const { return true; };

luke-jr commented at 2:32 am on August 28, 2015:

Probably any default should call ApproveScript for at least the outputs… maybe easier to just not have defaults at CPolicy for now?

jtimon commented at 2:43 am on August 28, 2015:

Initially it was a pure abstract class but if I remember correctly somebody complained about it, and it’s more useful for alternative policies that don’t want to restrict certain things for the base class to always return true in all Approve_ methods. CPolicy is not the default policy, CStandardPolicy is. CPolicy is just the interface. It just happen to also implement the simplest policy possible: never reject anything on policy grounds.

luke-jr commented at 2:53 am on August 28, 2015:

But if a subclass overrides ApproveScript, it would be unexpected for ApproveTx to bypass that.

jtimon commented at 3:01 am on August 28, 2015:

If they extend CPolicy directly, they shouldn’t expect ApproveTx to call ApproveScript unless they explicitly do so themselves. They can extend CStandardPolicy too tough (which I expect most alternative policies to do, unless they have radically different configurable options). In that case they can expect anything they don’t override to act like it does in CStandardPolicy.

luke-jr commented at 11:49 pm on September 3, 2015:

I don’t agree. Either there should be no default behaviour at all, or it should be reasonably expected default behaviour.

jtimon commented at 11:56 pm on September 3, 2015:

There is one default behavior: accept everything that follows the consensus rules. I can turn CPolicy back into a pure abstract class, though I’m afraid @sipa @laanwj @theuni or @gmaxwell may have issues with a pure abstract class because it may be worse for performance. I’m fine with either of the two options (not with implementing anything “reasonable” in CPolicy): either CPolicy accepts everything or it is a pure interface.

sipa commented at 11:59 pm on September 3, 2015:

I don’t think performance matters for this. The cost of accepting a transaction is already in the order of almost a millisecond anyway, the extra indirection of a virtual method is nothing compared to that.

jtimon commented at 3:47 pm on September 4, 2015:

Then I prefer the return true policy simply because it is more convenient when you create custom policies very different from CStandardPolicy (you save the effort to reimplement any method for which you were going to return true, for example, we could have a policy that requires a minimumfee but doesn’t have any special requirement for scripts). As said I can always go back to the pure abstract class, but I don’t see what the problem with a non-abstract interface base class (we have something similar in many places, for example BaseSignatureChecker which always returns false).

jtimon commented at 3:20 am on September 6, 2015:

But to be perfectly clear, I’m happy with either one. @luke-jr do you prefer a pure abstract class or a CPolicy that simply approves everything?

luke-jr commented at 5:16 am on September 6, 2015:

I prefer pure.

dcousens commented at 1:16 pm on September 7, 2015:

I think the pure abstract class is safer due to users who may inadvertently think the base class was standard. It doesn’t add any real value to default to true; as that behaviour can trivially be implemented by a user.

jtimon force-pushed on Sep 6, 2015

jtimon commented at 4:25 pm on September 6, 2015: contributor

Updated making CPolicy a pure abstract class instead of a regular class that always returns true.

dcousens commented at 1:15 pm on September 7, 2015: contributor

concept and utACK.

morcos commented at 11:37 pm on September 14, 2015: member

utACK

jtimon force-pushed on Sep 14, 2015

jtimon commented at 11:49 pm on September 14, 2015: contributor

@morcos pointed out that some documentation was left with the names of the old functions so I did a grep for each of them to make sure I wasn’t leaving anything outdated.

jtimon commented at 12:46 pm on September 22, 2015: contributor

ping @laanwj @theuni

jtimon commented at 4:40 pm on October 1, 2015: contributor

Ping

Policy: RENAME: Introduce CPolicy interface and hidden CStandardPolicy class implementing it

Rename 3 functions into CPolicy methods:

- IsStandard -> policy.ApproveScript
- IsStandardTx -> policy.ApproveTx
- AreInputsStandard -> policy.ApproveTxInputs

2dde8a25ea

jtimon force-pushed on Oct 1, 2015

jtimon commented at 10:48 pm on October 1, 2015: contributor

Needed rebase.

jaromil commented at 3:22 pm on November 1, 2015: contributor

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.

dgenr8 commented at 6:04 pm on November 1, 2015: contributor

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

luke-jr commented at 6:08 pm on November 1, 2015: member

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 commented at 6:28 pm on November 1, 2015: member

I don’t think it’s a reasonable expectation that policy can encapsulate that much of the mempool/relay behaviour.

IMHO policy ought to be the subset of configuration that is safe to change without hurting a node’s operations. For example, I don’t think that can encapsulate memory limiting, script verification, double spend prevention, … the mempool and the node’s resources have specific invariants that have be enforced for correct operation, regardless of what policy is.

Those invariants can change over time, and the logic to which the configuration applies can change too.

Frankly, I think this type of encapsulation needs to wait until the mempool code itself and the code relying on it is stable, and it is clear which parts are configurable and which aren’t.

luke-jr commented at 6:34 pm on November 1, 2015: member

@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…

sipa commented at 6:43 pm on November 1, 2015: member

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.

jtimon commented at 6:56 pm on November 1, 2015: contributor

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?

luke-jr commented at 7:17 pm on November 1, 2015: member

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.

jtimon commented at 7:38 pm on November 1, 2015: contributor

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.

sipa commented at 7:48 pm on November 1, 2015: member

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.

luke-jr commented at 7:59 pm on November 1, 2015: member

@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.

jtimon commented at 8:23 pm on November 1, 2015: contributor

@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).

jtimon commented at 1:57 pm on November 3, 2015: contributor

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

jtimon closed this on Nov 3, 2015

dcousens commented at 2:21 am on November 4, 2015: contributor

re-ACK (was rebased since last ACK)

jtimon commented at 2:37 am on December 1, 2015: contributor

closed forever

dcousens commented at 2:43 am on December 1, 2015: contributor

@jtimon why?

jtimon commented at 12:24 pm on December 1, 2015: contributor

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.

jaromil commented at 2:10 pm on December 1, 2015: contributor

wooo. lets talk about that…

dcousens commented at 1:53 am on December 2, 2015: contributor

@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.

MarcoFalke locked this on Sep 8, 2021

Policy: Create CPolicy interface and CStandardPolicy class implementing it #6068