Page MenuHomePhabricator

add sigcache tests
ClosedPublic

Authored by markblundeberg on Jan 24 2019, 14:42.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
T527: Add Schnorr support to OP_CHECKSIG and OP_CHECKDATASIG
Commits
rABCb1c3160da758: add sigcache tests
Summary

This adds tests for src/script/sigcache.* .

Test Plan
  • 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markblundeberg created this revision.Jan 24 2019, 14:42
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 24 2019, 14:42
Herald added a subscriber: schancel. · View Herald Transcript
deadalnix requested changes to this revision.Jan 24 2019, 15:14
deadalnix added inline comments.
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.

This revision now requires changes to proceed.Jan 24 2019, 15:14
markblundeberg added inline comments.Jan 24 2019, 15:28
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

renamed things; added a few more tests

markblundeberg marked 5 inline comments as done.Jan 24 2019, 15:44
deadalnix requested changes to this revision.Jan 24 2019, 15:54
deadalnix added inline comments.
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.

This revision now requires changes to proceed.Jan 24 2019, 15:54

refactored per @deadalnix comment (adjusted to work)

I'm curious, how efficient are abstractions like this in terms of compilation optimization? Should this end up creating the same assembly for VerifySignature(). ?

markblundeberg marked an inline comment as done.Jan 24 2019, 16:36
deadalnix requested changes to this revision.Jan 25 2019, 21:44

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 ↗(On Diff #6916)

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 ↗(On Diff #6916)

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.

This revision now requires changes to proceed.Jan 25 2019, 21:44

add more descriptive name and move around initialization in test

@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 ↗(On Diff #6916)

Do you have a suggestion?

deadalnix accepted this revision.Jan 26 2019, 02:15
This revision is now accepted and ready to land.Jan 26 2019, 02:15
This revision was automatically updated to reflect the committed changes.