Cache full script execution results in addition to signatures #10192

pull TheBlueMatt wants to merge 7 commits into bitcoin:master from TheBlueMatt:2017-04-cache-scriptchecks changing 6 files +449 −43
  1. TheBlueMatt commented at 9:57 pm on April 11, 2017: member

    This adds a new CuckooCache in validation, caching whether all of a transaction’s scripts were valid with a given set of script flags.

    Unlike previous attempts at caching an entire transaction’s validity, which have nearly universally introduced consensus failures, this only caches the validity of a transaction’s scriptSigs. As these are pure functions of the transaction and data it commits to, this should be much safer.

    This is somewhat duplicative with the sigcache, as entries in the new cache will also have several entries in the sigcache. However, the sigcache is kept both as ATMP relies on it and because it prevents malleability-based DoS attacks on the new higher-level cache. Instead, the -sigcachesize option is re-used - cutting the sigcache size in half and using the newly freed memory for the script execution cache.

    Transactions which match the script execution cache never even have entries in the script check thread’s workqueue created.

    Note that the cache is indexed only on the script execution flags and the transaction’s witness hash. While this is sufficient to make the CScriptCheck() calls pure functions, this introduces dependancies on the mempool calculating things such as the PrecomputedTransactionData object, filling the CCoinsViewCache, etc in the exact same way as ConnectBlock. I belive this is a reasonable assumption, but should be noted carefully.

    In a rather naive benchmark (reindex-chainstate up to block 284k with cuckoocache always returning true for contains(), -assumevalid=0 and a very large dbcache), this connected blocks ~1.7x faster.

  2. in src/validation.cpp:1218 in 7a0fb5a0a4 outdated
    1391+}; // anonymous namespace
    1392+
    1393+void InitScriptExecutionCache() {
    1394+    // nMaxCacheSize is unsigned. If -maxsigcachesize is set to zero,
    1395+    // setup_bytes creates the minimum possible cache (2 elements).
    1396+    size_t nMaxCacheSize = std::min(std::max((int64_t)0, GetArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_SIZE) / 2), MAX_MAX_SIG_CACHE_SIZE) * ((size_t) 1 << 20);
    


    sipa commented at 10:18 pm on April 11, 2017:
    I think the division should be outside of GetArg. Otherwise, if you specify -maxsigcachesize=32, you end up with a total of 64MiB worth of caches.

    gmaxwell commented at 9:42 am on April 12, 2017:
    Good, because the division is outside of the GetArg. :)

    sipa commented at 3:55 pm on April 13, 2017:
    It seems I am blind.
  3. TheBlueMatt force-pushed on Apr 11, 2017
  4. TheBlueMatt commented at 11:36 pm on April 11, 2017: member
    Fixed test_bitcoin segfaulting as it didnt init the script cache as it does the sigcache.
  5. TheBlueMatt force-pushed on Apr 12, 2017
  6. in src/validation.cpp:17 in b917cfd598 outdated
    11@@ -12,6 +12,7 @@
    12 #include "consensus/consensus.h"
    13 #include "consensus/merkle.h"
    14 #include "consensus/validation.h"
    15+#include "cuckoocache.h"
    


    JeremyRubin commented at 5:47 am on April 12, 2017:
    Maybe better to add a wrapper class around cuckoocache.h in a separate file so that you don’t depend on CuckooCache internals and can replace it with something more efficient more easily.

    TheBlueMatt commented at 5:28 pm on April 12, 2017:
    Is using the public interface of CuckooCache::cache really using its “internals”?

    JeremyRubin commented at 8:38 am on April 21, 2017:
    I guess not.
  7. in src/validation.cpp:1386 in b917cfd598 outdated
    1381@@ -1356,7 +1382,32 @@ bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoins
    1382 }
    1383 }// namespace Consensus
    1384 
    1385-bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, bool fScriptChecks, unsigned int flags, bool cacheStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks)
    1386+namespace {
    1387+class ShaHashSplitter {
    


    JeremyRubin commented at 5:52 am on April 12, 2017:
    #9480 exposes this class from the sigcache, we should probably just use that rather than adding this code again in a another location.

    TheBlueMatt commented at 5:28 pm on April 12, 2017:
    Rebased on that, removed this class.
  8. in src/validation.cpp:1409 in b917cfd598 outdated
    1426@@ -1376,13 +1427,26 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi
    1427         // Of course, if an assumed valid block is invalid due to false scriptSigs
    1428         // this optimization would allow an invalid chain to be accepted.
    1429         if (fScriptChecks) {
    1430+            // First check if script executions have been cached with the same
    1431+            // flags. Note that this assumes that the inputs provided are
    1432+            // correct (ie that the transaction hash which is in tx's prevouts
    1433+            // properly commits to the scriptPubKey in the inputs view of that
    1434+            // transaction).
    1435+            static uint256 nonce(GetRandHash());
    


    JeremyRubin commented at 6:02 am on April 12, 2017:
    static in the middle of a function is messy – add a wrapper class around cuckoocache.h

    TheBlueMatt commented at 5:29 pm on April 12, 2017:
    I’d much much rather have a static in the function than add yet another class to compile. We need to have fewer two-line wrapper classes, not more…our memory usage is already insane.

    JeremyRubin commented at 9:36 pm on April 12, 2017:

    Memory usage during compile time? Is it really that bad for an added class?

    Can you at least move the static decl to the top of the function?


    TheBlueMatt commented at 9:01 pm on April 18, 2017:
    Done. Moved the static to the top of the function with the static CuckooCache as well.
  9. in src/validation.cpp:1266 in b917cfd598 outdated
    1433+            // properly commits to the scriptPubKey in the inputs view of that
    1434+            // transaction).
    1435+            static uint256 nonce(GetRandHash());
    1436+            uint256 hashCacheEntry;
    1437+            CSHA256().Write(nonce.begin(), 32).Write(tx.GetWitnessHash().begin(), 32).Write((unsigned char*)&flags, sizeof(flags)).Finalize(hashCacheEntry.begin());
    1438+            AssertLockHeld(cs_main); //TODO: Remove this requirement by making CuckooCache not require external locks
    


    JeremyRubin commented at 6:04 am on April 12, 2017:

    Yes; adding a wrapper class would do this for you. Can be a read lock.

    Also if you’re using this class single threaded only ever, cuckoocache could be extended to offer a version without atomics…

  10. in src/validation.cpp:1437 in b917cfd598 outdated
    1432+            // correct (ie that the transaction hash which is in tx's prevouts
    1433+            // properly commits to the scriptPubKey in the inputs view of that
    1434+            // transaction).
    1435+            static uint256 nonce(GetRandHash());
    1436+            uint256 hashCacheEntry;
    1437+            CSHA256().Write(nonce.begin(), 32).Write(tx.GetWitnessHash().begin(), 32).Write((unsigned char*)&flags, sizeof(flags)).Finalize(hashCacheEntry.begin());
    


    JeremyRubin commented at 6:35 am on April 12, 2017:

    FYI You can cut the hashing overhead in half by either:

    • making the nonce 64 bytes & static caching then copyinh the midstate.
    • using only 19 bytes of nonce.

    TheBlueMatt commented at 5:29 pm on April 12, 2017:
    Using 19 bytes of nonce, thats way more than enough.
  11. fanquake added the label Validation on Apr 12, 2017
  12. gmaxwell commented at 9:44 am on April 12, 2017: contributor
    Concept ACK.
  13. JeremyRubin changes_requested
  14. JeremyRubin commented at 3:45 pm on April 12, 2017: contributor
    Concept Ack!
  15. in src/validation.cpp:1409 in b917cfd598 outdated
    1426@@ -1378,13 +1427,26 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi
    1427         // Of course, if an assumed valid block is invalid due to false scriptSigs
    1428         // this optimization would allow an invalid chain to be accepted.
    1429         if (fScriptChecks) {
    1430+            // First check if script executions have been cached with the same
    1431+            // flags. Note that this assumes that the inputs provided are
    1432+            // correct (ie that the transaction hash which is in tx's prevouts
    1433+            // properly commits to the scriptPubKey in the inputs view of that
    1434+            // transaction).
    1435+            static uint256 nonce(GetRandHash());
    


    instagibbs commented at 4:00 pm on April 12, 2017:
    Struggling to understand how a unique nonce per cache entry vs per cache works.

    JeremyRubin commented at 4:17 pm on April 12, 2017:
    it’s static so it is per cache.
  16. instagibbs commented at 4:29 pm on April 12, 2017: member
    concept ACK
  17. TheBlueMatt force-pushed on Apr 12, 2017
  18. TheBlueMatt commented at 5:30 pm on April 12, 2017: member
    Addressed Jeremy’s comments aside from the request for a wrapper class, I think we need fewer dummy classes, not more :/. Also rebased on #9480.
  19. TheBlueMatt force-pushed on Apr 12, 2017
  20. TheBlueMatt force-pushed on Apr 13, 2017
  21. TheBlueMatt force-pushed on Apr 13, 2017
  22. TheBlueMatt force-pushed on Apr 13, 2017
  23. TheBlueMatt force-pushed on Apr 13, 2017
  24. in src/validation.cpp:1263 in 8ef8ce5b4f outdated
    1407+            // properly commits to the scriptPubKey in the inputs view of that
    1408+            // transaction).
    1409+            static uint256 nonce(GetRandHash());
    1410+            uint256 hashCacheEntry;
    1411+            // We only use the first 19 bytes of nonce to avoid a second SHA
    1412+            // round - giving us 19 + 32 + 4 = 55 bytes (+ 8 + 1 = 64)
    


    gmaxwell commented at 6:27 pm on April 18, 2017:
    Static assert on the size of the flags so the nonce gets reduced if the flags are made 64-bits in the future?

    TheBlueMatt commented at 6:58 pm on April 18, 2017:
    Done.
  25. TheBlueMatt force-pushed on Apr 18, 2017
  26. TheBlueMatt force-pushed on Apr 18, 2017
  27. sdaftuar commented at 3:45 pm on April 19, 2017: member

    Note that the cache is indexed only on the script execution flags and the transaction’s witness hash. While this is sufficient to make the CScriptCheck() calls pure functions, this introduces dependancies on the mempool calculating things such as the PrecomputedTransactionData object, filling the CCoinsViewCache, etc in the exact same way as ConnectBlock. I belive this is a reasonable assumption, but should be noted carefully.

    So if I understand correctly, this makes CCoinsViewMempool, and therefore the mempool itself, consensus critical, which I would prefer to avoid. Would we still get a performance benefit if we were to include the coins being spent in the hash for the index lookup?

    Edited: as per offline discussion, perhaps just move CCoinsViewMemPool into validation.cpp to make it clear it’s part of consensus, and sanity check the results from the mempool.

  28. TheBlueMatt force-pushed on Apr 19, 2017
  29. TheBlueMatt commented at 6:06 pm on April 19, 2017: member
    @sdaftuar I’m not convinced its a massive concern, but I went ahead and added a wrapper which checks each scriptPubKey returned by the CCoinsViewCache is the one committed to by the input’s prevout hash, which I believe removes that dependancy entirely.
  30. in src/script/sigcache.cpp:79 in 1f75777760 outdated
    73@@ -74,7 +74,7 @@ void InitSignatureCache()
    74 {
    75     // nMaxCacheSize is unsigned. If -maxsigcachesize is set to zero,
    76     // setup_bytes creates the minimum possible cache (2 elements).
    77-    size_t nMaxCacheSize = std::min(std::max((int64_t)0, GetArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_SIZE)), MAX_MAX_SIG_CACHE_SIZE) * ((size_t) 1 << 20);
    78+    size_t nMaxCacheSize = std::min(std::max((int64_t)0, GetArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_SIZE) / 2), MAX_MAX_SIG_CACHE_SIZE) * ((size_t) 1 << 20);
    79     size_t nElems = signatureCache.setup_bytes(nMaxCacheSize);
    80     LogPrintf("Using %zu MiB out of %zu requested for signature cache, able to store %zu elements\n",
    


    JeremyRubin commented at 8:37 am on April 21, 2017:
    Would be nice to update the LogPrintf here (and in the new Init) to say something about how the space is divided among multiple caches now, so users aren’t confused why they aren’t getting the full allocation here.

    TheBlueMatt commented at 8:38 pm on April 21, 2017:
    OK, done, also updated the doc for -maxsigcachesize.
  31. morcos commented at 4:21 pm on April 25, 2017: member
    Concept ACK But I’d prefer if there were more safeguards in place against future changes that might cause consensus failure. For instance, I think anything that is inputted to CScriptCheck should be committed to by the hash. Right now it its the case that anything USED by CScriptCheck is committed to, but there is nothing stopping a future change to CScriptCheck that used the height from the Coins or something and caused problems.
  32. in src/validation.cpp:1751 in f8ceb94544 outdated
    1747@@ -1669,6 +1748,41 @@ class WarningBitsConditionChecker : public AbstractThresholdConditionChecker
    1748 // Protected by cs_main
    1749 static ThresholdConditionCache warningcache[VERSIONBITS_NUM_BITS];
    1750 
    1751+static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const CChainParams& chainparams) {
    


    jtimon commented at 4:53 pm on April 25, 2017:
    This function can take Consensus::Params directly instead of CChainParams Can we pass ThresholdConditionCache& cache to this and move it to src/versionbits ? I know not all flags are activated with bip9, but it seems the best place to me. Doing so would prevent you from calling IsWitnessEnabled() and AssertLockHeld(cs_main), but do you need to? You can do the latter from the caller like we do with VersionBitsState

    TheBlueMatt commented at 2:40 pm on April 27, 2017:

    I’d much prefer to encapsulate the version bits stuff in GetBlockScriptFlags, and keep the diff on the smaller side. You’re welcome to move things around afterwards, validation.cpp need to continue its trek towards better internal interfaces.

    I changed it to take Consensus::Params.


    jtimon commented at 3:19 pm on April 27, 2017:

    Moving it to src/versionbits.cpp would still encapsulate version bits stuff in GetBlockScriptFlags, you would just not be using the versionbitscache and cs_main globals from validation.cpp. You will also avoid using the unnecessary wrapper IsWitnessEnabled() (specially unnecessary in this function since you are already using VersionBitsState() directly for csv anyway).

    It is “free” to do it now vs moving it in the future (if that ever gets the review). It may not be important to your approach to libconsensus, but it is to mine. I’ve been trying to do this for a long time, @CodeShark tried something similar as preparation to bip9 too.


    TheBlueMatt commented at 3:51 pm on April 27, 2017:
    I dont think it makes any sense to move this entire function into src/versionbits.cpp? This function has stuff for BIP 65, 66 and 16, none of which are versionbits-activated? Moving all soft forks into a separate thing is definitely a separate issue from this PR.

    jtimon commented at 4:36 pm on April 27, 2017:

    I noted that at the beginning “I know not all flags are activated with bip9”, but I still think it’s the best place for this new function (even if some lines of the function don’t rely on versionbits, they also don’t rely on anything that versionbits didn’t already rely on).Doing a separate module only for this single function seems stupid to me. Anyway, as disappointing as your rejection to move the new function for free may be to me, let’s move on.

    Can you at least avoid the use of globals you don’t need to use, ie versionbitscache and cs_main (both used directly and also indirectly through IsWitnessEnabled())? That way, if we ever move this function out of validation (like all previous attempts at creating it did) it can be a clean moveonly commit (whether it goes to versionbits.o or somewhere else) instead of needing to change the function signature.

  33. in src/validation.cpp:455 in 82afc49955 outdated
    450@@ -451,6 +451,8 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& i
    451     return nSigOps;
    452 }
    453 
    454+// Returns the script flags which should be checked for a given block
    455+static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const CChainParams& chainparams);
    


    jtimon commented at 5:00 pm on April 25, 2017:
    why a separated declaration if it’s going to be static?

    JeremyRubin commented at 5:07 pm on April 25, 2017:
    It’s a forward declaration. As to why the definition isn’t just there, I’m not sure, maybe GetBlockScriptFlags also has some dependency that would need a forward decl.

    TheBlueMatt commented at 2:40 pm on April 27, 2017:
    The reason its down further is that I didnt want to move it up with the mempool stuff in validation.cpp, but its called from ATMP, so needed the forward declaration. Hopefully this stuff gets ironed out as we move towards better internal interfaces (see #10279).

    jtimon commented at 3:22 pm on April 27, 2017:
    You are creating the function as new, it won’t be any more disruptive to avoid the forward declaration. Anyway, my preference would be to move it to versionbits.o which is also ignored as “something that can be ironed out later”, so whatever.
  34. in src/validation.cpp:925 in bb136ee6f0 outdated
    921@@ -922,10 +922,15 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
    922         // There is a similar check in CreateNewBlock() to prevent creating
    923         // invalid blocks (using TestBlockValidity), however allowing such
    924         // transactions into the mempool can be exploited as a DoS attack.
    925-        if (!CheckInputs(tx, state, view, true, GetBlockScriptFlags(chainActive.Tip(), Params()), true, true, txdata))
    926+        unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(chainActive.Tip(), Params());
    


    jtimon commented at 10:40 pm on April 25, 2017:
    history bike-sheeding currentBlockScriptVerifyFlags should be part of the previous commit, not this one (and for my taste this commit could be an independent PR).

    TheBlueMatt commented at 2:40 pm on April 27, 2017:
    Its needed in this PR because its much more likely to trigger after the changes in this PR, though still unsupported. I’m happy to make it a PR on its own if you think its really worth it, but it seems to me this PR is still of very-reviewable-size.

    jtimon commented at 3:25 pm on April 27, 2017:
    You misunderstood. Creating a currentBlockScriptVerifyFlags local variable just makes the next line simpler, which is fine, but could have been done in “Cache full script execution results in addition to signatures” directly which already touches those lines instead of “Do not print soft-fork-script warning with -promiscuousmempool”, which would be clearer without that extra bikeshedding +2-1. Anyway, as said, history bikeshedding, not a big deal

    TheBlueMatt commented at 3:55 pm on April 27, 2017:
    Fixed.
  35. in src/validation.cpp:821 in 1f75777760 outdated
    954@@ -923,7 +955,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
    955         // invalid blocks (using TestBlockValidity), however allowing such
    956         // transactions into the mempool can be exploited as a DoS attack.
    957         unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(chainActive.Tip(), Params());
    958-        if (!CheckInputs(tx, state, view, true, currentBlockScriptVerifyFlags, true, true, txdata))
    959+        if (!CheckInputsFromMempoolAndCache(tx, state, view, pool, currentBlockScriptVerifyFlags, true, txdata))
    


    jtimon commented at 10:53 pm on April 25, 2017:
    Perhaps everything that is below this case could be moved to CheckInputsFromMempoolAndCache too

    TheBlueMatt commented at 2:40 pm on April 27, 2017:
    I dont think CheckInputsFromMempoolAndCache should make assumptions about having called CheckInputs already prior to this call? Maybe I’m confused as to what you’re suggesting here.

    jtimon commented at 3:32 pm on April 27, 2017:

    I mean moving the following inside the function too:

    0         {
    1              // If we're using promiscuousmempoolflags, we may hit this normally
    2              // Check if current block has some flags that scriptVerifyFlags
    3             // does not before printing an ominous warning
    4             if (!(~scriptVerifyFlags & currentBlockScriptVerifyFlags))
    5                 return error("%s: BUG! PLEASE REPORT THIS! ConnectInputs failed against MANDATORY but not STANDARD flags %s, %s",
    6                     __func__, hash.ToString(), FormatStateMessage(state));
    7         }
    

    Then here you would just call:

    0if (!CheckInputsFromMempoolAndCache(tx, state, view, pool, scriptVerifyFlags, currentBlockScriptVerifyFlags, true, txdata)) {
    1   return false;
    2}
    

    jtimon commented at 3:37 pm on April 27, 2017:

    TheBlueMatt commented at 3:55 pm on April 27, 2017:
    But that logging implicitly makes an assumption about the fact that you’ve already called CheckInputs prior to the CheckInputsFromMempoolAndCache call, which I’d prefer not to do. CheckInputsFromMempoolAndCache only does some additional mempool-shouldnt-pollute-cache checks, and then calls CheckInputs.
  36. TheBlueMatt force-pushed on Apr 27, 2017
  37. TheBlueMatt commented at 2:43 pm on April 27, 2017: member
    @morcos I added an additional commit to only pass the scriptPubKey and nValue from the prevout into CScriptCheck, so hopefully any such future changes would be super clear to reviewers as consensus bugs. Sadly I dont really want to just include the height in the hash, as there are many heights, but I think this is a sufficient change.
  38. in src/validation.cpp:820 in 061d06face outdated
    930-                __func__, hash.ToString(), FormatStateMessage(state));
    931+            // If we're using promiscuousmempoolflags, we may hit this normally
    932+            // Check if current block has some flags that scriptVerifyFlags
    933+            // does not before printing an ominous warning
    934+            if (!(~scriptVerifyFlags & currentBlockScriptVerifyFlags))
    935+                return error("%s: BUG! PLEASE REPORT THIS! ConnectInputs failed against MANDATORY but not STANDARD flags %s, %s",
    


    jtimon commented at 3:33 pm on April 27, 2017:
    The error should no be printed, but the function still needs to return false, no?

    TheBlueMatt commented at 3:55 pm on April 27, 2017:
    Fixed.
  39. TheBlueMatt force-pushed on Apr 27, 2017
  40. TheBlueMatt force-pushed on Apr 27, 2017
  41. sdaftuar commented at 5:25 pm on May 19, 2017: member
    Needs rebase
  42. gmaxwell commented at 9:01 am on May 20, 2017: contributor
    @TheBlueMatt REBASE ME
  43. TheBlueMatt force-pushed on May 22, 2017
  44. TheBlueMatt commented at 7:55 pm on May 22, 2017: member
    Rebased :).
  45. sipa commented at 1:26 am on May 24, 2017: member
    Needs moar rebase.
  46. in src/validation.cpp:827 in 847c35e9ad outdated
    743+            } else {
    744+                if (!CheckInputs(tx, state, view, true, MANDATORY_SCRIPT_VERIFY_FLAGS, true, false, txdata)) {
    745+                    return error("%s: ConnectInputs failed against MANDATORY but not STANDARD flags due to promiscuous mempool %s, %s",
    746+                        __func__, hash.ToString(), FormatStateMessage(state));
    747+                } else {
    748+                    LogPrintf("Warning: -promisuousmempool flags set to not include currently enforced soft forks, this may break mining or otherwise cause instability!\n");
    


    sdaftuar commented at 6:35 pm on May 24, 2017:
    spelling nit: “promiscuous”
  47. in src/validation.cpp:820 in 847c35e9ad outdated
    736-                __func__, hash.ToString(), FormatStateMessage(state));
    737+            // If we're using promiscuousmempoolflags, we may hit this normally
    738+            // Check if current block has some flags that scriptVerifyFlags
    739+            // does not before printing an ominous warning
    740+            if (!(~scriptVerifyFlags & currentBlockScriptVerifyFlags)) {
    741+                return error("%s: BUG! PLEASE REPORT THIS! ConnectInputs failed against MANDATORY but not STANDARD flags %s, %s",
    


    sdaftuar commented at 6:37 pm on May 24, 2017:
    This error message isn’t quite right; at this point we haven’t actually compared against the MANDATORY flags.
  48. sdaftuar commented at 6:44 pm on May 24, 2017: member
    Needs (simple) rebase, but code review ACK apart from a couple nits. Will test and profile.
  49. jtimon commented at 3:37 pm on May 25, 2017: contributor
    #10427 introduces GetScriptFlags like here but with some of my nits solved. If merged first, should make this a little bit smaller and simpler to review. This needs rebase again.
  50. in src/validation.cpp:316 in 39bdb8ba6b outdated
    309@@ -310,6 +310,9 @@ bool CheckSequenceLocks(const CTransaction &tx, int flags, LockPoints* lp, bool
    310 }
    311 
    312 
    313+// Returns the script flags which should be checked for a given block
    


    jtimon commented at 3:41 pm on May 25, 2017:
    Why this? if you want to use it between this and its deifinition, why not move the definition here directly?

    TheBlueMatt commented at 0:49 am on June 6, 2017:
    It feels much more reasonable next to the consensus code, as it is really critical consensus logic, instead of mixed in with all the ATMP code.

    jtimon commented at 12:39 pm on June 7, 2017:
    I’m not following, in this commit this additional declaration (when nobody is calling it between the declaration and the definition) is completely redundant. The definition is close to ConnectBlock, also consensus code, not ATMP.
  51. laanwj added this to the "Blockers" column in a project

  52. luke-jr commented at 7:42 pm on June 1, 2017: member
    This sounds like it would break (or at least complicate) CHECKBLOCKVERSION and possibly even CHECKBLOCKATHEIGHT…?
  53. sdaftuar commented at 8:02 pm on June 1, 2017: member

    This sounds like it would break (or at least complicate) CHECKBLOCKVERSION and possibly even CHECKBLOCKATHEIGHT…?

    I’m skeptical that we’d ever adopt such context-dependent script flags (which strike me as a bad idea), but putting that aside: I benchmarked this patch on data from December 2016, and observed a 31% reduction in total block validation time, which is such a large performance improvement that I don’t think this should be held up by a hypothetical type of consensus change, which we’ve not done before. We can always revisit this performance optimization in the future if necessary.

  54. sipa commented at 0:47 am on June 2, 2017: member
    Needs rebase.
  55. gmaxwell commented at 5:56 am on June 4, 2017: contributor
    @TheBlueMatt needs rebase
  56. TheBlueMatt force-pushed on Jun 6, 2017
  57. TheBlueMatt force-pushed on Jun 6, 2017
  58. TheBlueMatt commented at 0:50 am on June 6, 2017: member
    Addressed @sdaftuar’s comments and rebased.
  59. sipa commented at 1:54 am on June 6, 2017: member
    This does not look like it’s rebased on top of #10195?
  60. TheBlueMatt force-pushed on Jun 6, 2017
  61. TheBlueMatt commented at 3:22 am on June 6, 2017: member
    @sipa Yes it was, though I fucked up the second-to-last commit. Fixed now.
  62. gmaxwell commented at 8:25 am on June 6, 2017: contributor
    @TheBlueMatt the tests timed out and need kicking.
  63. in src/validation.cpp:1265 in 8d3c76d460 outdated
    1195+            // transaction).
    1196+            uint256 hashCacheEntry;
    1197+            // We only use the first 19 bytes of nonce to avoid a second SHA
    1198+            // round - giving us 19 + 32 + 4 = 55 bytes (+ 8 + 1 = 64)
    1199+            static_assert(55 - sizeof(flags) - 32 >= 128/8, "Want at least 128 bits of nonce for script execution cache");
    1200+            CSHA256().Write(scriptExecutionCacheNonce.begin(), 55 - sizeof(flags) - 32).Write(tx.GetWitnessHash().begin(), 32).Write((unsigned char*)&flags, sizeof(flags)).Finalize(hashCacheEntry.begin());
    


    ryanofsky commented at 10:18 pm on June 6, 2017:

    In commit “Cache full script execution results in addition to signatures”

    Would it be more efficient to avoid this hashing and the cache lookup below when cacheFullScriptStore is false?


    TheBlueMatt commented at 0:17 am on June 7, 2017:
    I believe actual block checking sets cacheFullScriptStore to false, so that’s kinda when you want it to cache lookup :).

    ryanofsky commented at 2:34 pm on June 7, 2017:

    I believe actual block checking sets cacheFullScriptStore to false, so that’s kinda when you want it to cache lookup :)

    I see, cacheFullScriptStore == false really means “check and erase from cache” not “don’t use cache” like I was thinking. Could add a comment about this, though in retrospect I guess it’s pretty obvious.


    TheBlueMatt commented at 3:05 pm on June 7, 2017:
    Well its the same thing that the sigcache itself does, though I added a comment to the top of ConnectInputs.
  64. in src/validation.cpp:1267 in 8d3c76d460 outdated
    1197+            // We only use the first 19 bytes of nonce to avoid a second SHA
    1198+            // round - giving us 19 + 32 + 4 = 55 bytes (+ 8 + 1 = 64)
    1199+            static_assert(55 - sizeof(flags) - 32 >= 128/8, "Want at least 128 bits of nonce for script execution cache");
    1200+            CSHA256().Write(scriptExecutionCacheNonce.begin(), 55 - sizeof(flags) - 32).Write(tx.GetWitnessHash().begin(), 32).Write((unsigned char*)&flags, sizeof(flags)).Finalize(hashCacheEntry.begin());
    1201+            AssertLockHeld(cs_main); //TODO: Remove this requirement by making CuckooCache not require external locks
    1202+            if (scriptExecutionCache.contains(hashCacheEntry, !cacheFullScriptStore)) {
    


    ryanofsky commented at 10:24 pm on June 6, 2017:

    In commit “Cache full script execution results in addition to signatures”

    If pvChecks is non-null, is it ok to just return true here without populating it? If so, it’d be good to have a comment here saying why, because it doesn’t seem completely obvious.


    TheBlueMatt commented at 0:19 am on June 7, 2017:
    I’m confused, pvChecks is passed in, you dont populate, just push back your set of to-be-validated CScriptChecks?

    ryanofsky commented at 2:50 pm on June 7, 2017:

    I’m confused, pvChecks is passed in, you dont populate, just push back your set of to-be-validated CScriptChecks?

    I guess I wouldn’t expect cacheFullScriptStore being set to true or false to have an effect on what gets returned in pvChecks, and that an interaction between the two parameters would be documented.

    The specific case where it looks like there is an interaction is after calling CheckInputs once with pvChecks = null and a script where check() == true for all inputs.

    If CheckInputs is called after that on the same script with pvChecks != null, it seems like pvChecks will be empty if cacheFullScriptStore was previously true, and populated if it was previously false.


    TheBlueMatt commented at 3:05 pm on June 7, 2017:
    Added a better comment.
  65. TheBlueMatt commented at 0:22 am on June 7, 2017: member
    Rebased.
  66. TheBlueMatt force-pushed on Jun 7, 2017
  67. TheBlueMatt force-pushed on Jun 7, 2017
  68. TheBlueMatt force-pushed on Jun 7, 2017
  69. Pull script verify flags calculation out of ConnectBlock 6d22b2b17b
  70. Cache full script execution results in addition to signatures
    This adds a new CuckooCache in validation, caching whether all of a
    transaction's scripts were valid with a given set of script flags.
    
    Unlike previous attempts at caching an entire transaction's
    validity, which have nearly universally introduced consensus
    failures, this only caches the validity of a transaction's
    scriptSigs. As these are pure functions of the transaction and
    data it commits to, this should be much safer.
    
    This is somewhat duplicative with the sigcache, as entries in the
    new cache will also have several entries in the sigcache. However,
    the sigcache is kept both as ATMP relies on it and because it
    prevents malleability-based DoS attacks on the new higher-level
    cache. Instead, the -sigcachesize option is re-used - cutting the
    sigcache size in half and using the newly freed memory for the
    script execution cache.
    
    Transactions which match the script execution cache never even have
    entries in the script check thread's workqueue created.
    
    Note that the cache is indexed only on the script execution flags
    and the transaction's witness hash. While this is sufficient to
    make the CScriptCheck() calls pure functions, this introduces
    dependancies on the mempool calculating things such as the
    PrecomputedTransactionData object, filling the CCoinsViewCache, etc
    in the exact same way as ConnectBlock. I belive this is a reasonable
    assumption, but should be noted carefully.
    
    In a rather naive benchmark (reindex-chainstate up to block 284k
    with cuckoocache always returning true for contains(),
    -assumevalid=0 and a very large dbcache), this connected blocks
    ~1.7x faster.
    b5fea8d0cc
  71. Do not print soft-fork-script warning with -promiscuousmempool eada04e778
  72. TheBlueMatt force-pushed on Jun 7, 2017
  73. in src/validation.cpp:1225 in d47d913eef outdated
    1222+ *
    1223+ * If pvChecks is not NULL, script checks are pushed onto it instead of being performed inline. Any
    1224+ * script checks which are not necessary (eg due to script execution cache hits) are, obviously,
    1225+ * not pushed onto pvChecks/run.
    1226+ *
    1227+ * Setting cacheSigStore/cacheFullScriptStore to true will remove elements from the corresponding cache
    


    ryanofsky commented at 3:10 pm on June 7, 2017:

    In commit “Better document CheckInputs parameter meanings”

    s/true/false I think. Thanks for the comments!

  74. TheBlueMatt force-pushed on Jun 7, 2017
  75. in src/validation.cpp:418 in 4653582139 outdated
    408+    assert(!tx.IsCoinBase());
    409+    {
    410+        LOCK(pool.cs);
    411+        for (const CTxIn& txin : tx.vin) {
    412+            const Coin& coin = view.AccessCoin(txin.prevout);
    413+            if (coin.IsSpent()) return false;
    


    ryanofsky commented at 2:46 pm on June 8, 2017:

    In commit “Add CheckInputs wrapper CCoinsViewMemPool -> non-consensus-critical”

    Could you add a comment explaining the return? It’s not clear to me if this is supposed to be an optimization or a paranoid safety check. If it’s meant to be the latter, maybe a way to make it more paranoid would be to do if (coin.IsSpent()) has_spent_input = true here and assert !CheckInputs || !has_spent_input at the end.


    sipa commented at 1:45 am on June 9, 2017:
    I believe the code is correct. No entry was found in the cache, so validation should fail - it’s not a consistency bug.

    ryanofsky commented at 2:39 pm on June 9, 2017:

    I believe the code is correct. No entry was found in the cache, so validation should fail - it’s not a consistency bug.

    I also think the code is correct, just would like a comment saying why this is a return rather than a continue, or something that asserts against the CheckInputs() result. The whole function except this one line seems intended to do a bunch of checks and catch logic errors, while this line bypasses checks and seems to be doing some kind of optimization. AFAICT. Right now the intent is unclear.


    TheBlueMatt commented at 5:56 pm on June 21, 2017:
    Added.
  76. in src/validation.cpp:791 in eada04e778 outdated
    788-                __func__, hash.ToString(), FormatStateMessage(state));
    789+            // If we're using promiscuousmempoolflags, we may hit this normally
    790+            // Check if current block has some flags that scriptVerifyFlags
    791+            // does not before printing an ominous warning
    792+            if (!(~scriptVerifyFlags & currentBlockScriptVerifyFlags)) {
    793+                return error("%s: BUG! PLEASE REPORT THIS! ConnectInputs failed against latest-block but not STANDARD flags %s, %s",
    


    sipa commented at 1:13 am on June 9, 2017:
    No way to structure this so you can avoid the duplicate error line?

    TheBlueMatt commented at 5:51 pm on June 21, 2017:
    They arent duplicates? One prints that an issue has been found (and should be reported), the other simply warns you about your promiscuousmempool flags.
  77. in src/validation.cpp:425 in 4653582139 outdated
    415+            const CTransactionRef& txFrom = pool.get(txin.prevout.hash);
    416+            if (txFrom) {
    417+                assert(txFrom->GetHash() == txin.prevout.hash);
    418+                assert(txFrom->vout.size() > txin.prevout.n);
    419+                assert(txFrom->vout[txin.prevout.n] == coin.out);
    420+            } else {
    


    sipa commented at 1:42 am on June 9, 2017:
    I don’t think this section is necessary. The goal of this function should just be checking that no mempool bugs are leaking into consensus validation. This branch looks like an unrelated sanity check.

    TheBlueMatt commented at 5:53 pm on June 21, 2017:
    The goal of this function includes checking CCoinsViewMempool, which is mempool code, from leaking into consensus code. As a part of that, this branch is required (and like the mempool get, this pcoinsTip lookup should be incredibly hot in cache, so it shouldn’t be a big deal).
  78. in src/validation.cpp:421 in 4653582139 outdated
    411+        for (const CTxIn& txin : tx.vin) {
    412+            const Coin& coin = view.AccessCoin(txin.prevout);
    413+            if (coin.IsSpent()) return false;
    414+
    415+            const CTransactionRef& txFrom = pool.get(txin.prevout.hash);
    416+            if (txFrom) {
    


    sipa commented at 1:43 am on June 9, 2017:
    You can make the pool lookup conditional on coin.nHeight == MEMPOOL_HEIGHT. I think it’s reasonable to rely on the fact the mempool - even if broken - will return Coin objects with nHeight set to that value.

    TheBlueMatt commented at 5:52 pm on June 21, 2017:
    I’d really prefer not to. The unordered_map lookup should essentially be free since anything required to find that transaction should already be very hot in cache, and if its free, why shouldnt we?
  79. JeremyRubin commented at 4:54 am on June 9, 2017: contributor
    Theoretical question: Have you considered making both caches store their hashes in the same instance of the data structure? Might work out a bit better as each can borrow unused capacity from one another.
  80. TheBlueMatt commented at 8:06 pm on June 9, 2017: member

    Looks like it is to me?

    On June 5, 2017 9:54:23 PM EDT, Pieter Wuille notifications@github.com wrote:

    This does not look like it’s rebased on top of 10195?

  81. sipa commented at 8:08 pm on June 9, 2017: member
    @TheBlueMatt Now it is. When I made the comment, this PR had CCoins in it in some places.
  82. TheBlueMatt force-pushed on Jun 21, 2017
  83. TheBlueMatt commented at 5:57 pm on June 21, 2017: member
    @JeremyRubin I have not, that would likely be interesting in the future (as well as possibly not making it an even 1/2-1/2 split in memory usage).
  84. in src/validation.cpp:410 in 81c63f25ba outdated
    405+                 unsigned int flags, bool cacheSigStore, PrecomputedTransactionData& txdata) {
    406+    AssertLockHeld(cs_main);
    407+
    408+    assert(!tx.IsCoinBase());
    409+    {
    410+        LOCK(pool.cs);
    


    sipa commented at 11:37 pm on June 21, 2017:
    I think this lock is unnecessary - CTxMemPool::get grabs a lock on its own.

    TheBlueMatt commented at 4:21 pm on June 22, 2017:
    I moved the lock up in the function and added a comment noting why we might want it (note that its already held outside of this function before calling it in ATMP).

    sipa commented at 0:34 am on June 23, 2017:
    I’m still unconvinced it is needed, as it does not operate on CCoinsViewMemPool directly, but on a dissociated cache with the mempool UTXO loaded into it. If it’s valid at one point with those inputs, it’s also valid later.
  85. in src/validation.cpp:1784 in c435d9fbb7 outdated
    1778@@ -1666,7 +1779,7 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
    1779 
    1780             std::vector<CScriptCheck> vChecks;
    1781             bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
    1782-            if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : NULL))
    1783+            if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : NULL))
    


    sipa commented at 1:50 am on June 22, 2017:
    I think the second fCacheResults here can be false; as we normally pass a non-NULL pvChecks here, fCacheFullScriptStore = true has no effects anyway. This only affects TestBlockValidity.

    TheBlueMatt commented at 4:20 pm on June 22, 2017:
    fCacheFullScriptStore has a second meaning - it also deletes the element from the cache if a match is found, so we really should pass it through here to avoid deleting (or marking available) cache entries for TBV.

    sipa commented at 0:32 am on June 23, 2017:
    Ah thanks, I missed that.
  86. sipa commented at 7:33 am on June 22, 2017: member
    utACK c435d9fbb76a5c4b5525ffdf8fc74afdac1a42df apart from nits
  87. Add CheckInputs wrapper CCoinsViewMemPool -> non-consensus-critical
    This wraps CheckInputs in ATMP's cache-inputs call to check that
    each scriptPubKey the CCoinsViewCache provides is the one which
    was committed to by the input's transaction hash.
    b014668e27
  88. Update -maxsigcachesize doc clarify init logprints for it 309ee1ae7b
  89. Better document CheckInputs parameter meanings a3543af3cc
  90. TheBlueMatt force-pushed on Jun 22, 2017
  91. TheBlueMatt commented at 8:25 pm on June 23, 2017: member
    @sdaftuar pointed out that we could directly test CheckInputs’ use of its own cache in unit tests, so I added a rather simple one that did so.
  92. in src/test/checkinputs_script_cache_tests.cpp:48 in 35b420a483 outdated
    43+    spend.vin[0].scriptSig << vchSig;
    44+
    45+    LOCK(cs_main);
    46+
    47+    CValidationState state;
    48+    PrecomputedTransactionData txdata(spends[0]);
    


    sipa commented at 11:16 pm on June 23, 2017:
    spends is undeclared

    TheBlueMatt commented at 11:30 pm on June 23, 2017:
    Heh, thats what happens when you change a variable type after fixing the test.
  93. TheBlueMatt force-pushed on Jun 23, 2017
  94. laanwj assigned laanwj on Jun 26, 2017
  95. sipa commented at 11:09 pm on June 26, 2017: member
    utACK 316d328b915f2e23bb750096ee9878b14dfd0a26
  96. gmaxwell approved
  97. gmaxwell commented at 11:28 pm on June 26, 2017: contributor
    ACK. It’s worth pointing out that we have no system test that fails when the flags are mishandled here– though the unit test correctly fails.
  98. TheBlueMatt commented at 0:37 am on June 27, 2017: member
    I believe @sdaftuar indicated that he had a better test written for this, not sure what format it takes, however.
  99. sipa commented at 4:57 am on June 27, 2017: member

    Verified empirically that this actually gives a performance improvement:

    Last 10 block verifications on my server (benchmarked using -debug=bench):

    On master as of a few weeks ago:

    02017-06-26 22:16:21.499109     - Verify 5160 txins: 118.87ms (0.023ms/txin) [96.86s]
    12017-06-26 22:22:34.710518     - Verify 3957 txins: 84.70ms (0.021ms/txin) [96.95s]
    22017-06-26 22:32:43.025738     - Verify 4333 txins: 85.08ms (0.020ms/txin) [97.03s]
    32017-06-26 22:41:47.084320     - Verify 4300 txins: 83.29ms (0.019ms/txin) [97.12s]
    42017-06-26 22:46:33.711967     - Verify 4650 txins: 60.67ms (0.013ms/txin) [97.18s]
    52017-06-26 22:53:00.210314     - Verify 4656 txins: 61.19ms (0.013ms/txin) [97.24s]
    62017-06-26 22:54:35.550521     - Verify 5128 txins: 100.05ms (0.020ms/txin) [97.34s]
    72017-06-26 23:16:50.156286     - Verify 4889 txins: 118.09ms (0.024ms/txin) [97.46s]
    82017-06-26 23:18:17.993009     - Verify 4973 txins: 104.94ms (0.021ms/txin) [97.56s]
    92017-06-26 23:27:55.039898     - Verify 4712 txins: 92.86ms (0.020ms/txin) [97.65s]
    

    After restarting with this PR:

    02017-06-26 23:38:16.352833     - Verify 6016 txins: 39.89ms (0.007ms/txin) [0.26s]
    12017-06-26 23:38:37.499180     - Verify 6650 txins: 33.67ms (0.005ms/txin) [0.29s]
    22017-06-26 23:52:52.539748     - Verify 4686 txins: 60.94ms (0.013ms/txin) [0.35s]
    32017-06-27 00:34:42.788390     - Verify 4417 txins: 61.53ms (0.014ms/txin) [0.42s]
    42017-06-27 00:38:47.022972     - Verify 3023 txins: 33.88ms (0.011ms/txin) [0.45s]
    52017-06-27 00:42:59.372504     - Verify 4289 txins: 38.45ms (0.009ms/txin) [0.49s]
    62017-06-27 00:44:06.469073     - Verify 5065 txins: 34.92ms (0.007ms/txin) [0.52s]
    72017-06-27 00:53:41.905643     - Verify 4378 txins: 72.81ms (0.017ms/txin) [0.60s]
    82017-06-27 01:04:23.953806     - Verify 4917 txins: 52.77ms (0.011ms/txin) [0.65s]
    92017-06-27 01:08:59.598035     - Verify 5002 txins: 48.47ms (0.010ms/txin) [0.70s]
    
  100. Add CheckInputs() unit tests
    Check that cached script execution results are only valid for the same
    script flags; that script execution checks are returned for non-cached
    transactions; and that cached results are only valid for transactions
    with the same witness hash.
    e3f9c05b96
  101. TheBlueMatt force-pushed on Jun 27, 2017
  102. TheBlueMatt commented at 8:05 pm on June 27, 2017: member
    Replaced test with one by @sdaftuar which is much better. Should be good to go now.
  103. sdaftuar commented at 9:00 pm on June 27, 2017: member
    ACK e3f9c05
  104. in src/test/txvalidationcache_tests.cpp:125 in e3f9c05b96
    120+        }
    121+        bool ret = CheckInputs(tx, state, pcoinsTip, true, test_flags, true, add_to_cache, txdata, nullptr);
    122+        // CheckInputs should succeed iff test_flags doesn't intersect with
    123+        // failing_flags
    124+        bool expected_return_value = !(test_flags & failing_flags);
    125+        if (expected_return_value && upgraded_nop) {
    


    sipa commented at 11:45 pm on June 28, 2017:
    This logic may become unnecessary with #10699.
  105. laanwj merged this on Jun 29, 2017
  106. laanwj closed this on Jun 29, 2017

  107. laanwj referenced this in commit 2935b469ae on Jun 29, 2017
  108. laanwj removed this from the "Blockers" column in a project

  109. gmaxwell commented at 6:29 pm on June 29, 2017: contributor
    reACK w/ new tests
  110. morcos commented at 8:18 pm on July 5, 2017: member

    posthumous utACK

    Thanks for doing this.

  111. JeremyRubin commented at 8:45 am on July 6, 2017: contributor

    probably wrong place to have this conversation, but for the sake of continuity…

    @JeremyRubin I have not, that would likely be interesting in the future (as well as possibly not making it an even 1/2-1/2 split in memory usage).

    Yeah I’ve (asynchronously) been thinking about this one a little bit in spare cycles. There are a couple tricks on could do to easily increase frequency, e.g. add a insert_n_times to the cuckoocache where you can specify that it should be put onto 1-8 of it’s hash locations. This makes the hash “stickier” in memory compared to other entries and is pretty easy to compute.

    Generally, this also provides a really interesting mechanism for further tuning the hit rate based on other priors. E.g., if a txn comes in and we estimate that it has a 0.5 chance of entering a block, we could set it to have, say, half of it’s hash values inserted onto. If a txn comes in and we estimate that it has 0.25 we could fill a quarter of them.

    This can also be simulated by inserting the hash N times with different salts or something without modifying the cuckoocache internally.

  112. MarcoFalke referenced this in commit 1fc783fc08 on Jul 16, 2017
  113. laanwj referenced this in commit bc679829e2 on Mar 6, 2018
  114. PastaPastaPasta referenced this in commit eda5dac9f4 on Sep 8, 2019
  115. wqking referenced this in commit 1882d75407 on Jan 13, 2020
  116. barrystyle referenced this in commit f9282565ac on Jan 22, 2020
  117. PastaPastaPasta referenced this in commit 098ccb3aaa on Jun 10, 2020
  118. tohsnoom referenced this in commit d5f9fbf305 on Jun 11, 2020
  119. PastaPastaPasta referenced this in commit a12c3950ac on Jun 12, 2020
  120. PastaPastaPasta referenced this in commit d40a7323cb on Jun 13, 2020
  121. PastaPastaPasta referenced this in commit 1a40ff1e6e on Jun 14, 2020
  122. PastaPastaPasta referenced this in commit a00da7fb0e on Jun 14, 2020
  123. wqking referenced this in commit 3bede711b0 on Aug 5, 2021
  124. MarcoFalke locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me