Precompile Caching MVP

[garyschulte]

PR description

PR adds precompile caching behavior for an MVP set of precompiles that are costly enough to benefit from caching. Provision is added to disable caching via command line arg (for gas costing reasons), but it is enabled by default in besu, and disabled by default in evmtool and benchmark subcommand.

Changes:

• add a static member and setter in AbstractPrecompiledContract used to control whether we want to cache results
• add precompile-specific LRU caches with rational size limits in each MVP precompile
• add a cli arg for precompile caching, defaulted to true

MVP precompiles include:

• altbn128/bn254 precompiles for add, mul and pairing
• ecrecover precompile
• blake2 precompile

Feedback welcome on the design choices:

• one cache per precompile contract (since each will have different input and output size characteristics)
• cache is <hashCode, input_and_result_tuple> in order to verify input is truly identical rather than just matching by hashCode (it is trivial to construct requests that have different inputs, but similar Bytes hashCode)

Parallel transaction execution should benefit from precompile caching when state conflicts are detected. Attached are preliminary results from the nethermind gas-benchmarks suite which indicate performance does not seem to take a hit for cache checking and misses, and the caching itself is effective for repetitive/identical inputs

blake2.pdf
ecmul.pdf
ecrec.pdf

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

• Checked out our contribution guidelines?
• Considered documentation and added the doc-change-required label to this PR if updates are required.
• Considered the changelog and included an update if required.
• For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

• unit tests: ./gradlew build
• acceptance tests: ./gradlew acceptanceTest
• integration tests: ./gradlew integrationTest
• reference tests: ./gradlew ethereum:referenceTests:referenceTests

[ahamlat]

I think it makes sens to have a cache per precompile as we discussed. Also you need to change the key to use a hashing function that has no collisions, as the hashcode method that returns int doesn't can have collisions

[ahamlat]

As discussed before, hashCode is not a good key here as you may have collisions. We need to use either the whole input or a hashing function that assumes no collisions.

[garyschulte]

I was expecting and embracing collisions. I think a cache limited to <=1000 items is unlikely to have collisions in the int space unless it is an intentional attempt to create collisions. In that case, the equals() check will prevent using the cache and it will be overwritten with the new computed value.

If we use input Bytes, it seems we can attack the cache by filling it with hashCode collisions and cause a worst case performance where we have to use byte-by-byte equals() method to distinguish between the hashCode collisions for every item in the cache.

For that reason I think leaning into a 1 cache item per hash code is a safer strategy. It will work well with few naturally occurring collisions, and won't result in the ability to attack the cache to cause slow blocks.

[daniellehrner]

In another caching test PR I've used xxHash to create the cache key, which is supposed to be very fast.

Here's the PR where I've used it:
daniellehrner@c75f975#diff-006f4a4dd9869817685273bb0bc45923b06e6a926661e7ac84615564576e9060R146

[ahamlat]

Now that I understand that you have a second check on the input, I think it is a pretty smart method, I'm interested to see what the profiling show, basically what is the overhead of the equals on the input when there is a collision.
When there no collision, it is pretty good algorithm complexity. We can also play with the size of the cache to reduce collisions, and check if the memory overhead is not too big. This will depend on the input size, which not fixed I guess.

[ahamlat]

Nice that you included enabling the feature with a flag

[ahamlat]

Not really urgent or mandatory but it would be nice to add metrics for it to have the cache hit ratio. Check :

besu/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/trie/diffbased/bonsai/cache/BonsaiCachedMerkleTrieLoader.java

Line 43 in 8cddcfd

private final Cache<Bytes, Bytes> accountNodes =

[garyschulte]

Good idea. I will add look at adding an all-precompile 'cache hit' counter.

[garyschulte]

Added hit/miss/false positive counters per precompile. ECRECOVER has 0% false positive, but eip 196, 197, and blake2 seem to need a better hashcode implementation to limit the false positives.

[garyschulte]

for blake2 and altbn128 methods, the false positive vs hit ratio is not good:

[garyschulte]

ecrecover, which is hit much more frequently has zero false positives, and a good enough cache hit ratio to make it worthwhile: