Use a proper factory for creating chainparams #8855
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jtimon
force-pushed
the
0.13-chainparams-factory
branch
2 times, most recently
from
45a4a48 to
183c143
Compare
Contributor
Author
|
Pushed with more changes added to the OP. |
jtimon
force-pushed
the
0.13-chainparams-factory
branch
from
183c143 to
0de6821
Compare
jtimon
force-pushed
the
0.13-chainparams-factory
branch
from
0de6821 to
180ee2a
Compare
Contributor
Author
|
Added 1 commit. |
jtimon
force-pushed
the
0.13-chainparams-factory
branch
from
180ee2a to
3d5a1af
Compare
12 tasks
jtimon
force-pushed
the
0.13-chainparams-factory
branch
from
3d5a1af to
cfa5c2b
Compare
Contributor
Author
|
Needed rebase |
jtimon
force-pushed
the
0.13-chainparams-factory
branch
from
cfa5c2b to
3ae62e4
Compare
Contributor
Author
|
Needed rebase to adapt to a new test using Params(std::string). |
jtimon
force-pushed
the
0.13-chainparams-factory
branch
from
3ae62e4 to
eb6c595
Compare
Contributor
Author
|
Needed rebase for bench/checkblock.cpp again |
sipa
reviewed
Nov 15, 2016
src/chainparamsbase.cpp
Outdated
Member
There was a problem hiding this comment.
Any reason why this does not return a unique_ptr?
sipa
reviewed
Nov 15, 2016
src/chainparamsbase.h
Outdated
Member
There was a problem hiding this comment.
I think this should be a function rather than a static method.
src/bitcoin-cli.cpp
Outdated
Member
There was a problem hiding this comment.
Lines like this (as there are many) can avoid some redundancy by using const auto defaultBaseParmams = ... instead.
src/chainparams.h
Outdated
Member
There was a problem hiding this comment.
I think this should be a function rather than a static method.
jtimon
force-pushed
the
0.13-chainparams-factory
branch
from
eb6c595 to
dd882d1
Compare
Contributor
Author
|
Updated, hopefully fixing all nits. |
jtimon
commented
Nov 18, 2016
src/chainparamsbase.cpp
Outdated
Contributor
Author
There was a problem hiding this comment.
auto-nit: why remove this?
jtimon
commented
Nov 18, 2016
src/test/pow_tests.cpp
Outdated
Contributor
Author
There was a problem hiding this comment.
Also all these could remain const, will push shortly...
sipa
reviewed
Nov 18, 2016
Member
sipa
left a comment
sipa
left a comment
There was a problem hiding this comment.
utACK dd882d15c846da56a3338626be4e411a003eb8b5
src/chainparams.cpp
Outdated
Member
There was a problem hiding this comment.
μnit: you can just use assert(globalChainParams) (std::unique_ptr has an implicit conversion to bool, which checks whether there is an owned object),
Contributor
Author
There was a problem hiding this comment.
Remains from boost::scoped_ptr, fixing now...
jtimon
force-pushed
the
0.13-chainparams-factory
branch
2 times, most recently
from
ea5175d to
e0e2a3e
Compare
Contributor
Author
|
Solved auto-nits, also rebased. |
morcos
reviewed
Nov 18, 2016
src/chainparamsbase.h
Outdated
Contributor
There was a problem hiding this comment.
Is this comment out of date now?
Contributor
Author
There was a problem hiding this comment.
Right, the caller doesn't have to explicitly delete the object anymore, thanks.
jtimon
force-pushed
the
0.13-chainparams-factory
branch
from
e0e2a3e to
f9794a5
Compare
Contributor
Author
|
Updated and hopefully fixed latest small nits. Thanks again for the nits. |
7 tasks
laanwj
added a commit
that referenced
this pull request
Aug 22, 2017
…n default 4aa2508 Bugfix: Use testnet RequireStandard for -acceptnonstdtxn default (Luke Dashjr) Pull request description: Fixes a bug introduced in #8855 `-acceptnonstdtxn` is a valid option only for testnet/regtest (in Core), and the help message reflects that. Currently, however, it is buggy in two ways: 1. It uses mainnet to get the default value, which doesn't make sense since the option is never available for mainnet, and the only time the option is available, is when the default is the opposite. 2. It uses the value of "require standard" directly as the default for "accept non-standard transactions", but these concepts are opposites: a negation must be performed to transform one to the other. Note the combination of these bugs results in the correct boolean output, but the logic to get there is completely wrong. Tree-SHA512: 06ce513f59ba31f7ab4b6422a08a17bb37a5652dea4c38a4bbefedd5e2752d17bfccc32a4b0508068fa4783e316bff00a821ef18a24b1a2bb02859995d188fdc
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jun 22, 2019
c1082a7 Chainparams: Use the factory for pow tests (Jorge Timón) 2351a06 Chainparams: Get rid of CChainParams& Params(std::string) (Jorge Timón) f87f362 Chainparams: Use a regular factory for creating chainparams (Jorge Timón) Tree-SHA512: 359c8a2a1bc9d02db7856d02810240ada28048ac088f878b575597a7255cdb0ffdd1a647085ee67a34c6a7e7ed9e6cfdb61240cf6e75139619b640dbb096072c
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jun 26, 2019
c1082a7 Chainparams: Use the factory for pow tests (Jorge Timón) 2351a06 Chainparams: Get rid of CChainParams& Params(std::string) (Jorge Timón) f87f362 Chainparams: Use a regular factory for creating chainparams (Jorge Timón) Tree-SHA512: 359c8a2a1bc9d02db7856d02810240ada28048ac088f878b575597a7255cdb0ffdd1a647085ee67a34c6a7e7ed9e6cfdb61240cf6e75139619b640dbb096072c
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Dec 9, 2019
…onstdtxn default 4aa2508 Bugfix: Use testnet RequireStandard for -acceptnonstdtxn default (Luke Dashjr) Pull request description: Fixes a bug introduced in bitcoin#8855 `-acceptnonstdtxn` is a valid option only for testnet/regtest (in Core), and the help message reflects that. Currently, however, it is buggy in two ways: 1. It uses mainnet to get the default value, which doesn't make sense since the option is never available for mainnet, and the only time the option is available, is when the default is the opposite. 2. It uses the value of "require standard" directly as the default for "accept non-standard transactions", but these concepts are opposites: a negation must be performed to transform one to the other. Note the combination of these bugs results in the correct boolean output, but the logic to get there is completely wrong. Tree-SHA512: 06ce513f59ba31f7ab4b6422a08a17bb37a5652dea4c38a4bbefedd5e2752d17bfccc32a4b0508068fa4783e316bff00a821ef18a24b1a2bb02859995d188fdc
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jan 2, 2020
…onstdtxn default 4aa2508 Bugfix: Use testnet RequireStandard for -acceptnonstdtxn default (Luke Dashjr) Pull request description: Fixes a bug introduced in bitcoin#8855 `-acceptnonstdtxn` is a valid option only for testnet/regtest (in Core), and the help message reflects that. Currently, however, it is buggy in two ways: 1. It uses mainnet to get the default value, which doesn't make sense since the option is never available for mainnet, and the only time the option is available, is when the default is the opposite. 2. It uses the value of "require standard" directly as the default for "accept non-standard transactions", but these concepts are opposites: a negation must be performed to transform one to the other. Note the combination of these bugs results in the correct boolean output, but the logic to get there is completely wrong. Tree-SHA512: 06ce513f59ba31f7ab4b6422a08a17bb37a5652dea4c38a4bbefedd5e2752d17bfccc32a4b0508068fa4783e316bff00a821ef18a24b1a2bb02859995d188fdc
furszy
added a commit
to PIVX-Project/PIVX
that referenced
this pull request
Feb 10, 2021
…cture d1244f3 Fix UB in CChainParams unique_ptr subclasses destructor. (furszy) cf20f43 Chainparams: Get rid of CChainParams& Params(std::string) (Jorge Timón) eeea862 Chainparams: Use a regular factory for creating chainparams (Jorge Timón) f9d585a More zerocoin unused functions and classes cleanup. (furszy) 029ce7b pivx-cli align -rpcport help message to init.cpp help message. (furszy) 3431977 Chainparams: Translations: DRY: options and error strings Also remove SelectBaseParamsFromCommandLine and SelectParamsFromCommandLin (Jorge Timón) ce7a792 Removed unused AreBaseParamsConfigured() (furszy) 508fec1 Chainparams: Replace CBaseChainParams::Network enum with string constants (furszy) 2a1f16c Chainparams: CTestNetParams and CRegTestParams extend directly from CChainParams (furszy) 0ed1a48 Refactor `Params().NetworkID() == CBaseChainParams::TESTNET` for IsTestnet() direct call. (furszy) Pull request description: Made several updates to the base chain params structure and further cleanups over the zerocoin code: * Replaced `CBaseChainParams::Network` enum with string constants and unified better the network selection error + network help message (adaptation of dashpay#6235). * Removed unused NetworkID field from chain params (dashpay#5598). * Improved the network params creation using a proper factory (adaptation of bitcoin#8855). * Cleaned almost all the zerocoin related functions inside `zerocoin.h/cpp` and `zpivchain.h/cpp`. ACKs for top commit: random-zebra: ACK d1244f3 Fuzzbawls: ACK d1244f3 Tree-SHA512: c7ecb34045f9e14dc60130d3f7142c2f32c4c16f5678e871036db0b513ec3c416762a2464ff0d50383a50d345ea90f2c5958284657479c2becaaf9bf3936fcd3
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
2 global variables are currently enough for basechainparams and CChainParams, but we're
using N + 1 globals instead. Although N is likely to remain 3 for the
foreseeable future
(and not many people seemed interested in turning NI believe segwit was an example on how starting a new testchain can be very useful and anything that makes that easier is potentially valuable.into infinite in #6382)
Traceability: Replaces #6907.
Changes from final non-merged #6907:
UpdateBIP9Parameters since we don't have a separate global for
regtest anymore. An additional assert/error could be added if we
wanted to restrict its usage beyond regtest, bit I don't think the
restriction makes sense. Remember that Params() now returns a const ref and init.cpp was the only context was the only context where UpdateRegtestBIP9Parameters was called from.