Improvements in CChainParams

jtimon commented at 7:08 am on March 8, 2014: contributor

Replace direct references to RegTest() with constant parameters on CChainParams interface. EDIT: dependency merged

laanwj commented at 7:18 am on March 8, 2014: member

@jtimon You’re creating a lot of minor pulls that depend on each other. Submitting it as one pull grouping multiple git commits would make it easier to review and generates less clutter.

jtimon commented at 3:06 pm on March 8, 2014: contributor

@laanwj only this one is dependent on the others. The rest are valid independently, just related. Well, by depedent you may mean that since they change common files they need to be rebased on top of each other. I thought this would increase the chances each of them would have to be accepted. My reasoning was more or less like “if the commit at #3819 is not accepted for some reason, the other commits will be automatically rejected and I will have to start again from scratch or commit on top of that previous ‘invalid’ commit”. I also thought that it would be easier to review them individually, but, yes, they’re all related, so probably is easier to understand the purpose by putting them all together and also the history will have less merges.

So I have no problem putting them all on a single pull request, but to save me some pain, could you please tell me if there’s any problem with each of the individual changes (or improvement, renaming of method…) before I do that?

Note: It is the first time that I use git in a project with this level of concurrency in development so I appreciate your patience.

laanwj commented at 3:54 pm on March 8, 2014: member

Well people’s opinion may vary but I generally like ‘one pull request solves one issue’. Too granular makes indeed it hard to merge (especially if they’re ‘change the world’ commits), but too fine and everyone loses track of the larger view of what you’re trying to do.

This may be just right, I just noticed a flood of pulls and thought I’d warn you :-)

It does make sense to split up GUI and core changes. Merging GUI changes is much easier, in general.

jtimon commented at 4:39 pm on March 8, 2014: contributor

