New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: Fix testnet-in-a-box use case #5987

Merged
merged 1 commit into from Oct 1, 2015

Conversation

Projects
None yet
4 participants
@luke-jr
Member

luke-jr commented Apr 9, 2015

Remove testnet checkpoint and allow for older tip blocks

Show outdated Hide outdated src/main.cpp
@@ -1200,7 +1200,7 @@ bool IsInitialBlockDownload()
if (lockIBDState)
return false;
bool state = (chainActive.Height() < pindexBestHeader->nHeight - 24 * 6 ||
pindexBestHeader->GetBlockTime() < GetTime() - 24 * 60 * 60);
pindexBestHeader->GetBlockTime() < GetTime() - Params().MaxTipAge());

This comment has been minimized.

@laanwj

laanwj Apr 10, 2015

Member

Why change this? It only determines whether the client is in initial block download, it shouldn't prevent the client from accepting blocks.

@laanwj

laanwj Apr 10, 2015

Member

Why change this? It only determines whether the client is in initial block download, it shouldn't prevent the client from accepting blocks.

This comment has been minimized.

@luke-jr

luke-jr Apr 10, 2015

Member

IsInitialBlockDownload disables mining.

@luke-jr

luke-jr Apr 10, 2015

Member

IsInitialBlockDownload disables mining.

This comment has been minimized.

@laanwj

laanwj Apr 15, 2015

Member

Oh right, that's ugly...

@laanwj

laanwj Apr 15, 2015

Member

Oh right, that's ugly...

This comment has been minimized.

@jtimon

jtimon Apr 25, 2015

Member

would you mind to declare const CChainParams& chainparams = Params(); at the beginning of the function and use chainparams.MaxTipAge() instead?
That will make passing CChainParams as an argument to IsInitialBlockDownload() simpler later.

@jtimon

jtimon Apr 25, 2015

Member

would you mind to declare const CChainParams& chainparams = Params(); at the beginning of the function and use chainparams.MaxTipAge() instead?
That will make passing CChainParams as an argument to IsInitialBlockDownload() simpler later.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jun 21, 2015

Member

ut ACK, though it needs rebase

Member

jtimon commented Jun 21, 2015

ut ACK, though it needs rebase

@luke-jr luke-jr closed this Jun 23, 2015

@luke-jr luke-jr reopened this Jun 23, 2015

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 23, 2015

Member

Rebased.

Member

luke-jr commented Jun 23, 2015

Rebased.

