Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Oct 6, 2014
Merged

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

merged 3 commits into from
Oct 6, 2014

Conversation

sipa
Copy link
Member

@sipa sipa commented Sep 10, 2014

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 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 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 sipa mentioned this pull request Sep 12, 2014
Merged
@sipa sipa force-pushed the optioncache branch 2 times, most recently from ef0289b to 035a4de Compare September 12, 2014 18:08
@theuni
Copy link
Member

theuni commented Sep 12, 2014

@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
Copy link
Contributor

jtimon commented Sep 13, 2014

@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
Copy link
Member Author

sipa commented Sep 14, 2014

@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 sipa force-pushed the optioncache branch 2 times, most recently from 82acca7 to 4e8f7e1 Compare September 16, 2014 00:36
@sipa sipa mentioned this pull request Sep 16, 2014
Closed
@laanwj
Copy link
Member

laanwj commented Sep 16, 2014

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

@sipa sipa force-pushed the optioncache branch 2 times, most recently from bc889ef to 078ff4a Compare September 23, 2014 19:48
@jtimon jtimon mentioned this pull request Sep 27, 2014
Closed
@sipa
Copy link
Member Author

sipa commented Sep 29, 2014

@laanwj Would you mind verifying code movement?

@laanwj
Copy link
Member

laanwj commented Sep 29, 2014

Yes I'll take a look.

laanwj
laanwj reviewed Sep 29, 2014
View reviewed changes
src/main.cpp
@@ -697,7 +697,7 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
// IsStandard() will have already returned false
// and this method isn't called.
vector<vector<unsigned char> > stack;
if (!EvalScript(stack, tx.vin[i].scriptSig, tx, i, false))
if (!EvalScript(stack, tx.vin[i].scriptSig, false, BaseSignatureChecker()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@laanwj
Copy link
Member

laanwj commented Sep 29, 2014

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

laanwj
laanwj reviewed Sep 29, 2014
View reviewed changes
src/main.h
@@ -342,12 +342,13 @@ class CScriptCheck
const CTransaction *ptxTo;
unsigned int nIn;
unsigned int nFlags;
bool cache;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@laanwj
Copy link
Member

laanwj commented Sep 29, 2014

utACK apart from above nits, going to test this

@sipa
Copy link
Member Author

sipa commented Sep 29, 2014

Addressed all nits, I think.

@BitcoinPullTester
Copy link

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
Copy link
Member

laanwj commented Oct 2, 2014

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

@laanwj laanwj mentioned this pull request Oct 2, 2014
Merged
@sipa
Copy link
Member Author

sipa commented Oct 2, 2014

Rebased.

@TheBlueMatt
Copy link
Contributor

utACK. Can we move this one forward?

@laanwj laanwj merged commit e790c37 into bitcoin:master Oct 6, 2014
laanwj added a commit that referenced this pull request Oct 6, 2014
@laanwj
Merge pull request #4890
5f1aee0 
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)
btcdrak added a commit to viacoin/viacoin-obsolete that referenced this pull request Dec 15, 2014
@btcdrak
Hack around API changes bitcoin/bitcoin#4890 which breaks
234ddf8 
CHECKLOCKTIMEVERIFY patch because EvalScript() no longer
receives txTo and nIn.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sipa @theuni @jtimon @laanwj @BitcoinPullTester @TheBlueMatt