This adds tests for src/script/sigcache.* .
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project - Maniphest Tasks
- T527: Add Schnorr support to OP_CHECKSIG and OP_CHECKDATASIG
- Commits
- rSTAGINGb1c3160da758: add sigcache tests
rABCb1c3160da758: add sigcache tests
- Make sure that cache hits when parameters are all repeated.
- Make sure that varying any of three parameters results in a cache miss.
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- refactor-sigcache-and-test
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 4698 Build 7459: Bitcoin ABC Buildbot (legacy) Build 7458: arc lint + arc unit
Event Timeline
src/script/sigcache.cpp | ||
---|---|---|
96 ↗ | (On Diff #6903) | The new function should be used here. Because the entry needs to be reused, that call for a slightly different API. |
src/script/sigcache.h | ||
46 ↗ | (On Diff #6903) | This name is not very good. It needs to convey what is cached, so what it returns is understood. What does CheckCache means ? That the cache in a self consistent state ? That the entry we are looking for is in the cache ? |
61 ↗ | (On Diff #6903) | There is nothing called GetSigCache. What about simply TestCachingTransactionSignatureChecker? |
src/test/sigcache_tests.cpp | ||
20 ↗ | (On Diff #6903) | Include standard lib last |
45 ↗ | (On Diff #6903) | Relayout. Aggregate the string pieces in one big string and let clang-format do its job. |
90 ↗ | (On Diff #6903) | You may want to check that no entry is inserted into the cache here. |
src/script/sigcache.cpp | ||
---|---|---|
96 ↗ | (On Diff #6903) | the other function calls signatureCache.Get(entry, false) (second argument is "erase" which doesn't immediately erase, but adds entry to garbage collection) so it's not quite the same... |
src/test/sigcache_tests.cpp | ||
20 ↗ | (On Diff #6903) | k |
45 ↗ | (On Diff #6903) | k |
90 ↗ | (On Diff #6903) | Excellent idea |
src/script/sigcache.cpp | ||
---|---|---|
95 ↗ | (On Diff #6913) | This still needs to use the same code as above, or you are testing a piece of code while running another one, which is not very useful. There are many option to achieve this. For isntance you can have a wrapper like template<typename F> RunMemoizedCheck(const std::vector<uint8_t> &vchSig, const CPubKey &pubkey, const uint256 &sighash, const F& fun) { if (signatureCache.Get(entry, !store)) { return true; } if (!fun()) { return false; } if (store) { signatureCache.Set(entry); } return true; } bool CachingTransactionSignatureChecker::VerifySignature( const std::vector<uint8_t> &vchSig, const CPubKey &pubkey, const uint256 &sighash) const { return RunMemoizedCheck(vchSig, pubkey, sighash, [&]{ return TransactionSignatureChecker::VerifySignature(vchSig, pubkey, sighash); }); } bool CachingTransactionSignatureChecker::IsCached( const std::vector<uint8_t> &vchSig, const CPubKey &pubkey, const uint256 &sighash) const { return RunMemoizedCheck(vchSig, pubkey, sighash, []{ return false; }); } There are many options. |
I'm curious, how efficient are abstractions like this in terms of compilation optimization? Should this end up creating the same assembly for VerifySignature(). ?
The cost of using the template should be fairly negligible. The compiler will inline fun as it is really simple.
src/script/sigcache.cpp | ||
---|---|---|
85 | Store is a very bad name. This controls whether thing are stored in the cache, for sure, but this is also to mark an entry as to be cleaned or not. This needs a better name. | |
src/test/sigcache_tests.cpp | ||
33 | Arguably, the checker shouldn't be a member here. It doesn't make a lot fo sense This class's responsability is to provide an interface to features of CachingTransactionSignatureChecker which are not publicly accessible for testing purposes. Parsing transaction and all are clearly out of scope. |
@deadalnix If you have any more changes to request please commandeer the revision and go right ahead and make the changes you have in mind. I would like to be working on building actual tests for Schnorr security rather than this.
src/script/sigcache.cpp | ||
---|---|---|
85 | Do you have a suggestion? |