Page MenuHomePhabricator

add sigcache tests
ClosedPublic

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

Details

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
Branch
add-sigcache-tests
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4695
Build 7453: Bitcoin ABC Buildbot (legacy)
Build 7452: arc lint + arc unit

Event Timeline

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
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

deadalnix requested changes to this revision.Jan 24 2019, 15:54
deadalnix added inline comments.
src/script/sigcache.cpp
95

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(). ?

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?

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