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

jtimon · 2015-01-03T15:50:04Z

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" (jtimon@572f129) which may change with suggestions.
The commit "Policy: MOVEONLY: script/standard.o::IsStandard() -> CPolicy::ValidateScript()" (jtimon@9e99eba) acts as an example for adding a method to CPolicy.
The commit "Policy: Refactor: Move datacarrier policy logic to policy.o" (jtimon@f141dd0) 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 jtimon@a295463 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.

Diapolo · 2015-01-03T17:47:17Z

src/qt/coincontroldialog.cpp

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

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

sipa · 2015-01-04T15:17:03Z

Untested ACK

jtimon · 2015-01-04T18:45:16Z

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

jtimon · 2015-01-07T23:30:28Z

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?

luke-jr · 2015-01-08T15:57:22Z

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

jtimon · 2015-01-08T20:29:39Z

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 · 2015-01-10T18:17:21Z

Squashed to a single commit.

jtimon · 2015-01-21T02:00:16Z

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 (jtimon@1a5a191) without causing the tests to fail .

jtimon · 2015-01-21T12:02:36Z

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 · 2015-01-21T13:21:48Z

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

jtimon · 2015-01-23T16:31:13Z

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

jtimon · 2015-03-26T15:25:05Z

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

luke-jr · 2015-03-26T18:33:27Z

src/policy/policy.h

This isn't policy.

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 jtimon@0779bbc)
Longer term, I think it should become an attribute of CStandardPolicy (see jtimon@d1fec10)

jtimon · 2015-04-01T22:32:20Z

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

jtimon · 2015-04-13T19:09:20Z

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.

luke-jr · 2015-04-13T22:30:54Z

ACK 9ddf5e5

…Script()

jtimon · 2015-04-20T19:28:54Z

Needed rebase.

jtimon · 2015-04-23T00:56:14Z

Closing until #5696 is resolved

jtimon · 2015-04-26T14:35:21Z

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.

Diapolo reviewed Jan 3, 2015
View reviewed changes

jtimon force-pushed the policy branch from fa1770f to 005e754 Compare January 7, 2015 22:53

laanwj added Mining TX fees and policy labels Jan 8, 2015

jtimon mentioned this pull request Jan 8, 2015

Encapsulate policy #4943

Closed

jtimon force-pushed the policy branch from 005e754 to 420bfdd Compare January 10, 2015 18:16

jtimon mentioned this pull request Jan 13, 2015

Policy: Continue policy movements #5652

Closed

jtimon force-pushed the policy branch 2 times, most recently from 0e28aef to 5f18826 Compare January 21, 2015 01:44

jtimon force-pushed the policy branch from 5f18826 to a5346b8 Compare January 21, 2015 11:25

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

jtimon force-pushed the policy branch from 55df59a to f888f6a Compare January 21, 2015 13:00

This was referenced Jan 21, 2015

Policy: Move CTxOut::IsDust() to policy.o #5114

Closed

MOVEONLY: Move constants to consensus.h #5696

Merged

jtimon force-pushed the policy branch from f888f6a to ae5b16d Compare January 23, 2015 16:22

jtimon force-pushed the policy branch 4 times, most recently from f141dd0 to d66803e Compare January 26, 2015 12:35

jtimon mentioned this pull request Feb 2, 2015

Change the default maximum OP_RETURN size to 80 bytes #5286

Merged

jtimon changed the title ~~PoC: Policy: Create CPolicy interface and CStandardPolicy class implementing it~~ Policy: Create CPolicy interface and CStandardPolicy class implementing it Mar 26, 2015

luke-jr reviewed Mar 26, 2015
View reviewed changes

jtimon force-pushed the policy branch from 0241346 to c26b9de Compare April 1, 2015 22:28

jtimon force-pushed the policy branch from c26b9de to 9322482 Compare April 5, 2015 20:27

jtimon force-pushed the policy branch from 9322482 to 4830823 Compare April 13, 2015 19:07

jtimon force-pushed the policy branch 2 times, most recently from 7d48d38 to 9ddf5e5 Compare April 13, 2015 22:19

jtimon mentioned this pull request Apr 20, 2015

Separate CValidationState from main #5669

Merged

jtimon and others added 5 commits April 20, 2015 21:27

Consensus: Create consensus/consensus.h with some constants

691161d

Policy: Create policy.o with some constants and globals

ae6329b

Policy: Prepare utils for policy

573a2b3

Policy: Create CPolicy interface and CStandardPolicy implementation

c1c81ea

Policy: MOVEONLY: script/standard.o::IsStandard() -> CPolicy::Approve…

ffacae8

…Script()

jtimon force-pushed the policy branch from 9ddf5e5 to ffacae8 Compare April 20, 2015 19:28

jtimon closed this Apr 23, 2015

jtimon reopened this Apr 24, 2015

jtimon closed this Apr 24, 2015

jtimon mentioned this pull request Apr 26, 2015

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

Closed

jtimon mentioned this pull request May 7, 2015

Accept any sequence of PUSHDATAs in OP_RETURN outputs #5079

Closed

jtimon mentioned this pull request Feb 26, 2016

Encapsulate options for mempool policy #7557

Closed

jtimon mentioned this pull request Dec 3, 2016

Clean up mapArgs and mapMultiArgs Usage #9243

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

jtimon commented Jan 3, 2015

Uh oh!

Diapolo Jan 3, 2015

Choose a reason for hiding this comment

Uh oh!

jtimon Jan 4, 2015

Choose a reason for hiding this comment

Uh oh!

sipa commented Jan 4, 2015

Uh oh!

jtimon commented Jan 4, 2015

Uh oh!

jtimon commented Jan 7, 2015

Uh oh!

luke-jr commented Jan 8, 2015

Uh oh!

jtimon commented Jan 8, 2015

Uh oh!

jtimon commented Jan 10, 2015

Uh oh!

jtimon commented Jan 21, 2015

Uh oh!

jtimon commented Jan 21, 2015

Uh oh!

jtimon commented Jan 21, 2015

Uh oh!

jtimon commented Jan 23, 2015

Uh oh!

jtimon commented Mar 26, 2015

Uh oh!

luke-jr Mar 26, 2015

Choose a reason for hiding this comment

Uh oh!

jtimon Mar 27, 2015

Choose a reason for hiding this comment

Uh oh!

jtimon commented Apr 1, 2015

Uh oh!

jtimon commented Apr 13, 2015

Uh oh!

luke-jr commented Apr 13, 2015

Uh oh!

jtimon commented Apr 20, 2015

Uh oh!

jtimon commented Apr 23, 2015

Uh oh!

jtimon commented Apr 26, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants