CTestNetParams and CRegTestParams extend directly from CChainParams

jtimon commented at 4:03 pm on July 6, 2015: contributor

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.

jtimon force-pushed on Jul 6, 2015

rustyrussell commented at 6:56 am on July 7, 2015: contributor

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

in src/chainparams.cpp: in 7cc689ee6d outdated

53+    const char* pszTimestamp = "The Times 03/Jan/2009 Chancellor on brink of second bailout for banks";
54+    CScript genesisOutputScript = CScript() << ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f") << OP_CHECKSIG;
55+    return CreateGenesisBlock(pszTimestamp, genesisOutputScript, nTime, nNonce, nBits, nVersion, genesisReward);
56+}
57+
58+static void AssignBase58PrefixStyle(std::vector<unsigned char>* base58Prefixes, Base58PrefixStyle base58PrefixStyle)

sipa commented at 4:21 pm on July 9, 2015:

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?

jtimon commented at 7:52 pm on July 9, 2015:

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

sipa commented at 8:13 pm on July 9, 2015:

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.

jtimon commented at 8:24 pm on July 9, 2015:

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.

sipa commented at 8:27 pm on July 9, 2015:

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.

jtimon commented at 8:36 pm on July 9, 2015:

Then the only way to decouple base58 from the Params globals is passing a CChainParams const reference (not as easy as it may seem either).

But I still don’t understand why a new chain should have new prefixes (doesn’t seem to scale to many chains anyway). Neither Freicoin nor Alpha have new prefixes, what can go wrong with that? Specially assuming payment protocol increases in usage.

sipa commented at 8:49 pm on July 9, 2015:

Whether they should or not doesn’t really matter… the fact is that base58 prefixes are chain-specific property. Moving the base58 prefixes out seems like a hack to avoid the dependency.

You could create a Base58Prefixes object, defined based base58.h, but constructed and kept inside CChainParams, and retrievable using Params().GetBase58Prefixes(), and pass that to the necessary base58 functions?

jtimon commented at 9:01 pm on July 9, 2015:

I’m not insisting on moving them to base58, I just want base58 decoupled from the chainparams’ globals, not so important that it’s also decoupled from chainparams.

You could create a Base58Prefixes object, defined based base58.h, but constructed and kept inside CChainParams, and retrievable using Params().GetBase58Prefixes(), and pass that to the necessary base58 functions?

That sounds good to me and would also fully decouple base58 from chainparams. But as said that would greatly increase the total diff of this PR: this can be improved further later.

sipa commented at 9:26 pm on July 9, 2015:

In my opinion, this PR makes the situation worse. It moves chain-specific data into a shared function. I agree with the goal you’re trying to reach, but I don’t think we should make the code uglier (even temporarily) in order to achieve a non-urgent goal?

jtimon commented at 9:42 pm on July 9, 2015:

I disagree. You need to reuse, at least, the test style for regtest. If you don’t have an external function like this you will need CMainBase58Prefixes and CTestBase58Prefixes or something equivalent. I think this is the simplest way to reuse that code and I disagree it’s ugly. But if you have another proposal that doesn’t involve touching base58 I’m happy to replace this. But remember, being able to reuse the testnet’s code in regtest is important, os the data simply can’t be on each chainparams constructor (or you would repeat yourself). This is simple DRY, I don’t understand why we’re wasting so much time on this. I can’t understand how this “makes the situation worse”.

sipa commented at 10:02 pm on July 9, 2015:

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.

sipa commented at 10:08 pm on July 9, 2015:

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.

sipa commented at 10:10 pm on July 9, 2015:

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.

jtimon commented at 10:12 pm on July 9, 2015:

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” notifications@github.com wrote:

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

* 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.

sipa commented at 10:25 pm on July 9, 2015:

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

jtimon commented at 10:30 pm on July 9, 2015:

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” notifications@github.com wrote:

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

* 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 commented at 10:27 pm on July 9, 2015: member

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: Introduce CreateGenesisBlock() static function d3cf546ec2

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

c4973aaaf6

jtimon force-pushed on Jul 12, 2015

jtimon commented at 9:28 am on July 12, 2015: contributor

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 commented at 9:33 pm on July 14, 2015: member

Untested ACK.

laanwj added the label Refactoring on Jul 17, 2015

laanwj commented at 4:49 pm on July 20, 2015: member

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 commented at 9:32 am on July 21, 2015: contributor

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 https://github.com/jtimon/bitcoin/commit/25e99a045138ba25d21fa028144d729fb94ebaf3 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 commented at 11:13 am on July 21, 2015: member

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.

laanwj merged this on Jul 21, 2015

laanwj closed this on Jul 21, 2015

laanwj referenced this in commit 45d1f5932b on Jul 21, 2015

jtimon referenced this in commit eee594428a on Jul 21, 2015

zkbot referenced this in commit e1c68f0631 on Jan 19, 2018

zkbot referenced this in commit 4f77ce5cb1 on Jan 22, 2018

zkbot referenced this in commit cc571a3ccd on Jan 22, 2018

zkbot referenced this in commit 50c23880c7 on Jan 22, 2018

zkbot referenced this in commit a4a020de7b on Jan 22, 2018

MarcoFalke locked this on Sep 8, 2021

CTestNetParams and CRegTestParams extend directly from CChainParams #6381