Show outdated Hide outdated src/chainparams.cpp
@@ -179,9 +181,9 @@ class CTestNetParams : public CMainParams {
checkpointData = (Checkpoints::CCheckpointData) {
boost::assign::map_list_of
( 546, uint256S("000000002a936ca763904c3c35fce2f3556c559c0214345d31b1bcebf76acb70")),

This comment has been minimized.

@jtimon

jtimon Jun 26, 2015

Member

Why remove the checkpoint? why not just add the genesis checkpoint as in jtimon@dc16119 ?
Also, why not just use consensus.hashGenesisBlock in the genesis checkpoint instead of repeating the hardcoded value?

@jtimon

jtimon Jun 26, 2015

Member

Why remove the checkpoint? why not just add the genesis checkpoint as in jtimon@dc16119 ?
Also, why not just use consensus.hashGenesisBlock in the genesis checkpoint instead of repeating the hardcoded value?

This comment has been minimized.

@jtimon

jtimon Jun 26, 2015

Member

In fact, why not add the genesis checkpoint (the genesis block, the only checkpoint that cannot be reorged) to the mainchain too (and while at it use consensus.hashGenesisBlock for regtest's gensis checkpoint for uniformity).

@jtimon

jtimon Jun 26, 2015

Member

In fact, why not add the genesis checkpoint (the genesis block, the only checkpoint that cannot be reorged) to the mainchain too (and while at it use consensus.hashGenesisBlock for regtest's gensis checkpoint for uniformity).

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jun 29, 2015

Member

Any thoughts on the nits and proposed changes to this PR?

Member

jtimon commented Jun 29, 2015

Any thoughts on the nits and proposed changes to this PR?

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Sep 15, 2015

Contributor

Along the lines of @jtimon 's comments - a genesis checkpoint does not appear to be needed.

Contributor

jgarzik commented Sep 15, 2015

Along the lines of @jtimon 's comments - a genesis checkpoint does not appear to be needed.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Sep 15, 2015

Member

@jgarzik I'm actually saying the opposite: the genesis checkpoint that can be trusted (genesis block) is good, but why remove any existing checkpoint in testnet3 ?

Member

jtimon commented Sep 15, 2015

@jgarzik I'm actually saying the opposite: the genesis checkpoint that can be trusted (genesis block) is good, but why remove any existing checkpoint in testnet3 ?

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Sep 15, 2015

Contributor

@jtimon That was covered in the opening PR description? "allow for older tip blocks"

If we don't want that then sure, keep the checkpoint.

Contributor

jgarzik commented Sep 15, 2015

@jtimon That was covered in the opening PR description? "allow for older tip blocks"

If we don't want that then sure, keep the checkpoint.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Sep 15, 2015

Member

The op says:

Remove testnet checkpoint

why?

...and allow for older tip blocks

I want this (like in jtimon@dc16119). jgarzik seems not to want this for some reason.

Member

jtimon commented Sep 15, 2015

The op says:

Remove testnet checkpoint

why?

...and allow for older tip blocks

I want this (like in jtimon@dc16119). jgarzik seems not to want this for some reason.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Sep 15, 2015

Contributor

I have no real opinion. Just moving things along.

Contributor

jgarzik commented Sep 15, 2015

I have no real opinion. Just moving things along.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Sep 20, 2015

Member

Checkpoints beyond the genesis block are a problem because testnet-in-a-box networks won't have those blocks.

MaxTipAge is needed because otherwise mining the first (non-genesis) block is impossible on such a network.

Member

luke-jr commented Sep 20, 2015

Checkpoints beyond the genesis block are a problem because testnet-in-a-box networks won't have those blocks.

MaxTipAge is needed because otherwise mining the first (non-genesis) block is impossible on such a network.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Sep 21, 2015

Member

Checkpoints beyond the genesis block are a problem because
testnet-in-a-box networks won't have those blocks.

I see. They could also disable checkpoints in their test, but I don't have
a strong opinion: I'm fine removing it if that's the motivation.
What about the rest of my feedback (ie making sure all ghains have a
genesis checkpoint and using hardcoded values as little as possible)?

Member

jtimon commented Sep 21, 2015

Checkpoints beyond the genesis block are a problem because
testnet-in-a-box networks won't have those blocks.

I see. They could also disable checkpoints in their test, but I don't have
a strong opinion: I'm fine removing it if that's the motivation.
What about the rest of my feedback (ie making sure all ghains have a
genesis checkpoint and using hardcoded values as little as possible)?

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Sep 21, 2015

Member

@jtimon I don't care about that proposed change, but it's certainly not a bugfix or in the scope of this PR...

Member

luke-jr commented Sep 21, 2015

@jtimon I don't care about that proposed change, but it's certainly not a bugfix or in the scope of this PR...

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 22, 2015

Member

Instead of removing the checkpoint, users of testnet-in-a-box could specify the option -checkpoints=0 - right?

I don't see a simple alternative solution for the tip age, though I still think having it per network is hacky.

Member

laanwj commented Sep 22, 2015

Instead of removing the checkpoint, users of testnet-in-a-box could specify the option -checkpoints=0 - right?

I don't see a simple alternative solution for the tip age, though I still think having it per network is hacky.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Sep 22, 2015

Member

+1 on @laanwj 's suggestion to avoid removing testnet3's checkpoint.

@luke-jr I'm just asking to add the genesis checkpoint in main, just like you are adding it to testnet (and already exists on regtest), but if you want to leave that for later (see #6382), that's fine too.

Member

jtimon commented Sep 22, 2015

+1 on @laanwj 's suggestion to avoid removing testnet3's checkpoint.

@luke-jr I'm just asking to add the genesis checkpoint in main, just like you are adding it to testnet (and already exists on regtest), but if you want to leave that for later (see #6382), that's fine too.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Sep 22, 2015

Member

@laanwj If I remove the checkpoint changes here, can we merge the other part with an understanding it will go away when a better solution is found?

Member

luke-jr commented Sep 22, 2015

@laanwj If I remove the checkpoint changes here, can we merge the other part with an understanding it will go away when a better solution is found?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 23, 2015

Member

@luke-jr fine with me

Member

laanwj commented Sep 23, 2015

@luke-jr fine with me

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj
Member

laanwj commented Sep 29, 2015

@luke-jr ping

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Sep 29, 2015

Member

Sorry for the delay, done.

Member

luke-jr commented Sep 29, 2015

Sorry for the delay, done.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Oct 1, 2015

Contributor

ut ACK

Contributor

jgarzik commented Oct 1, 2015

ut ACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 1, 2015

Member

utACK

Member

laanwj commented Oct 1, 2015

utACK

@laanwj laanwj merged commit e761d7a into bitcoin:master Oct 1, 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 1, 2015

Merge pull request #5987
e761d7a Bugfix: Allow mining on top of old tip blocks for testnet (fixes testnet-in-a-box use case) (Luke Dashjr)

laanwj added a commit to laanwj/bitcoin that referenced this pull request Dec 14, 2015

Make max tip age an option instead of chainparam
After discussion in #7164 I think this is better.

Max tip age was introduced in #5987 to make it possible to run
testnet-in-a-box. But associating this behavior with the testnet chain
is wrong conceptually, as it is not needed in normal usage.
Should aim to make testnet test the software as-is.

Replace it with a (debug) option `-maxtipage`, which can be
specified only in the specific case.

deadalnix added a commit to deadalnix/bitcoin that referenced this pull request Jan 11, 2017

Make max tip age an option instead of chainparam
After discussion in #7164 I think this is better.

Max tip age was introduced in #5987 to make it possible to run
testnet-in-a-box. But associating this behavior with the testnet chain
is wrong conceptually, as it is not needed in normal usage.
Should aim to make testnet test the software as-is.

Replace it with a (debug) option `-maxtipage`, which can be
specified only in the specific case.

deadalnix added a commit to deadalnix/bitcoin that referenced this pull request Jan 11, 2017

Make max tip age an option instead of chainparam
After discussion in #7164 I think this is better.

Max tip age was introduced in #5987 to make it possible to run
testnet-in-a-box. But associating this behavior with the testnet chain
is wrong conceptually, as it is not needed in normal usage.
Should aim to make testnet test the software as-is.

Replace it with a (debug) option `-maxtipage`, which can be
specified only in the specific case.

deadalnix added a commit to deadalnix/bitcoin that referenced this pull request Jan 15, 2017

Make max tip age an option instead of chainparam
After discussion in #7164 I think this is better.

Max tip age was introduced in #5987 to make it possible to run
testnet-in-a-box. But associating this behavior with the testnet chain
is wrong conceptually, as it is not needed in normal usage.
Should aim to make testnet test the software as-is.

Replace it with a (debug) option `-maxtipage`, which can be
specified only in the specific case.

deadalnix added a commit to deadalnix/bitcoin that referenced this pull request Jan 16, 2017

Make max tip age an option instead of chainparam
After discussion in #7164 I think this is better.

Max tip age was introduced in #5987 to make it possible to run
testnet-in-a-box. But associating this behavior with the testnet chain
is wrong conceptually, as it is not needed in normal usage.
Should aim to make testnet test the software as-is.

Replace it with a (debug) option `-maxtipage`, which can be
specified only in the specific case.

deadalnix added a commit to deadalnix/bitcoin that referenced this pull request Jan 17, 2017

Make max tip age an option instead of chainparam
After discussion in #7164 I think this is better.

Max tip age was introduced in #5987 to make it possible to run
testnet-in-a-box. But associating this behavior with the testnet chain
is wrong conceptually, as it is not needed in normal usage.
Should aim to make testnet test the software as-is.

Replace it with a (debug) option `-maxtipage`, which can be
specified only in the specific case.

deadalnix added a commit to deadalnix/bitcoin that referenced this pull request Jan 19, 2017

Make max tip age an option instead of chainparam
After discussion in #7164 I think this is better.

Max tip age was introduced in #5987 to make it possible to run
testnet-in-a-box. But associating this behavior with the testnet chain
is wrong conceptually, as it is not needed in normal usage.
Should aim to make testnet test the software as-is.

Replace it with a (debug) option `-maxtipage`, which can be
specified only in the specific case.

deadalnix added a commit to deadalnix/bitcoin that referenced this pull request Jan 19, 2017

Make max tip age an option instead of chainparam
After discussion in #7164 I think this is better.

Max tip age was introduced in #5987 to make it possible to run
testnet-in-a-box. But associating this behavior with the testnet chain
is wrong conceptually, as it is not needed in normal usage.
Should aim to make testnet test the software as-is.

Replace it with a (debug) option `-maxtipage`, which can be
specified only in the specific case.

deadalnix added a commit to deadalnix/bitcoin that referenced this pull request Jan 19, 2017

Make max tip age an option instead of chainparam
After discussion in #7164 I think this is better.

Max tip age was introduced in #5987 to make it possible to run
testnet-in-a-box. But associating this behavior with the testnet chain
is wrong conceptually, as it is not needed in normal usage.
Should aim to make testnet test the software as-is.

Replace it with a (debug) option `-maxtipage`, which can be
specified only in the specific case.

deadalnix added a commit to deadalnix/bitcoin that referenced this pull request Jan 26, 2017

Make max tip age an option instead of chainparam
After discussion in #7164 I think this is better.

Max tip age was introduced in #5987 to make it possible to run
testnet-in-a-box. But associating this behavior with the testnet chain
is wrong conceptually, as it is not needed in normal usage.
Should aim to make testnet test the software as-is.

Replace it with a (debug) option `-maxtipage`, which can be
specified only in the specific case.

deadalnix added a commit to deadalnix/bitcoin that referenced this pull request Feb 12, 2017

Make max tip age an option instead of chainparam
After discussion in #7164 I think this is better.

Max tip age was introduced in #5987 to make it possible to run
testnet-in-a-box. But associating this behavior with the testnet chain
is wrong conceptually, as it is not needed in normal usage.
Should aim to make testnet test the software as-is.

Replace it with a (debug) option `-maxtipage`, which can be
specified only in the specific case.

deadalnix added a commit to deadalnix/bitcoin that referenced this pull request Feb 27, 2017

Make max tip age an option instead of chainparam
After discussion in #7164 I think this is better.

Max tip age was introduced in #5987 to make it possible to run
testnet-in-a-box. But associating this behavior with the testnet chain
is wrong conceptually, as it is not needed in normal usage.
Should aim to make testnet test the software as-is.

Replace it with a (debug) option `-maxtipage`, which can be
specified only in the specific case.

deadalnix added a commit to deadalnix/bitcoin that referenced this pull request Feb 27, 2017

Make max tip age an option instead of chainparam
After discussion in #7164 I think this is better.

Max tip age was introduced in #5987 to make it possible to run
testnet-in-a-box. But associating this behavior with the testnet chain
is wrong conceptually, as it is not needed in normal usage.
Should aim to make testnet test the software as-is.

Replace it with a (debug) option `-maxtipage`, which can be
specified only in the specific case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment