Policy: Create CPolicy interface and CStandardPolicy class implementing it

jtimon commented at 3:50 pm on January 3, 2015: contributor

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.

OUTDATED: This is an attempt to get a first commit for moving policy-related code together as proposed in #4943. The main purpose of this PR is therefore discuss the commit “Policy: Create CPolicy interface and CStandardPolicy implemention” (https://github.com/jtimon/bitcoin/commit/572f12948285b14fcb2c2d1c9e2f51e749cc6cb6) which may change with suggestions. The commit “Policy: MOVEONLY: script/standard.o::IsStandard() -> CPolicy::ValidateScript()” (https://github.com/jtimon/bitcoin/commit/9e99ebafc38718ca54730e764091a82a248a1a3c) acts as an example for adding a method to CPolicy. The commit “Policy: Refactor: Move datacarrier policy logic to policy.o” (https://github.com/jtimon/bitcoin/commit/f141dd0b9b4059c2d54d0762d5dd0fe86976a18a) is an example of adding an attribute to CStandardPolicy without exposing it on CPolicy or exposing CStandardPolicy itself. An example of a crazy custom policy that a user could implement for its local node can be found in https://github.com/jtimon/bitcoin/compare/policy_example

The commit “Policy: Refactor: Move datacarrier policy logic to policy.o” may be considered too risky and may be left for later to avoid delaying the first step. Maybe https://github.com/jtimon/bitcoin/commit/a2954632879076d3b9763001aca21bf768797e54 is not welcomed. It is only necessary if you want to make InitPolicyFromArgs() and CPolicy::InitFromArgs() param mapArgs const and don’t want to duplicate code.

I am very sorry for letting the ut acks rot.

in src/qt/coincontroldialog.cpp: in c45030f4ad outdated

 9@@ -10,6 +10,7 @@
10 #include "guiutil.h"
11 #include "init.h"
12 #include "optionsmodel.h"
13+#include "policy.h"

Diapolo commented at 5:47 pm on January 3, 2015:

Nit: This should be moved to the core includes block :).

jtimon commented at 6:25 pm on January 4, 2015:

Mhmm, I guess init should be with main and wallet as well…

sipa commented at 3:17 pm on January 4, 2015: member

Untested ACK

jtimon commented at 6:45 pm on January 4, 2015: contributor

@Diapolo Added a squashme commit to fix the include nit.

jtimon force-pushed on Jan 7, 2015

jtimon commented at 11:30 pm on January 7, 2015: contributor

Squashed @diapolo’s nit. Also rebased. Does people agree that InitPolicyFromCommandLine() is a good idea to squash its commit? @luke-jr ? Now there’s 3 more commits from #5180 that I think you can agree with. You also moved main::IsStandardTx() and main::AreInputsStandard() in the more advanced policy branch you showed me (luke-jr/nodepolicy2). The difference is that I’m also moving some constants/globals they use to policy.o at the same time and that I’m moving things to InitPolicyFromCommandLine() from init.cpp. Well, and that I’m moving these two functions before creating the class, while you create 4 new functions/methods when you create the class. Our main disagreements seem to be in the way we access the class and the abstract class/interface, as well as on policy.o depending on main.o. But since this PR is about an uncontroversial start on policy.o, I think I could extend this PR with those movements and it will still make #5114, #5180, a rebased version of #5071 and any other policy-related PR easier to review. What do you think, should I bring more commits from #5180 here?

laanwj added the label Mining on Jan 8, 2015

laanwj added the label TX fees and policy on Jan 8, 2015

luke-jr commented at 3:57 pm on January 8, 2015: member

ACK, although I don’t think we ought to be claiming copyright on policy.h yet as it contains no copyrightable content.

jtimon commented at 8:29 pm on January 8, 2015: contributor

Thanks. Well, making later commits smaller is the whole point and putting the copyright text already serves that purpose. Should I squash the second commit then?

jtimon force-pushed on Jan 10, 2015

jtimon commented at 6:17 pm on January 10, 2015: contributor

Squashed to a single commit.

jtimon force-pushed on Jan 20, 2015

jtimon force-pushed on Jan 21, 2015

jtimon commented at 2:00 am on January 21, 2015: contributor

