Implement sighash cache in CHECKMULTISIG #14079

1. 
   jl2012 commented at 11:41 am on August 27, 2018: contributor

   Currently CHECKMULTISIG recalculates the sighash every time it verifies a key-signature pair. It is not necessary because sighash for the same nHashType is always the same inside a CHECKMULTISIG. Therefore, we could reuse a sighash as long as the nHashType is not changed.

   Note that in CHECKMULTISIG, the trimming of scriptCode (with CODESEPARATOR and FindAndDelete()) is done before the signature checking loop. So the scriptCode is always a constant in the same CHECKMULTISIG operation. However, this is not guaranteed across different CHECKSIG or CHECKMULTISIG so a cross-opcode sighash cache could not be done in the same way.

2. 
   DrahtBot commented at 2:09 pm on August 27, 2018: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #13360 ([Policy] Reject SIGHASH_SINGLE with output out of bound by jl2012)
   • #13266 ([moveonly] privatize SignatureExtractorChecker by Empact)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

3. Implement sighash cache in CHECKMULTISIG 0727e2bb70
4. jl2012 force-pushed on Aug 27, 2018
5. 
   practicalswift commented at 3:41 pm on August 27, 2018: contributor

   @jl2012 Would it be possible to generate some benchmarks to quantify the performance effect? :-)
6. 
   gmaxwell commented at 3:59 pm on August 27, 2018: contributor

   Awesome! I thought we already did that!
7. DrahtBot added the label Consensus on Mar 19, 2019
8. DrahtBot closed this on May 7, 2019

   ---

9. 
   DrahtBot commented at 5:06 pm on May 7, 2019: member
10. DrahtBot reopened this on May 7, 2019

    ---

11. fanquake added the label Needs Conceptual Review on Jun 24, 2019
12. fanquake added this to the "Chasing Concept ACK" column in a project

    ---

13. in src/script/sign.cpp:288 in 0727e2bb70

    281@@ -282,12 +282,12 @@ class SignatureExtractorChecker final : public BaseSignatureChecker
    282 
    283 public:
    284     SignatureExtractorChecker(SignatureData& sigdata, BaseSignatureChecker& checker) : sigdata(sigdata), checker(checker) {}
    285-    bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const override;
    286+    bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion, uint256* sighash_copy, int* sighash_copy_type) const override;
    287 };
    288 
    289-bool SignatureExtractorChecker::CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const
    290+bool SignatureExtractorChecker::CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion, uint256* sighash_copy, int* sighash_copy_type) const
    

    ---

    ---

    

    promag commented at 9:03 pm on June 27, 2019:

    nit, unused parameters could be commented.
14. in src/script/interpreter.cpp:1327 in 0727e2bb70

    1325     int nHashType = vchSig.back();
    1326     vchSig.pop_back();
    1327 
    1328-    uint256 sighash = SignatureHash(scriptCode, *txTo, nIn, nHashType, amount, sigversion, this->txdata);
    1329+    uint256 sighash;
    1330+    if (sighash_copy_type && sighash_copy && *sighash_copy_type == nHashType)
    

    ---

    ---

    

    promag commented at 9:04 pm on June 27, 2019:

    nit, add braces please.
15. 
    promag commented at 9:07 pm on June 27, 2019: member

    it is not necessary

    Unless there’s a performance penalty it doesn’t matter? Or is there another reason to cache it?

16. 
    kallewoof commented at 11:36 am on July 28, 2019: member

    Agree with @promag; I would like to see benchmark results indicating whether this actually has any effects.
17. fanquake removed this from the "Chasing Concept ACK" column in a project

    ---

18. fanquake added the label Up for grabs on Aug 14, 2019
19. 
    fanquake commented at 9:07 am on August 14, 2019: member

    Closing this as “Up for grabs”. If anyone is interested in picking this up, as mentioned multiple times above, the best place to start is likely writing some benchmarks that would demonstrate any performance improvements.
20. fanquake closed this on Aug 14, 2019

    ---

21. DrahtBot locked this on Dec 16, 2021

Contributors
jl2012 DrahtBot practicalswift gmaxwell promag kallewoof fanquake

Labels
Consensus Up for grabs Needs Conceptual Review