CTestNetParams and CRegTestParams extend directly from CChainParams #6381

jtimon · 2015-07-06T16:03:39Z

As mentioned by @theuni on #6359 , the hierarchy between the different chainparam implementations is not necessary. I think that in fact it makes each of them less readable (since the leave part of their initialization to parent classes).

It also simplifies the introduction of new chains, like in #6382.

rustyrussell · 2015-07-07T06:56:17Z

Seems fairly trivial and clean. Tested on regtest, testnet and mainnet.

sipa · 2015-07-09T16:21:28Z

src/chainparams.cpp

I'd rather not see the 'data' (the actual prefix bytes) move to code. Can you pass them in as parameters instead, and call this function from each of the constructors?

I think we should only keep a base58PrefixStyle here and move everything else to base58.o in a later PR.

I don't think that's a good idea. All chain-specific data should be here, IMHO. Minimize the number of places where code needs to be changed to add a new chain.

Exactly, that's what I'm doing in #6382.
To create a new chain you just choose one of the existing base58Prefix styles and you're good to go.
Unless you need to create a new base58Prefix style...when is that a good idea anyway?
In any case, it seems you're not against anything I'm doing here (which reduces the number of places where code needs to be changed to add a new chain), but you would prefer that I go beyond this with respect to base58 prefixes.
To me the next step is decoupling base58 from Params() global-like function, but that's not as simple as it may seem.
Things can be improved in later PRs: here I'm doing the minimum to eliminate inheritance between the different CChainParams implementations.

I consider the base58 prefixes to be a property of a chain, and something that should be unique to it. I don't see why a new chain should keep using the base58 style of another chain.

So, no, I think the actual prefixes should stay right where they are. Moving them to a different function feels like unnecessarily changing data into code, and moving it to a different file is even worse.

I think you're too focussed on DRY to see the bigger picture: the reason you don't want to repeat yourself is because if the same logic is implemented in several places, you don't want to change two things when improving one. But this is not the case here: it's simple data that happens to be the same for both chains.

What I find worse is the fact that chain-specific data used to be nicely grouped per chain. This breaks that for no good reason.