I’m sorry for changing the PR again but the goal was always to find a common base and hopefully make later policy-code movements cleaner. I think IsStandard(CStript) with luke’s commits is a good example that will likely be easy to maintain. All those functions and interfaces like Policy(“standard”) allow you to easily create your own crazy policy and activate it with -policy=custom as shown in the example (https://github.com/jtimon/bitcoin/commit/1a5a1916fa6d07a10b77f7bba163d52bd3e1d331) without causing the tests to fail .

jtimon force-pushed on Jan 21, 2015

jtimon renamed this:
~~Policy: Create policy.o with global minRelayTxFee from main.o~~
Policy: Create CPolicy interface and CStandardPolicy class implementing it
on Jan 21, 2015

jtimon commented at 12:02 pm on January 21, 2015: contributor

Well, I’m sorry, the latest version is actually failing on windows… EDIT: found the bug: https://travis-ci.org/bitcoin/bitcoin/builds/47773190 Now squashing and updating the initial description.

jtimon force-pushed on Jan 21, 2015

jtimon commented at 1:21 pm on January 21, 2015: contributor

There was a bug in src/test/test_bitcoin.cpp. I updated the code and the initial description.

jtimon force-pushed on Jan 23, 2015

jtimon commented at 4:31 pm on January 23, 2015: contributor

Rebased on top of #5696 and updated the initial description adding new links to commits.

jtimon force-pushed on Jan 24, 2015

jtimon force-pushed on Jan 26, 2015

jtimon renamed this:
~~Policy: Create CPolicy interface and CStandardPolicy class implementing it~~
PoC: Policy: Create CPolicy interface and CStandardPolicy class implementing it
on Feb 4, 2015

in src/init.cpp: in d66803ee06 outdated

753@@ -748,7 +754,6 @@ bool AppInit2(boost::thread_group& threadGroup)
754 #endif // ENABLE_WALLET
755 
756     fIsBareMultisigStd = GetArg("-permitbaremultisig", true) != 0;

dexX7 commented at 4:33 am on February 11, 2015:

This should probably be moved as well. The help message is currently in “connection options”: https://github.com/jtimon/bitcoin/blob/d66803ee06175d71ff7605ed1cfa8b91aec23f62/src/init.cpp#L284

Very exciting branch by the way! :)

jtimon commented at 8:10 pm on February 13, 2015: contributor

@dexX7 this is just a proof of concept branch to make sure everyone is in the same page when it comes to the interface (the abstract class), the functions to initialize the policy, etc. It is not complete. See #5768 for a more complete branch (which includes your suggestion). I’m mostly waiting for @luke-jr ack/nack on the interface to rebase and reduce the scope of this PR to something relatively trivial (leave d66803e for later).

in src/policy.cpp: in d66803ee06 outdated

51+{
52+    assert(pCurrentPolicy);
53+    return *pCurrentPolicy;
54+}
55+
56+std::string GetPolicyUsageStr()

luke-jr commented at 11:54 pm on February 13, 2015:

Rather, perhaps put -policy in the main usage func, and add a -help-policy= that calls a per-policy function returning some kind of list?

jtimon commented at 0:09 am on February 14, 2015:

Mhmm, so move -policy back to init and then have this function take a string parameter so that it can return different things for different policies?. Is that what you’re suggesting?

luke-jr commented at 0:12 am on February 14, 2015:

more like have init call Policy(str).GetUsage() for -help-policy=

GetUsage can return a bunch of std::pairs which get formatted by init

jtimon commented at 0:20 am on February 14, 2015:

ok, I see what you’re saying. Makes sense to me. But why return pairs instead of a string? The latter seems much simpler.

luke-jr commented at 0:35 am on February 14, 2015:

It’s cleaner, avoids breaking policy formatting when the main codebase changes, and fits #5749 well.

jtimon commented at 0:59 am on February 14, 2015:

#5749 seems like a good enough reason, thanks.

in src/policy.cpp: in d66803ee06 outdated

76+        nMaxDatacarrierBytes = GetArg("-datacarriersize", nMaxDatacarrierBytes, mapArgs);
77+    else
78+        nMaxDatacarrierBytes = 0;
79+}
80+
81+bool CStandardPolicy::ValidateScript(const CScript& scriptPubKey, txnouttype& whichType) const

