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

Make max tip age an option instead of chainparam #7208

Merged
merged 1 commit into from Jan 18, 2016

Conversation

Projects
None yet
6 participants
@laanwj
Member

laanwj commented Dec 14, 2015

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.

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.

@laanwj laanwj added the Tests label Dec 14, 2015

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Dec 14, 2015

Contributor

ACK

Contributor

paveljanik commented Dec 14, 2015

ACK

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Dec 14, 2015

Contributor

concept ACK, utACK 64360f1

Contributor

dcousens commented Dec 14, 2015

concept ACK, utACK 64360f1

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke
Member

MarcoFalke commented Dec 17, 2015

utACK 64360f1

@@ -994,6 +995,8 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
if (GetBoolArg("-peerbloomfilters", true))
nLocalServices |= NODE_BLOOM;
nMaxTipAge = GetArg("-maxtipage", DEFAULT_MAX_TIP_AGE);

This comment has been minimized.

@jtimon

jtimon Jan 3, 2016

Member

minor nit, maybe

nMaxTipAge = GetArg("-maxtipage", nMaxTipAge);

to not use DEFAULT_MAX_TIP_AGE twice (nMaxTipAge is already initialized to DEFAULT_MAX_TIP_AGE in main.cpp, this seems a little bit redundant)?
Anyway, minor minor minor style nit. Feel more than free to ignore.

@jtimon

jtimon Jan 3, 2016

Member

minor nit, maybe

nMaxTipAge = GetArg("-maxtipage", nMaxTipAge);

to not use DEFAULT_MAX_TIP_AGE twice (nMaxTipAge is already initialized to DEFAULT_MAX_TIP_AGE in main.cpp, this seems a little bit redundant)?
Anyway, minor minor minor style nit. Feel more than free to ignore.

This comment has been minimized.

@laanwj

laanwj Jan 18, 2016

Member

I've always tried to avoid using the current value as default anywhere for GetArg. Specifying DEFAULT_MAX_TIP_AGE is more clear when reading the code, and when at some point argument parsing is refactored it's nice to have the defaults on hand without having to 'reason back' about the value.

@laanwj

laanwj Jan 18, 2016

Member

I've always tried to avoid using the current value as default anywhere for GetArg. Specifying DEFAULT_MAX_TIP_AGE is more clear when reading the code, and when at some point argument parsing is refactored it's nice to have the defaults on hand without having to 'reason back' about the value.

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 18, 2016

Member

@jtimon We use the DEFAULT_* values to make it easier to implement #1044.

@MarcoFalke

MarcoFalke Jan 18, 2016

Member

@jtimon We use the DEFAULT_* values to make it easier to implement #1044.

This comment has been minimized.

@jtimon

jtimon Jan 19, 2016

Member

It's fine, as said it was a very minor nit. Maybe we can remove the redundant initialization in main.cpp?
Whatever we do, I'm fine, as long as we try to be consistent among all the globals (which imo should be in globals/server.o of something of the short instead of main chainparams, etc [globals/common.o could take the globals in unit.o and basechainparams, for example]; but one step a a time...).

@jtimon

jtimon Jan 19, 2016

Member

It's fine, as said it was a very minor nit. Maybe we can remove the redundant initialization in main.cpp?
Whatever we do, I'm fine, as long as we try to be consistent among all the globals (which imo should be in globals/server.o of something of the short instead of main chainparams, etc [globals/common.o could take the globals in unit.o and basechainparams, for example]; but one step a a time...).

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jan 3, 2016

Member

utACK laanwj@64360f1
There's definitely way too many things in chainparams that shouldn't be there.
Ping @luke-jr

Member

jtimon commented Jan 3, 2016

utACK laanwj@64360f1
There's definitely way too many things in chainparams that shouldn't be there.
Ping @luke-jr

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jan 10, 2016

Member

Concept ACK. Do any RPC tests need this set for them?

Member

luke-jr commented Jan 10, 2016

Concept ACK. Do any RPC tests need this set for them?

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 10, 2016

Member

Extended tests seem to pass.

Member

MarcoFalke commented Jan 10, 2016

Extended tests seem to pass.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 18, 2016

Member

Concept ACK. Do any RPC tests need this set for them?

I don't think this is used by any RPC tests. This was introduced because of testnet-in-a-box, which we don't use for RPC tests. @MarcoFalke's test seems to confirm this.

Member

laanwj commented Jan 18, 2016

Concept ACK. Do any RPC tests need this set for them?

I don't think this is used by any RPC tests. This was introduced because of testnet-in-a-box, which we don't use for RPC tests. @MarcoFalke's test seems to confirm this.

@laanwj laanwj merged commit 64360f1 into bitcoin:master Jan 18, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Jan 18, 2016

Merge pull request #7208
64360f1 Make max tip age an option instead of chainparam (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge pull request #7208
64360f1 Make max tip age an option instead of chainparam (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge pull request #7208
64360f1 Make max tip age an option instead of chainparam (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Dec 9, 2017

Merge pull request #7208
64360f1 Make max tip age an option instead of chainparam (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Dec 9, 2017

Merge pull request #7208
64360f1 Make max tip age an option instead of chainparam (Wladimir J. van der Laan)

zkbot added a commit to zcash/zcash that referenced this pull request Jul 16, 2018

Auto merge of #3263 - str4d:ibd-upstream-changes, r=<try>
InitialBlockDownload upstream changes

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7208
- bitcoin/bitcoin#8007
- bitcoin/bitcoin#9053
  - Excluding second commit (requires bitcoin/bitcoin#8865)
- bitcoin/bitcoin#10388

zkbot added a commit to zcash/zcash that referenced this pull request Jul 17, 2018

Auto merge of #3263 - str4d:ibd-upstream-changes, r=bitcartel
InitialBlockDownload upstream changes

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7208
- bitcoin/bitcoin#8007
- bitcoin/bitcoin#9053
  - Excluding second commit (requires bitcoin/bitcoin#8865)
- bitcoin/bitcoin#10388

zkbot added a commit to zcash/zcash that referenced this pull request Jul 17, 2018

Auto merge of #3263 - str4d:ibd-upstream-changes, r=bitcartel
InitialBlockDownload upstream changes

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7208
- bitcoin/bitcoin#8007
- bitcoin/bitcoin#9053
  - Excluding second commit (requires bitcoin/bitcoin#8865)
- bitcoin/bitcoin#10388
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment