If you really are going that road, I would advise to check for the boundary conditions as SCRIPT_CACHE_MAX_SIGCHECKS and SCRIPT_CACHE_MAX_SIGCHECKS + 1 in addition of a few randomly picked values. This is better than magic constant as it makes it clear what the code is actually doing (I wrote another comment before figuring out I was wrong and write this one).

Magic constant in the code are almost always a bad idea.

You'd benefit encapsulating each of these tests and make the variable local. This code is more brittle than it needs to be.

Using a new freshly initialized int every time ensures that no spooky action at a distance is possible.

Maybe you do not want a new one for every test here, but you probably don't want to use the same one as line 465 or line 496.

Is this the behavior we want? I did not ask myself the question, but it seems like a gotcha, no? Maybe we are just completely fucked if it ever comes to this anyways.

You'll note that due to the call line 491, this may not be testing what you think it is. You can argue it's obvious from that patch and I'd agree, but the point is that it's fragile, as this may not remain obvious or even true going forward.

For instance a simple reordering of the previous test would look harmless when reviewed, yet may cause this test to become 100% moot.

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.

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

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.

Very good call.

Even then, why not put the right value in there? I get that it doesn't need to be put in there in this very patch, but it going to be computed anyway.

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.

Hmm how do you mean -- what's the right value?

That would actually end up throwing away one of the 'good' bits (the highest bit in u, 2**63) since 11400714819323198486 * 2**63 is a multiple of 2**64.

I dunno, all that stuff under crypto/ (where CSHA256 lives) seems to be pretty independent of the bitcoin constructs. You could perhaps add a wrapper under hash.h.

Yeah to be clear, I don't want to expose the internal storage format via a type here, for a few reasons:

• Don't want silent conversion happening in caller when squishing down to a small type, rather I really want the exception.
• The next thing we add to script cache might be a bool, in which case it would be totally legitimate to use a bitfield, trimming the ScriptCacheElement's sigchecks field be only 15 bits long. I definitely want caller to expect that SCRIPT_CACHE_MAX_SIGCHECKS may have any value, not just a type limit.

Alternately we can make AddKeyInScriptCache just return a bool false if it's unable to store the requested value, which frees the caller from having to perform range checks.

Yeah that was my thinking, we'd be screwed either way. :D

Yes I prefer the former (PoD) as well.

The main reason I'm adding these members is so that code using ScriptExecutionMetrics does not start silently being buggy, if it turns out that we need to add another value:

• Unlike struct initializer list (where you can leave off arguments), using the constructor forces you to provide the correct number of arguments, i.e., if we add another value to this struct then all the callsites creating ScriptExecutionMetrics with specific values will turn into compile errors until they provide the new value.
• Likewise operator== makes sure that you're doing a proper comparison of all values; operator+= ensures that all metrics are being accumulated.

Besides that, I want access via PoD semantics.

Heh yeah, that's leftover behaviour from replicating the example in cuckoocache test :D

How?

Yes. See where it is used in this diff.

?? I'm not sure how ScriptCacheKey's pod/nonpodness makes a difference. I think it's good to check the keys are distinct and that they are self-equal.

Just to add to this, I mean I want neither a strict PoD type (bare struct) nor a fully encapsulated class. Is there no middle ground allowed here?

Not sure what you mean...

As discussed in the other diff D4941, the metric class in some edge cases may need to support values > 65535, at least up to 5 million and more if the script opcode count limit is increased in future. So we can either:

• Use more bytes in cache, and remove error conditions.

• Keep the error conditions and maintain a distinction between the general purpose usage/arithmetic type and the (space-efficient) private cache storage type.

I think the first option is a bit sad since we are trying to be economical in the cache and those bytes will always be 0 in regular operation, at least with current rules.

The cache being of this size assumes that this is sufficient to store all the sigops of one transaction. You can have an accumulator to accumulate across transactions that use larger types internally.