Parametrize buried soft fork in regtest and refactor #8460

NicolasDorier commented at 9:26 PM on August 4, 2016: contributor

This PR has three goals:

Parametrizes buried soft fork in regtest (Same approach than @sdaftuar on #8418)
Refactor by using a vector for buried deployment. Buried deployments will have its own BIP (https://github.com/bitcoin/bitcoin/pull/8391#issuecomment-237584871) so it makes sense to have a consistent way of adding a new one which can be parametrized out of the box by 297a65728de5fdd843335c8c9fc3821ae5d5bac4.
Introduce CChainParams::AllowsOverriddenSoftFork so we don't abuse MineBlocksOnDemand for bip9 and buried sf overrides. @sipa, you asked for it on #8391 (comment). @jtimon, using similar code you used on https://github.com/jtimon/bitcoin/compare/remove-ism...consensus-post-remove-ism

in src/chainparams.cpp:None in f36be17578 outdated

 327 | +        if (deployment == Consensus::BIP66_HEIGHT_ACTIVE) {
 328 | +                consensus.BIP66Height = nStartHeight;
 329 | +        }
 330 | +}
 331 | +
 332 | +void UpdateRegtestBuriedDeploymentParameters(Consensus::BuriedDeploymentPos deployment, int64_t nStartHeight)

jtimon commented at 9:48 PM on August 4, 2016:

More generic to take the the chainparams petname. Maybe assert that is equal to CBaseChainParams::REGTEST.

in src/init.cpp:None in f36be17578 outdated

1026 | @@ -1026,6 +1027,35 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
1027 |          }
1028 |      }
1029 |  
1030 | +    if (!mapMultiArgs["-buriedsfparams"].empty()) {
1031 | +        // Allow overriding ism parameters for testing
1032 | +        if (!Params().MineBlocksOnDemand()) {

jtimon commented at 9:53 PM on August 4, 2016:

Instead of abusing MineBlocksOnDemand(), maybe a new attribute in CChainParams? What about fAllowOverwriteBuriedSoftforks? We don't even need a new getter, CChainParams is accessed as a const ref everywhere outside chainparams.cpp.

sipa commented at 5:23 PM on August 26, 2016:

Agree with using a new attribute.

jtimon commented at 10:06 PM on August 4, 2016: contributor

Concept ACK

NicolasDorier force-pushed on Aug 4, 2016

in src/chainparams.h:None in 1a97576a63 outdated

 104 | @@ -101,6 +105,8 @@ class CChainParams
 105 |   */
 106 |  const CChainParams &Params();
 107 |  
 108 | +void UpdateRegtestBuriedDeploymentParameters(Consensus::BuriedDeploymentPos deployment, int64_t nStartHeight);

jtimon commented at 11:48 PM on August 4, 2016:

This is not necessary anymore.

sipa commented at 5:25 PM on August 26, 2016:

What is not necessary anymore?

NicolasDorier force-pushed on Aug 4, 2016

in src/chainparams.h:None in 35028679a1 outdated

  55 | @@ -56,7 +56,8 @@ class CChainParams
  56 |      const Consensus::Params& GetConsensus() const { return consensus; }
  57 |      const CMessageHeader::MessageStartChars& MessageStart() const { return pchMessageStart; }
  58 |      int GetDefaultPort() const { return nDefaultPort; }
  59 | -
  60 | +    /** This chain can have buried and bip9 soft fork parameters overridden with -bip9params and -buriedsfparams */
  61 | +    bool AllowsOverriddenSoftFork() const { return fAllowsOverriddenSoftFork; }

jtimon commented at 11:54 PM on August 4, 2016:

Is the getter necessary? Params() already returns a const ref, why not use Params().fAllowsOverriddenSoftFork directly in init (making fAllowsOverriddenSoftFork public)?

NicolasDorier commented at 11:56 PM on August 4, 2016:

I'm following the coding style of previous code in CChainParams which does not expose the fields.

jtimon commented at 11:58 PM on August 4, 2016:

fair enough

jtimon commented at 11:57 PM on August 4, 2016: contributor

utACK 3502867

NicolasDorier renamed this:
~~parametrize buried soft fork in regtest and refactor~~
Parametrize buried soft fork in regtest and refactor
on Aug 5, 2016

MarcoFalke added the label Refactoring on Aug 6, 2016

MarcoFalke added the label Consensus on Aug 6, 2016

in qa/rpc-tests/bip65-cltv-p2p.py:None in 35028679a1 outdated

  42 | @@ -43,7 +43,7 @@ def __init__(self):
  43 |      def setup_network(self):
  44 |          # Must set the blockversion for this test
  45 |          self.nodes = start_nodes(self.num_nodes, self.options.tmpdir,
  46 | -                                 extra_args=[['-debug', '-whitelist=127.0.0.1', '-blockversion=3']],
  47 | +                                 extra_args=[['-debug', '-whitelist=127.0.0.1', '-blockversion=3', '-buriedsfparams=bip65:1351']],

MarcoFalke commented at 4:55 PM on August 6, 2016:

Nit: Would it make sense to revert https://github.com/bitcoin/bitcoin/pull/8391/files#diff-30641a8ce712805f484b8305049f04b3 to the old code as well?

NicolasDorier commented at 1:10 AM on August 7, 2016:

This particular change does not change anything in fact, because it is setting the buriefsf height at its default value anyway. I can't really revert the rest. The reason is that the previous test was testing activation and enforcement. But now we don't have different height for activation and enforcement anymore.

MarcoFalke commented at 8:26 AM on August 7, 2016:

I have in mind you changed the heights for enforcement to be the same for all tests? So you could revert the patch that makes the test mine additional blocks and reduce buriedsfparams accordingly.

NicolasDorier commented at 10:22 PM on August 7, 2016:

yes good point.

in src/init.cpp:None in 35028679a1 outdated

 993 | @@ -993,9 +994,10 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
 994 |  
 995 |      if (!mapMultiArgs["-bip9params"].empty()) {
 996 |          // Allow overriding BIP9 parameters for testing
 997 | -        if (!Params().MineBlocksOnDemand()) {
 998 | -            return InitError("BIP9 parameters may only be overridden on regtest.");
 999 | +        if (!Params().AllowsOverriddenSoftFork()) {

MarcoFalke commented at 4:57 PM on August 6, 2016:

Nit: I think keeping this the way it was is just fine. (Note that fAllowsOverriddenSoftFork is not really a chain param, is it?)

jtimon commented at 5:05 PM on August 6, 2016:

He is creating a new one. Maybe a future testnet allows this but does not mine blocks on demand. That's why we have different properties for chainparams instead of calling IsTestnet() or IsRegtest () like we did before.

NicolasDorier commented at 1:14 AM on August 7, 2016:

@MarcoFalke What do you mean by "fAllowsOverriddenSoftFork is not really a chain param" ? where would you put it ?

sipa commented at 5:28 PM on August 26, 2016:

I'd say this is fine as a chain param.

jtimon commented at 1:28 PM on August 30, 2016:

nit s/Params()/chainparams/

NicolasDorier force-pushed on Aug 7, 2016

NicolasDorier commented at 11:10 PM on August 7, 2016: contributor

@MarcoFalke I reverted the python test height to old value, as well as deactivating the buried soft forks by default on regtest.

sipa commented at 1:19 PM on August 25, 2016: member

What is the status here?

NicolasDorier commented at 5:54 PM on August 25, 2016: contributor

I think it is ready, would appreciate some feedback. As suggested by @sdaftuar on #8391 (comment) I did a small BIP draft for buried deployments as well.

in src/main.cpp:None in 6751c0e0c0 outdated

2347 | @@ -2348,7 +2348,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
2348 |      // before the first had been spent.  Since those coinbases are sufficiently buried its no longer possible to create further
2349 |      // duplicate transactions descending from the known pairs either.
2350 |      // If we're on the known chain at height greater than where BIP34 activated, we can save the db accesses needed for the BIP30 check.
2351 | -    CBlockIndex *pindexBIP34height = pindex->pprev->GetAncestor(chainparams.GetConsensus().BIP34Height);
2352 | +    CBlockIndex *pindexBIP34height = pindex->pprev->GetAncestor(chainparams.GetConsensus().vBuriedDeploymentHeights[Consensus::BIP34_HEIGHT_ACTIVE]);
2353 |      //Only continue to enforce if we're below BIP34 activation height or the block hash at that height doesn't correspond.
2354 |      fEnforceBIP30 = fEnforceBIP30 && (!pindexBIP34height || !(pindexBIP34height->GetBlockHash() == chainparams.GetConsensus().BIP34Hash));

sipa commented at 5:27 PM on August 26, 2016:

If you make the bip34 height configurable, you should also make the hash configurable, or this test will fail.

NicolasDorier commented at 8:23 AM on August 27, 2016:

meh, on Regtest BIP34Hash has always been 0. Ability to override BIP34Hash should be done when at least one unit test will make use of it, which is not the case for now.

in src/chainparams.cpp:None in 6751c0e0c0 outdated

 347 | @@ -356,4 +348,4 @@ void UpdateRegtestBIP9Parameters(Consensus::DeploymentPos d, int64_t nStartTime,
 348 |  {
 349 |      regTestParams.UpdateBIP9Parameters(d, nStartTime, nTimeout);
 350 |  }
 351 | -

jtimon commented at 1:25 PM on August 30, 2016:

style nit: unnecessary diff.

in src/init.cpp:None in 5bbf52c6ec outdated

 993 | @@ -993,9 +994,10 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
 994 |  
 995 |      if (!mapMultiArgs["-bip9params"].empty()) {
 996 |          // Allow overriding BIP9 parameters for testing
 997 | -        if (!Params().MineBlocksOnDemand()) {
 998 | -            return InitError("BIP9 parameters may only be overridden on regtest.");
 999 | +        if (!Params().AllowsOverriddenSoftFork()) {
1000 | +            return InitError("BIP9 parameters can't be overriden on this chain.");
1001 |          }
1002 | +        CChainParams& mutableParams = Params(Params().NetworkIDString());

jtimon commented at 1:28 PM on August 30, 2016:

nit s/Params()/chainparams/

in src/init.cpp:None in 5bbf52c6ec outdated

1027 | @@ -1026,6 +1028,36 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
1028 |          }
1029 |      }
1030 |  
1031 | +    if (!mapMultiArgs["-buriedsfparams"].empty()) {
1032 | +        // Allow overriding ism parameters for testing
1033 | +        if (!Params().AllowsOverriddenSoftFork()) {

jtimon commented at 1:28 PM on August 30, 2016:

nit s/Params()/chainparams/

in src/init.cpp:None in 5bbf52c6ec outdated

1027 | @@ -1026,6 +1028,36 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
1028 |          }
1029 |      }
1030 |  
1031 | +    if (!mapMultiArgs["-buriedsfparams"].empty()) {
1032 | +        // Allow overriding ism parameters for testing
1033 | +        if (!Params().AllowsOverriddenSoftFork()) {
1034 | +            return InitError("Buried soft fork parameters can't be overridden on this chain.");
1035 | +        }
1036 | +        CChainParams& mutableParams = Params(Params().NetworkIDString());

jtimon commented at 1:30 PM on August 30, 2016:

nit s/Params()/chainparams/

jtimon commented at 1:31 PM on August 30, 2016: contributor

re-utACK besides nits.

NicolasDorier commented at 3:29 PM on August 30, 2016: contributor

nit addressed @MarcoFalke this PR might interest you as it touches the tests.

NicolasDorier commented at 11:04 AM on September 8, 2016: contributor

is it possible to run the tests again ? I don't think the failure is related to this PR.

sipa commented at 11:07 AM on September 8, 2016: member

@NicolasDorier I restarted the failing tests.

MarcoFalke commented at 10:44 PM on September 9, 2016: member

@NicolasDorier as I see correctly, the tests are no longer failing when there is an off-by-one error. What do you think of https://github.com/MarcoFalke/bitcoin/commits/Mf1609-qaReworkISM ?

NicolasDorier commented at 5:22 AM on September 10, 2016: contributor

@MarcoFalke good point, just cherry picked your commit. EDIT: +fix typo in your commit message

NicolasDorier force-pushed on Sep 10, 2016

MarcoFalke commented at 7:31 PM on September 15, 2016: member

Concept ACK 97c4507 (can't review my own changes, though)

jtimon approved

jtimon commented at 2:33 AM on September 20, 2016: contributor

Mhmm, there is a new review feature... utACK 43de0ce concept ACK 97c4507

MarcoFalke commented at 11:55 PM on October 29, 2016: member

This needs the merge conflict solved.

NicolasDorier force-pushed on Nov 10, 2016

Can pass buried soft fork parameters to regtest 27d56c82ef

Revert buriedsf python tests to previous height 9d37a7a94e

Refactor: Adding a buried deployment heights vector d9e47c563c

Refactor: Introduce CChainParams::AllowsOverriddenSoftFork for testing a chain 75a17d698b

[qa] Rework bip65 and bip66 soft fork enforcement tests after ISM removal 5b5d5f4567

NicolasDorier force-pushed on Nov 10, 2016

NicolasDorier commented at 7:57 PM on November 10, 2016: contributor

rebased

NicolasDorier commented at 1:17 PM on February 15, 2017: contributor

closing it, too much conflict, if someone is interested I might do the work again.

NicolasDorier closed this on Feb 15, 2017

DrahtBot locked this on Sep 8, 2021