as per @laanwj comment #6162 (comment)

Concept ACK

utACK

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.

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.

@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

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.

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.

utACK

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

@jtimon: I was referring to:

or to have another regtest chain that does not disable it

@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).

utACK