Make signature caching optional, and move it out. by sipa · Pull Request #4890 · bitcoin/bitcoin

sipa · 2014-09-10T14:18:39Z

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.

theuni · 2014-09-12T18:42:59Z

@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 · 2014-09-13T21:40:41Z

@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 · 2014-09-14T02:57:51Z

@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.

laanwj · 2014-09-16T09:38:22Z

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

sipa · 2014-09-29T03:22:07Z

@laanwj Would you mind verifying code movement?

laanwj · 2014-09-29T08:22:11Z

Yes I'll take a look.

laanwj · 2014-09-29T09:19:23Z

src/main.cpp

flags doesn't match against signature EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& script, unsigned int flags, const BaseSignatureChecker& checker)

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

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

laanwj · 2014-09-29T09:56:30Z

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

laanwj · 2014-09-29T10:10:17Z

src/main.h

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.

laanwj · 2014-09-29T10:35:24Z

utACK apart from above nits, going to test this

sipa · 2014-09-29T17:00:33Z

Addressed all nits, I think.

BitcoinPullTester · 2014-09-30T03:35:57Z

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 · 2014-10-02T06:39:20Z

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

sipa · 2014-10-02T18:28:58Z

Rebased.

TheBlueMatt · 2014-10-05T20:59:02Z

utACK. Can we move this one forward?

e790c37 Replace SCRIPT_VERIFY_NOCACHE by flag directly to checker (Pieter Wuille) 5c1e798 Make signature cache optional (Pieter Wuille) c7829ea Abstract out SignatureChecker (Pieter Wuille)

CHECKLOCKTIMEVERIFY patch because EvalScript() no longer receives txTo and nIn.

sipa force-pushed the optioncache branch from cd8a57a to d89be31 Compare September 10, 2014 14:19

sipa changed the title ~~Move signature caching optional, and move it to another file.~~ Make signature caching optional, and move it to another file. Sep 10, 2014

sipa changed the title ~~Make signature caching optional, and move it to another file.~~ Make signature caching optional, and move it out. Sep 10, 2014

sipa mentioned this pull request Sep 12, 2014

Don't pass nHashType down to script.h/.cpp #4555

Merged

sipa force-pushed the optioncache branch 2 times, most recently from ef0289b to 035a4de Compare September 12, 2014 18:08

sipa force-pushed the optioncache branch 2 times, most recently from 82acca7 to 4e8f7e1 Compare September 16, 2014 00:36

sipa mentioned this pull request Sep 16, 2014

Don't hash what you're not going to sign #4694

Closed

sipa force-pushed the optioncache branch 2 times, most recently from bc889ef to 078ff4a Compare September 23, 2014 19:48

jtimon mentioned this pull request Sep 27, 2014

Replace SignatureHash() with class TxSignatureHasher #4989

Closed

laanwj reviewed Sep 29, 2014
View reviewed changes

sipa force-pushed the optioncache branch from 078ff4a to 77d2694 Compare September 29, 2014 16:59

sipa force-pushed the optioncache branch from 77d2694 to c41cd3a Compare September 30, 2014 03:18

laanwj mentioned this pull request Oct 2, 2014

Remove core dependencies from CScript #4981

Merged

Abstract out SignatureChecker

c7829ea

sipa added 2 commits October 2, 2014 20:26

Make signature cache optional

5c1e798

Replace SCRIPT_VERIFY_NOCACHE by flag directly to checker

e790c37

sipa force-pushed the optioncache branch from c41cd3a to e790c37 Compare October 2, 2014 18:28

laanwj merged commit e790c37 into bitcoin:master Oct 6, 2014

btcdrak added a commit to viacoin/viacoin-obsolete that referenced this pull request Dec 15, 2014

Hack around API changes bitcoin/bitcoin#4890 which breaks

234ddf8

CHECKLOCKTIMEVERIFY patch because EvalScript() no longer receives txTo and nIn.

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

sipa commented Sep 10, 2014

Uh oh!

theuni commented Sep 12, 2014

Uh oh!

jtimon commented Sep 13, 2014

Uh oh!

sipa commented Sep 14, 2014

Uh oh!

laanwj commented Sep 16, 2014

Uh oh!

sipa commented Sep 29, 2014

Uh oh!

laanwj commented Sep 29, 2014

Uh oh!

laanwj Sep 29, 2014

Choose a reason for hiding this comment

Uh oh!

sipa Sep 29, 2014

Choose a reason for hiding this comment

Uh oh!

laanwj Sep 29, 2014

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 29, 2014

Uh oh!

laanwj Sep 29, 2014

Choose a reason for hiding this comment

Uh oh!

sipa Sep 29, 2014

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 29, 2014

Uh oh!

sipa commented Sep 29, 2014

Uh oh!

BitcoinPullTester commented Sep 30, 2014

Uh oh!

laanwj commented Oct 2, 2014

Uh oh!

sipa commented Oct 2, 2014

Uh oh!

TheBlueMatt commented Oct 5, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants