perf: cache packed proof nonces for cheaper header/block hash#3899
Conversation
block.hash() / header.hash() re-bitpacked Proof nonces on every call. Keep the on-wire packed form when deserializing a Proof so subsequent Hash serialization skips pack_bits. Locally built proofs pack on demand; clear_packed_cache after mutating edge_bits/nonces (stratum share path). Closes mimblewimble#3372
wiesche89
left a comment
wiesche89
left a comment
There was a problem hiding this comment.
This addresses the main idea of #3372, but not all of its proposed safeguards yet. The cache is populated on read and reused for hashing, but it is also used for full serialization, and the public fields can still be changed without invalidating it. I think those two points should be resolved in this PR before closing the issue. Some before/after numbers would also help confirm the expected benefit.
| /// **Important:** if you mutate `edge_bits` or `nonces` after construction, | ||
| /// call [`Proof::clear_packed_cache`] so a stale cache cannot poison hashes. | ||
| #[serde(skip)] | ||
| packed_nonces: Option<Vec<u8>>, |
There was a problem hiding this comment.
The cache depends on edge_bits and nonces, but both are still publicly mutable. A missed clear_packed_cache() makes verification and hashing use different proof data. Could we make mutation invalidate the cache automatically in this PR?
| } | ||
| writer.write_fixed_bytes(&self.pack_nonces()) | ||
| // Prefer the deserialized packed form (hash-hot path); otherwise pack on demand. | ||
| if let Some(ref packed) = self.packed_nonces { |
There was a problem hiding this comment.
Could we use the cache only for hash serialization and always repack full wire/disk writes from the fields? That matches #3372 and prevents stale cached bytes from being emitted.
| let hash_before = proof.hash(); | ||
| let packed = proof.compute_packed_nonces(); | ||
|
|
||
| // Simulate a deserialized proof: same nonces, cache filled. |
There was a problem hiding this comment.
Could this use a real serialize/deserialize round trip instead of setting the private cache directly? That would cover the actual read => cache => hash/write path.
|
|
||
| /// Pack the nonces of the proof to their exact bit size as described above. | ||
| /// Uses the deserialized cache when available. | ||
| pub fn pack_nonces(&self) -> Vec<u8> { |
There was a problem hiding this comment.
Could you add before/after numbers for repeated header hashing? The cache keeps and clones an extra allocation per proof, so it would be good to confirm the net win.
2 participants
Summary
block.hash()/header.hash()ultimately hash the PoWProofby bit-packing all nonces on every call. That packing is relatively expensive and runs very often during sync and validation.This caches the packed form when a
Proofis deserialized (the on-wire bytes we already read), so later hash/write uses that buffer instead of re-runningpack_bits.Closes #3372
Approach
packed_nonces: Option<Vec<u8>>onProof(ignored byPartialEq/ serde)Readable::readstores the packed bytes alongside extracted noncesWriteable::writeprefers the cache; otherwise packs on demand (mining / constructed proofs)Proof::clear_packed_cache()for safe mutation ofedge_bits/noncesLocally constructed proofs (mining templates, tests, genesis) keep
Noneand behave as before.Test plan
cargo test -p grin_core --lib pow::typescargo test -p grin_core --lib genesiscargo check -p grin_servers