CreateNewBlock: Unit tests and runtime validation check #1246

luke-jr commented at 5:25 PM on May 9, 2012: member

CreateNewBlock: Check that the produced CBlock is acceptable (except for proof-of-work and merkletree, since those need to be provided later)

This throws an exception from CreateNewBlock otherwise, which is not safe without #1245!

luke-jr commented at 12:38 AM on May 10, 2012: member

This is bugged. ConnectBlock requires the block to be on the disk already :(

luke-jr closed this on May 10, 2012

luke-jr commented at 12:50 AM on May 10, 2012: member

In fact, this might even corrupt the blkindex.dat somehow, so if you tried it, I suggest rebuilding with -loadblock :/

luke-jr reopened this on May 10, 2012

luke-jr commented at 4:49 AM on May 10, 2012: member

OK, this seems to work now.

luke-jr commented at 7:19 PM on May 19, 2012: member

Eligius has been running this from block 179513 (56 blocks found) and EclipseMC from 180573 (11 blocks), totalling 67 valid blocks with no problems found.

luke-jr commented at 5:45 AM on May 23, 2012: member

Now includes tests for CreateNewBlock

CreateNewBlock: Check that the produced CBlock is acceptable (except for proof-of-work and merkletree, since those need to be provided later)

This throws an exception from CreateNewBlock otherwise, which is not safe without #1245!

3cd01fdf0e

in src/main.cpp:None in 9567f7e3c4 outdated

1345 | @@ -1337,7 +1346,11 @@ bool CBlock::ConnectBlock(CTxDB& txdb, CBlockIndex* pindex)
1346 |      bool fStrictPayToScriptHash = (pindex->nTime >= nBIP16SwitchTime);
1347 |  
1348 |      //// issue here: it doesn't know the version
1349 | -    unsigned int nTxPos = pindex->nBlockPos + ::GetSerializeSize(CBlock(), SER_DISK, CLIENT_VERSION) - 1 + GetSizeOfCompactSize(vtx.size());
1350 | +    unsigned int nTxPos;
1351 | +    if (fJustCheck)
1352 | +        nTxPos = 1;
1353 | +    else
1354 | +        nTxPos = pindex->nBlockPos + ::GetSerializeSize(CBlock(), SER_DISK, CLIENT_VERSION) - 1 + GetSizeOfCompactSize(vtx.size());

sipa commented at 5:56 PM on May 26, 2012:

Why can't this formula be used in the justcheck case as well?

luke-jr commented at 6:24 PM on May 26, 2012:

Position 1,1,1 is "magic" in FetchInputs to refer to the memory pool. Otherwise, it will try to find it on disk

sipa commented at 11:55 AM on May 27, 2012:

Ok, can you add a comment to explain that?

in src/main.cpp:None in 9567f7e3c4 outdated

3468 | -    pblock->hashMerkleRoot = pblock->BuildMerkleTree();
3469 |      pblock->UpdateTime(pindexPrev);
3470 |      pblock->nBits          = GetNextWorkRequired(pindexPrev, pblock.get());
3471 |      pblock->nNonce         = 0;
3472 |  
3473 | +        pblock->vtx[0].vin[0].scriptSig = scriptDummy;

sipa commented at 6:02 PM on May 26, 2012:

Why the indenting change? And is this part always necessary?

luke-jr commented at 6:26 PM on May 26, 2012:

The end-brace releasing the main/mempool lock scopes is removed so that it remains constant until ConnectBlock finishes checking it. The code in between could be indented to clarify, but the diff is easier to read this way.

sipa commented at 11:57 AM on May 27, 2012:

And is this piece of the code necessary in non-fJustCheck mode?

luke-jr commented at 12:06 PM on May 27, 2012:

This is part of CreateNewBlock, which always calls ConnectBlock with fJustCheck. A dummy scriptSig is needed to pass the "scriptSig must be between 2 and 100 octets long" rule.

sipa commented at 12:27 PM on May 27, 2012:

Of course - you always want to check whether the created block is valid.

in src/test/miner_tests.cpp:None in 9567f7e3c4 outdated

 151 | +    tx.vin[0].prevout.hash = txFirst[0]->GetHash();
 152 | +    tx.vin[0].prevout.n = 0;
 153 | +    tx.vin[0].scriptSig = CScript() << OP_1;
 154 | +    tx.vout[0].nValue = 4900000000;
 155 | +    script = CScript() << OP_0;
 156 | +    tx.vout[0].scriptPubKey.SetPayToScriptHash(script);

sipa commented at 12:11 PM on May 27, 2012:

This line becomes tx.vout[0].scriptPubKey.SetDestination(script.GetID()); after #1357.

luke-jr commented at 1:06 PM on May 27, 2012: member

Added comments per sipa's request, rebased on master, and fixed a bug in diskless BDB I found testing.

luke-jr commented at 10:56 PM on May 28, 2012: member

getmemorypool's caching mostly makes testing this impossible, but I did some vague performance testing by disabling it (so every call goes to CreateNewBlock): git master: 1063 transactions/sec checknewblock: 473 transactions/sec checknewblock with signature checking skipped: 782 transactions/sec

IMO, even the worst-case scenario of 0.00321 seconds per transaction processed is worth the additional safety checking, especially when considering the extra caching getmemorypool and getwork both do to minimize CreateNewBlock calls.

sipa commented at 4:42 PM on May 30, 2012: member

ACK (together with #1245). I really like having unit tests that test blockchains.

jgarzik commented at 2:51 PM on June 27, 2012: contributor

ACK

sipa commented at 2:57 PM on June 27, 2012: member

@luke-jr Can you rebase #1245 and this? I'd like to merge.

luke-jr commented at 3:17 PM on June 27, 2012: member

Rebased. #1245 did not need rebasing.

Merge branch 'checknewblock_0.6.0' into checknewblock

Conflicts:
	src/main.cpp

fbf99a9cdc

Run BDB disk-less for test_bitcoin 148e107da6

in src/db.cpp:None in d6236004e1 outdated

 103 | @@ -103,12 +104,48 @@ bool CDBEnv::Open(boost::filesystem::path pathEnv_)
 104 |          return error("CDB() : error %d opening database environment", ret);
 105 |  
 106 |      fDbEnvInit = true;
 107 | +    fMockDb = false;

gavinandresen commented at 8:28 PM on June 28, 2012:

This is a very un-C++-ish way of doing this. The C++ Way would be an abstract base CDBEnv class, with concrete subclasses for unit testing (CMockDbEnv or CMemoryDbEnv) and records-to-disk.

But the use of the global bitdb variable is also very un-c++-ish, and if Mike Hearn's experiments with LevelDB and Pieter's experiments with a new wallet storage format go well this whole subsystem might get junked anyway....

gavinandresen commented at 3:19 PM on July 12, 2012: contributor

Does not compile 32 bit:

test/miner_tests.cpp:89: error: integer constant is too large for ‘long’ type test/miner_tests.cpp:109: error: integer constant is too large for ‘long’ type test/miner_tests.cpp:131: error: integer constant is too large for ‘long’ type test/miner_tests.cpp:139: error: integer constant is too large for ‘long’ type test/miner_tests.cpp:161: error: integer constant is too large for ‘long’ type test/miner_tests.cpp:178: error: integer constant is too large for ‘long’ type

Tests for CreateNewBlock 639b61d78e

luke-jr commented at 4:38 PM on July 12, 2012: member

Explicitly made the literals long long. Apparently "long long" was not a standard type until C++11, and compilers implementing it as an extension did not support mere numeric literals bigger than long (the C++11 standard requires these literals be handled properly). GCC 4.5 works with both ways. In any case, being explicit doesn't seem like it hurts, so I've added that. Let me know if it still doesn't work.

gavinandresen merged this on Jul 26, 2012

gavinandresen closed this on Jul 26, 2012

DrahtBot locked this on Sep 8, 2021