Skip to content

Conversation

@jtimon
Copy link
Contributor

@jtimon jtimon commented Nov 6, 2014

Continues #4804.
Deprecate -testnet and -regtest in favor of -network=testnet and -network=regtest.
When the old args are used a warning will be logged.
The repetition of the invalid combination error string is also avoided.
Additionally, the chainparam networkID is removed since it's not used anymore (as it shouldn't).

@jtimon jtimon changed the title Generic chainparam selection with -network=<network> Generic chainparam selection with -network=<strNetworkID> Nov 6, 2014
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't actually return anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will correct it.

@jgarzik
Copy link
Contributor

jgarzik commented Nov 6, 2014

The value of removing -testnet and -regtest is unclear.

Also, please test and make sure bitcoin-cli and bitcoin-tx utilities remain consistent with bitcoind.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 times "testnet"? Guess this should read "regtest".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ops, I will correct it.

@jtimon
Copy link
Contributor Author

jtimon commented Nov 6, 2014

@jgarzik -testnet and -regtest still work, I'm just logging a warning to discourage its use.
I thought that we can remove them later.
About the errors, I will catch them (like in the qt case) and return false for consistency.

@laanwj
Copy link
Member

laanwj commented Nov 7, 2014

NACK on removing/deprecating -testnet and -regtest. Selecting the network is a primary function of the client, so it can be a top-level option.

@jtimon
Copy link
Contributor Author

jtimon commented Nov 7, 2014

I think it has been a misunderstanding. This PR doesn't disable -testnet and -regtest, just allows using -network instead. #5238 does disable them.
Maybe I should replace "is deprecated" with "will be deprecated" (maybe that only makes sense after we have at least one more network [since unitest is unlikely to be selected on startup]).
As said in #5238, we cannot disable these flags without first providing the alternative and warning about the old flags for a while (maybe one release? next major release? 0.12? 1.0? 5 years from now? I don't know). This is just a first step which also makes it easier to add new networks.
I've always used "deprecated" as "still enabled but with a recommended alternative, will eventually disappear", like the java anotation @deprecated. It seems everyone else means "disabled, it used to be enabled in the past".

@laanwj
Copy link
Member

laanwj commented Nov 7, 2014

I just don't want them to be deprecated. People rely on them, and they work fine. Forcing people to use another option seems like a boondoggle.

I'm not against also adding -network= - but do we really plan on adding more networks in the future? What is the use-case here for Bitcoin?

@jtimon
Copy link
Contributor Author

jtimon commented Nov 7, 2014

So are you against the warning messages?
For bitcoin, right now it only adds the ability to select "unittest" which I admit may not be very useful.
I don't know if we will add another mode anytime soon, but it makes it easier to do so in the future.
It also simplifies things for people maintaining their own custom modes locally.
Finally, If the final goal is to load chainparams from files I think this is a step in the right direction.

@laanwj
Copy link
Member

laanwj commented Nov 7, 2014

Indeed, remove the warning messages. It makes sense to keep those quick shortcut option to select regtest and testnet (but not for more obscure networks like unittest).

@jtimon
Copy link
Contributor Author

jtimon commented Nov 7, 2014

Warnings removed. I'm fine just not adding new network shortcuts from now on.

@jtimon
Copy link
Contributor Author

jtimon commented Nov 8, 2014

Should I squash already?

@petertodd
Copy link
Contributor

NACK on removing/deprecating -testnet and -regtest.

@jtimon
Copy link
Contributor Author

jtimon commented Nov 10, 2014

@petertodd I told @jgarzik and then @Diapolo that this does not disable -testnet or -regtest.
Then @laanwj complained about the warnings saying they are deprecated and I removed the warnings too. Please, read the code, or at least the thread.
Maybe it is less clear due to all the "squashme" commits, I'll squash now...

@jtimon jtimon force-pushed the chainparams4 branch 2 times, most recently from 33371b6 to 83eb80a Compare November 13, 2014 19:44
@jtimon
Copy link
Contributor Author