Thanks again for the feedback, much appreciated. So I guess the optimal approach would have been to request pulling the gui change first (#3823) and then the rest together warning that they’re dependent on that one. I’ll leave them as they are for now since, as said, I’m concerned about people having issues with one of them individually (and honestly, also due to laziness: I don’t want to work more on this until I hear what more people have to say about this).

Probably I should have referenced our previous discussion at #3812

jtimon commented at 2:01 am on March 10, 2014: contributor

I’ve putted them all together except the GUI change. So now this is only dependent on #3823 (The test won’t pass until that’s merged).

in src/chainparams.cpp: in 8a1635ec4f outdated

112@@ -112,6 +113,7 @@ class CMainParams : public CChainParams {
113         nRPCPort = 8332;
114         bnProofOfWorkLimit = CBigNum(~uint256(0) >> 32);
115         nSubsidyHalvingInterval = 210000;
116+        nMinerThreads = boost::thread::hardware_concurrency();

sipa commented at 3:51 pm on March 10, 2014:

I don’t like chainparams accessing the OS, makes it an ugly dependency for people who want to use this module in other code. I prefer it to just contain constants.

mikehearn commented at 6:13 pm on March 10, 2014: contributor

Does this actually make the code clearer, though?

Now when checking the various places regtest mode was special cased, you can see that a special case exists but you have no idea without rummaging through chainparams.cpp (reading all of it) when it will actually happen.

Was this change motivated by some other work or do you think it’s just clearer this way?

gavinandresen commented at 6:49 pm on March 10, 2014: contributor

I prefer the old code, NACK from me on this change.

sipa commented at 7:01 pm on March 10, 2014: member

I really hate how we’re assigning weird properties to regtest, and making them only accessible under IsRegTest(). Imho, these should just be separate command-line options or separate RPCs, instead of overloading their meaning for regtest.

sipa commented at 7:02 pm on March 10, 2014: member

So to be clear: I consider this pullreq a step towards making special behaviour not hardcoded to regtest, but it’s not enough.

jtimon commented at 7:03 pm on March 10, 2014: contributor

The motivation is to make it easier to create new modes. Particularly, I plan to create a mode where proof of work is completely replaced by signatures of a “centralized miner”. Other full nodes can connect and validate, but only whoever can provide a valid scriptSig can produce new blocks.

Also RegTest() global-like function feels a bit ugly to me, which brings me to some related questions I have:

Shouldn’t be TestNet() and RegTest() be methods of CChainParams? Shouldn’t there be consistency in the use of “Params().NetworkID() == CChainParams::REGTEST” vs RegTest() ?

Maybe a virtual bool CChainParams::isNetwork(Network) could be a solution.

sipa commented at 7:04 pm on March 10, 2014: member

Meh, all the same. I dislike explicit tests for particular modes. The high-level code should not need to know nor care about which mode is active. It should just care about its properties.

sipa commented at 7:05 pm on March 10, 2014: member

Apart from the boost:hardware_concurrence call, ACK from me.

mikehearn commented at 7:06 pm on March 10, 2014: contributor

Leaving aside the style issues for a moment - such a “centralised miner” mode seems pointless. You could just as well have a regular SQL database and dispose with the block chain algorithm entirely at that point?

sipa: as I said, I feel like that’s applying style theory over practice here. In practice, just testing properties of the mode simply makes the code harder to understand without improving modularity or maintainability in any real way.

sipa commented at 7:10 pm on March 10, 2014: member

I think reading:

if (Params().MineBlocksOnDemand()) weird special-case code

is just as obvious as

if (RegTest()) weird special-case code

Though the question should really be: why is this behaviour specific to regtest at all?

mikehearn commented at 7:28 pm on March 10, 2014: contributor

The former simply repeats logic redundantly:

if (Params().MineBlocksOnDemand()) { // Mine some blocks, because the user demanded it. }

The conditional tells you nothing beyond “this behaviour is network mode specific” and requires you to go hunting in order to find out more.

if (RegTest()) { // Mine blocks on demand }

is much more informative - it tells you when that code path actually becomes active, in easy to read language.

It’s specific to regtest because that’s the only time it would appear to be useful. If someone came up with another case where it was wanted (that doesn’t involve weird entirely non-Bitcoinish uses) then it’d make sense to generalise it some more and document when it can happen with a comment.

jtimon commented at 8:07 pm on March 10, 2014: contributor

How is any of this different from RequireRPCPassword() ?

It is true that the private chain is “non-bitcoinish” (related to off-chain transactions) and it’s probably only useful with more changes that won’t possibly get into bitcoin. I would just like to minimize the divergence from bitcoin’s codebase. But still, this makes it easier to create more testing modes I honestly haven’t thought about yet.

Anyway, we should focus on the readability and maintainability, and I still think RegTest() is pretty ugly. Any thoughts on the consistency of the use of these global functions as opposed to methods in CChainParams interface?

jtimon commented at 10:48 pm on March 10, 2014: contributor

@mikehearn we can also change the comment to be non-redundant and avoid you to “hunt on chainparams.cpp” if that’s the problem.

if (Params().MineBlocksOnDemand()) { // Used on regtest mode } vs if (RegTest()) { // Mine blocks on demand }

maaku commented at 9:38 pm on March 11, 2014: contributor

I’m not sure how RegTest() is in any way descriptive of what what is going on unless you happen to already know that the bitcoin pull-tester uses a regression testing mode to mine blocks on demand. Params().MineBlocksOnDemand() clearly says what is being tested for.

laanwj commented at 7:31 am on March 12, 2014: member

As I understood it (and explained to @jtimon) the idea of ChainParams is to contain

properties of the chain itself and (like genesis)
environmental properties that define how bitcoind handles this chain (like RequireRPCPassword)

Especially the second set of properties could made data-driven (moved to a configuration file) to make it possible to define your own testing modes.

Using a clunky ‘is a’ operation like RegTest() breaks this. So IMO this pull is an improvement. @jtimon Please do document the new methods of CChainParams clearly with a comment: what does this flag do, and why would someone want to do that. @sipa +1 CChainParams should only contain constants. The boost concurrency call is not a constant and depends on the execution environment.

jtimon commented at 0:56 am on March 15, 2014: contributor

Updates:

-Non-constant value for default threads removed as suggested by @sipa -References to regtest added/restored and redundancies removed on comments as suggested by @mikehearn -New methods documented in the interface as suggested by @laanwj

sipa commented at 1:26 am on March 15, 2014: member

ACK.

Enough bikeshedding.

jtimon commented at 1:30 am on March 15, 2014: contributor

Fixed typo “doesn’t requires” -> “doesn’t require”

jtimon renamed this:
~~Get rid of RegTest()~~
Improvements in CChainParams
on Mar 22, 2014

jtimon commented at 7:35 pm on March 22, 2014: contributor

Also stop using TestNet(), now Params.NetworkID() is only called from 3 places:

https://github.com/bitcoin/bitcoin/blob/master/src/qt/bitcoin.cpp#L526 https://github.com/bitcoin/bitcoin/blob/master/src/checkpoints.cpp#L86 https://github.com/bitcoin/bitcoin/blob/master/src/util.cpp#L998

I would say the latest changes complete the pull request which almost achieves the goal of chain modes “quacking like a duck”.

in src/chainparams.h: in d04c83dbdd outdated

57@@ -58,9 +58,29 @@ class CChainParams
58     int GetDefaultPort() const { return nDefaultPort; }
59     const CBigNum& ProofOfWorkLimit() const { return bnProofOfWorkLimit; }
60     int SubsidyHalvingInterval() const { return nSubsidyHalvingInterval; }
61+    /* Used to check majorities for block version upgrade */
62+    int EnforceBlockUpgradeMajority() const { return nEnforceBlockUpgradeMajority; }
63+    int RejectBlockOutdatedMajority() const { return nRejectBlockOutdatedMajority; }
64+    int ToCkeckBlockUpgradeMajority() const { return nToCkeckBlockUpgradeMajority; }

laanwj commented at 7:50 am on April 4, 2014:

Shouldn’t ToCkeck be ToCheck ?

jtimon commented at 6:04 pm on April 5, 2014: contributor

Thanks @laanwj, corrected. Also getmininginfo and getinfo will cointain “testnet”: True for regtest, so this is a change in behavior but I think it’s more appropriate (as regtest is also for testing). Ultimately I would add a new field “network” or “mode” to replace “testnet”, although it may be still needed for backwards compatibility. If people believe it should return False for regtest I can correct it as well. This is the relevant commit: https://github.com/jtimon/bitcoin/commit/2154c6e30e8a52a7ff9bae3a1ab938157016f437

laanwj commented at 6:04 am on April 7, 2014: member

~~I’m all in favor of adding a ’network’ ID to ‘getinfo’ output. The GUI uses CChainParams::DataDir() to get a network identifier for display in the debug window (and replaces it with ‘mainnet’ if empty). But let’s add method to CChainParams that returns a canonical network ID string, and use the in the GUI as well. In any case, that’s something for another pull. ~~ (edit: this was added to getnetworkinfo)

Returning ’true’ for ’testnet’ on the regtest network in RPC output is OK with me.

maaku commented at 2:57 pm on April 7, 2014: contributor

Although not as human friendly, the magic byte string serves this purpose. On Apr 6, 2014 11:05 PM, “Wladimir J. van der Laan” < notifications@github.com> wrote:

I’m all in favor of adding a ’network’ ID to ‘getinfo’ output. The GUI uses CChainParams::DataDir() to get a network identifier for display in the debug window (and replaces it with ‘mainnet’ if empty). But let’s add method to CChainParams that returns a canonical network ID string, and use the in the GUI as well. In any case, that’s something for another pull.

Returning ’true’ for ’testnet’ on the regtest network in RPC output is OK with me.

Reply to this email directly or view it on GitHubhttps://github.com/bitcoin/bitcoin/pull/3824#issuecomment-39698559 .

laanwj added this to the milestone 0.10.0 on May 5, 2014

laanwj closed this on May 20, 2014

jtimon commented at 11:18 am on May 21, 2014: contributor

I’m confused. Why did this PR got closed? Should I rebase it on top of the latest changes? Is there anything else I can do to get it accepted? Is there any reason why these (in my opinion, pretty harmless) refactoring changes will never make it into bitcoind?

laanwj commented at 12:25 pm on May 21, 2014: member

Huh?!?!

I don’t remember closing this. I must have misclicked. Sorry!

laanwj reopened this on May 21, 2014

sipa commented at 11:27 am on June 1, 2014: member

Rebase please?

jtimon commented at 2:24 pm on June 1, 2014: contributor

Rebased

in src/chainparams.h: in d866193d25 outdated

67     virtual const CBlock& GenesisBlock() const = 0;
68     virtual bool RequireRPCPassword() const { return true; }
69+    /* Make miner wait to have peers to avoid wasting work */
70+    virtual bool MiningRequiresPeers() const { return true; }
71+    /* Default value for -checkmempool argument */
72+    virtual bool CheckMemPool() const { return false; }

sipa commented at 8:30 am on June 2, 2014:

Not: maybe “DefaultCheckMemPool”, as to not confuse people that this is the actual setting for checking the mempool or not?

in src/chainparams.h: in d866193d25 outdated

65+    /* Used if GenerateBitcoins is called with a negative number of threads */
66+    int DefaultMinerThreads() const { return nMinerThreads; }
67     virtual const CBlock& GenesisBlock() const = 0;
68     virtual bool RequireRPCPassword() const { return true; }
69+    /* Make miner wait to have peers to avoid wasting work */
70+    virtual bool MiningRequiresPeers() const { return true; }

sipa commented at 8:33 am on June 2, 2014:

Nit: for these simple booleans, don’t use a virtual method, but a local field and an implementation in the base class that returns it. That means less indirection at runtime.

jtimon commented at 9:12 am on June 2, 2014:

I was just copying the other booleans. Should I change only the newly introduced booleans or all of them?

laanwj commented at 9:19 am on June 2, 2014:

@jtimon I think it makes sense for all the simple values, not just for booleans.

jtimon commented at 9:38 am on June 2, 2014:

including networkID? The rest are all booleans I think. I was just being consistent with what was there, but I have no problem changing that and have them in variables just as the other types.

By the way, is there a reason why FixedSeeds is declared as pure virtual but then only defined for mainparams?

laanwj commented at 10:59 am on June 2, 2014:

Maybe I’m missing something, I don’t see the reason why virtuals are used here at all. All the methods of CChainsParams and subclasses simply return [references to] properties of the object (no input arguments or state). They could be moved to the superclass, and the only difference between the classes would be the constructor. Then one wouldn’t even need a subclass per network at all. They could just be three instances of the base class, making the entire thing data-driven.

… what am I missing?

sipa commented at 11:16 am on June 2, 2014:

Virtual methods really only make sense when there’s methods that take arguments, and the computation depends on them. Until we have such cases (which perhaps wouldn’t fit in the name “parameters” anymore), I think we can drop all the virtuals.

jtimon commented at 11:30 am on June 2, 2014:

With only constant values, maybe a reason the keep the subclasses is to use Inheriting constructors, but they’re not being used. If in the future there’s methods with inputs that may behave differently on each class, so maybe we should maintain them just in case. Otherwise we could just have a factory that takes the network id as parameter and returns an instance, reusing the same parametrized constructor of the same class for all networks. The current virtual methods are definitively not needed, but I will maintain the subclasses in this pull request.

sipa commented at 11:31 am on June 2, 2014:

+1

jtimon commented at 11:50 am on June 2, 2014:

Should the boolean variables start with b? Is there a style guide?

sipa commented at 11:55 am on June 2, 2014:

If you want to follow Satoshi’s style (which a lot of the code does, even new code, but we don’t really bother enforcing it anymore), boolean variables should start with an ‘f’ (from flag), and floating point ones with a ’d’ (double).

laanwj commented at 12:34 pm on June 2, 2014:

With a factory function you could even load the network settings from a json file. But agreed, no need to do so in this pull. Let’s just get rid of the virtuals and get this merged.

There is a style guide in doc/coding.md. That said, it really needs expanding, it doesn’t even mention how much to indent and when to use a new line after {} etc. And I think satoshi’s style with type prefixes on variables is crazy.

jtimon commented at 1:30 pm on June 2, 2014:

Ok, even if I don’t specially like the type prefix convention I think I should follow it. Removal of virtual methods it is.

sipa commented at 8:34 am on June 2, 2014: member

ACK

jtimon commented at 11:03 am on June 4, 2014: contributor

Virtual methods removed. In the process chainparams.h had to include protocol.h and a constant (MESSAGE_START_SIZE) was moved to protocol.h to avoid including chainparams.h from there. On net.h a line used map instead of std::map, which didn’t previously fail because net.h included protocol.h which included chainparams.h Those problems are solved. Now chainparams.h is only included from .cpp files and base58.h

Maybe the last two commits should go together?

jtimon commented at 11:28 am on June 4, 2014: contributor

It works locally, I guess I need to rebase again?

Add MiningRequiresPeers chain parameter 1712adbe0b

Add MineBlocksOnDemand chain parameter bfa9a1a638

Add DefaultMinerThreads chain parameter 2595b9ac23

Add DefaultCheckMemPool chain parameter cb9bd83bba

Get rid of RegTest() 8d26721498

Move majority constants to chainparams d754f34e8d

Add AllowMinDifficultyBlocks chain parameter 21913a9ac9

Add RequireStandard chain parameter cfeb8235fd

Add RPCisTestNet chain parameter 6fc0fa63d9

Get rid of TestNet() a3d946ebdc

Replace virtual methods with static attributes, chainparams.h depends on

protocol.h instead of the other way around

c8c52de3a0

net.h was using std namespace through chainparams.h included in protocol.h 2871889e83

sipa commented at 11:38 am on June 4, 2014: member

paymentserver.cpp:502: error: “TestNet” was not declared in this scope

jtimon commented at 12:15 pm on June 4, 2014: contributor

This is the commit and line that breaks the PR: https://github.com/bitcoin/bitcoin/commit/bdc83e8f450456c9f547f1c4eab43571bac631c2#diff-7823229685ea2f6633c45d94ebab1aa9R502

Feedback by @Diapolo welcomed. Right now it doesn’t fail if the network in the details is “regtest”. If that is never expected,

0    if ((details.network() == "main" && TestNet()) ||
1        (details.network() == "test" && !TestNet()))

Could be replace with:

0    if ((details.network() == "main" && Params().NetworkID() != CChainParams::MAIN) ||
1        (details.network() == "test" && Params().NetworkID() != CChainParams::TESTNET) ||
2        (details.network() != "test" && details.network() != "main" ))

If we want to support regtest, the following would be better:

0    if ((details.network() == "main" && Params().NetworkID() != CChainParams::MAIN) ||
1        (details.network() == "test" && Params().NetworkID() != CChainParams::TESTNET) ||
2        (details.network() == "regtest" && Params().NetworkID() != CChainParams::REGTEST))

Another possibility is creating a new networkStr parameter and do something like this:

0    if (details.network() != Params().NetworkStr())

In this case, I would have preferred that the NetworkStr parameter contained “testnet” instead of “test”, but maybe it’s too late to change that on details.network()?

Thoughts?

jtimon commented at 1:28 pm on June 4, 2014: contributor

I created a mapping networkID -> Payment Protocol network name method as suggested by @wumpus on IRC instead of creating a new field in chainparams for that.

Use Params().NetworkID() instead of TestNet() from the payment protocol f0a83fc256

BitcoinPullTester commented at 2:10 pm on June 4, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f0a83fc256023f68cc046bd096de69f16ce9d394 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

sipa commented at 11:09 pm on June 8, 2014: member

ACK

laanwj commented at 8:24 am on June 9, 2014: member

ACK

ghost commented at 9:08 am on June 9, 2014: none

ACK

laanwj merged this on Jun 9, 2014

laanwj closed this on Jun 9, 2014

laanwj referenced this in commit 62fdf381fa on Jun 9, 2014

in src/miner.cpp: in f0a83fc256

651@@ -652,8 +652,9 @@ void GenerateBitcoins(bool fGenerate, CWallet* pwallet, int nThreads)
652     static boost::thread_group* minerThreads = NULL;
653 
654     if (nThreads < 0) {
655-        if (Params().NetworkID() == CChainParams::REGTEST)
656-            nThreads = 1;
657+        // In regtest threads defaults to 1
658+        if (Params().DefaultMinerThreads())

Diapolo commented at 5:02 pm on June 10, 2014:

@jtimon Are you sure this check is correct? Seems like a hack to use the 0 in DefaultMinerThreads() to query for regtest mode, no?

jtimon commented at 10:56 am on June 11, 2014:

That’s the point of this PR, not to have to check for regtest at all. Right now the default is 0 for main and testnet, and 1 for regtest. But now you can create a testnet2 or regtest2 which defaults at 5, for example.

sipa commented at 10:58 am on June 11, 2014:

Perhaps the comment shouldn’t explicitly refer to regtest anymore either :)

jtimon commented at 11:21 am on June 11, 2014:

@mikehearn suggested that is was clearer since otherwise you need to go to chainparams to know if it’s regtest behaviour, that’s why I restored the old comments or added comments referencing regtest or testnet in some places.

laanwj commented at 12:18 pm on June 11, 2014:

In principle a boolean ‘MineWithMultipleThreads’ would have been good enough here, as I don’t see a realistic reason why a network would want ‘5 mining threads!’. Then again, that seems like bikeshed painting, this is fine.

jtimon commented at 1:37 pm on June 11, 2014:

yes, since it’s only regtest what uses 1 as default, a boolean would have served as well. an int seemed more generic, but it is true that the chances of it being used at all are almost null.

in src/rpcmining.cpp: in f0a83fc256

173@@ -174,7 +174,7 @@ Value setgenerate(const Array& params, bool fHelp)
174     }
175 
176     // -regtest mode: don't return until nGenProcLimit blocks are generated
177-    if (fGenerate && Params().NetworkID() == CChainParams::REGTEST)
178+    if (fGenerate && Params().MineBlocksOnDemand())

Diapolo commented at 5:06 pm on June 10, 2014:

@jtimon Can you take a look a few lines below at GenerateBitcoins(fGenerate, pwalletMain, 1);. We use the supplied 1 as thread-number, so Params().DefaultMinerThreads() seems totally unused at all.

jtimon commented at 11:15 am on June 11, 2014:

It is functionally equivalent to what was previously doing, so if the check is unused it was unused before.

Right now MineBlocksOnDemand set to true (only used by regtest), makes the number of threds default to 1, overwritting the defaultThreads variable (again, only regtest has a non-zero value here). So this whole check in miner.cpp seems meaninless unless you’re using miner.cpp without using rpcmining.cpp (it seems there are some calls from init.cpp too)

The check is inside a nThreads < 0 anyway, so that seems to indicate that somebody is actually using negative values when calling the function to get the default number of threads (or boost’s default if the default in chainparams is 0, like in the case of main and testnet).

With the changes in the PR the check in miner.cpp would be also used if you created a mode that has a default different than 0, but you don’t set the blocksOnDemand flag to true.

But maybe the 1 in rpcmining should be replaced with the default (which is going to be 1 for regtest) when blocksOnDemand is true (only for regtest right now).

Diapolo commented at 12:50 pm on June 11, 2014:

I think it’s too much logic for too litle that comes out here…

jtimon commented at 1:38 pm on June 11, 2014:

I agree that this can probably be simplified and maybe some of the parameters I have created can be eliminated some time in the future, but I didn’t want to alter the behaviour in this PR, just refactor. Now that the different special cases don’t depend on testnet or regtest, it should be easier to think about them and simplify the code around the usage of the parameters.

wumpus commented at 5:23 pm on June 11, 2014: none

@jtimon, please stop mixing irc and github. I’m not the guy you’re talking to on irc.

jtimon commented at 5:52 pm on June 11, 2014: contributor

I’m sorry, my mistake, I believe wumpus on irc is @laanwj here.

Michagogo commented at 7:04 pm on June 11, 2014: contributor

@jtimon That’s correct.

patricklodder referenced this in commit a354834cfe on Feb 2, 2015

patricklodder referenced this in commit b7edffaf1f on Feb 2, 2015

DrahtBot locked this on Sep 8, 2021

Improvements in CChainParams #3824