MOVEONLY: Move constants to consensus.h

jtimon commented at 12:36 pm on January 23, 2015: contributor

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 closed this on Jan 23, 2015

jtimon reopened this on Jan 23, 2015

jtimon force-pushed on Jan 24, 2015

laanwj added the label Improvement on Jan 26, 2015

jtimon force-pushed on Jan 26, 2015

jtimon commented at 12:31 pm on January 26, 2015: contributor

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

jtimon force-pushed on Feb 3, 2015

jtimon commented at 7:45 pm on February 3, 2015: contributor

Needed rebase.

in src/consensus/consensus.h: in 3163087972 outdated

21+ * strict DER encoding.
22+ * 
23+ * Failing one of these tests may trigger a DoS ban - see CheckInputs() for
24+ * details.
25+ */
26+static const unsigned int MANDATORY_SCRIPT_VERIFY_FLAGS = SCRIPT_VERIFY_P2SH;

TheBlueMatt commented at 0:06 am on February 4, 2015:

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”

jtimon commented at 1:14 am on February 4, 2015:

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

0if (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 ?

in src/script/standard.h: in 3163087972 outdated

27@@ -28,33 +28,6 @@ class CScriptID : public uint160
28 static const unsigned int MAX_OP_RETURN_RELAY = 80;      //! bytes

TheBlueMatt commented at 0:07 am on February 4, 2015:

Why not also move this?

jtimon commented at 1:08 am on February 4, 2015:

As explained in the PR: “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.”

It can be done later, see https://github.com/jtimon/bitcoin/commit/d66803ee06175d71ff7605ed1cfa8b91aec23f62

jtimon force-pushed on Feb 15, 2015

jtimon commented at 9:03 pm on February 15, 2015: contributor

Rebased adding LOCKTIME_THRESHOLD to consensus.h

jtimon force-pushed on Feb 21, 2015

jtimon commented at 9:46 pm on February 21, 2015: contributor

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

jtimon force-pushed on Mar 3, 2015

jtimon commented at 7:33 pm on March 3, 2015: contributor

Renamed policy.o to policy/policy.o

jtimon force-pushed on Mar 11, 2015

jtimon commented at 1:28 pm on March 11, 2015: contributor

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 force-pushed on Mar 11, 2015

jtimon commented at 5:09 pm on March 11, 2015: contributor

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

maaku commented at 5:12 am on March 12, 2015: contributor

utACK. I like the direction this is going towards.

jtimon force-pushed on Mar 13, 2015

jtimon commented at 12:38 pm on March 13, 2015: contributor

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

theuni commented at 8:42 pm on March 15, 2015: member

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 force-pushed on Mar 17, 2015

jtimon commented at 10:58 am on March 17, 2015: contributor

@theuni All globals and constants should ideally become attributes of CStandardPolicy and thus encapsulated and not exposed to the rest of the code. Rebased moving the #include "script/interpreter.h" from consensus.h to policy.h as pointed out by @theuni

jtimon force-pushed on Mar 24, 2015

jtimon commented at 5:16 pm on March 24, 2015: contributor

I improved the includes (ie not including consensus/consensus.h and policy/policy.h from main.h): even if there’s going to be an include cleanup commit later, there’s no reason not to do things right from the beginning here. After that, github says that this needs rebase but it’s not true, I rebased it to master locally just fine. I’m not sure about what should I do next…should I still rebase? wait for other people to confirm that I’ve only changed includes from last time?

jtimon commented at 5:24 pm on March 24, 2015: contributor

Never mind, it does need rebase now.

jtimon force-pushed on Mar 24, 2015

jtimon force-pushed on Mar 26, 2015

jtimon commented at 11:56 am on March 26, 2015: contributor

Needed rebase.

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

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

jtimon closed this on Apr 1, 2015

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

jtimon reopened this on Apr 23, 2015

jtimon force-pushed on Apr 23, 2015

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

Reopened as suggested by @theuni

jtimon force-pushed on Apr 23, 2015

jtimon renamed this:
~~MOVEONLY: Move constants and globals to consensus.h and policy.o~~
MOVEONLY: Move constants consensus.h
on Apr 23, 2015

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

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

jtimon renamed this:
~~MOVEONLY: Move constants consensus.h~~
MOVEONLY: Move constants to consensus.h
on Apr 23, 2015

theuni commented at 3:19 am on April 23, 2015: member

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 commented at 8:34 pm on April 23, 2015: member

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 commented at 9:54 am on April 24, 2015: member

ACK

laanwj merged this on Apr 26, 2015

laanwj closed this on Apr 26, 2015

laanwj referenced this in commit 1d9d314573 on Apr 26, 2015

in src/qt/transactiondesc.cpp: in 691161d419

14 #include "main.h"
15 #include "script/script.h"
16 #include "timedata.h"
17 #include "ui_interface.h"
18 #include "util.h"
19+#include "wallet/db.h"

Diapolo commented at 6:48 am on April 27, 2015:

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

jtimon commented at 8:42 am on April 27, 2015:

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

0#include "coins.h"
1#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.

MarcoFalke locked this on Sep 8, 2021

MOVEONLY: Move constants to consensus.h #5696