Make signature caching optional, and move it out.

sipa commented at 2:18 pm on September 10, 2014: member

This will make it easier to export the script verification code to a thread-agnostic library.

By moving some of the parameters into a SignatureChecker object, some of the long argument lists are reduced.

Builds on top of (a rebased version of) #4555.

sipa force-pushed on Sep 10, 2014

sipa renamed this:
~~Move signature caching optional, and move it to another file.~~
Make signature caching optional, and move it to another file.
on Sep 10, 2014

sipa renamed this:
~~Make signature caching optional, and move it to another file.~~
Make signature caching optional, and move it out.
on Sep 10, 2014

sipa force-pushed on Sep 12, 2014

theuni commented at 6:42 pm on September 12, 2014: member

@TheBlueMatt fwiw, this pull-tester false-negative is the same one we get on Travis: http://jenkins.bluematt.me/pull-tester/p4890_035a4de88b03fcdfd9e0bf747f301fcc8f1bb299/test.log

So it seems it’s not anything related to a specific setup.

Not really worth worrying about, just pointing it out.

jtimon commented at 9:40 pm on September 13, 2014: contributor

~~@sipa you didn’t explained it, but I assume the purpose of the base class is to allow #4692 to reimplement the checker without cache? Also,~~ Can’t the flags be an attribute of SignatureChecker instead of a parameter in BaseSignatureChecker::CheckSig() ?

sipa commented at 2:57 am on September 14, 2014: member

@jtimon Yes indeed; this would allow #4692 to do checking without needing to depend on the cache, by using SignatureChecker() and not CachingSignatureChecker().

And you’re very right: the flags passing through CheckSig and VerifySignature is ugly, as SCRIPT_VERIFY_NOCACHE shouldn’t be an interpreter flag at all, as it’s no longer a feature of the interpreter. I added a commit that removes that flag, and turns it into a constructor argument to CachingSignatureChecker.

sipa force-pushed on Sep 14, 2014

sipa force-pushed on Sep 16, 2014

laanwj commented at 9:38 am on September 16, 2014: member

Haven’t verified code movements, but I like these code changes.

sipa force-pushed on Sep 17, 2014

sipa force-pushed on Sep 23, 2014

sipa commented at 3:22 am on September 29, 2014: member

@laanwj Would you mind verifying code movement?

laanwj commented at 8:22 am on September 29, 2014: member

Yes I’ll take a look.

in src/main.cpp: in 078ff4a462 outdated

696@@ -697,7 +697,7 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
697         // IsStandard() will have already returned false
698         // and this method isn't called.
699         vector<vector<unsigned char> > stack;
700-        if (!EvalScript(stack, tx.vin[i].scriptSig, tx, i, false))
701+        if (!EvalScript(stack, tx.vin[i].scriptSig, false, BaseSignatureChecker()))

laanwj commented at 9:19 am on September 29, 2014:

flags doesn’t match against signature

EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& script, unsigned int flags, const BaseSignatureChecker& checker)

sipa commented at 9:23 am on September 29, 2014:

Indeed, the false should be 0 or SCRIPT_VERIFY_NONE, but I didn’t want to touch that here.

laanwj commented at 10:10 am on September 29, 2014:

Okay, agreed, didn’t notice that was the case in the original code as well.

in src/script/interpreter.h: in 078ff4a462 outdated

45-bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, const CTransaction& txTo, unsigned int nIn, unsigned int flags);
46+
47+class BaseSignatureChecker
48+{
49+public:
50+    virtual bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode) const

laanwj commented at 9:53 am on September 29, 2014:

Defining a class with a virtual function without a virtual destructor is going to give warnings in some compilers

sipa commented at 5:00 pm on September 29, 2014:

Fixed.

laanwj commented at 9:56 am on September 29, 2014: member

This comment about NOCACHE script flag can also be removed: https://github.com/bitcoin/bitcoin/blob/master/src/test/transaction_tests.cpp#L34

in src/main.h: in 078ff4a462 outdated

341@@ -342,12 +342,13 @@ class CScriptCheck
342     const CTransaction *ptxTo;
343     unsigned int nIn;
344     unsigned int nFlags;
345+    bool cache;

laanwj commented at 10:10 am on September 29, 2014:

I think this argument should be called cacheWrite or cacheStore, to make it apparent that the cache is always used, but the difference that this boolean makes is whether the result is stored.

sipa commented at 5:00 pm on September 29, 2014:

Fixed.

laanwj commented at 10:35 am on September 29, 2014: member

utACK apart from above nits, going to test this

sipa force-pushed on Sep 29, 2014

sipa commented at 5:00 pm on September 29, 2014: member

Addressed all nits, I think.

sipa force-pushed on Sep 30, 2014

BitcoinPullTester commented at 3:35 am on September 30, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4890_c41cd3a3b2424748a701437d49f3622afd9ca5be/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj commented at 6:39 am on October 2, 2014: member

Bleh, needs rebase after #5017 (a trivial debug message change).

Abstract out SignatureChecker c7829ea797

Make signature cache optional 5c1e798a8e

Replace SCRIPT_VERIFY_NOCACHE by flag directly to checker e790c370b5

sipa force-pushed on Oct 2, 2014

sipa commented at 6:28 pm on October 2, 2014: member

Rebased.

TheBlueMatt commented at 8:59 pm on October 5, 2014: member

utACK. Can we move this one forward?

laanwj merged this on Oct 6, 2014

laanwj closed this on Oct 6, 2014

laanwj referenced this in commit 5f1aee066a on Oct 6, 2014

btcdrak referenced this in commit 234ddf8a42 on Dec 15, 2014

MarcoFalke locked this on Sep 8, 2021

Make signature caching optional, and move it out. #4890