Skip PrecomputedTransactionData hashing for cache hits. #13233

1. 
   TheBlueMatt commented at 5:48 pm on May 14, 2018: member

   We currently calculate PrecomputedTransactionData for all transactitons that appear in blocks, but this is entirely uneccessary on the vast majority of transactions near the tip due to the script cache. Thus, we move the calculation into CheckInputs after the script cache check. I played around with a number of refactors to CheckInputs to do this a bit cleaner but it seems rather difficult to do so, so I just stuck with the obvious version
2. Remove unused fScriptChecks parameter from CheckInputs

   fScriptChecks = false just short-circuits the entire function, so
   passing it in is entirely useless.

   26a965c33e
3. Skip PrecomputedTransactionData hashing for cache hits. feda9b37de
4. in src/validation.cpp:1348 in 26a965c33e outdated

   1342@@ -1343,83 +1343,72 @@ void InitScriptExecutionCache() {
   1343  *
   1344  * Non-static (and re-declared) in src/test/txvalidationcache_tests.cpp
   1345  */
   1346-bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, bool fScriptChecks, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks)
   1347+bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks)
   1348 {
   1349     if (!tx.IsCoinBase())
   

   ---

   ---

   

   MarcoFalke commented at 5:56 pm on May 14, 2018:

   nit: since you re-indent the whole function body you might also remove this scope and put a if (tx.IsCoinBase()) return true; in the first line.

   ---

   

   promag commented at 7:57 pm on May 14, 2018:

   Yeah, I also prefer early return in these cases.
5. 
   promag commented at 7:57 pm on May 14, 2018: member

   Nice, concept ACK.
6. in src/script/interpreter.cpp:1201 in feda9b37de

   1194@@ -1195,10 +1195,10 @@ uint256 GetOutputsHash(const CTransaction& txTo) {
   1195 
   1196 } // namespace
   1197 
   1198-PrecomputedTransactionData::PrecomputedTransactionData(const CTransaction& txTo)
   1199+void PrecomputedTransactionData::ComputeHashes(const CTransaction& txTo)
   1200 {
   1201     // Cache is calculated only for transactions with witness
   1202-    if (txTo.HasWitness()) {
   1203+    if (!ready && txTo.HasWitness()) {
   

   ---

   ---

   

   promag commented at 10:23 pm on May 14, 2018:

   It should not be possible to call ComputeHashes with different transactions right? I have verified that currently it’s the case, but maybe we should prevent invalid usage?
7. 
   gmaxwell commented at 6:54 pm on May 15, 2018: contributor

   No benchmarks?
8. MarcoFalke added the label Needs rebase on Jun 6, 2018
9. TheBlueMatt closed this on Jun 14, 2018

   ---

10. fanquake referenced this in commit 6519be6054 on Sep 2, 2019
11. sidhujag referenced this in commit aa78f3d3fe on Sep 3, 2019
12. laanwj removed the label Needs rebase on Oct 24, 2019
13. MarcoFalke locked this on Dec 16, 2021

Contributors
TheBlueMatt MarcoFalke promag gmaxwell