Refactor chain-specific tweaks into a CChainParams class and introduce a regtest mode #2632
Conversation
|
The pull tester fails because I deleted the now obsolete patch file it relies on. Matt said he'll update pull tester after it's merged to use the new flag. |
|
ACK on the general idea, but if we're going this way, we should probably do it all the way. Given that 0.8.2 becomes sort-of urgent, I'd rather pull this afterwards. |
|
That's fine, I can finish it off in the coming weeks. |
|
Nice! An option to set the network magic bytes would be good to allow you to setup a temporary private testnet easily if you are doing something that shouldn't be broadcast to testnet main. (like creating large bloat-blocks that you don't want to burden everyone else with) I found out the hard way a while back when I was playing with max-sized blocks that information is easy to spread and difficult to stifle - I'd firewalled bitcoin but forgotten my tor daemon still had a hidden service running. Also we need a way to set the keypair alerts are signed with on the command line. I'll look at the patch more after the conference. |
|
Maybe too many little commits here, but essentially this completes removal of fTestNet. You can still find out if you're on testnet (or a derivative) with a simple if test, so it's not any less convenient, but most of the variable stuff has moved to CChainParams. The few places that didn't move are things like the alternative block rules for testnet or the checkpoint definitions, where trying to refactor that out didn't seem appropriate. |
|
Rebased onto latest master. |
There was a problem hiding this comment.
Not really against this, but it feels a bit strange to have the RPC security settings depend on the chain.
There was a problem hiding this comment.
Given that regtest mode uses a different data directory, and thus different wallet, I'll grudgingly accept this. Feels like a security issue waiting to happen though, albeit a very rare one.
There was a problem hiding this comment.
I agree it's a security issue waiting to happen in a future code change.
I'd strongly prefer a different solution here, ie one where the password is
set to empty in the testing code instead of bypassing the check.
There was a problem hiding this comment.
As pointed out, everything is compartmentalised. Do you have a specific security concern here or not? To repeat the reasons for this - regtest mode is not only for running regression tests (perhaps it should be renamed), but also useful for ad-hoc app development. In that situation it's very convenient to not have to mess around with RPC users or passwords. Because no real money is at stake, having RPC security is rather pointless.
There was a problem hiding this comment.
I agree it is convenient for testing. But the extent in which testing and
production code are mixed worries me a bit. I have no specific security
concern but it feels fragile.
There was a problem hiding this comment.
I I'd prefer not having the network config not influence client-level config like this. A --unsafe-allow-empty-rpc-password option would be a better fit, but I don't care that much.
|
Ugh. It looks I screwed up with a force push and the work I did on another machine got lost. The last commits I worked on in the states are gone and I can't find a way to get them back. So don't review or merge this right now. I'll have to redo the last parts of removing fTestNet again :-( |
|
Nothing left of it in your local git reflog? That usually saves me in cases |
|
Unfortunately the work was done on a laptop I don't have access to anymore and was since wiped. If there's no copy retrievable from github servers then the work is gone :-( It was only a few commits extra to clear out the last uses of fTestNet, nothing I can't quickly reproduce. But it's annoying. |
|
@mikehearn If you don't expect this pull to be ready soon, you'll likely need to rebase on top of #2154. |
|
I've rebased this and finished off the whole thing. Could I get a review for the last set of commits and some LGTMs/ACKs please? |
There was a problem hiding this comment.
Presumably whoever wants to move CBlock can solve that? I mean, main.cpp is
pretty fundamental in the current codebase. I don't see a way to avoid
using CBlock without having a really weird design.
|
ACK apart from a few minor nits (see inline). |
|
Thanks. Looking for one more ACK. Gavin/Jeff? |
|
Sorry Pieter, two more commits to look at. First fixes a bug I introduced and I could squash it back down. The second is new - I noticed that "bitcoind -regtest help" didn't work any more and noticed that chain params were never being configured properly for sending RPCs. The reason it used to work is that in my old code regtest mode used the mainnet rpc port. Seemed better to configure params properly in command line mode too. |
|
OK, replaced the last commits with one that fixes the unsigned warning, fixes a bug with repeated spamming of fixed seed nodes I noticed whilst testing regtest mode and makes "./bitcoind -regtest help" work by using the params for command line rpcs too. |
|
ACK, with some taste complaints |
src/chainparams.cpp
Outdated
There was a problem hiding this comment.
nitpick: add a fRegTest && fTestNet error message to be clear both options aren't valid.
|
ACK, though all I did was review the code line-by-line - no testing. |
|
@mikehearn You'll need to fix the Qt code too: |
|
Oops, how did I forget that? It turns out that the Qt code not only uses fTestNet, but in some places also just calls GetBoolArg as well. What a mess. Will work on it. |
|
OK, I tested the gui in main and testnet mode, it seems to be working. Probably now the reviews are done, it's simpler to squash all the commits before merging. Otherwise the Qt GUI will be broken between some range of the commits and it's a pain to go back and fix that. |
|
Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/39cdc2d76159a02b47fa23b3d15359b869a17959 for binaries and test log. This could happen for one of several reasons:
If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here. 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/ |
|
The only place it just calls GetBoolArg should be in main, on purpose: to |
|
@mikehearn Yes, please try to keep every commit a usable state - otherwise you risk breaking git bisect. Also can you have a look at @petertodd's nits? After that, I think we can merge. |
Move out of main.h to improve compile times and add documentation for what the methods do.
The new class is accessed via the Params() method and holds most things that vary between main, test and regtest networks. The regtest mode has two purposes, one is to run the bitcoind/bitcoinj comparison tool which compares two separate implementations of the Bitcoin protocol looking for divergence. The other is that when run, you get a local node which can mine a single block instantly, which is highly convenient for testing apps during development as there's no need to wait 10 minutes for a block on the testnet.
|
OK, I addressed Peter's comments and squashed into one commit. Thanks for the reviews, everyone. |
|
Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/0e4b31755534fac4ea6c20a60f719e3694252220 for binaries and test log. This could happen for one of several reasons:
If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here. 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/ |
|
ACK Note that @sipa only said each commit needs to build on its own -- which does not necessarily imply all commits must be squashed together, only that commits should each be able to exist on their own, buildable and testable. |
|
Well, "move chain-specifc stuff to separate class" is reasonable to have in one commit - it could be multiple as well, I don't care. I only asked that the CBlockLocation move to .cpp remained separate as it's not related. |
|
ACK @laanwj You have any comments still? |
|
Note that the pull tester will need fixing after this goes in. |
Refactor chain-specific tweaks into a CChainParams class and introduce a regtest mode
7 participants
Matt Corallo's bitcoind/bitcoinj comparison tool relies on a patch that sets difficulty to zero, thus ensuring blocks can be found instantly without any delay. It also drops the subsidy halving interval so that can be tested and makes a few other tweaks.
This series of patches creates a new CChainParams abstraction that hides various things that vary between main, test and regression test networks. It then adds a regtest mode triggered by the -regtest flag that disables external connections and uses the "insta-block" rule set. Also, the miner thread will shut down once it finds a block, meaning you can generate a single block from the command line on demand by running
./bitcoind -regtest setgenerate true
This change is incomplete - some things like DNS seeds, checkpoints and difficulty transition formulas aren't refactored out yet. However I wanted to get feedback on the approach before continuing, and the current code is useful even though there's still more refactoring to go.
Signed off by: Matt Corallo