There's nothing wrong with not repeating yourself either (or I've failed to
understand what is wrong with it. I think that the fact that testnet and
regtest is what is historical and there's no need to have different
prefixes with each chain. In fact, as said, it doesn't scale. For example,
I'm creating N new chains for sizetest, should they have different
prefixes? IMO either main or test prefixes are fine for it, there's really
no good reason to ever introduce a third one (or I haven't hear it yet).
On Jul 10, 2015 12:03 AM, "Pieter Wuille" [email protected] wrote:

In src/chainparams.cpp
#6381 (comment):

* database.

* CBlock(hash=000000000019d6, ver=1, hashPrevBlock=00000000000000, hashMerkleRoot=4a5e1e, nTime=1231006505, nBits=1d00ffff, nNonce=2083236893, vtx=1)

* CTransaction(hash=4a5e1e, ver=1, vin.size=1, vout.size=1, nLockTime=0)

* CTxIn(COutPoint(000000, -1), coinbase 04ffff001d0104455468652054696d65732030332f4a616e2f32303039204368616e63656c6c6f72206f6e206272696e6b206f66207365636f6e64206261696c6f757420666f722062616e6b73)

* CTxOut(nValue=50.00000000, scriptPubKey=0x5F1DF16B2B704C8A578D0B)

* vMerkleTree: 4a5e1e

*/
+static CBlock CreateGenesisBlock(uint32_t nTime=1231006505, uint32_t nNonce=2083236893, uint32_t nBits=0x1d00ffff, int32_t nVersion=1, const CAmount& genesisReward=50 * COIN)
+{

const char* pszTimestamp = "The Times 03/Jan/2009 Chancellor on brink of second bailout for banks";

CScript genesisOutputScript = CScript() << ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f") << OP_CHECKSIG;

return CreateGenesisBlock(pszTimestamp, genesisOutputScript, nTime, nNonce, nBits, nVersion, genesisReward);
+}

+static void AssignBase58PrefixStyle(std::vector* base58Prefixes, Base58PrefixStyle base58PrefixStyle)

The fact that regtest and testnet use the same style addresses is
historical IMHO. There is nothing wrong with copying the values - they just
happen to have the values, for unrelated reasons. Maybe some day we want to
change the regtest ones without changing the testnet one.

—
Reply to this email directly or view it on GitHub
https://github.com/bitcoin/bitcoin/pull/6381/files#r34309159.

That seems odd to me. I think different network should have different prefixes. Otherwise you risk using the wrong address on the wrong chain...

If you always use the same prfix there's no such risk. Again, what could go
wrong with Alpha or Freicoin for not having unique (again, this doesn't
scale) prefixes?

In any case, I'm not only focusing on DRY: I just don't understand what's
wrong with the static function (assuming there's going to be prefixes
reuse).
On Jul 10, 2015 12:26 AM, "Pieter Wuille" [email protected] wrote:

In src/chainparams.cpp
#6381 (comment):

* database.

* CBlock(hash=000000000019d6, ver=1, hashPrevBlock=00000000000000, hashMerkleRoot=4a5e1e, nTime=1231006505, nBits=1d00ffff, nNonce=2083236893, vtx=1)

* CTransaction(hash=4a5e1e, ver=1, vin.size=1, vout.size=1, nLockTime=0)

* CTxIn(COutPoint(000000, -1), coinbase 04ffff001d0104455468652054696d65732030332f4a616e2f32303039204368616e63656c6c6f72206f6e206272696e6b206f66207365636f6e64206261696c6f757420666f722062616e6b73)

* CTxOut(nValue=50.00000000, scriptPubKey=0x5F1DF16B2B704C8A578D0B)

* vMerkleTree: 4a5e1e

*/
+static CBlock CreateGenesisBlock(uint32_t nTime=1231006505, uint32_t nNonce=2083236893, uint32_t nBits=0x1d00ffff, int32_t nVersion=1, const CAmount& genesisReward=50 * COIN)
+{

const char* pszTimestamp = "The Times 03/Jan/2009 Chancellor on brink of second bailout for banks";

CScript genesisOutputScript = CScript() << ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f") << OP_CHECKSIG;

return CreateGenesisBlock(pszTimestamp, genesisOutputScript, nTime, nNonce, nBits, nVersion, genesisReward);
+}

+static void AssignBase58PrefixStyle(std::vector* base58Prefixes, Base58PrefixStyle base58PrefixStyle)

That seems odd to me. I think different network should have different
prefixes. Otherwise you risk using the wrong address on the wrong chain...

—
Reply to this email directly or view it on GitHub
https://github.com/bitcoin/bitcoin/pull/6381/files#r34311076.

sipa · 2015-07-09T22:27:31Z

Sorry for the rant above, @jtimon

It took me a while to see your point of view. If you think that there is no need for more than 2 base58 styles, I fully agree with the approach you're taking. You see "use mainnet or testnet style addresses" as the chain property. I see "what prefix byte to use" as the chain property.

…ChainParams ...instead of CMainParams and CTestNetParams respectively Do the same for CBaseChainParams. The inheritance was only reducing readibility in this case

jtimon · 2015-07-12T09:28:31Z

Needed rebase.
Yes, I don't think there's any need for more than a few styles (ie production, testing). Even just one would be ok IMO.
Anyway, since it is not essential to achieve its main goal, I've left the polemic static function AssignBase58PrefixStyle() out of this PR (it's still in #6382 for further discussion) .

sipa · 2015-07-14T21:33:36Z

Untested ACK.

laanwj · 2015-07-20T16:49:39Z

utACK.

As mentioned by @theuni on #6359 , the hierarchy between the different chainparam implementations is not necessary.

That has always been a gripe of me, too.
Do we need a class hierarchy at all? There is no network-specific code anymore - only the constructor differs. We could move a step further and make it completely data-driven.

jtimon · 2015-07-21T09:32:20Z

Yeah, I tried just using parametrized constructors for the factory in an alternate version of #6382 but the code didn't look better but actually worse (even using jtimon@25e99a0 in the unified constructor).
I think the only next step in "data-driven-ness" that it's worth taking is to directly load the values from files (ie main.dat, testnet3.dat, regtest.dat [maybe .json or something instead of .dat]).

Anyway, feel free to improve things later: this is just good enough for something like #6382.

laanwj · 2015-07-21T11:13:16Z

I think the only next step in "data-driven-ness" that it's worth taking is to directly load the values from files (ie main.dat, testnet3.dat, regtest.dat [maybe .json or something instead of .dat]).

Agreed. That could be kind of useful for testing.

c4973aa Chainparams: CTestNetParams and CRegTestParams extend directly from CChainParams (Jorge Timón) d3cf546 Chainparams: Introduce CreateGenesisBlock() static function (Jorge Timón)

…chy-0.11.99 Chainparams: Introduce CreateGenesisBlock() static function Chainparams: CTestNetParams and CRegTestParams extend directly from CChainParams ...instead of CMainParams and CTestNetParams respectively Do the same for CBaseChainParams. The inheritance was only reducing readibility in this case

Bitcoin 0.12 chainparams cleanups Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6222 - bitcoin/bitcoin#6381 - bitcoin/bitcoin#6473 - bitcoin/bitcoin#6242 Part of #2074.

jtimon mentioned this pull request Jul 6, 2015

DEPENDENT: Chainparams: Introduce N testnet chains to test different block sizes #6382

Closed

12 tasks

jtimon force-pushed the chainparams-nohierarchy-0.11.99 branch from 421de17 to 7cc689e Compare July 6, 2015 16:35

sipa reviewed Jul 9, 2015
View reviewed changes

jtimon added 2 commits July 12, 2015 11:03

Chainparams: Introduce CreateGenesisBlock() static function

d3cf546

Chainparams: CTestNetParams and CRegTestParams extend directly from C…

c4973aa

…ChainParams ...instead of CMainParams and CTestNetParams respectively Do the same for CBaseChainParams. The inheritance was only reducing readibility in this case

jtimon force-pushed the chainparams-nohierarchy-0.11.99 branch from 7cc689e to c4973aa Compare July 12, 2015 09:23

laanwj added the Refactoring label Jul 17, 2015

laanwj merged commit c4973aa into bitcoin:master Jul 21, 2015

laanwj added a commit that referenced this pull request Jul 21, 2015

Merge pull request #6381

45d1f59

c4973aa Chainparams: CTestNetParams and CRegTestParams extend directly from CChainParams (Jorge Timón) d3cf546 Chainparams: Introduce CreateGenesisBlock() static function (Jorge Timón)

dagurval mentioned this pull request Mar 14, 2017

Chainparams backports bitcoinxt/bitcoinxt#189

Merged

str4d mentioned this pull request Mar 28, 2017

Bitcoin 0.12 chainparams cleanups zcash/zcash#2217

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

CTestNetParams and CRegTestParams extend directly from CChainParams #6381

CTestNetParams and CRegTestParams extend directly from CChainParams #6381

Uh oh!

Conversation

jtimon commented Jul 6, 2015

Uh oh!

rustyrussell commented Jul 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Jul 9, 2015

Uh oh!

jtimon commented Jul 12, 2015

Uh oh!

sipa commented Jul 14, 2015

Uh oh!

laanwj commented Jul 20, 2015

Uh oh!

jtimon commented Jul 21, 2015

Uh oh!

laanwj commented Jul 21, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants