all: Unify keccaking

[pmikolajczyk41]

pulled in by OffchainLabs/nitro#4122

upstream PR was rejected as non-essential
closes NIT-4123

Context

Keccak hashing algorithm can be used in two ways: (1) as a one-shot hash function (we are just interested in hash of the data that we already have in full) or (2) as a cryptographic sponge (for cases where we don't know all the data to be hashed yet, we need some intermediate hashes, etc.)

From the caller perspective, (1) is a completely stateless process, but (2) requires keeping some state.

The crypto package exposes Keccak256Hash() and Keccak256() functions realizing (1) and NewKeccakState() for (2).

Problem

There are ~20 places where we use stateful approach (2) for (1). In other words, we:

• use NewKeccakState() to create the sponge state
• (optionally) keep it as a field
• use lower-level methods on this object to compute the hash (Write(), Read(), Sum())
  whereas we could (and should) just use Keccak256() utility.

Proposed changes

1. Use stateless keccaking wherever possible. It seems that the only place we actually need stateful one is the spongeDb in some testing.
2. Do some minor refactor to deduplicate logic between Keccak256Hash() and Keccak256()
3. Make KeccakState interface just extend io.Reader instead of inlining io.Reader's method

Motivation

When running geth within ZK VM (like SP1) it is highly beneficial to outsource heavy computation (like Keccak) to the dedicated VM precompiles. By removing all unnecessarily stateful hashing and reusing the Keccak256Hash() and Keccak256() utilities, it is way easier to replace all keccak invocations than to worry that some of them will persist in this strange form.

[bragaigor]

LGTM, wondering if we should run a complete set of tests on nitro side just to be double sure everything still works?

[pmikolajczyk41]

LGTM, wondering if we should run a complete set of tests on nitro side just to be double sure everything still works?

@bragaigor good point! I have opened a companion PR in nitro: OffchainLabs/nitro#4108 - CI is green ✅

[eljobe]

I'm nervous about this change.

I could be completely wrong. But, I seem to remember that the resetting the incremental version of the KeccakState instance is less computationally expensive than allocating and destroying it over and over again (which is what happens inside Keccak256.

Did you run any benchmarks to test for possible regressions?

I could be wrong. But, I don't want to cause a big performance slow-down in our current validators as we get ready for our future ZK-Validators.

[pmikolajczyk41]

@eljobe please notice that I'm using Keccak256 and Keccak256Hash functions that do not allocate and destroy any state - they both use a common pool of states and reset a 'borrowed' state.

[eljobe]

@eljobe please notice that I'm using Keccak256 and Keccak256Hash functions that do not allocate and destroy any state - they both use a common pool of states and reset a 'borrowed' state.

This does make me more comfortable. In fact, I'm going approve and merge the change. But, I'd still love to have seen the output of a benchmarking run that showed that the replacement doesn't introduce any performance overhead. (For example, I could imagine that a heavily contended for pool of hashers might introduce some locking or something.) But, at least, the code is trying to be as efficient as the previous implementation.