/bitcoin

Reuse sighash computations across evaluation (rebase of #4562) #8654

Open
wants to merge 4 commits into
from
+593 −1

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
10 participants
@jl2012 @sipa @NicolasDorier @btcdrak @morcos @laanwj @TheBlueMatt @fanquake @MarcoFalke @jonasschnelli
@jl2012
Member

jl2012 commented Sep 2, 2016

Rebase of #4562 by @sipa

The effect of doing this would not be very obvious in normal use, but could have >10x improvement for a O(n^2) hashing attack

@sipa sipa and 1 other commented on an outdated diff Sep 2, 2016

View full outdated diff
src/script/interpreter.h
@@ -105,6 +105,24 @@ struct PrecomputedTransactionData
PrecomputedTransactionData(const CTransaction& tx);
};
+struct SigHashCache
+{
+ bool set[6];
+ uint256 value[6];
+
+ void Clear()
+ {
+ for (int i=0; i<12; i++) {
@sipa

sipa Sep 2, 2016

Owner

s/12/6/ ?

@jl2012

jl2012 Sep 2, 2016

Member

fixed

@sipa sipa and 1 other commented on an outdated diff Sep 2, 2016

View full outdated diff
src/script/interpreter.cpp
@@ -1229,7 +1231,12 @@ bool TransactionSignatureChecker::CheckSig(const vector<unsigned char>& vchSigIn
int nHashType = vchSig.back();
vchSig.pop_back();
- uint256 sighash = SignatureHash(scriptCode, *txTo, nIn, nHashType, amount, sigversion, this->txdata);
+ int entry = ((nHashType & 0x1F) == SIGHASH_SINGLE ? 0 : ((nHashType & 0x1F) == SIGHASH_NONE ? 1 : 2)) + ((nHashType & 0x80) ? 3 : 0);
@sipa

sipa Sep 2, 2016

Owner

Perhaps add a comment here that sigversion is not used in the entry calculation because we know there will only be one sigversion across an entire input. Also, if future extra sighashes are defined, this may need extension (or, alternatively, cache read/write could be skipped for sigversion != 0).

@jl2012

jl2012 Sep 2, 2016

Member

yes, I changed that to 12 as I thought there are 12 combinations (sw and pre-sw). Later I figured out that was not needed but forgot to change it back

fanquake added the Refactoring label Sep 3, 2016

@jl2012
Member

jl2012 commented Sep 5, 2016

fixed a bug in #4562. It requires 256 cache slots instead of just 6 for the common types, since the nHashType is part of the sighash so each one is unique.

Need more tests for witness txs.
@sipa
Owner

sipa commented Sep 5, 2016

Nice work with the tests so far!

jl2012 referenced this pull request Sep 5, 2016

Merged

Fix SIGHASH_SINGLE bug in test_framework SignatureHash #8667

@jl2012
Member

jl2012 commented Sep 6, 2016

Test updated to cover #8524

Benchmarking with and without #8524/#8654: https://gist.github.com/jl2012/f3262fd7cf47664ce43f036b9539e831

@jl2012 jl2012 commented on an outdated diff Sep 6, 2016

View full outdated diff
qa/rpc-tests/test_framework/script.py
@@ -882,7 +882,7 @@ def SignatureHash(script, txTo, inIdx, hashtype):
tmp = txtmp.vout[outIdx]
txtmp.vout = []
for i in range(outIdx):
- txtmp.vout.append(CTxOut())
+ txtmp.vout.append(CTxOut(-1))
@jl2012

jl2012 Sep 6, 2016

Member

See #8667
@NicolasDorier
Member

NicolasDorier commented Sep 6, 2016

is it be possible to put the cache as a field of the TransactionChecker rather than as yet another parameter in CheckSig ?
@sipa
Owner

sipa commented Sep 6, 2016

@NicolasDorier I believe so, yes!
@sipa
Owner

sipa commented Sep 6, 2016

@NicolasDorier Actually, no. Not until we outlaw OP_CODESEPARATOR, as the script execution needs to be able to wipe the cache.
@NicolasDorier
Member

NicolasDorier commented Sep 6, 2016

@sipa you can add a ScriptCode field to SigHashCache, and add a check in CheckSig to use the cache only if the ScriptCode match.
@NicolasDorier
Member

NicolasDorier commented Sep 6, 2016

I hope nobody intend to outlaw OP_CODESEPARATOR, it is a nice way for a signer to sign a particular executed path without requiring multiple public key. May have potential cool things to do with it and MAST, as with MAST we can have lots of different branches.
@sipa
Owner

sipa commented Sep 6, 2016 • edited

@NicolasDorier Nice idea. Implemented in https://github.com/sipa/bitcoin/commits/sighashcache. (@jl2012: feel free to squash if you like it)

I don't understand what you're suggesting about OP_CODESEPARATOR. Perhaps we should discuss that on IRC instead.
@NicolasDorier
Member

NicolasDorier commented Sep 6, 2016 • edited

Code Review ACK for https://github.com/sipa/bitcoin/commits/sighashcache (this PR + the commit removing the param to CheckSig)

However, I'm not sure it is really useful, in a case of O(n^2) hashing attack, having a 10x performance improvement for a 5 second block verification is not going to do huge difference.
@jl2012
Member

jl2012 commented Sep 6, 2016

Squashed with @sipa 's patch

@NicolasDorier, the effect would be limited for a miner-initiated attack. However, for standard transaction, 10x could be a make or break difference
@sipa
Owner

sipa commented Sep 6, 2016

utACK 9e4cf76
@NicolasDorier
Member

NicolasDorier commented Sep 6, 2016 • edited

@jl2012 as implemented now, the cache is per input. So how can it increase 10x for standard transaction ?
Right now it improve the case of one input with lots of checksig.
@sipa
Owner

sipa commented Sep 6, 2016

@NicolasDorier Please move back-on-forth discussions to IRC.
@NicolasDorier
Member

NicolasDorier commented Sep 6, 2016

utACK 9e4cf76
@jl2012
Member

jl2012 commented Sep 8, 2016

Is this a target of 0.13.1?

laanwj added this to the 0.13.1 milestone Sep 8, 2016

MarcoFalke added the Needs backport label Sep 9, 2016

@btcdrak
Member

btcdrak commented Sep 9, 2016

utACK 9e4cf76
@NicolasDorier
Member

NicolasDorier commented Sep 17, 2016

I think it would be even better to tie the lifetime of the cache to the EvalScript function only instead of the TransactionSignatureChecker. Just dropping it as an idea, still utACK 9e4cf76.

jl2012 referenced this pull request Sep 18, 2016

Open

Implement excessive sighashing protection policy with tight sighash estimation #8755

@jl2012 jl2012 added a commit to jl2012/bitcoin that referenced this pull request Sep 19, 2016

@jl2012 jl2012 Implement sighash cache for signature within CHECKMULTISIG 
This makes sure that SignatureHash is performed once only for each signature within a CHECKMULTISIG. Alternative to #8654.
 de93019

jl2012 referenced this pull request Sep 19, 2016

Closed

Implement excessive sighashing protection policy with loose sighash estimation #8756

@jl2012 jl2012 added a commit to jl2012/bitcoin that referenced this pull request Sep 19, 2016

@jl2012 jl2012 Implement sighash cache for signature within CHECKMULTISIG 
This makes sure that SignatureHash is performed once only for each signature within a CHECKMULTISIG. Alternative to #8654.
 63db0c6

@jl2012 jl2012 added a commit to jl2012/bitcoin that referenced this pull request Sep 19, 2016

@jl2012 jl2012 Implement sighash cache for signature within CHECKMULTISIG 
This makes sure that SignatureHash is performed once only for each signature within a CHECKMULTISIG. Alternative to #8654.
 ed71079
@jl2012
Member

jl2012 commented Sep 19, 2016

I have an alternative implementation as ed71079 in #8756. It only reuse a sighash for a signature within CHECKMULTISIG. It is less risky as we don't need to worry about any unexpected behavior due to OP_CODESEPARATOR and FindAndDelete. Nonetheless, #8756 could provide the same level of sighash attack protection as #8654 combined with #8755.
@jl2012
Member

jl2012 commented Sep 22, 2016

I think it would take more time to analysis this issue and figure out the best approach. I'm inclined not to include this in 0.13.1

laanwj removed this from the 0.13.1 milestone Sep 22, 2016

MarcoFalke removed the Needs backport label Sep 24, 2016

@jl2012 jl2012 added a commit to jl2012/bitcoin that referenced this pull request Sep 30, 2016

@jl2012 jl2012 Implement sighash cache for signature within CHECKMULTISIG 
This makes sure that SignatureHash is performed once only for each signature within a CHECKMULTISIG. Alternative to #8654.
 6b194c8

@jl2012 jl2012 added a commit to jl2012/bitcoin that referenced this pull request Oct 27, 2016

@jl2012 jl2012 Implement sighash cache for signature within CHECKMULTISIG 
This makes sure that SignatureHash is performed once only for each signature within a CHECKMULTISIG. Alternative to #8654.
 c1eea2c
@jl2012
Member

jl2012 commented Oct 29, 2016

Tests are updated with FindAndDelete tests. The earlier version (#4562) did not reset the cache after FindAndDelete and the new tests would have caught the bug.

However, the tests require the public key recovery code from https://github.com/petertodd/python-bitcoinlib , which may have copyright issues. Any suggestions?

jonasschnelli added this to the 0.14.0 milestone Dec 22, 2016

@morcos
Contributor

morcos commented Jan 5, 2017

I'm interested in understanding the benchmark results a little bit more.

It seems there are huge speed ups in big CHECKMULTISIG transactions, but there appear to be a non-neglible slowdown in more regular transactions (the last test in the benchmark).

I'd be concerned that this code will on average be a slow down to block validation. Has it been benchmarked for typical blocks either during IBD or normal operation.
@jl2012
Member

jl2012 commented Jan 5, 2017

@morcos: good question. I'll try to do some benchmarking in real operation
@morcos
Contributor

morcos commented Jan 9, 2017

I did some benchmarking but did not see any noticeable performance difference either way for typical usage

laanwj removed this from the 0.14.0 milestone Jan 12, 2017

@laanwj
Owner

laanwj commented Jan 12, 2017

Untagging this for 0.14 as discussed in the meeting
@TheBlueMatt
Contributor

TheBlueMatt commented Jul 11, 2017

Needs rebase (though if you dont get to it early this week maybe just wait until post-15).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment