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

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

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.

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

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

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?

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

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.

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

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.

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.

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

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.

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?

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.

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?

@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
}

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.

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.

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

ACK.

Enough bikeshedding.

Fixed typo "doesn't requires" -> "doesn't require"

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

This is the commit and line that breaks the PR: bdc83e8#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,

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

Could be replace with:

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

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

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

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

    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?

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.

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.

ACK

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

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

@jtimon That's correct.