MOVEONLY: Move constants to consensus.h #5696

jtimon · 2015-01-23T12:36:39Z

This is blocking #6051 and #5595

There's no way to start moving consensus and policy code effectively out of main without creating a first .h file.

UPDATE:
It was initially a tiny first step to sepearate consensus and policy but afer a lot of discussion it ended up being a tiny step towards separating only more consensus.

The motivation for this PR is stop blocking the 2 PRs mentioned in line 1.

OUTDATED (for archive):

While we discuss what the CPolicy interface should be #5595, we can start by just moving constants and globals to consensus.h and policy.o.
This will also open the door to some cleaning up on the includes.
nMaxDatacarrierBytes and MAX_OP_RETURN_RELAY are not moved from script/standard.o to policy.o because policy.o is in server while script/standard.o is in common and wouldn't link.

Please propose any other constant or global variable related to consensus or policy that I may have missed.

New but irrelevant:
Are you still reading?

This PR is so simple that the following comments are equivalent:

utACK
ACK
ACK: I didn't read the code but I fully tested in all ways I could think of and it doesn't seem to break anything.
I didn't tested it but I did finally read the code.

jtimon · 2015-01-26T12:31:57Z

Updated adding MANDATORY_SCRIPT_VERIFY_FLAGS to consensus.h and STANDARD_SCRIPT_VERIFY_FLAGS and STANDARD_NOT_MANDATORY_VERIFY_FLAGS to policy.h

jtimon · 2015-02-03T19:45:24Z

Needed rebase.

TheBlueMatt · 2015-02-04T00:06:18Z

src/consensus/consensus.h

+ * Failing one of these tests may trigger a DoS ban - see CheckInputs() for
+ * details.
+ */
+static const unsigned int MANDATORY_SCRIPT_VERIFY_FLAGS = SCRIPT_VERIFY_P2SH;


Not sold on this part...MANDATORY_SCRIPT_VERIFY_FLAGS is more about "what we accept now", whereas consensus code is generally "what we accept(ed) at the time the block was generated based on its version"

I've been thinking that Consensus could be a factory of an interface taking nHeight as parameter instead of a namespace.
Example:

if (Consensus(nHeight).CheckBlock(block, state))

Otherwise all consensus functions affected by softforks will have to take int nHeight as parameter.
But it's probably too soon for something like that.
I'm not sure what you're proposing though, leaving it on script/standard ?

jtimon · 2015-02-15T21:03:00Z

Rebased adding LOCKTIME_THRESHOLD to consensus.h

jtimon · 2015-02-21T21:46:28Z

Moved MANDATORY_SCRIPT_VERIFY_FLAGS to policy as suggested by @TheBlueMatt and @sipa

jtimon · 2015-03-03T19:33:10Z

Renamed policy.o to policy/policy.o

jtimon · 2015-03-11T13:28:21Z

Not exactly sure why, but it needed rebase. Ping @laanwj @theuni
This is quite trivial and the best first step for any consensus and policy code movements IMO.

jtimon · 2015-03-11T17:09:26Z

Rebased with @laanwj 's fix that should make travis happy again

maaku · 2015-03-12T05:12:23Z

utACK. I like the direction this is going towards.

jtimon · 2015-03-13T12:38:50Z

Rebased without @laanwj 's fix after #5883 has been merged

theuni · 2015-03-15T20:42:22Z

I really don't like the introduction of globals into policy.h, but I understand that they're a temporary measure.

utACK, with the assumption that the globals will be removed fairly quickly.

jtimon · 2015-03-26T11:56:16Z

Needed rebase.

jtimon · 2015-04-01T13:10:02Z

Closing in favor of #5669 and #5595. The first commit will be in both, the second commit only on the policy PR.

jtimon · 2015-04-23T00:30:16Z

Reopened as suggested by @theuni

jtimon · 2015-04-23T00:46:06Z

Updated only with the consensus constants but not the policy constants and globals.
This should be hyper-mergeable.

theuni · 2015-04-23T03:19:27Z

ACK. I asked @jtimon to re-open this (and slim it down to what it is now) in an effort to focus some of the current tangled PRs. I'm hoping we can serialize some of this work to get it in faster, so I'll take the blame for blowing up everyone's mailboxes with closed/reworked PRs today :).

morcos · 2015-04-23T20:34:21Z

I like the idea of going ahead and getting some of this reorganization out of the way if we're going to do it. I reviewed this and did some basic testing and it looks good to me.

sipa · 2015-04-24T09:54:19Z

ACK

691161d Consensus: Create consensus/consensus.h with some constants (jtimon)

Diapolo · 2015-04-27T06:48:08Z

src/qt/transactiondesc.cpp

 #include "main.h"
 #include "script/script.h"
 #include "timedata.h"
 #include "ui_interface.h"
 #include "util.h"
+#include "wallet/db.h"


Why was this needed, seems I can compile without it.

The question is not whether you can compile with it or not, the question is whether qt/transactiondesc.cpp is using anything declared in wallet/db.h or not. It's better to indicate dependencies explicitly.

Try commenting #include "main.h" and #include "wallet/db.h" and build again.

You will notice that

#include "coins.h" #include <boost/foreach.hpp>