luke-jr commented at 11:57 pm on February 13, 2015:

“Validate” is the wrong term here. Perhaps “AcceptScript”?

jtimon commented at 0:11 am on February 14, 2015:

I’m using CheckX and VerifyX so I chose ValidateX for policy to avoid “grep conflicts”. I can change Validate for Accept but I would rather use the same for all the methods..

luke-jr commented at 0:15 am on February 14, 2015:

We are already using Accept for AcceptToMemoryPool, and IsStandard for the current equivalent of this. I agree IsStandard is not very well-named.

jtimon commented at 0:24 am on February 14, 2015:

The fact that we’re using Accept in main may actually be a reason against also using it in policy rather than a reason in favor. If you look at #5768 you may understand why I definitely prefer ValidateX over AcceptX at this point. I’m not changing it unless you get more people to support this bikeshed.

luke-jr commented at 0:37 am on February 14, 2015:

The Accept in main ought to be moved to policy. “Validate” is entirely wrong, as validity is not related to policy.

jtimon commented at 0:38 am on February 14, 2015:

That being said, if some of the methods are not strictly check-only, it will be better that they get another name. For example, CPolicy::AcceptMemPoolEntry() in https://github.com/jtimon/bitcoin/commit/0f69efa2876580255f40edad95d15180f3ffa31c That commit could also hide some methods to only belong to CStandardPolicy . For example, you can hide CheckTxPreInputs() can be hiden since exposing AcceptTxPoolPreInputs() is enough.

jtimon commented at 0:55 am on February 14, 2015:

You’re checking that things are valid for a given policy. I understand the concern now though since the term “validate” is often used in the documentation of consensus code so it may be misleading for policy. Another possibility would be to use ValidateX and VerifyX in consensus functions instead of CheckX and VerifyX. That would leave CheckX free for policy. But that would be even more work… Let’s just use AcceptX for policy unless someone else comes up with another color for the bike shed…I mean, another name for the methods before I update this

jtimon commented at 5:54 am on February 14, 2015:

mhmm, what about ApproveX() ?

in src/policy.h: in d66803ee06 outdated

