Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor chain-specific tweaks into a CChainParams class and introduce a regtest mode #2632

Merged
merged 2 commits into from Jun 22, 2013

Conversation

@mikehearn
Copy link
Contributor

commented May 8, 2013

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

@mikehearn

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2013

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.

@sipa

This comment has been minimized.

Copy link
Member

commented May 9, 2013

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.

@mikehearn

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2013

That's fine, I can finish it off in the coming weeks.

@petertodd

This comment has been minimized.

Copy link
Contributor

commented May 9, 2013

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.

@mikehearn

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2013

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.

@mikehearn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2013

Rebased onto latest master.

if ((mapArgs["-rpcpassword"] == "") ||
(mapArgs["-rpcuser"] == mapArgs["-rpcpassword"]))
if (((mapArgs["-rpcpassword"] == "") ||
(mapArgs["-rpcuser"] == mapArgs["-rpcpassword"])) && Params().RequireRPCPassword())

This comment has been minimized.

Copy link
@laanwj

laanwj Jun 5, 2013

Member

Not really against this, but it feels a bit strange to have the RPC security settings depend on the chain.

This comment has been minimized.

Copy link
@sipa

sipa Jun 15, 2013

Member

Agree.

This comment has been minimized.

Copy link
@petertodd

petertodd Jun 17, 2013

Contributor

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.

This comment has been minimized.

Copy link
@laanwj

laanwj Jun 18, 2013

Member

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.

This comment has been minimized.

Copy link
@mikehearn

mikehearn Jun 18, 2013

Author Contributor

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.

This comment has been minimized.

Copy link
@laanwj

laanwj Jun 18, 2013

Member

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.

This comment has been minimized.

Copy link
@sipa

sipa Jun 18, 2013

Member

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.

