Added fPowNoRetargeting field to Consensus::Params #6853

Merged
merged 1 commit into from Oct 20, 2015

Conversation

Projects
None yet
9 participants
@CodeShark
Contributor

CodeShark commented Oct 19, 2015

-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

This comment has been minimized.

Show comment
Hide comment
Member

btcdrak commented Oct 19, 2015

as per @laanwj comment #6162 (comment)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 19, 2015

Member

Concept ACK

Member

laanwj commented Oct 19, 2015

Concept ACK

@laanwj laanwj added the Tests label Oct 19, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
Member

jonasschnelli commented Oct 19, 2015

utACK

@davecgh

This comment has been minimized.

Show comment
Hide comment
@davecgh

davecgh Oct 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.

Contributor

davecgh commented Oct 19, 2015

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

This comment has been minimized.

Show comment
Hide comment
@CodeShark

CodeShark Oct 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.

Contributor

CodeShark commented Oct 19, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Oct 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

Member

sipa commented Oct 19, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 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.

Member

laanwj commented Oct 19, 2015

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

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen Oct 19, 2015

Contributor
Contributor

gavinandresen commented Oct 19, 2015

@davecgh

This comment has been minimized.

Show comment
Hide comment
@davecgh

davecgh Oct 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.

Contributor

davecgh commented Oct 19, 2015

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

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Oct 19, 2015

Member

utACK

Member

btcdrak commented Oct 19, 2015

utACK

@jtimon

View changes

src/consensus/params.h
@@ -22,6 +22,7 @@ struct Params {
/** Proof of work parameters */
uint256 powLimit;
bool fPowAllowMinDifficultyBlocks;
+ bool fNoRetargeting;

This comment has been minimized.

@jtimon

jtimon Oct 19, 2015

Member

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

@jtimon

jtimon Oct 19, 2015

Member

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

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Oct 19, 2015

Member

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

Member

jtimon commented Oct 19, 2015

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

This comment has been minimized.

Show comment
Hide comment
@davecgh

davecgh Oct 19, 2015

Contributor

@jtimon: I was referring to:

or to have another regtest chain that does not disable it

Contributor

davecgh commented Oct 19, 2015

@jtimon: I was referring to:

or to have another regtest chain that does not disable it

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Oct 19, 2015

Member

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

Member

jtimon commented Oct 19, 2015

@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 CodeShark changed the title from Added fNoRetargeting field to Consensus::Params to Added fPowNoRetargeting field to Consensus::Params Oct 19, 2015

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Oct 20, 2015

Contributor

utACK

Contributor

dcousens commented Oct 20, 2015

utACK

@laanwj laanwj merged commit 7801f43 into bitcoin:master Oct 20, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Oct 20, 2015

Merge pull request #6853
7801f43 Added fPowNoRetargeting field to Consensus::Params that disables nBits recalculation. (Eric Lombrozo)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment