From 6fb8d0c2d64583a9a5b852bd014f63a11ea89eff Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 23 Nov 2016 17:04:20 +1300 Subject: [PATCH] Skip JoinSplit verification before the last checkpoint Part of #1749 --- src/gtest/test_checkblock.cpp | 5 +++- src/main.cpp | 43 ++++++++++++++++++++++------------ src/main.h | 14 ++++++++--- src/test/checkblock_tests.cpp | 4 +++- src/test/transaction_tests.cpp | 29 +++++++++++++---------- src/wallet/walletdb.cpp | 4 +++- 6 files changed, 66 insertions(+), 33 deletions(-) diff --git a/src/gtest/test_checkblock.cpp b/src/gtest/test_checkblock.cpp index eab73afad..edd8a617d 100644 --- a/src/gtest/test_checkblock.cpp +++ b/src/gtest/test_checkblock.cpp @@ -3,6 +3,7 @@ #include "consensus/validation.h" #include "main.h" +#include "zcash/Proof.hpp" class MockCValidationState : public CValidationState { public: @@ -22,12 +23,14 @@ public: }; TEST(CheckBlock, VersionTooLow) { + auto verifier = libzcash::ProofVerifier::Strict(); + CBlock block; block.nVersion = 1; MockCValidationState state; EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "version-too-low", false)).Times(1); - EXPECT_FALSE(CheckBlock(block, state, false, false)); + EXPECT_FALSE(CheckBlock(block, state, verifier, false, false)); } TEST(ContextualCheckBlock, BadCoinbaseHeight) { diff --git a/src/main.cpp b/src/main.cpp index 491714f95..a843724ac 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -833,7 +833,8 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& in return nSigOps; } -bool CheckTransaction(const CTransaction& tx, CValidationState &state) +bool CheckTransaction(const CTransaction& tx, CValidationState &state, + libzcash::ProofVerifier& verifier) { // Don't count coinbase transactions because mining skews the count if (!tx.IsCoinBase()) { @@ -844,7 +845,6 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state) return false; } else { // Ensure that zk-SNARKs verify - auto verifier = libzcash::ProofVerifier::Strict(); BOOST_FOREACH(const JSDescription &joinsplit, tx.vjoinsplit) { if (!joinsplit.Verify(*pzcashParams, verifier, tx.joinSplitPubKey)) { return state.DoS(100, error("CheckTransaction(): joinsplit does not verify"), @@ -1056,7 +1056,8 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa if (pfMissingInputs) *pfMissingInputs = false; - if (!CheckTransaction(tx, state)) + auto verifier = libzcash::ProofVerifier::Strict(); + if (!CheckTransaction(tx, state, verifier)) return error("AcceptToMemoryPool: CheckTransaction failed"); // Coinbase is only valid in a block, not as a loose transaction @@ -2041,8 +2042,13 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin { const CChainParams& chainparams = Params(); AssertLockHeld(cs_main); - // Check it again in case a previous version let a bad block in - if (!CheckBlock(block, state, !fJustCheck, !fJustCheck)) + + bool fExpensiveChecks = (!fCheckpointsEnabled || pindex->nHeight >= Checkpoints::GetTotalBlocksEstimate(chainparams.Checkpoints())); + auto verifier = libzcash::ProofVerifier::Strict(); + auto disabledVerifier = libzcash::ProofVerifier::Disabled(); + + // Check it again to verify JoinSplit proofs, and in case a previous version let a bad block in + if (!CheckBlock(block, state, fExpensiveChecks ? verifier : disabledVerifier, !fJustCheck, !fJustCheck)) return false; // verify that the view's current state corresponds to the previous block @@ -2061,8 +2067,6 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin return true; } - bool fScriptChecks = (!fCheckpointsEnabled || pindex->nHeight >= Checkpoints::GetTotalBlocksEstimate(chainparams.Checkpoints())); - // Do not allow blocks that contain transactions which 'overwrite' older transactions, // unless those are already completely spent. BOOST_FOREACH(const CTransaction& tx, block.vtx) { @@ -2088,7 +2092,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin CBlockUndo blockundo; - CCheckQueueControl control(fScriptChecks && nScriptCheckThreads ? &scriptcheckqueue : NULL); + CCheckQueueControl control(fExpensiveChecks && nScriptCheckThreads ? &scriptcheckqueue : NULL); int64_t nTimeStart = GetTimeMicros(); CAmount nFees = 0; @@ -2149,7 +2153,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin nFees += view.GetValueIn(tx)-tx.GetValueOut(); std::vector vChecks; - if (!ContextualCheckInputs(tx, state, view, fScriptChecks, flags, false, chainparams.GetConsensus(), nScriptCheckThreads ? &vChecks : NULL)) + if (!ContextualCheckInputs(tx, state, view, fExpensiveChecks, flags, false, chainparams.GetConsensus(), nScriptCheckThreads ? &vChecks : NULL)) return false; control.Add(vChecks); } @@ -2976,7 +2980,9 @@ bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, bool f return true; } -bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bool fCheckMerkleRoot) +bool CheckBlock(const CBlock& block, CValidationState& state, + libzcash::ProofVerifier& verifier, + bool fCheckPOW, bool fCheckMerkleRoot) { // These are checks that are independent of context. @@ -3021,7 +3027,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo // Check transactions BOOST_FOREACH(const CTransaction& tx, block.vtx) - if (!CheckTransaction(tx, state)) + if (!CheckTransaction(tx, state, verifier)) return error("CheckBlock(): CheckTransaction failed"); unsigned int nSigOps = 0; @@ -3209,7 +3215,9 @@ bool AcceptBlock(CBlock& block, CValidationState& state, CBlockIndex** ppindex, if (fTooFarAhead) return true; // Block height is too high } - if ((!CheckBlock(block, state)) || !ContextualCheckBlock(block, state, pindex->pprev)) { + // See method docstring for why this is always disabled + auto verifier = libzcash::ProofVerifier::Disabled(); + if ((!CheckBlock(block, state, verifier)) || !ContextualCheckBlock(block, state, pindex->pprev)) { if (state.IsInvalid() && !state.CorruptionPossible()) { pindex->nStatus |= BLOCK_FAILED_VALID; setDirtyBlockIndex.insert(pindex); @@ -3258,7 +3266,8 @@ static bool IsSuperMajority(int minVersion, const CBlockIndex* pstart, unsigned bool ProcessNewBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, bool fForceProcessing, CDiskBlockPos *dbp) { // Preliminary checks - bool checked = CheckBlock(*pblock, state); + auto verifier = libzcash::ProofVerifier::Disabled(); + bool checked = CheckBlock(*pblock, state, verifier); { LOCK(cs_main); @@ -3294,11 +3303,13 @@ bool TestBlockValidity(CValidationState &state, const CBlock& block, CBlockIndex CBlockIndex indexDummy(block); indexDummy.pprev = pindexPrev; indexDummy.nHeight = pindexPrev->nHeight + 1; + // JoinSplit proofs are verified in ConnectBlock + auto verifier = libzcash::ProofVerifier::Disabled(); // NOTE: CheckBlockHeader is called by CheckBlock if (!ContextualCheckBlockHeader(block, state, pindexPrev)) return false; - if (!CheckBlock(block, state, fCheckPOW, fCheckMerkleRoot)) + if (!CheckBlock(block, state, verifier, fCheckPOW, fCheckMerkleRoot)) return false; if (!ContextualCheckBlock(block, state, pindexPrev)) return false; @@ -3619,6 +3630,8 @@ bool CVerifyDB::VerifyDB(CCoinsView *coinsview, int nCheckLevel, int nCheckDepth CBlockIndex* pindexFailure = NULL; int nGoodTransactions = 0; CValidationState state; + // No need to verify JoinSplits twice + auto verifier = libzcash::ProofVerifier::Disabled(); for (CBlockIndex* pindex = chainActive.Tip(); pindex && pindex->pprev; pindex = pindex->pprev) { boost::this_thread::interruption_point(); @@ -3630,7 +3643,7 @@ bool CVerifyDB::VerifyDB(CCoinsView *coinsview, int nCheckLevel, int nCheckDepth if (!ReadBlockFromDisk(block, pindex)) return error("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); // check level 1: verify block validity - if (nCheckLevel >= 1 && !CheckBlock(block, state)) + if (nCheckLevel >= 1 && !CheckBlock(block, state, verifier)) return error("VerifyDB(): *** found bad block at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); // check level 2: verify undo validity if (nCheckLevel >= 2 && pindex) { diff --git a/src/main.h b/src/main.h index 66808a57a..fd99d28e5 100644 --- a/src/main.h +++ b/src/main.h @@ -339,7 +339,7 @@ bool NonContextualCheckInputs(const CTransaction& tx, CValidationState &state, c void UpdateCoins(const CTransaction& tx, CValidationState &state, CCoinsViewCache &inputs, int nHeight); /** Context-independent validity checks */ -bool CheckTransaction(const CTransaction& tx, CValidationState& state); +bool CheckTransaction(const CTransaction& tx, CValidationState& state, libzcash::ProofVerifier& verifier); bool CheckTransactionWithoutProofVerification(const CTransaction& tx, CValidationState &state); /** Check for standard transaction types @@ -416,7 +416,9 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin /** Context-independent validity checks */ bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, bool fCheckPOW = true); -bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW = true, bool fCheckMerkleRoot = true); +bool CheckBlock(const CBlock& block, CValidationState& state, + libzcash::ProofVerifier& verifier, + bool fCheckPOW = true, bool fCheckMerkleRoot = true); /** Context-dependent validity checks */ bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, CBlockIndex *pindexPrev); @@ -425,7 +427,13 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIn /** Check a block is completely valid from start to finish (only works on top of our current best block, with cs_main held) */ bool TestBlockValidity(CValidationState &state, const CBlock& block, CBlockIndex *pindexPrev, bool fCheckPOW = true, bool fCheckMerkleRoot = true); -/** Store block on disk. If dbp is non-NULL, the file is known to already reside on disk */ +/** + * Store block on disk. + * JoinSplit proofs are never verified, because: + * - AcceptBlock doesn't perform script checks either. + * - The only caller of AcceptBlock verifies JoinSplit proofs elsewhere. + * If dbp is non-NULL, the file is known to already reside on disk + */ bool AcceptBlock(CBlock& block, CValidationState& state, CBlockIndex **pindex, bool fRequested, CDiskBlockPos* dbp); bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, CBlockIndex **ppindex= NULL); diff --git a/src/test/checkblock_tests.cpp b/src/test/checkblock_tests.cpp index 51530c4de..c813c9af9 100644 --- a/src/test/checkblock_tests.cpp +++ b/src/test/checkblock_tests.cpp @@ -7,6 +7,7 @@ #include "main.h" #include "test/test_bitcoin.h" #include "utiltime.h" +#include "zcash/Proof.hpp" #include @@ -56,7 +57,8 @@ BOOST_AUTO_TEST_CASE(May15) // After May 15'th, big blocks are OK: forkingBlock.nTime = tMay15; // Invalidates PoW - BOOST_CHECK(CheckBlock(forkingBlock, state, false, false)); + auto verifier = libzcash::ProofVerifier::Strict(); + BOOST_CHECK(CheckBlock(forkingBlock, state, verifier, false, false)); } SetMockTime(0); diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 86d59e809..64fa4b298 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -29,6 +29,7 @@ #include "zcash/Note.hpp" #include "zcash/Address.hpp" +#include "zcash/Proof.hpp" using namespace std; using namespace json_spirit; @@ -97,6 +98,7 @@ BOOST_AUTO_TEST_CASE(tx_valid) // verifyFlags is a comma separated list of script verification flags to apply, or "NONE" Array tests = read_json(std::string(json_tests::tx_valid, json_tests::tx_valid + sizeof(json_tests::tx_valid))); + auto verifier = libzcash::ProofVerifier::Strict(); ScriptError err; BOOST_FOREACH(Value& tv, tests) { @@ -141,7 +143,7 @@ BOOST_AUTO_TEST_CASE(tx_valid) stream >> tx; CValidationState state; - BOOST_CHECK_MESSAGE(CheckTransaction(tx, state), strTest); + BOOST_CHECK_MESSAGE(CheckTransaction(tx, state, verifier), strTest); BOOST_CHECK(state.IsValid()); for (unsigned int i = 0; i < tx.vin.size(); i++) @@ -173,6 +175,7 @@ BOOST_AUTO_TEST_CASE(tx_invalid) // verifyFlags is a comma separated list of script verification flags to apply, or "NONE" Array tests = read_json(std::string(json_tests::tx_invalid, json_tests::tx_invalid + sizeof(json_tests::tx_invalid))); + auto verifier = libzcash::ProofVerifier::Strict(); ScriptError err; BOOST_FOREACH(Value& tv, tests) { @@ -217,7 +220,7 @@ BOOST_AUTO_TEST_CASE(tx_invalid) stream >> tx; CValidationState state; - fValid = CheckTransaction(tx, state) && state.IsValid(); + fValid = CheckTransaction(tx, state, verifier) && state.IsValid(); for (unsigned int i = 0; i < tx.vin.size() && fValid; i++) { @@ -246,11 +249,12 @@ BOOST_AUTO_TEST_CASE(basic_transaction_tests) CMutableTransaction tx; stream >> tx; CValidationState state; - BOOST_CHECK_MESSAGE(CheckTransaction(tx, state) && state.IsValid(), "Simple deserialized transaction should be valid."); + auto verifier = libzcash::ProofVerifier::Strict(); + BOOST_CHECK_MESSAGE(CheckTransaction(tx, state, verifier) && state.IsValid(), "Simple deserialized transaction should be valid."); // Check that duplicate txins fail tx.vin.push_back(tx.vin[0]); - BOOST_CHECK_MESSAGE(!CheckTransaction(tx, state) || !state.IsValid(), "Transaction with duplicate txins should be invalid."); + BOOST_CHECK_MESSAGE(!CheckTransaction(tx, state, verifier) || !state.IsValid(), "Transaction with duplicate txins should be invalid."); } // @@ -373,6 +377,7 @@ BOOST_AUTO_TEST_CASE(test_basic_joinsplit_verification) BOOST_AUTO_TEST_CASE(test_simple_joinsplit_invalidity) { + auto verifier = libzcash::ProofVerifier::Strict(); CMutableTransaction tx; tx.nVersion = 2; { @@ -424,23 +429,23 @@ BOOST_AUTO_TEST_CASE(test_simple_joinsplit_invalidity) JSDescription *jsdesc = &newTx.vjoinsplit[0]; jsdesc->vpub_old = -1; - BOOST_CHECK(!CheckTransaction(newTx, state)); + BOOST_CHECK(!CheckTransaction(newTx, state, verifier)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-vpub_old-negative"); jsdesc->vpub_old = MAX_MONEY + 1; - BOOST_CHECK(!CheckTransaction(newTx, state)); + BOOST_CHECK(!CheckTransaction(newTx, state, verifier)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-vpub_old-toolarge"); jsdesc->vpub_old = 0; jsdesc->vpub_new = -1; - BOOST_CHECK(!CheckTransaction(newTx, state)); + BOOST_CHECK(!CheckTransaction(newTx, state, verifier)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-vpub_new-negative"); jsdesc->vpub_new = MAX_MONEY + 1; - BOOST_CHECK(!CheckTransaction(newTx, state)); + BOOST_CHECK(!CheckTransaction(newTx, state, verifier)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-vpub_new-toolarge"); jsdesc->vpub_new = (MAX_MONEY / 2) + 10; @@ -450,7 +455,7 @@ BOOST_AUTO_TEST_CASE(test_simple_joinsplit_invalidity) JSDescription *jsdesc2 = &newTx.vjoinsplit[1]; jsdesc2->vpub_new = (MAX_MONEY / 2) + 10; - BOOST_CHECK(!CheckTransaction(newTx, state)); + BOOST_CHECK(!CheckTransaction(newTx, state, verifier)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-txintotal-toolarge"); } { @@ -464,7 +469,7 @@ BOOST_AUTO_TEST_CASE(test_simple_joinsplit_invalidity) jsdesc->nullifiers[0] = GetRandHash(); jsdesc->nullifiers[1] = jsdesc->nullifiers[0]; - BOOST_CHECK(!CheckTransaction(newTx, state)); + BOOST_CHECK(!CheckTransaction(newTx, state, verifier)); BOOST_CHECK(state.GetRejectReason() == "bad-joinsplits-nullifiers-duplicate"); jsdesc->nullifiers[1] = GetRandHash(); @@ -475,7 +480,7 @@ BOOST_AUTO_TEST_CASE(test_simple_joinsplit_invalidity) jsdesc2->nullifiers[0] = GetRandHash(); jsdesc2->nullifiers[1] = jsdesc->nullifiers[0]; - BOOST_CHECK(!CheckTransaction(newTx, state)); + BOOST_CHECK(!CheckTransaction(newTx, state, verifier)); BOOST_CHECK(state.GetRejectReason() == "bad-joinsplits-nullifiers-duplicate"); } { @@ -494,7 +499,7 @@ BOOST_AUTO_TEST_CASE(test_simple_joinsplit_invalidity) CTransaction finalNewTx(newTx); BOOST_CHECK(finalNewTx.IsCoinBase()); } - BOOST_CHECK(!CheckTransaction(newTx, state)); + BOOST_CHECK(!CheckTransaction(newTx, state, verifier)); BOOST_CHECK(state.GetRejectReason() == "bad-cb-has-joinsplits"); } } diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index d4966b186..ec6c55ee3 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -14,6 +14,7 @@ #include "util.h" #include "utiltime.h" #include "wallet/wallet.h" +#include "zcash/Proof.hpp" #include #include @@ -411,7 +412,8 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, CWalletTx wtx; ssValue >> wtx; CValidationState state; - if (!(CheckTransaction(wtx, state) && (wtx.GetHash() == hash) && state.IsValid())) + auto verifier = libzcash::ProofVerifier::Strict(); + if (!(CheckTransaction(wtx, state, verifier) && (wtx.GetHash() == hash) && state.IsValid())) return false; // Undo serialize changes in 31600