Repository navigation
fix(crypto): correct Keccak digest API behavior - #7018
Conversation
76337ca to
43742b5
Compare
|
Finished a review pass. The issues I found match @3for two open comments. I'll approve once those are addressed. |
3for
left a comment
There was a problem hiding this comment.
LGTM.
Please document the branch-wide Digest API compatibility changes in the release notes:
-
Source compatibility:
Digest.digest(byte[], int, int)now declares the checkedDigestException. External callers may need to catch or declare it to compile, while existing compiled binaries remain compatible. -
Insufficient output buffer: Instead of returning truncated output, the method now throws
DigestException. -
Input validation:
update()now rejects negative lengths instead of silently ignoring them. Out-of-bounds arguments andnullinput now consistently throwIllegalArgumentException. -
Subclass contract: Subclasses of
DigestEnginethat overrideengineReset()must callsuper.engineReset()to ensure the buffered state is fully reset.
|
Both SPI issues are fixed in 301b22e. The reset and short-buffer regression tests pass for Keccak256 and Keccak512, and the crypto tests are green on my side. LGTM. |
What does this PR do?
Fixes Keccak-256/512 Java digest handling.
update(ByteBuffer)now hashes the remaining bytes and advances the buffer position to its limit. The SPI update and digest methods delegate to the existing hash implementation.Output-buffer digests now require space for the complete 32/64-byte result. Short output raises
DigestExceptionbefore finalization, invalid output ranges raiseIllegalArgumentException, and successful calls reset the digest. Removes the temporary buffer used only for truncated output.Why are these changes required?
The empty SPI update methods consumed
ByteBufferinput without hashing it. The output-buffer overload also returned truncated hashes instead of requiring a complete digest.This PR has been tested by:
76337caedb: 28 tests passed against Bouncy Castle 1.84'sKeccakDigest, covering buffer variants, mixed updates, output boundaries, reset, SPI methods, and copied state.1ff2aae94b, head76337caedb, and Bouncy Castle 1.84; see the consensus analysis below.git diff --checkpassed.Follow up
Run the targeted Gradle tests and full CI suite before merging.
Extra details
Consensus impact
The Keccak permutation and padding are unchanged. The inspected TVM SHA3, address derivation, storage-key, and code-hash paths use
Hash.sha3with byte-array input and full-length output. All 1,488 base/head digest records matched Bouncy Castle 1.84.No consensus-result change or coordinated fork activation is expected for these paths.
Compatibility impact
Digest,DigestEngine, or concrete Keccak classes now require handling or declaringDigestException.MessageDigestcallers already have this requirement.ByteBufferbytes now contribute to the hash. Short output throwsDigestExceptionwithout resetting state or modifying the output buffer; obtain the full digest and truncate explicitly if needed. Invalid output ranges raiseIllegalArgumentException.Related: #6996.