Cache hashes #8422
Cache hashes #8422
Conversation
|
test_big_witness_transaction add significant test duration because of the creation of many signature, I don't feel like removing it as it may still be useful though. |
3529366 to
6c52333
Compare
|
travis fail probably unrelated to this PR |
| addCachedHashesToMap &= !cachedHashes.IsFull(); | ||
| bool result = VerifyScript(scriptSig, scriptPubKey, witness, nFlags, CachingTransactionSignatureChecker(ptxTo, nIn, amount, cacheStore, cachedHashes), &error); | ||
| // do not update the cache if cachedHashes is still empty, it can happen if the evaluation ends before any segwit CheckSig operation could run | ||
| addCachedHashesToMap &= !cachedHashes.IsEmpty(); |
There was a problem hiding this comment.
Might make sense to check if any "progress" was made instead to avoid grabbing locks for setting any time the retrieved cachedHashes isn't empty already.
6c52333 to
4fd4105
Compare
|
@instagibbs I've just updated the PR following your suggestion. |
4fd4105 to
ec6a8f2
Compare
|
You're making script/interpreter depend on sync, which causes a compile failure as libconsensus (which uses script/interpreter) does not link in Boost. One way would be to make CachedHashesMap a virtual class, with only a naive implementation (which does not cache anything). Then elsewhere you can create a derived class which adds the map and the lock. EDIT: oh no, I think you just need to stop including cachedhashesmap.h from interpreter.h. |
|
Can you also follow the coding style? Spaces after |
ec6a8f2 to
16bc578
Compare
|
@sipa yes, it was a mistake just removed it and fixed coding style. I am suprised, that https://travis-ci.org/bitcoin/bitcoin/builds/148371080 and when I compiled myself I did not experienced any build fails because of this mistake. |
|
Did you run |
|
nop, only make -j4. What does the check do ? I thought it was only running tests. |
| // if the scriptPubKey is not witness program, no need for cachedHashes, so do not read the map (it would acquire the lock needlessly) | ||
| bool addCachedHashesToMap = cachedHashesMap != NULL && | ||
| scriptPubKey.IsWitnessProgram(version, program); | ||
| if(addCachedHashesToMap) { |
16bc578 to
5547521
Compare
|
@sipa nits fix |
|
utACK 5547521c0ae38583e9f0504b3302bba928f5477d |
|
Needs rebase. |
522b23d to
7dbd2e7
Compare
|
Rebased, it made the diff smaller as this commit removed a code path where CachedHashesMap was previously needed. (but not used) |
|
Noting that this requires backport to 0.13 before segwit is activated. |
| #define SRC_CACHEDHASHESMAP_H | ||
|
|
||
|
|
||
| #include "script/interpreter.h" |
There was a problem hiding this comment.
You can move this include to the cpp or save the class CachedHashes; below.
|
utACK 7dbd2e7 |
| @@ -0,0 +1,21 @@ | |||
| // Copyright (c) 2009-2015 The Bitcoin Core developers | |||
|
utACK 7dbd2e7 |
7dbd2e7 to
910e7bb
Compare
|
@btcdrak updated the file header. |
|
Tested ACK 910e7bb: I timed the verification in the added test_big_witness_transaction test, and backported it to master (without sighash cache); this PR made it 7.5 times faster. |
|
Merely listing this for exposure, as I am perfectly fine with merging this PR here as it is: I made an altered implementation in https://github.com/sipa/bitcoin/commits/noprecomputecachedhashes that simply precomputes the 3 hashes for each transaction. In the listed benchmark, it is around 1% faster than the PR here. It may be preferable if we want to avoid contention on the sighash cache like in #8464. |
|
supersede by #8524 |
Alternative of #8259 as advised by @sipa.
Mid hashes are calculated lazily instead of aggressively.