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

mikehearn commented at 3:55 PM on May 8, 2013: contributor

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 commented at 3:57 PM on May 8, 2013: contributor

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 commented at 2:50 PM on May 9, 2013: member

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 commented at 2:52 PM on May 9, 2013: contributor

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

petertodd commented at 6:30 PM on May 9, 2013: contributor

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 commented at 10:07 PM on May 17, 2013: contributor

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 commented at 11:57 AM on June 5, 2013: contributor

Rebased onto latest master.

in src/bitcoinrpc.cpp:None in 9eff73d63d outdated

 724 | @@ -724,8 +725,8 @@ static void RPCAcceptHandler(boost::shared_ptr< basic_socket_acceptor<Protocol,
 725 |  void StartRPCThreads()
 726 |  {
 727 |      strRPCUserColonPass = mapArgs["-rpcuser"] + ":" + mapArgs["-rpcpassword"];
 728 | -    if ((mapArgs["-rpcpassword"] == "") ||
 729 | -        (mapArgs["-rpcuser"] == mapArgs["-rpcpassword"]))
 730 | +    if (((mapArgs["-rpcpassword"] == "") ||
 731 | +         (mapArgs["-rpcuser"] == mapArgs["-rpcpassword"])) && Params().RequireRPCPassword())

laanwj commented at 4:41 PM on June 5, 2013:

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

sipa commented at 3:06 PM on June 15, 2013:

Agree.

petertodd commented at 3:26 PM on June 17, 2013:

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.

laanwj commented at 10:01 AM on June 18, 2013:

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.

mikehearn commented at 10:13 AM on June 18, 2013:

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.

laanwj commented at 10:48 AM on June 18, 2013:

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.

sipa commented at 1:29 PM on June 18, 2013:

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 commented at 10:04 AM on June 6, 2013: contributor

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 commented at 11:40 AM on June 6, 2013: member

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

mikehearn commented at 1:49 PM on June 6, 2013: contributor

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 commented at 4:40 PM on June 7, 2013: member

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

mikehearn commented at 9:37 AM on June 14, 2013: contributor

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?

in src/chainparams.cpp:None in a2b69057e5 outdated

   0 | @@ -0,0 +1,267 @@
   1 | +// Copyright (c) 2010 Satoshi Nakamoto
   2 | +// Copyright (c) 2009-2012 The Bitcoin developers
   3 | +// Distributed under the MIT/X11 software license, see the accompanying
   4 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +
   6 | +#include "assert.h"
   7 | +
   8 | +#include "chainparams.h"
   9 | +#include "main.h"

sipa commented at 9:50 AM on June 14, 2013:

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.

mikehearn commented at 9:58 AM on June 14, 2013:

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.

sipa commented at 10:28 AM on June 14, 2013:

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

in src/util.cpp:None in a2b69057e5 outdated

1067 | @@ -1068,8 +1068,8 @@ void PrintExceptionContinue(std::exception* pex, const char* pszThread)
1068 |      } else {
1069 |          path = GetDefaultDataDir();
1070 |      }
1071 | -    if (fNetSpecific && GetBoolArg("-testnet", false))
1072 | -        path /= "testnet3";
1073 | +    if (fNetSpecific)
1074 | +        path /= Params().DataDir();

sipa commented at 3:18 PM on June 15, 2013:

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

mikehearn commented at 8:11 AM on June 17, 2013:

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

sipa commented at 8:46 AM on June 17, 2013:

Great, no problem in that case.

in src/main.cpp:None in a2b69057e5 outdated

4541 | @@ -4513,8 +4542,12 @@ void static BitcoinMiner(CWallet *pwallet)
4542 |      unsigned int nExtraNonce = 0;
4543 |  
4544 |      try { loop {
4545 | -        while (vNodes.empty())
4546 | -            MilliSleep(1000);
4547 | +        if (Params().NetworkID() != CChainParams::REGTEST) {
4548 | +            // Busy-wait for the network to come online so we don't waste time mining
4549 | +            // on an obsolete chain. In regtest mode we expect to fly solo.

sipa commented at 3:23 PM on June 15, 2013:

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.

mikehearn commented at 8:11 AM on June 17, 2013:

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

sipa commented at 1:30 PM on June 18, 2013:

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 commented at 3:24 PM on June 15, 2013: member

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

mikehearn commented at 8:12 AM on June 17, 2013: contributor

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

mikehearn commented at 10:54 AM on June 17, 2013: contributor

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 commented at 1:03 PM on June 17, 2013: contributor

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 commented at 3:32 PM on June 17, 2013: contributor

ACK, with some taste complaints

in src/chainparams.cpp:None in 1a0aac9e09 outdated

 269 | +
 270 | +void SelectParamsFromCommandLine() {
 271 | +    bool fRegTest = GetBoolArg("-regtest", false);
 272 | +    bool fTestNet = GetBoolArg("-testnet", false);
 273 | +
 274 | +    if (fRegTest) {

petertodd commented at 3:53 PM on June 17, 2013:

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

in src/chainparams.h:None in 1a0aac9e09 outdated

  94 | +inline bool TestNet() {
  95 | +    // Note: it's deliberate that this returns "false" for regression test mode.
  96 | +    return Params().NetworkID() == CChainParams::TESTNET;
  97 | +}
  98 | +
  99 | +#endif

petertodd commented at 3:54 PM on June 17, 2013:

nitpick: add a newline here

petertodd commented at 4:02 PM on June 17, 2013: contributor

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

sipa commented at 10:01 AM on June 19, 2013: member

@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 commented at 11:20 AM on June 19, 2013: contributor

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 commented at 11:55 AM on June 19, 2013: contributor

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 commented at 12:06 PM on June 19, 2013: none

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:

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)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
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 commented at 12:35 PM on June 19, 2013: member

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 commented at 12:54 PM on June 19, 2013: member

@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 implementation of some CBlockLocator methods

Move out of main.h to improve compile times and add documentation
for what the methods do.

70e7fba06d

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.

0e4b317555

mikehearn commented at 2:31 PM on June 19, 2013: contributor

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

BitcoinPullTester commented at 2:42 PM on June 19, 2013: none

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:

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)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
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.

jgarzik commented at 2:45 PM on June 19, 2013: contributor

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 commented at 3:03 PM on June 19, 2013: member

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 commented at 3:09 PM on June 19, 2013: member

ACK @laanwj You have any comments still?

mikehearn commented at 3:11 PM on June 19, 2013: contributor

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

sipa referenced this in commit 01b45731b7 on Jun 22, 2013

sipa merged this on Jun 22, 2013

sipa closed this on Jun 22, 2013

DrahtBot locked this on Sep 8, 2021