jtimon commented Jan 4, 2015

Closing until #5598 gets merged.

@jtimon jtimon closed this Jan 4, 2015
@jtimon jtimon reopened this Jan 7, 2015
@jtimon
Copy link
Contributor Author

jtimon commented Jan 7, 2015

Reopened and rebased after #5598 has been merged. Now it's easier to review.

@petertodd
Copy link
Contributor

Rather that having the argument be "-network" why don't we rename it to "-chain"?

Network is a term that's already overloaded in multiple contexts. After all, we called it chain params rather than network params.

On 7 January 2015 18:48:08 GMT-05:00, "Jorge Timón" [email protected] wrote:

Reopened and rebased after #5598 has been merged. Now it's easier to
review.


Reply to this email directly or view it on GitHub:
#5229 (comment)

@jtimon
Copy link
Contributor Author

jtimon commented May 22, 2015

Added 3 commits to hopefully make it more interesting.
The added tests are kind of redundant, but won't hurt.
I'm specially interested on @theuni 's opinion on (jtimon@65e2a72) commit named "Chainparams: Use a regular factory for creating chainparams"] (since I know he wants to eventually separate the state-related functions out, but ParamsFactory() doesn't rely on any state).

@jtimon
Copy link
Contributor Author

jtimon commented May 22, 2015

Removed jtimon@ac1c555 since is now a proposal for the trivial branch (theuni#27).

To better read commit "Chainparams: The hash of the genesis block it's the genesis checkpoint and chain id", please rgrep hashGenesisBlock

All mentions of it are whether to:

  • Its own asignments, of course.
  • Make sure the genesis block is in a block index we're maintaining, like in ./init.cpp:1112, of course.
  • Asserts in chainparams.cpp (no need to aditionally check genesis.hashMerkleRoot), let's keep this, why not?
  • Avoid unnecessary checkpoints for the genesis block, in case we want to apply what's inside it somewhere, for example: ./main.cpp:1708, ./main.cpp:2641, ./main.cpp:2732, ./main.cpp:3385, ./main.cpp:3400, ./main.cpp:3495

Why the heck are we trying to check the genesis block in the first place?
The genesis block is not something to test, it is actually the first rule of the chain, it has to be right or the chain rules are broken (softforks and hardforks will come at a later nHeight).

But what about checkpoints?

The logic of checkpoints can always safely rely on the first checkpoint being true.
The last point may not be true if three's a reorg behind it, otherwise you're ready to optimize.
We could introduce a configurable third layer of "this is my new genesis no matter what", which would be good for pruning and skipping all validations on blocks that are too old to believe they can be reorg-ed, but this would only really make sense with some form of utxo commitment (did I say that append-only-txo + append-only-confirmed is enough for this?).

Therefore, we should not:

  • Disperse hardcoded values.
  • Double-check hashes of hashes
  • Leave checkpoints without the only true first safe checkpoint: the genesis block

@jtimon jtimon force-pushed the chainparams4 branch 2 times, most recently from 1b5fda1 to 424a85c Compare May 23, 2015 09:45
@jtimon
Copy link
Contributor Author

jtimon commented May 23, 2015

Sorry, pushed too fast. Now I use containers instead of those ugly ClearSelectedParams functions, and the first commit actually compiles (now all of them do).

@jtimon jtimon force-pushed the chainparams4 branch 2 times, most recently from c7db474 to c535d85 Compare May 23, 2015 10:05
@jtimon
Copy link
Contributor Author

jtimon commented May 27, 2015

Closing for now, I will open a version with some of the changes but without adding the new option.

@jtimon jtimon closed this May 27, 2015
@jtimon
Copy link
Contributor Author

jtimon commented Jun 4, 2015

This is the promised version with some of the improvements but without adding the new option: #6229
Opened for discussion.

@jtimon jtimon closed this Jun 4, 2015
jtimon referenced this pull request in sipa/bitcoin Apr 6, 2016
Rebased by Pieter Wuille.
Cleanup by Matt Corallo.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants