IsSuperMajority() moved to separate soft forks unit #6774

pull CodeShark wants to merge 3 commits into bitcoin:master from CodeShark:ISM_to_softforks_unit changing 4 files +55 −21

CodeShark commented at 2:22 PM on October 7, 2015: contributor

CODE MOVE ONLY

This PR creates a new unit which will be handling soft fork logic. For now all we do is move IsSuperMajority() from main.cpp to consensus/softforks.cpp.
CodeShark force-pushed on Oct 7, 2015
dcousens commented at 12:40 AM on October 8, 2015: contributor

utACK
CodeShark renamed this:
~~ISM to softforks unit~~
IsSuperMajority() moved to separate soft forks unit
on Oct 8, 2015
jonasschnelli commented at 9:16 AM on October 8, 2015: contributor

utACK

paveljanik commented at 9:27 AM on October 8, 2015: contributor

Please clean up class/struct Params:

src pavel$ grep "[class|strict] Params" consensus/*h
consensus/params.h:struct Params {
consensus/softforks.h:class Params;
src pavel$

in src/consensus/softforks.h:None in 539702dea9 outdated

   6 | +#define BITCOIN_SOFTFORKS_H
   7 | +
   8 | +class CBlockIndex;
   9 | +namespace Consensus {
  10 | +
  11 | +class Params;

dcousens commented at 9:29 AM on October 8, 2015:

As pointed out by @paveljanik, this should be a struct.

laanwj added the label Refactoring on Oct 8, 2015
CodeShark force-pushed on Oct 9, 2015
Created a SoftForks unit containing only IsSuperMajority() function. bfbba3952f
Calling IsSuperMajority() inside SoftForks unit in main.cpp 4dea8e3cbc
Removed IsSuperMajority() from main.cpp df089cf1f6
CodeShark commented at 1:14 AM on October 9, 2015: contributor

@paveljanik @dcousens Fixed class->struct
in src/Makefile.am:None in df089cf1f6
```
  98 | @@ -99,6 +99,7 @@ BITCOIN_CORE_H = \
  99 |    compressor.h \
 100 |    consensus/consensus.h \
 101 |    consensus/params.h \
 102 | +  consensus/softforks.h \
```
jtimon commented at 9:37 AM on October 11, 2015:

If we're going to eventually use this for Hardfork consensus changes too, maybe it would be better to have a more generic name for the file/module: forks, changes, upgrades?

dcousens commented at 11:56 PM on October 13, 2015:

@jtimon could be done later

jtimon commented at 11:32 AM on October 20, 2015:

Or could be done now with less total work, that's the point.

in src/consensus/softforks.cpp:None in df089cf1f6

   0 | @@ -0,0 +1,25 @@
   1 | +// Copyright (c) 2009-2015 Eric Lombrozo, The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#include "chain.h"
   6 | +#include "consensus/params.h"
   7 | +#include "softforks.h"
   8 | +
   9 | +using namespace Consensus::SoftForks;

jtimon commented at 9:38 AM on October 11, 2015:

The additional namespace Softforks seems unnecessarily verbose, just Consensus seems enough.

jtimon commented at 9:38 AM on October 11, 2015: contributor

Apart from my bike-shedding nits, ut ACK.
jtimon commented at 7:56 PM on January 3, 2016: contributor

I changed my mind about this. I prefer something like https://github.com/jtimon/bitcoin/commit/8ec82416cdb48f6f01d49688406918be864d80b2 again. NACK In any case rebase or close?
dcousens commented at 10:47 PM on January 3, 2016: contributor

@jtimon why do you prefer it?

Still utACK from me, but needs rebase
jtimon commented at 10:53 PM on January 3, 2016: contributor

It's simpler and achieves the task of encapsulating consensus code more simply (although of course this is relative). I don't think a UseRule() function is interesting getting the consensus validation flags that include those rules (and I would unify other future consensus flags [like the one for bip113] with the script ones too).
jtimon commented at 10:59 PM on January 3, 2016: contributor

Sorry, that was a mistake, I thought I was talking on #6747 . This is just a tiny and pointless moveonly IMO. Instead of this, I would prefer something like this https://github.com/jtimon/bitcoin/commit/f8c34f27d4020880647cbdbafad70793882cef79 (see https://github.com/jtimon/bitcoin/commits/libconsensus-f2 for the "big picture" and #7091 for the pre-document-with-words-and-pictures opened first step).
CodeShark commented at 4:14 AM on January 4, 2016: contributor

@dcousens Rebasing - will push shortly. #6747 that is, not this one.
btcdrak commented at 2:30 PM on April 5, 2016: contributor

I think we can close this now. Also I recall talk of replacing the ISM checks with block heights.
laanwj closed this on Apr 6, 2016
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)