@mikehearn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2013

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 :-(

@laanwj

This comment has been minimized.

Copy link
Member

commented Jun 6, 2013

Nothing left of it in your local git reflog? That usually saves me in cases
like this.

@mikehearn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2013

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.

@sipa

This comment has been minimized.

Copy link
Member

commented Jun 7, 2013

@mikehearn If you don't expect this pull to be ready soon, you'll likely need to rebase on top of #2154.

@mikehearn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2013

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?

#include "assert.h"

#include "chainparams.h"
#include "main.h"

This comment has been minimized.

Copy link
@sipa

sipa Jun 14, 2013

Member

This again introduces an implicit everything-depends-on-main issue, which #2154 solved. Would it be possible to write CChainParams without needing CBlock? Alternatively, maybe wait/rebase for #2758 so CBlock is in core.h.

This comment has been minimized.

Copy link
@mikehearn

mikehearn Jun 14, 2013

Author Contributor

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.

This comment has been minimized.

Copy link
@sipa

sipa Jun 14, 2013

Member

Ok, let's take care of that in #2758.

if (fNetSpecific && GetBoolArg("-testnet", false))
path /= "testnet3";
if (fNetSpecific)
path /= Params().DataDir();

This comment has been minimized.

Copy link
@sipa

sipa Jun 15, 2013

Member

Is this guaranteed to be a no-op on every platform, when the appended string is ""?

This comment has been minimized.

Copy link
@mikehearn

mikehearn Jun 17, 2013

Author Contributor

According to the documentation, yes it's a no-op:

http://www.boost.org/doc/libs/1_46_1/libs/filesystem/v3/doc/reference.html#path-appends

Appends the preferred directory separator to the contained pathname, unless ...... p.empty()

This comment has been minimized.

Copy link
@sipa

sipa Jun 17, 2013

Member

Great, no problem in that case.

MilliSleep(1000);
if (Params().NetworkID() != CChainParams::REGTEST) {
// Busy-wait for the network to come online so we don't waste time mining
// on an obsolete chain. In regtest mode we expect to fly solo.

This comment has been minimized.

Copy link
@sipa

sipa Jun 15, 2013

Member

I'd feel more comfortable with just a much lower sleep timeout, like 10ms. Just to prevent excessive CPU usage when for some reason the peer doesn't appear.

This comment has been minimized.

Copy link
@mikehearn

mikehearn Jun 17, 2013

Author Contributor

Maybe so, but it's a separate patch I think.

This comment has been minimized.

Copy link
@sipa

sipa Jun 18, 2013

Member

Well you're introducing the busy loop; I'd prefer not having it in the code in the first place.

EDIT: never mind, the context shown by github for this patch confused me. You're just not waiting for the network to come online in regtest mode - that's fine.

@sipa

This comment has been minimized.

Copy link
Member

commented Jun 15, 2013

ACK apart from a few minor nits (see inline).

@mikehearn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2013

Thanks. Looking for one more ACK. Gavin/Jeff?

@mikehearn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2013

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.

@mikehearn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2013

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.

@jgarzik

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2013

ACK, with some taste complaints

@petertodd

View changes

src/chainparams.cpp Outdated
bool fRegTest = GetBoolArg("-regtest", false);
bool fTestNet = GetBoolArg("-testnet", false);

if (fRegTest) {

This comment has been minimized.

Copy link
@petertodd

petertodd Jun 17, 2013

Contributor

nitpick: add a fRegTest && fTestNet error message to be clear both options aren't valid.

return Params().NetworkID() == CChainParams::TESTNET;
}

#endif

This comment has been minimized.

Copy link
@petertodd

petertodd Jun 17, 2013

Contributor

nitpick: add a newline here

@petertodd

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2013

ACK, though all I did was review the code line-by-line - no testing.

@sipa

This comment has been minimized.

Copy link
Member

commented Jun 19, 2013

@mikehearn You'll need to fix the Qt code too:

src/qt/clientmodel.cpp: In member function ‘bool ClientModel::isTestNet() const’:
src/qt/clientmodel.cpp:113:12: error: ‘fTestNet’ was not declared in this scope
@mikehearn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2013

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.

@mikehearn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2013

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.

@BitcoinPullTester

This comment has been minimized.

Copy link

commented Jun 19, 2013

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:

  1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in contrib/test-scripts)
  2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
  3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
  4. The test suite fails on either Linux i386 or Win32
  5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

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/
Contact BlueMatt on freenode if something looks broken.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jun 19, 2013

The only place it just calls GetBoolArg should be in main, on purpose: to
show the right icon immediately it needs to know whether the program is
starting in testnet mode before the fTestNet flag is set in Init.

@sipa

This comment has been minimized.

Copy link
Member

commented Jun 19, 2013

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

Mike Hearn
Move implementation of some CBlockLocator methods
Move out of main.h to improve compile times and add documentation
for what the methods do.
Mike Hearn
Introduce a CChainParameters singleton class and regtest mode.
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.
@mikehearn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2013

OK, I addressed Peter's comments and squashed into one commit. Thanks for the reviews, everyone.

@BitcoinPullTester

This comment has been minimized.

Copy link

commented Jun 19, 2013

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:

  1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in contrib/test-scripts)
  2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
  3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
  4. The test suite fails on either Linux i386 or Win32
  5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

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/
Contact BlueMatt on freenode if something looks broken.

@sipa

This comment has been minimized.

Copy link

commented on src/bitcoind.cpp in 0e4b317 Jun 19, 2013

Please use InitError() (which also works in GUI mode), and _() (to get it into translations).

This comment has been minimized.

Copy link

replied Jun 19, 2013

Ignore this - I mistakenly assumed this was init.cpp.

@jgarzik

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2013

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.

@sipa

This comment has been minimized.

Copy link
Member

commented Jun 19, 2013

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.

@sipa

This comment has been minimized.

Copy link
Member

commented Jun 19, 2013

ACK

@laanwj You have any comments still?

@mikehearn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2013

Note that the pull tester will need fixing after this goes in.

sipa added a commit that referenced this pull request Jun 22, 2013
Merge pull request #2632 from mikehearn/chainparams
Refactor chain-specific tweaks into a CChainParams class and introduce a regtest mode

@sipa sipa merged commit 01b4573 into bitcoin:master Jun 22, 2013

@Diapolo

This comment has been minimized.

Copy link

commented on src/init.cpp in 0e4b317 Jun 23, 2013

Seems you included chainparams.h twice...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.