diff --git a/src/script/interpreter.h b/src/script/interpreter.h --- a/src/script/interpreter.h +++ b/src/script/interpreter.h @@ -31,7 +31,7 @@ public: virtual bool VerifySignature(const std::vector &vchSig, const CPubKey &vchPubKey, - const uint256 &sighash) const; + const uint256 &sighash, uint32_t flags) const; virtual bool CheckSig(const std::vector &vchSigIn, const std::vector &vchPubKey, diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -942,7 +942,8 @@ .Write(vchMessage.data(), vchMessage.size()) .Finalize(vchHash.data()); fSuccess = checker.VerifySignature( - vchSig, CPubKey(vchPubKey), uint256(vchHash)); + vchSig, CPubKey(vchPubKey), uint256(vchHash), + flags); } if (!fSuccess && (flags & SCRIPT_VERIFY_NULLFAIL) && @@ -1464,7 +1465,8 @@ bool BaseSignatureChecker::VerifySignature(const std::vector &vchSig, const CPubKey &pubkey, - const uint256 &sighash) const { + const uint256 &sighash, + uint32_t flags) const { return pubkey.VerifyECDSA(sighash, vchSig); } @@ -1487,7 +1489,7 @@ uint256 sighash = SignatureHash(scriptCode, *txTo, nIn, sigHashType, amount, this->txdata, flags); - if (!VerifySignature(vchSig, pubkey, sighash)) { + if (!VerifySignature(vchSig, pubkey, sighash, flags)) { return false; } diff --git a/src/script/sigcache.h b/src/script/sigcache.h --- a/src/script/sigcache.h +++ b/src/script/sigcache.h @@ -44,7 +44,7 @@ bool store; bool IsCached(const std::vector &vchSig, const CPubKey &vchPubKey, - const uint256 &sighash) const; + const uint256 &sighash, uint32_t flags) const; public: CachingTransactionSignatureChecker(const CTransaction *txToIn, @@ -55,8 +55,8 @@ store(storeIn) {} bool VerifySignature(const std::vector &vchSig, - const CPubKey &vchPubKey, - const uint256 &sighash) const override; + const CPubKey &vchPubKey, const uint256 &sighash, + uint32_t flags) const override; friend class TestCachingTransactionSignatureChecker; }; diff --git a/src/script/sigcache.cpp b/src/script/sigcache.cpp --- a/src/script/sigcache.cpp +++ b/src/script/sigcache.cpp @@ -16,6 +16,21 @@ namespace { +/** + * Declare which flags absolutely do not affect VerifySignature() result. + * We this to reduce unnecessary cache misses (such as when policy and consensus + * flags differ on unrelated aspects). + */ +static const uint32_t INVARIANT_FLAGS = + SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC | SCRIPT_VERIFY_DERSIG | + SCRIPT_VERIFY_LOW_S | SCRIPT_VERIFY_NULLDUMMY | SCRIPT_VERIFY_SIGPUSHONLY | + SCRIPT_VERIFY_MINIMALDATA | SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS | + SCRIPT_VERIFY_CLEANSTACK | SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY | + SCRIPT_VERIFY_CHECKSEQUENCEVERIFY | SCRIPT_VERIFY_MINIMALIF | + SCRIPT_VERIFY_NULLFAIL | SCRIPT_VERIFY_COMPRESSED_PUBKEYTYPE | + SCRIPT_ENABLE_SIGHASH_FORKID | SCRIPT_ENABLE_REPLAY_PROTECTION | + SCRIPT_ENABLE_CHECKDATASIG; + /** * Valid signature cache, to avoid doing expensive ECDSA signature checking * twice for every transaction (once when accepted into memory pool, and @@ -23,7 +38,8 @@ */ class CSignatureCache { private: - //! Entries are SHA256(nonce || signature hash || public key || signature): + //! Entries are SHA256(nonce || flags || signature hash || public key || + //! signature): uint256 nonce; typedef CuckooCache::cache map_type; map_type setValid; @@ -32,11 +48,13 @@ public: CSignatureCache() { GetRandBytes(nonce.begin(), 32); } - void ComputeEntry(uint256 &entry, const uint256 &hash, - const std::vector &vchSig, - const CPubKey &pubkey) { + void ComputeEntry(uint256 &entry, const std::vector &vchSig, + const CPubKey &pubkey, const uint256 &hash, + uint32_t flags) { + flags &= ~INVARIANT_FLAGS; CSHA256() .Write(nonce.begin(), 32) + .Write(reinterpret_cast(&flags), sizeof(flags)) .Write(hash.begin(), 32) .Write(&pubkey[0], pubkey.size()) .Write(&vchSig[0], vchSig.size()) @@ -82,9 +100,10 @@ template bool RunMemoizedCheck(const std::vector &vchSig, const CPubKey &pubkey, - const uint256 &sighash, bool storeOrErase, const F &fun) { + const uint256 &sighash, uint32_t flags, bool storeOrErase, + const F &fun) { uint256 entry; - signatureCache.ComputeEntry(entry, sighash, vchSig, pubkey); + signatureCache.ComputeEntry(entry, vchSig, pubkey, sighash, flags); if (signatureCache.Get(entry, !storeOrErase)) { return true; } @@ -99,16 +118,16 @@ bool CachingTransactionSignatureChecker::IsCached( const std::vector &vchSig, const CPubKey &pubkey, - const uint256 &sighash) const { - return RunMemoizedCheck(vchSig, pubkey, sighash, true, + const uint256 &sighash, uint32_t flags) const { + return RunMemoizedCheck(vchSig, pubkey, sighash, flags, true, [] { return false; }); } bool CachingTransactionSignatureChecker::VerifySignature( const std::vector &vchSig, const CPubKey &pubkey, - const uint256 &sighash) const { - return RunMemoizedCheck(vchSig, pubkey, sighash, store, [&] { + const uint256 &sighash, uint32_t flags) const { + return RunMemoizedCheck(vchSig, pubkey, sighash, flags, store, [&] { return TransactionSignatureChecker::VerifySignature(vchSig, pubkey, - sighash); + sighash, flags); }); } diff --git a/src/test/sigcache_tests.cpp b/src/test/sigcache_tests.cpp --- a/src/test/sigcache_tests.cpp +++ b/src/test/sigcache_tests.cpp @@ -22,10 +22,34 @@ static const std::string strSecret1C = "Kwr371tjA9u2rFSMZjTNun2PXXP3WPZu2afRHTcta6KxEUdm1vEw"; +/* We will be testing that these flags do not affect the cache entry. + * This list must match the one found in script/sigcache.cpp , however + * we duplicate it here to make sure that changes in cache behaviour also + * require an intentional change to this test. + */ +static const uint32_t TEST_INVARIANT_FLAGS = + SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC | SCRIPT_VERIFY_DERSIG | + SCRIPT_VERIFY_LOW_S | SCRIPT_VERIFY_NULLDUMMY | SCRIPT_VERIFY_SIGPUSHONLY | + SCRIPT_VERIFY_MINIMALDATA | SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS | + SCRIPT_VERIFY_CLEANSTACK | SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY | + SCRIPT_VERIFY_CHECKSEQUENCEVERIFY | SCRIPT_VERIFY_MINIMALIF | + SCRIPT_VERIFY_NULLFAIL | SCRIPT_VERIFY_COMPRESSED_PUBKEYTYPE | + SCRIPT_ENABLE_SIGHASH_FORKID | SCRIPT_ENABLE_REPLAY_PROTECTION | + SCRIPT_ENABLE_CHECKDATASIG; +/* We will be testing that these flags DO affect the cache entry. The expected + * behaviour is that flags which are not explicitly listed as invariant in + * script/sigcache.cpp will affect the cache entry. Here we will thus enforce + * that certain flags are omitted from that sigcache.cpp list. + * + * Currently, no flags affect VerifySignature. + */ +static const uint32_t TEST_VARIANT_FLAGS = 0; + +/** + * Sigcache is only accessible via CachingTransactionSignatureChecker + * as friend. + */ class TestCachingTransactionSignatureChecker { - /** - * Sigcache is only accessible via CachingTransactionSignatureChecker - */ CachingTransactionSignatureChecker *pchecker; public: @@ -35,21 +59,23 @@ } inline bool VerifyAndStore(const std::vector &vchSig, - const CPubKey &pubkey, const uint256 &sighash) { - return pchecker->VerifySignature(vchSig, pubkey, sighash); + const CPubKey &pubkey, const uint256 &sighash, + uint32_t flags) { + return pchecker->VerifySignature(vchSig, pubkey, sighash, flags); } inline bool IsCached(const std::vector &vchSig, - const CPubKey &pubkey, const uint256 &sighash) { - return pchecker->IsCached(vchSig, pubkey, sighash); + const CPubKey &pubkey, const uint256 &sighash, + uint32_t flags) { + return pchecker->IsCached(vchSig, pubkey, sighash, flags); } }; BOOST_FIXTURE_TEST_SUITE(sigcache_tests, BasicTestingSetup) -BOOST_AUTO_TEST_CASE(test1) { +BOOST_AUTO_TEST_CASE(sig_pubkey_hash_variations) { /** - * Making CachingTransactionSignatureChecke requires a tx. So we make a + * Making CachingTransactionSignatureChecker requires a tx. So we make a * dummy transaction (doesn't matter what it is) to construct it. */ CDataStream stream( @@ -64,6 +90,8 @@ TestCachingTransactionSignatureChecker testChecker(checker); + uint32_t flags = 0; + CBitcoinSecret bsecret1, bsecret1C; BOOST_CHECK(bsecret1.SetString(strSecret1)); BOOST_CHECK(bsecret1C.SetString(strSecret1C)); @@ -87,32 +115,95 @@ BOOST_CHECK(key1.SignECDSA(hashMsg2, sig2)); // cross-check - BOOST_CHECK(!testChecker.VerifyAndStore(sig2, pubkey1, hashMsg)); - BOOST_CHECK(!testChecker.VerifyAndStore(sig, pubkey1, hashMsg2)); + BOOST_CHECK(!testChecker.VerifyAndStore(sig2, pubkey1, hashMsg, flags)); + BOOST_CHECK(!testChecker.VerifyAndStore(sig, pubkey1, hashMsg2, flags)); // that should not have put them in cache... - BOOST_CHECK(!testChecker.IsCached(sig2, pubkey1, hashMsg)); - BOOST_CHECK(!testChecker.IsCached(sig, pubkey1, hashMsg2)); + BOOST_CHECK(!testChecker.IsCached(sig2, pubkey1, hashMsg, flags)); + BOOST_CHECK(!testChecker.IsCached(sig, pubkey1, hashMsg2, flags)); // check that it's not in cache at start - BOOST_CHECK(!testChecker.IsCached(sig, pubkey1, hashMsg)); - BOOST_CHECK(!testChecker.IsCached(sig2, pubkey1, hashMsg2)); + BOOST_CHECK(!testChecker.IsCached(sig, pubkey1, hashMsg, flags)); + BOOST_CHECK(!testChecker.IsCached(sig2, pubkey1, hashMsg2, flags)); // Insert into cache - BOOST_CHECK(testChecker.VerifyAndStore(sig, pubkey1, hashMsg)); - BOOST_CHECK(testChecker.VerifyAndStore(sig2, pubkey1, hashMsg2)); + BOOST_CHECK(testChecker.VerifyAndStore(sig, pubkey1, hashMsg, flags)); + BOOST_CHECK(testChecker.VerifyAndStore(sig2, pubkey1, hashMsg2, flags)); // check that it's in - BOOST_CHECK(testChecker.IsCached(sig, pubkey1, hashMsg)); - BOOST_CHECK(testChecker.IsCached(sig2, pubkey1, hashMsg2)); + BOOST_CHECK(testChecker.IsCached(sig, pubkey1, hashMsg, flags)); + BOOST_CHECK(testChecker.IsCached(sig2, pubkey1, hashMsg2, flags)); // check that different signature hits different entry - BOOST_CHECK(!testChecker.IsCached(sig2, pubkey1, hashMsg)); + BOOST_CHECK(!testChecker.IsCached(sig2, pubkey1, hashMsg, flags)); // check that compressed pubkey hits different entry - BOOST_CHECK(!testChecker.IsCached(sig, pubkey1C, hashMsg)); + BOOST_CHECK(!testChecker.IsCached(sig, pubkey1C, hashMsg, flags)); // check that different message hits different entry - BOOST_CHECK(!testChecker.IsCached(sig, pubkey1, hashMsg2)); + BOOST_CHECK(!testChecker.IsCached(sig, pubkey1, hashMsg2, flags)); // compressed key is for same privkey, so verifying works: - BOOST_CHECK(testChecker.VerifyAndStore(sig, pubkey1C, hashMsg)); + BOOST_CHECK(testChecker.VerifyAndStore(sig, pubkey1C, hashMsg, flags)); // now we *should* get a hit - BOOST_CHECK(testChecker.IsCached(sig, pubkey1C, hashMsg)); + BOOST_CHECK(testChecker.IsCached(sig, pubkey1C, hashMsg, flags)); + } +} + +BOOST_AUTO_TEST_CASE(flag_invariants) { + /** + * Making CachingTransactionSignatureChecker requires a tx. So we make a + * dummy transaction (doesn't matter what it is) to construct it. + */ + CDataStream stream( + ParseHex( + "010000000122739e70fbee987a8be1788395a2f2e6ad18ccb7ff611cd798071539" + "dde3c38e000000000151ffffffff010000000000000000016a00000000"), + SER_NETWORK, PROTOCOL_VERSION); + CTransaction dummyTx(deserialize, stream); + PrecomputedTransactionData txdata(dummyTx); + CachingTransactionSignatureChecker checker(&dummyTx, 0, 0 * SATOSHI, true, + txdata); + + TestCachingTransactionSignatureChecker testChecker(checker); + + CBitcoinSecret bsecret1; + bsecret1.SetString(strSecret1); + CKey key1 = bsecret1.GetKey(); + CPubKey pubkey1 = key1.GetPubKey(); + + // there should not be any overlap + BOOST_REQUIRE((TEST_VARIANT_FLAGS & TEST_INVARIANT_FLAGS) == 0); + + for (int n = 0; n < 16; n++) { + std::string strMsg = strprintf("Sigcache testflags %i: xx", n); + uint256 hashMsg = Hash(strMsg.begin(), strMsg.end()); + + std::vector sig; + BOOST_CHECK(key1.SignECDSA(hashMsg, sig)); + + // choose random background flagset to test + uint32_t base_flags = insecure_rand(); + + // shouldn't be in cache at start + BOOST_CHECK(!testChecker.IsCached(sig, pubkey1, hashMsg, base_flags)); + // insert into cache + BOOST_CHECK( + testChecker.VerifyAndStore(sig, pubkey1, hashMsg, base_flags)); + // check that it's in + BOOST_CHECK(testChecker.IsCached(sig, pubkey1, hashMsg, base_flags)); + + // now we flip each of 32 flags one by one, checking cache + for (uint32_t flag = 1; flag; flag <<= 1) { + uint32_t alt_flags = base_flags ^ flag; + BOOST_CHECK(alt_flags != base_flags); + bool hit = testChecker.IsCached(sig, pubkey1, hashMsg, alt_flags); + if (TEST_VARIANT_FLAGS & flag) { + // if it's in TEST_VARIANT_FLAGS, we must miss cache + BOOST_CHECK_MESSAGE(!hit, n << " bad cache hit 0x" << std::hex + << base_flags << " ^ 0x" << flag); + } else if (TEST_INVARIANT_FLAGS & flag) { + // if it's in TEST_INVARIANT_FLAGS, we must hit cache + BOOST_CHECK_MESSAGE(hit, n << " bad cache miss 0x" << std::hex + << base_flags << " ^ 0x" << flag); + } else { + // if it's in neither, don't care. + } + } } }