Added fPowNoRetargeting field to Consensus::Params #6853

CodeShark commented at 12:26 PM on October 19, 2015: contributor

-regtest cannot currently handle chains longer than one retargeting period. This PR allows arbitrary length chains to be created on -regtest by trivially preserving the nBits value of the last block.

btcdrak commented at 12:27 PM on October 19, 2015: contributor

as per @laanwj comment #6162 (comment)

laanwj commented at 1:23 PM on October 19, 2015: member

Concept ACK

laanwj added the label Tests on Oct 19, 2015

jonasschnelli commented at 1:49 PM on October 19, 2015: contributor

utACK

davecgh commented at 1:51 PM on October 19, 2015: contributor

I personally think this is a bad idea as it is a slippery slope. One of the main purposes of regtest is to test that the consensus code works as expected. As mentioned in #6162, there is currently no test coverage which exercises the retarget code and is why commit df9eb5e, which introduced the retargetting bug, ended up making it to master. In my opinion, providing a flag to work around a bug that was introduced instead of fixing the bug is a mistake.

What is the point of having a regression test network if you aren't really testing the behavior of the code executed on mainnet due to adding special case branches? With this commit, it will be possible to introduce a subtle consensus breaking change to mainnet for retargets that the tests will not be able to detect.

CodeShark commented at 1:57 PM on October 19, 2015: contributor

I agree we need tests for retargeting - but there are a bunch of regtest use cases that test critical things entirely orthogonal to retargeting that cannot really be tested without disabling it (i.e. #6816). I would suggest adding an option to turn this on or off...or to have another regtest chain that does not disable it.

sipa commented at 2:06 PM on October 19, 2015: member

@davecgh I fully agree that it's suboptimal that regtest would end up not testing mainnet's retargetting code. But regtest is not the only means through which the consensus rules are tested or exercised, and it's certainly easier to use it to test many other things this way. The alternative you suggest is changing the code that the network is already running on just to be able to test it, which IMHO unnecessarily throws away the trust in that code

laanwj commented at 2:13 PM on October 19, 2015: member

For me, and quite a few other users that are using regtest, the idea that regtest has retargeting at all was completely unexpected. In my mind, and how it has been always advertised, regtest = easy block generation. And I closed #6162 because I don't find it acceptable to change consensus code just to accommodate regtest.

But absolutely - the retargeting code needs to be tested, but there are other ways.

gavinandresen commented at 2:15 PM on October 19, 2015: contributor

Concept ACK from me-- in my experience writing regression tests, the default should be no retargeting; unless you are specifically testing retargeting, it just gets in the way and you're forced to use hack-y workarounds like calling setmocktime/generateblock to create a chain with timestamps that don't retarget.

davecgh commented at 2:55 PM on October 19, 2015: contributor

For what it's worth, I'm not only advocating for fixing the bug simply so the regression tests can exercise it although obviously testing the retarget code is certainly an extremely important factor and one it seems everybody agrees should be done, albeit perhaps through a different means.

We have an application (outside of tests) that uses the retarget capabilities of regtest in order to test it works properly across retarget boundaries (not of Bitcoin Core, the application itself). This is what drove the original issue to begin with since Bitcoin Core simply no longer handles retargets properly on regtest. We currently use simnet in btcd for this since it works properly across retargets, however we also wanted to be be able to test it against Bitcoin Core.

So, as you can see, this creates a situation where 3rd-party software can't properly be tested across retarget intervals on the regression test network with Bitcoin Core. Also, with this PR, this would now be a regtest option that when set to default disables all retargeting so it doesn't match mainnet, and when enabled causes a broken retarget because the overflow is still not fixed.

btcdrak commented at 2:59 PM on October 19, 2015: contributor

utACK

in src/consensus/params.h:None in 461b85cd5b outdated

  21 | @@ -22,6 +22,7 @@ struct Params {
  22 |      /** Proof of work parameters */
  23 |      uint256 powLimit;
  24 |      bool fPowAllowMinDifficultyBlocks;
  25 | +    bool fNoRetargeting;

jtimon commented at 4:51 PM on October 19, 2015:

Nit: can you s/fNoRetargeting/fPowNoRetargeting for naming consistency?

jtimon commented at 4:55 PM on October 19, 2015: contributor

ut ACK

Also, with this PR, this would now be a regtest option that when set to default disables all retargeting so it doesn't match mainnet, and when enabled causes a broken retarget because the overflow is still not fixed.

No, read the code again: the new field in Consensus::Params is always true for the regtest testchain (just like nSubsidyHalvingInterval is always 150 for regtest).

davecgh commented at 5:07 PM on October 19, 2015: contributor

@jtimon: I was referring to:

or to have another regtest chain that does not disable it

jtimon commented at 5:28 PM on October 19, 2015: contributor

@davecgh I see. Then I agree that it doesn't make sense to create another testchain (retargetRegtest?), different from regtest that doesn't skip retargeting but it actually doesn't retarget either. We already have 2 chains to test retargetting anyway (main and testnet3).

CodeShark force-pushed on Oct 19, 2015

Added fPowNoRetargeting field to Consensus::Params that disables nBits recalculation. 7801f4387d

CodeShark renamed this:
~~Added fNoRetargeting field to Consensus::Params~~
Added fPowNoRetargeting field to Consensus::Params
on Oct 19, 2015

dcousens commented at 9:17 AM on October 20, 2015: contributor

utACK

laanwj merged this on Oct 20, 2015

laanwj closed this on Oct 20, 2015

laanwj referenced this in commit c834f56869 on Oct 20, 2015

zkbot referenced this in commit fd7fb03f46 on Nov 18, 2020

zkbot referenced this in commit 949fdca3a6 on Nov 18, 2020

DrahtBot locked this on Sep 8, 2021