Page MenuHomePhabricator
Differential D4834 Diff 15155 src/script/scriptcache.cpp

Changeset View

Standalone View

View Options

src/script/scriptcache.cpp

// Copyright (c) 2017 The Bitcoin developers // Copyright (c) 2017 The Bitcoin developers
// Distributed under the MIT software license, see the accompanying // Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php. // file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <script/scriptcache.h> #include <script/scriptcache.h>
#include <crypto/sha256.h> #include <crypto/sha256.h>
#include <cuckoocache.h> #include <cuckoocache.h>
#include <primitives/transaction.h> #include <primitives/transaction.h>
#include <random.h> #include <random.h>
#include <script/sigcache.h> #include <script/sigcache.h>
#include <sync.h> #include <sync.h>
#include <uint256.h>
#include <util/system.h> #include <util/system.h>
#include <validation.h> #include <validation.h>
static CuckooCache::cache<CuckooCache::KeyOnly<uint256>, SignatureCacheHasher> #include <limits>
/**
* In future if many more values are added, it should be considered to
* expand the element size to 64 bytes (with padding the spare space as
* needed) so the key can be long. Shortening the key too much risks
* opening ourselves up to consensus-failing collisions, however it should
* be noted that our cache nonce is private and unique, so collisions would
* affect only one node and attackers have no way of offline-preparing a
* collision attack even on short keys.
*/
struct ScriptCacheElement {
using KeyType = ScriptCacheKey;
KeyType key;
uint16_t nSigChecksCount;
static_assert(SCRIPT_CACHE_MAX_SIGCHECKS ==
std::numeric_limits<decltype(nSigChecksCount)>::max(),
"the declared maximum must match the type");
ScriptCacheElement() = default;
ScriptCacheElement(const ScriptCacheElement &rhs) = default;
ScriptCacheElement(const KeyType &keyIn, uint16_t nSigChecksIn)
: key(keyIn), nSigChecksCount(nSigChecksIn) {}
const KeyType &getKey() const { return key; }
deadalnixUnsubmitted
Done
Inline Actions

That's a good indicator the key is not meant to be public.

deadalnix: That's a good indicator the key is not meant to be public.
markblundebergAuthorUnsubmitted
Done
Inline Actions

Hmm, I interpret it as an API requirement of cuckoocache :-D

(anyway this struct is purely local to this cpp so I'm not bothered by it).

markblundeberg: Hmm, I interpret it as an API requirement of cuckoocache :-D (anyway this struct is purely…
};
static_assert(
sizeof(ScriptCacheElement) == 32,
"ScriptCacheElement should be 32 bytes, for cache line alignment purposes");
deadalnixUnsubmitted
Done
Inline Actions

This has nothing to do with alignment, in fact your alignment will be 2, not 32. This message does nto make sense. Keep the assert.

deadalnix: This has nothing to do with alignment, in fact your alignment will be 2, not 32. This message…
markblundebergAuthorUnsubmitted
Done
Inline Actions

Hmmm ok.... So, what is the advantage of having a 32 byte element rather than 34 byte?

markblundeberg: Hmmm ok.... So, what is the advantage of having a 32 byte element rather than 34 byte?
deadalnixUnsubmitted
Not Done
Inline Actions

You can have way more of them in a given amount of memory. The entropy we have in there is actually 32bytes, so we want to make sure we don't use more.

deadalnix: You can have way more of them in a given amount of memory. The entropy we have in there is…
class ScriptCacheHasher {
public:
template <uint8_t hash_select>
uint32_t operator()(const ScriptCacheKey &k) const {
static_assert(hash_select < 8, "only has 8 hashes available.");
const auto &d = k.data;
static_assert(sizeof(d) == 30,
deadalnixUnsubmitted
Done
Inline Actions

This doesn't check what you think it is checking. Use .size().

deadalnix: This doesn't check what you think it is checking. Use .size().
markblundebergAuthorUnsubmitted
Done
Inline Actions

Yeah I've tried using .size() in several different ways, for some reason the compiler seems to hate it being used in a static_assert. Do you know what syntax would work out?

Anyway, it's an array and we're doing memcpy so sizeof is definitely the right thing.

markblundeberg: Yeah I've tried using .size() in several different ways, for some reason the compiler seems to…
"modify the following if key size changes");
uint32_t u;
if (hash_select < 7) {
std::memcpy(&u, d.begin() + 4 * hash_select, 4);
deadalnixUnsubmitted
Done
Inline Actions

data is probably more appropriate here as you are looking for a pointer on the data, not an iterator.

deadalnix: data is probably more appropriate here as you are looking for a pointer on the data, not an…
} else {
// We are required to produce 8 subhashes. We have only two
// key bytes left over but this is fine. We put the leftovers
// on the higher bits, as these are what primarily get used to
// determine the index. The lower bits are filled by reusing
// some other bytes.
u = (uint32_t(d[28]) << 24) + (uint32_t(d[29]) << 16) +
(uint32_t(d[3]) << 8) + uint32_t(d[7]);
}
return u;
}
};
static CuckooCache::cache<ScriptCacheElement, ScriptCacheHasher>
scriptExecutionCache; scriptExecutionCache;
static uint256 scriptExecutionCacheNonce(GetRandHash()); static uint256 scriptExecutionCacheNonce(GetRandHash());
void InitScriptExecutionCache() { void InitScriptExecutionCache() {
// nMaxCacheSize is unsigned. If -maxscriptcachesize is set to zero, // nMaxCacheSize is unsigned. If -maxscriptcachesize is set to zero,
// setup_bytes creates the minimum possible cache (2 elements). // setup_bytes creates the minimum possible cache (2 elements).
size_t nMaxCacheSize = size_t nMaxCacheSize =
std::min( std::min(
std::max(int64_t(0), gArgs.GetArg("-maxscriptcachesize", std::max(int64_t(0), gArgs.GetArg("-maxscriptcachesize",
DEFAULT_MAX_SCRIPT_CACHE_SIZE)), DEFAULT_MAX_SCRIPT_CACHE_SIZE)),
MAX_MAX_SCRIPT_CACHE_SIZE) * MAX_MAX_SCRIPT_CACHE_SIZE) *
(size_t(1) << 20); (size_t(1) << 20);
size_t nElems = scriptExecutionCache.setup_bytes(nMaxCacheSize); size_t nElems = scriptExecutionCache.setup_bytes(nMaxCacheSize);
LogPrintf("Using %zu MiB out of %zu requested for script execution cache, " LogPrintf("Using %zu MiB out of %zu requested for script execution cache, "
"able to store %zu elements\n", "able to store %zu elements\n",
(nElems * sizeof(uint256)) >> 20, nMaxCacheSize >> 20, nElems); (nElems * sizeof(uint256)) >> 20, nMaxCacheSize >> 20, nElems);
} }
uint256 GetScriptCacheKey(const CTransaction &tx, uint32_t flags) { ScriptCacheKey GetScriptCacheKey(const CTransaction &tx, uint32_t flags) {
uint256 key; uint256 hash;
// We only use the first 19 bytes of nonce to avoid a second SHA round - // We only use the first 19 bytes of nonce to avoid a second SHA round -
// giving us 19 + 32 + 4 = 55 bytes (+ 8 + 1 = 64) // giving us 19 + 32 + 4 = 55 bytes (+ 8 + 1 = 64)
static_assert(55 - sizeof(flags) - 32 >= 128 / 8, static_assert(55 - sizeof(flags) - 32 >= 128 / 8,
"Want at least 128 bits of nonce for script execution cache"); "Want at least 128 bits of nonce for script execution cache");
CSHA256() CSHA256()
.Write(scriptExecutionCacheNonce.begin(), 55 - sizeof(flags) - 32) .Write(scriptExecutionCacheNonce.begin(), 55 - sizeof(flags) - 32)
.Write(tx.GetHash().begin(), 32) .Write(tx.GetHash().begin(), 32)
.Write((uint8_t *)&flags, sizeof(flags)) .Write((uint8_t *)&flags, sizeof(flags))
.Finalize(key.begin()); .Finalize(hash.begin());
return key; ScriptCacheKey ret;
static_assert(sizeof(ret.data) <= sizeof(hash), "assumed by copy");
deadalnixUnsubmitted
Done
Inline Actions

This doesn't check what you actually want to check. It is not because the structure is n bytes that the beffuer pointed by begin is. You want to use size.

deadalnix: This doesn't check what you actually want to check. It is not because the structure is n bytes…
markblundebergAuthorUnsubmitted
Done
Inline Actions

They are actually correct in practice -- sizeof(ret.data) == 30 and sizeof(hash)==32 but yeah I get your point, maybe uint32 will one day have a vtable or something. And same problem as above regarding array and uint256's size() in static_assert. :/

So, changing to regular assert.

markblundeberg: They are actually correct in practice -- sizeof(ret.data) == 30 and sizeof(hash)==32 but yeah I…
deadalnixUnsubmitted
Not Done
Inline Actions

Regular assert are fine. size() returns a constant so the compiler will be able to remove all of this anyways.

deadalnix: Regular assert are fine. size() returns a constant so the compiler will be able to remove all…
std::copy(hash.begin() + sizeof(hash) - sizeof(ret.data), hash.end(),
ret.data.begin());
return ret;
} }
bool IsKeyInScriptCache(uint256 key, bool erase) { bool IsKeyInScriptCache(ScriptCacheKey key, bool erase, int &nSigChecksRet) {
// TODO: Remove this requirement by making CuckooCache not require external // TODO: Remove this requirement by making CuckooCache not require external
// locks // locks
AssertLockHeld(cs_main); AssertLockHeld(cs_main);
return scriptExecutionCache.contains(key, erase); ScriptCacheElement elem(key, 0);
bool ret = scriptExecutionCache.get(elem, erase);
if (ret) {
nSigChecksRet = elem.nSigChecksCount;
}
return ret;
} }
void AddKeyInScriptCache(uint256 key) { void AddKeyInScriptCache(ScriptCacheKey key, int nSigChecks) {
if (0 > nSigChecks || nSigChecks > SCRIPT_CACHE_MAX_SIGCHECKS) {
deadalnixUnsubmitted
Done
Inline Actions

I don't understand why you persists making the signature count signed. This is clearly causing extra edge cases that don't need to be.

deadalnix: I don't understand why you persists making the signature count signed. This is clearly causing…
markblundebergAuthorUnsubmitted
Done
Inline Actions

I'm trying to follow what I understand to be the modern 'best practices' advice

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es106-dont-try-to-avoid-negative-values-by-using-unsigned
https://www.reddit.com/r/cpp/comments/7y0o6r/is_it_a_good_practice_to_use_unsigned_values/ and links therein

markblundeberg: I'm trying to follow what I understand to be the modern 'best practices' advice https://github.
deadalnixUnsubmitted
Done
Inline Actions

I see. They definitely have a point, but it seems like putting bubble wrap around the atrocity they have created rather than anything else :) Some of the argument they use are a bit bogus, as int also wrap around, except in completely unpredictable ways. We should probably get to a point where we use -Wconversion, but the codebase clearly ain't ready.

What's funny is the compiler will 100% transform this into unsigned(nSigChecks) >= SCRIPT_CACHE_MAX_SIGCHECKS

Another thing you want to consider is that the sigops count in metrics is public, which causes anyone to be able to play with it in way you can't predict. But in practice it is only incremented and no complex math operations are done on it. Even it it came to be the case, you could convert to an int then to do the computations.

void registerSigOps(uint16_t count = 1) {
    uint16_t oldCount = nSigCheck;
    nSigCheck += count;
    if (nSigCheck < oldCount) {
        throw std::range_error("sigchecks out of range");
    }
}

And voila.

deadalnix: I see. They definitely have a point, but it seems like putting bubble wrap around the atrocity…
markblundebergAuthorUnsubmitted
Done
Inline Actions

No such transformation happens -- both sides are int.

For the sigops thing being struct-public, I don't think it matters much since the whole cache implementation details are local to this CPP file. Callers don't even have a way of determining a pointer to any Element object/member since the cache is static and nothing returns references.

markblundeberg: No such transformation happens -- both sides are int. For the sigops thing being struct-public…
throw std::range_error("sigchecks out of range");
}
// TODO: Remove this requirement by making CuckooCache not require external // TODO: Remove this requirement by making CuckooCache not require external
// locks // locks
AssertLockHeld(cs_main); AssertLockHeld(cs_main);
scriptExecutionCache.insert(key); ScriptCacheElement elem(key, nSigChecks);
scriptExecutionCache.insert(elem);
} }