is missing but...yeah, you're right. wallet/db.h doesn't seem to be needed anymore.
None of nWalletDBUpdated, CDBEnv, bitdb and CDB is used here.
I am very sorry for putting it there unnecessarily. It seems that you removed it and I added it back in a rebase by accident.
Mhmm, I'll review #6068 in case something similar has happened there.

jtimon closed this Jan 23, 2015

jtimon reopened this Jan 23, 2015

This was referenced Jan 23, 2015

Includes: Cleanup includes #5697

Merged

MOVEONLY-ish: Do not include main.h from any other header #5681

Merged

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

Closed

jtimon force-pushed the consensus_policy0 branch from 6dcf663 to b2272b8 Compare January 24, 2015 16:55

laanwj added the Refactoring label Jan 26, 2015

jtimon force-pushed the consensus_policy0 branch from b2272b8 to 1c96143 Compare January 26, 2015 12:30

jtimon mentioned this pull request Jan 26, 2015

Separate CValidationState from main #5669

Merged

jtimon mentioned this pull request Feb 2, 2015

Change the default maximum OP_RETURN size to 80 bytes #5286

Merged

jtimon force-pushed the consensus_policy0 branch from 1c96143 to 3163087 Compare February 3, 2015 19:44

TheBlueMatt reviewed Feb 4, 2015
View reviewed changes

jtimon mentioned this pull request Feb 8, 2015

WIP: Encapsulate consensus and policy code #5768

Closed

jtimon force-pushed the consensus_policy0 branch from 3163087 to c37cac0 Compare February 15, 2015 05:20

jtimon force-pushed the consensus_policy0 branch from c37cac0 to c7f0ece Compare February 21, 2015 21:45

jtimon force-pushed the consensus_policy0 branch from c7f0ece to df861be Compare March 3, 2015 19:31

jtimon force-pushed the consensus_policy0 branch from df861be to 71a9bef Compare March 11, 2015 13:26

jtimon force-pushed the consensus_policy0 branch from 71a9bef to ca6793f Compare March 11, 2015 17:08

jtimon force-pushed the consensus_policy0 branch from ca6793f to d5ab935 Compare March 13, 2015 12:21

jtimon force-pushed the consensus_policy0 branch 4 times, most recently from 5737c3d to 386ea01 Compare March 26, 2015 11:55

jtimon mentioned this pull request Mar 26, 2015

WIP: DEPENDENT: API: Expose bitcoinconsensus_verify_block() in libconsensus #5946

Closed

jtimon closed this Apr 1, 2015

Consensus: Create consensus/consensus.h with some constants

691161d

jtimon reopened this Apr 23, 2015

jtimon force-pushed the consensus_policy0 branch from 386ea01 to 660cca3 Compare April 23, 2015 00:29

jtimon force-pushed the consensus_policy0 branch 2 times, most recently from ae6329b to 691161d Compare April 23, 2015 00:43

jtimon changed the title ~~MOVEONLY: Move constants and globals to consensus.h and policy.o~~ MOVEONLY: Move constants consensus.h Apr 23, 2015

jtimon changed the title ~~MOVEONLY: Move constants consensus.h~~ MOVEONLY: Move constants to consensus.h Apr 23, 2015

laanwj merged commit 691161d into bitcoin:master Apr 26, 2015

laanwj added a commit that referenced this pull request Apr 26, 2015

Merge pull request #5696

1d9d314

691161d Consensus: Create consensus/consensus.h with some constants (jtimon)

Diapolo reviewed Apr 27, 2015
View reviewed changes

jtimon mentioned this pull request Aug 26, 2015

Consensus: Adapt declarations of most obviously consensus functions #6591

Closed

1 task

KolbyML mentioned this pull request Aug 19, 2019

Creating the consensus folder a few pr from bitcoin are in this pr. ZENZO-Ecosystem/ZENZO-Core-Archive#41

Open

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

MOVEONLY: Move constants to consensus.h #5696

MOVEONLY: Move constants to consensus.h #5696

Uh oh!

Conversation

jtimon commented Jan 23, 2015

Uh oh!

jtimon commented Jan 26, 2015

Uh oh!

jtimon commented Feb 3, 2015

Uh oh!

TheBlueMatt Feb 4, 2015

Choose a reason for hiding this comment

Uh oh!

jtimon Feb 4, 2015

Choose a reason for hiding this comment

Uh oh!

jtimon commented Feb 15, 2015

Uh oh!

jtimon commented Feb 21, 2015

Uh oh!

jtimon commented Mar 3, 2015

Uh oh!

jtimon commented Mar 11, 2015

Uh oh!

jtimon commented Mar 11, 2015

Uh oh!

maaku commented Mar 12, 2015

Uh oh!

jtimon commented Mar 13, 2015

Uh oh!

theuni commented Mar 15, 2015

Uh oh!

jtimon commented Mar 26, 2015

Uh oh!

jtimon commented Apr 1, 2015

Uh oh!

jtimon commented Apr 23, 2015

Uh oh!

jtimon commented Apr 23, 2015

Uh oh!

theuni commented Apr 23, 2015

Uh oh!

morcos commented Apr 23, 2015

Uh oh!

sipa commented Apr 24, 2015

Uh oh!

Diapolo Apr 27, 2015

Choose a reason for hiding this comment

Uh oh!

jtimon Apr 27, 2015

Choose a reason for hiding this comment

Uh oh!

Uh oh!