MOVEONLY: Move constants to consensus.h #5696

pull jtimon wants to merge 1 commits into bitcoin:master from jtimon:consensus_policy0 changing 12 files +34 −16
  1. 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.
  2. jtimon closed this on Jan 23, 2015
  3. jtimon reopened this on Jan 23, 2015
  4. jtimon cross-referenced this on Jan 23, 2015 from issue Includes: Cleanup includes by jtimon
  5. jtimon cross-referenced this on Jan 23, 2015 from issue MOVEONLY-ish: Do not include main.h from any other header by jtimon
  6. jtimon cross-referenced this on Jan 23, 2015 from issue Policy: Create CPolicy interface and CStandardPolicy class implementing it by jtimon
  7. jtimon force-pushed on Jan 24, 2015
  8. laanwj added the label Improvement on Jan 26, 2015
  9. jtimon force-pushed on Jan 26, 2015
  10. 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

  11. jtimon cross-referenced this on Jan 26, 2015 from issue Separate CValidationState from main by jtimon
  12. jtimon cross-referenced this on Feb 2, 2015 from issue Change the default maximum OP_RETURN size to 80 bytes by Flavien
  13. jtimon force-pushed on Feb 3, 2015
  14. jtimon commented at 7:45 PM on February 3, 2015: contributor

    Needed rebase.

  15. in src/consensus/consensus.h:None 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 12: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:

    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 ?

  16. in src/script/standard.h:None 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 12: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

  17. jtimon cross-referenced this on Feb 8, 2015 from issue WIP: Encapsulate consensus and policy code by jtimon
  18. jtimon force-pushed on Feb 15, 2015
  19. jtimon commented at 9:03 PM on February 15, 2015: contributor

    Rebased adding LOCKTIME_THRESHOLD to consensus.h

  20. jtimon force-pushed on Feb 21, 2015
  21. jtimon commented at 9:46 PM on February 21, 2015: contributor

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

  22. jtimon force-pushed on Mar 3, 2015
  23. jtimon commented at 7:33 PM on March 3, 2015: contributor

    Renamed policy.o to policy/policy.o

  24. jtimon force-pushed on Mar 11, 2015
  25. 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.

  26. jtimon force-pushed on Mar 11, 2015
  27. jtimon commented at 5:09 PM on March 11, 2015: contributor

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

  28. maaku commented at 5:12 AM on March 12, 2015: contributor

    utACK. I like the direction this is going towards.

  29. jtimon force-pushed on Mar 13, 2015
  30. jtimon commented at 12:38 PM on March 13, 2015: contributor

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

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

  32. jtimon force-pushed on Mar 17, 2015
  33. 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

  34. jtimon force-pushed on Mar 24, 2015
  35. 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?

  36. jtimon commented at 5:24 PM on March 24, 2015: contributor

    Never mind, it does need rebase now.

  37. jtimon force-pushed on Mar 24, 2015
  38. jtimon force-pushed on Mar 24, 2015
  39. jtimon force-pushed on Mar 24, 2015
  40. jtimon force-pushed on Mar 24, 2015
  41. jtimon force-pushed on Mar 24, 2015
  42. jtimon force-pushed on Mar 26, 2015
  43. jtimon commented at 11:56 AM on March 26, 2015: contributor

    Needed rebase.

  44. jtimon cross-referenced this on Mar 26, 2015 from issue WIP: DEPENDENT: API: Expose bitcoinconsensus_verify_block() in libconsensus by jtimon
  45. 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.

  46. jtimon closed this on Apr 1, 2015
  47. Consensus: Create consensus/consensus.h with some constants 691161d419
  48. jtimon reopened this on Apr 23, 2015
  49. jtimon force-pushed on Apr 23, 2015
  50. jtimon commented at 12:30 AM on April 23, 2015: contributor

    Reopened as suggested by @theuni

  51. jtimon force-pushed on Apr 23, 2015
  52. jtimon force-pushed on Apr 23, 2015
  53. jtimon renamed this:
    MOVEONLY: Move constants and globals to consensus.h and policy.o
    MOVEONLY: Move constants consensus.h
    on Apr 23, 2015
  54. jtimon commented at 12: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.

  55. jtimon cross-referenced this on Apr 23, 2015 from issue MOVEONLY: Move to consensus/consensus.h: by jtimon
  56. jtimon cross-referenced this on Apr 23, 2015 from issue DEPENDENT: MOVEONLY: Move most transaction validation function defintions to consensus/txverify.cpp by jtimon
  57. jtimon cross-referenced this on Apr 23, 2015 from issue MOVEONLY-ish: Move most block header validation function defintions to consensus/blockverify.cpp by jtimon
  58. jtimon cross-referenced this on Apr 23, 2015 from issue MOVEONLY: Consensus: Move most of consensus functions (pre-block) by jtimon
  59. jtimon renamed this:
    MOVEONLY: Move constants consensus.h
    MOVEONLY: Move constants to consensus.h
    on Apr 23, 2015
  60. 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 :).

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

  62. sipa commented at 9:54 AM on April 24, 2015: member

    ACK

  63. laanwj merged this on Apr 26, 2015
  64. laanwj closed this on Apr 26, 2015
  65. laanwj referenced this in commit 1d9d314573 on Apr 26, 2015
  66. in src/qt/transactiondesc.cpp:None 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

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

  67. jtimon cross-referenced this on Aug 26, 2015 from issue Consensus: Adapt declarations of most obviously consensus functions by jtimon
  68. KolbyML cross-referenced this on Aug 19, 2019 from issue Creating the consensus folder a few pr from bitcoin are in this pr. by KolbyML
  69. bitcoin locked this on Sep 8, 2021
Contributors
jtimonTheBlueMattmaakutheunimorcossipaDiapolo
Labels
Refactoring
Linked (view graph)
#5286 Change the default maximum OP_RETURN size to 80 bytes#5595 Policy: Create CPolicy interface and CStandardPolicy class implementing it#5669 Separate CValidationState from main#5681 MOVEONLY-ish: Do not include main.h from any other header#5697 Includes: Cleanup includes#5768 WIP: Encapsulate consensus and policy code#5883 tests: add a BasicTestingSetup and apply to all tests#5946 WIP: DEPENDENT: API: Expose bitcoinconsensus_verify_block() in libconsensus#6048 MOVEONLY-ish: Move most block header validation function defintions to consensus/blockverify.cpp#6049 DEPENDENT: MOVEONLY: Move most transaction validation function defintions to consensus/txverify.cpp#6050 MOVEONLY: Move to consensus/consensus.h:#6051 MOVEONLY: Consensus: Move most of consensus functions (pre-block)#6068 Policy: Create CPolicy interface and CStandardPolicy class implementing it#6591 Consensus: Adapt declarations of most obviously consensus functions
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:55 UTC