[consensus] Make coinbase maturity a network parameter instead of an app-wide constant #9595

pull kallewoof wants to merge 2 commits into bitcoin:master from kallewoof:consensus-coin-maturity changing 9 files +15 −13

kallewoof commented at 3:50 AM on January 20, 2017: member

The coinbase maturity is a constant in consensus/consensus.h but ideally should be a parameter on a per network basis. Although all networks right now have the same coinbase maturity, there is no reason why this should be the case forever.
Consensus: coinbase maturity a consensus parameter rather than an app-wide constant. 44fa621669
kallewoof renamed this:
~~[consensus] make coinbase maturity a network parameter instead of an app-wide constant~~
[consensus] Make coinbase maturity a network parameter instead of an app-wide constant
on Jan 20, 2017
fanquake added the label Consensus on Jan 20, 2017

in src/test/test_bitcoin.cpp:None in 44fa621669 outdated

  98 | @@ -100,7 +99,8 @@ TestChain100Setup::TestChain100Setup() : TestingSetup(CBaseChainParams::REGTEST)
  99 |      // Generate a 100-block chain:
 100 |      coinbaseKey.MakeNewKey(true);
 101 |      CScript scriptPubKey = CScript() <<  ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG;
 102 | -    for (int i = 0; i < COINBASE_MATURITY; i++)
 103 | +    int coinbaseMaturity = Params().GetConsensus().coinbaseMaturity;

dcousens commented at 5:35 AM on January 20, 2017:

const?

kallewoof commented at 5:39 AM on January 20, 2017:

You mean const int coinbaseMaturity = [...]? Sure thing.

dcousens commented at 5:35 AM on January 20, 2017: contributor

concept ACK
NicolasDorier commented at 6:04 AM on January 20, 2017: contributor

ACK 90bf56f
theuni commented at 6:09 AM on January 20, 2017: member

concept ACK, but please don't mix in all of the include/whitespace changes.

Surely that header is still needed in most cases, no? Just because it's indirectly include already doesn't mean the explicit include should be removed.
[to-squash] Restored includes where consensus/consensus.h content was being used and added const to coinbaseMaturity where it was retrieved. ca96a5463d
kallewoof commented at 6:21 AM on January 20, 2017: member

@theuni Sorry about whitespace. Fixed. I went through and restored the include in the files that were using other values from consensus/consensus.h.
kallewoof force-pushed on Jan 20, 2017
morcos commented at 6:55 PM on January 20, 2017: member

Can someone explain the rationale for this change?

I don't see any reason to over generalize the code unless we anticipate wanting to take advantage of it and I feel the code is slightly cleaner and clearer as existing.
TheBlueMatt commented at 6:56 PM on January 20, 2017: member

I generally always thought of the consensus params stuff as parameters which change between networks. Unless we want a testnet with a different parameter here I'm not sure why bother?
kallewoof commented at 9:02 PM on January 20, 2017: member

It never made sense to me that it was not a part of the network parameters, even if they happen to be the same for all 3 networks. (Personally I don't see why regtest requires a 100 coinbase, but that's a separate topic, I guess.)
luke-jr commented at 11:34 PM on January 20, 2017: member

NACK, I agree with @morcos. There's no reason to have this specific constant as a network parameter unless we are planning to make every single constant such also.
MarcoFalke added the label Brainstorming on Jan 20, 2017
kallewoof commented at 1:08 AM on January 23, 2017: member

I guess you're right that it doesn't make a lot of sense and is easy enough to change if there ever is a need for a new network with a different coin base maturity. Closing. Thanks for the feedback! :)
kallewoof closed this on Jan 23, 2017
kallewoof deleted the branch on Oct 17, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Brainstorming Consensus