47+/** Abstract interface for Policy */
48+class CPolicy
49+{
50+public:
51+    virtual void InitFromArgs(const std::map<std::string, std::string>&) = 0;
52+    virtual bool ValidateScript(const CScript&, txnouttype&) const = 0;

luke-jr commented at 0:00 am on February 14, 2015:

This should be a CStandardPolicy thing - the base class should simply have one for transactions (AcceptToMemoryPool equivalent). Some policies may not have a script-specific check.

jtimon commented at 0:18 am on February 14, 2015:

The point is to never expose CStandardPolicy to the rest of the code. You can change the interface (CPolicy) later and hide some methods in CStandardPolicy only.

Your approach is not the only possibility. The replacements themselves could be maintained in main or txmempool and here only have a method that compares two transactions (or rather, two groups of transactions for child-pay-for-parent like replacemnts).

In any case, the main point is that the interface doesn’t have to be perfect from the beginning, It is more important to not expose CStandardPolicy.

luke-jr commented at 0:30 am on February 14, 2015:

I’m not suggesting exposing CStandardPolicy… see how I did this in my branch; CStandardPolicy’s CPolicy::AcceptTransaction can call CStandardPolicy::AcceptScript

jtimon commented at 0:43 am on February 14, 2015:

I know, that can be done later as shown in https://github.com/jtimon/bitcoin/compare/luke_policy2 (a rebased version of your stuff). That’s not the only possible interface, it may not be the best (specially since we lack a criterion of quality for policy interfaces) and, again, the interface doesn’t need to be perfect from the beginning. The priority is encapsulating policy-specific code. The fastest way to do it is by doing the most obvious code movements first. Let’s do the uncontroversial code movements first, trying to get it perfect from the beginning may result in having this delayed indefinitely.

jtimon force-pushed on Mar 26, 2015

jtimon commented at 3:25 pm on March 26, 2015: contributor

Updated with @luke-jr ’s suggestions:

Replace ValidateX with ApproveX
Added a method std::vector<std::pair<std::string, std::string> > GetOptionsHelp() to the CPolicy interface to make GUI-based policy configuration easier.

This is no longer a Proof of concept, this is ready to be merged. For more examples on how this will be used see #5768

jtimon renamed this:
~~PoC: Policy: Create CPolicy interface and CStandardPolicy class implementing it~~
Policy: Create CPolicy interface and CStandardPolicy class implementing it
on Mar 26, 2015

in src/policy/policy.h: in 02413461a1 outdated

33+ * strict DER encoding.
34+ * 
35+ * Failing one of these tests may trigger a DoS ban - see CheckInputs() for
36+ * details.
37+ */
38+static const unsigned int MANDATORY_SCRIPT_VERIFY_FLAGS = SCRIPT_VERIFY_P2SH;

luke-jr commented at 6:33 pm on March 26, 2015:

This isn’t policy.

jtimon commented at 11:08 am on March 27, 2015:

MANDATORY_SCRIPT_VERIFY_FLAGS isn’t policy? At first I put it in consensus, but @TheBlueMatt wasn’t convinced that was consensus, and @sipa said it was policy in #5696 (or maybe #5669). Ideally consensus should always use something like Consensus::GetFlags() (see https://github.com/jtimon/bitcoin/commit/0779bbc15d91706380f23219cecf472728099b46) Longer term, I think it should become an attribute of CStandardPolicy (see https://github.com/jtimon/bitcoin/commit/d1fec1073c5472515d8a4d3646230cc46da744bf)

in src/test/script_P2SH_tests.cpp: in 02413461a1 outdated

53@@ -53,6 +54,7 @@ BOOST_FIXTURE_TEST_SUITE(script_P2SH_tests, BasicTestingSetup)
54 BOOST_AUTO_TEST_CASE(sign)
55 {
56     LOCK(cs_main);
57+    SelectPolicy("standard");

luke-jr commented at 6:43 pm on March 26, 2015:

Isn’t this redundant with BasicTestingSetup::BasicTestingSetup?

jtimon commented at 11:10 am on March 27, 2015:

Maybe after the rebase there’s some redundancies with SelectPolicy()… @laanwj IIRC you recently changed the places where SelectParams() was called, can you confirm this is now redundant?

laanwj commented at 12:02 pm on March 27, 2015:

Yes BasicTestingSetup sets the standard policy so there is no reason to do it in the individual test cases too.

jtimon force-pushed on Apr 1, 2015

jtimon commented at 10:32 pm on April 1, 2015: contributor

Fixed the redundancies but MANDATORY_SCRIPT_VERIFY_FLAGS remains in policy/policy.h @luke-jr is that a nack for you?

jtimon force-pushed on Apr 5, 2015

jtimon force-pushed on Apr 13, 2015

jtimon commented at 7:09 pm on April 13, 2015: contributor

As discussed on IRC, I’ll just leave MANDATORY_SCRIPT_VERIFY_FLAGS in script/standard.h for now. I believe all the nits have been solved now.

jtimon force-pushed on Apr 13, 2015

luke-jr commented at 10:30 pm on April 13, 2015: member

ACK 9ddf5e5f84c9450b6125d9124866d8d50c116959

Consensus: Create consensus/consensus.h with some constants 691161d419

Policy: Create policy.o with some constants and globals ae6329bb79

Policy: Prepare utils for policy 573a2b36eb

Policy: Create CPolicy interface and CStandardPolicy implementation c1c81ea468

Policy: MOVEONLY: script/standard.o::IsStandard() -> CPolicy::ApproveScript() ffacae83ef

jtimon force-pushed on Apr 20, 2015

jtimon commented at 7:28 pm on April 20, 2015: contributor

Needed rebase.

jtimon commented at 0:56 am on April 23, 2015: contributor

Closing until #5696 is resolved

jtimon closed this on Apr 23, 2015

jtimon reopened this on Apr 24, 2015

jtimon closed this on Apr 24, 2015

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

I’m sorry but I can’t reopen this, more explanations and the rebased equivalent (needed rebase and it will also get slightly simpler after a reduced version of #5696 has been merged) to this PR in: #6068.

I’m very sorry, @luke-jr when I created this PR competing with #5071 it wasn’t my intention to slow things down with policy.

MarcoFalke locked this on Sep 8, 2021

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