Make halving interval parametric using configuration
In order to test all the possible scenarios that can be faced before or after an halving it can be useful to set in regtest mode the halving interval. So I've just added a parameter (-halvinginterval) that can be passed to bitcoind daemon in regtest mode in order to override the default halving interval.
In order to test all the possible scenarios that can be faced before or after an halving it can be useful to set in regtest mode the halving interval.
So I've just added a parameter (-halvinginterval) that can be passed to bitcoind daemon in regtest mode in order to override the default halving interval.
Why not. Concept ACK. What about hiding this option under showDebug as this is regtest only?
Yes sure, have a look here: https://github.com/Conio/bitcoin/blob/37584b2191d1d7bc8cc39822b488abd989780994/src/init.cpp#L400-L416
245 | class CRegTestParams : public CChainParams {
246 | public:
247 | CRegTestParams() {
248 | strNetworkID = "regtest";
249 | - consensus.nSubsidyHalvingInterval = 150;
250 | + consensus.nSubsidyHalvingInterval = DEFAULT_REGTEST_HALVING_INTERVAL;
What speaks against directly use
consensus.nSubsidyHalvingInterval = GetArg("-halvinginterval", DEFAULT_REGTEST_HALVING_INTERVAL);
... and remove the UpdateSubsidyHalvingIntervalParameter function loop?
Yes, that was the initial solution I have opted for, but then I discovered that CRegTestParams is constructed before ReadConfigFile is called and the hasmaps GetArg read were empty, so it returned always the DEFAULT_REGTEST_HALVING_INTERVAL. I noticed that few lines below in https://github.com/Conio/bitcoin/blob/37584b2191d1d7bc8cc39822b488abd989780994/src/chainparams.cpp#L344-L347 was used this function to update bip39 parameters so I decided to go for a similar solution.
Concept ACK.
Sorry, I need some help to understand why this is useful, especially why it should be possible to set a halving interval less than it takes the coinbase to mature.
test all the possible scenarios that can be faced before or after an halving
What relevant scenarios are there that are not present with the default halving interval? Also, why is there need to test them, considering that there are no plans to change the halving interval for bitcoin main net?
You're right @MarcoFalke, I didn't think about an halving interval lower than the coinbase maturity and maybe this should be avoided. Thank you! By the way, please let me try to explain my point of view while coding this pull request. In our pipeline we run a series of tests which stimulate the generation of more than 150 blocks to be completed. Far from testing the halving mechanism, we normally generate blocks in order to produce new Bitcoins to be spent in our test scenarios. After a while, though, the reward drops to zero and this causes failures in our CI environment as we can not generate any more Bitcoins. We believe this could be a very common scenario in a CI environment. Since it seems hard to find an arbitrary value which would fit everyone's needs, I thought it could come in handy to leave the choice to developers. Also, this would allow to re-run the suite consecutively without having the miners reward to zero after few thousands blocks.
You could send smaller amounts in regtest to avoid draining the wallet too fast, but utACK 37584b2 from me.
Not so sure about this from a design perspective. We've kind of been here. We switched forth and back between mutable and non-mutable parameters in chainparams.cpp:
This does the same, in yet another way. Back then we decided we didn't actually need modifyable parameters for testing.
Did that change again?
The last post may sound excessively negative, I'm not against doing this (again) if it can result in better test coverage. But we should think how to go at it, instead of just slapping on arguments.
Another thing proposed as future idea was to be able to load the entire test chain definition from a configuration file. This would be preferable to adding a command line option and special-purpose interface per individual parameter (ping @jtimon).
Concept ACK. This would be very useful for some of the testing that I do at BitPay.
Closing this for now. The patch itself looks fine and is of good quality, but it is not clear that we want to take this direction. Please feel free to open an issue to track ideas/brainstorm on this topic.
+1
+1 I have the same CI problem
Yeah, #8994 could solve this "once and for all" but making all chainparams configurable for regtest (in comand line or in config file, or at one point I had a version from a separated config file only) instead of doing them one by one. Well, actually not for regtest since regtest conserves a different genesis block hash, for "custom chains". All that without introducing any setters to chainparams, making sure they're immutable after initialization. It needs rebase and have some nits. People also have been going back and forth about some things. For example, at first you had to chose -chain=custom and then -petname=mychainname (the name is used in the genesis block to easily create different genesis blocks). then someone suggested that the name could be taken directly from -chain, allowing any value and just reserving regtest, test and main as special values, which I did. Now somebody fears that someone wanting regtest may typo -chain=regtet and use a custom chain without noticing #8994 (review) But otherwise I think there's not that many proposed changes to that PR that haven't been proposed yet. Oh, I guess it would be nice to chose which ones to document, perhaps all, perhaps none? I asked somewhere in the PR IIRC, but now it's too long and it's kind of a pain to look for old discussions.