-
Notifications
You must be signed in to change notification settings - Fork 723
perf(zkevm): install the hash seed from the payload root #13166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
3850a52
55c4d86
5b50358
f60b6cd
2e008de
b2538e4
8626125
d7af2d5
1a08b73
041076b
1a072ec
65e79e1
22f4b61
b56b5bb
664d9b8
f8604c6
24d03d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,9 +2,13 @@ | |
| // SPDX-License-Identifier: LGPL-3.0-only | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Runtime.CompilerServices; | ||
| using System.Runtime.InteropServices; | ||
| using Nethermind.Consensus.Stateless; | ||
| using Nethermind.Core; | ||
| using Nethermind.Core.Crypto; | ||
| using Nethermind.Core.Extensions; | ||
| using Nethermind.Db; | ||
| using Nethermind.Trie; | ||
| using NUnit.Framework; | ||
|
|
@@ -98,19 +102,16 @@ public void Keeps_the_seeded_empty_root_whatever_is_written_to_it([Values] bool | |
| } | ||
|
|
||
| [Test] | ||
| public void Separates_keys_that_share_their_leading_bytes() | ||
| public void Separates_keys_that_share_a_hash_code() | ||
| { | ||
| // The hash code is the keccak's leading four bytes, so equality is what has to tell these apart. | ||
| byte[] first = new byte[32]; | ||
| byte[] second = new byte[32]; | ||
| second[31] = 1; | ||
| (ValueHash256 first, ValueHash256 second) = FindHashCodeCollision(); | ||
|
|
||
| HashKeyedNodeStorage storage = Storage(); | ||
| storage.Set(null, TreePath.Empty, new ValueHash256(first), [0x01]); | ||
| storage.Set(null, TreePath.Empty, new ValueHash256(second), [0x02]); | ||
| storage.Set(null, TreePath.Empty, first, [0x01]); | ||
| storage.Set(null, TreePath.Empty, second, [0x02]); | ||
|
|
||
| Assert.That(storage.Get(null, TreePath.Empty, new ValueHash256(first)), Is.EqualTo(new byte[] { 0x01 })); | ||
| Assert.That(storage.Get(null, TreePath.Empty, new ValueHash256(second)), Is.EqualTo(new byte[] { 0x02 })); | ||
| Assert.That(storage.Get(null, TreePath.Empty, first), Is.EqualTo(new byte[] { 0x01 })); | ||
| Assert.That(storage.Get(null, TreePath.Empty, second), Is.EqualTo(new byte[] { 0x02 })); | ||
| } | ||
|
|
||
| [Test] | ||
|
|
@@ -165,4 +166,37 @@ private static void Write(HashKeyedNodeStorage storage, bool throughBatch, in Va | |
| storage.Set(null, TreePath.Empty, hash, data); | ||
| } | ||
| } | ||
|
|
||
| /// <summary>Finds two distinct keys the store buckets together, so equality is what tells them apart.</summary> | ||
| /// <remarks> | ||
| /// Searched rather than hard-coded: the hash code is seeded, so no fixed pair collides across runs. | ||
| /// A 32-bit birthday collision is overwhelmingly likely well inside <c>Attempts</c>. | ||
| /// </remarks> | ||
| private static (ValueHash256, ValueHash256) FindHashCodeCollision() | ||
| { | ||
| const int Attempts = 1 << 19; | ||
| Dictionary<int, ValueHash256> seen = new(Attempts); | ||
|
|
||
| for (int i = 0; i < Attempts; i++) | ||
| { | ||
| ValueHash256 candidate = ValueKeccak.Compute(MemoryMarshal.AsBytes(new ReadOnlySpan<int>(in i))); | ||
| int hashCode = NodeKeyHashCode(in candidate); | ||
|
|
||
| if (seen.TryGetValue(hashCode, out ValueHash256 previous)) | ||
| { | ||
| if (previous != candidate) return (previous, candidate); | ||
| } | ||
| else | ||
| { | ||
| seen[hashCode] = candidate; | ||
| } | ||
| } | ||
|
|
||
| Assert.Fail($"No hash-code collision within {Attempts} keys."); | ||
| return default; | ||
| } | ||
|
Comment on lines
+175
to
+197
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Medium — the rewrite drops the coverage the old test existed for, and the fix is a two-line change to the search. The old pair was The new pair is two unrelated keccaks that happen to share a 32-bit hash code. They differ in word 0 with probability ~1, so an Both properties are recoverable at the same cost, because [Test]
public void Separates_keys_that_share_a_hash_code([Range(0, 3)] int word)
{
(ValueHash256 first, ValueHash256 second) = FindHashCodeCollision(word);
…
}
private static (ValueHash256, ValueHash256) FindHashCodeCollision(int word)
{
…
ValueHash256 candidate = default;
BinaryPrimitives.WriteInt64LittleEndian(candidate.BytesAsSpan[(word * 8)..], i);
…
}That pins all four comparisons instead of none, and drops the keccak per candidate. Two smaller notes on the helper while it is being touched:
|
||
|
|
||
| /// <summary>Mirrors the store's private key hash so the search targets the same buckets.</summary> | ||
| private static int NodeKeyHashCode(in ValueHash256 hash) => | ||
| (int)SpanExtensions.FastHash64For32Bytes(ref Unsafe.As<ValueHash256, byte>(ref Unsafe.AsRef(in hash))); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,9 +111,11 @@ public void Compact() { } | |
| /// Equality is spelled out word-wise rather than deferred to | ||
| /// <see cref="ValueHash256.Equals(ValueHash256)"/>, which compares | ||
| /// <see cref="System.Runtime.Intrinsics.Vector256{T}"/>s and so expands to a byte-at-a-time loop on | ||
| /// the guest's target. The hash code is the keccak's own leading bytes: they are already uniformly | ||
| /// distributed, so <see cref="ValueHash256.GetHashCode"/>'s re-mix of all 32 buys nothing. Reading | ||
| /// the leading word and truncating it instead measured 0.15% worse. | ||
| /// the guest's target. The hash code goes through the run-seeded mixer rather than the keccak's own | ||
| /// leading bytes: those are uniformly distributed, which answers accidental collisions but not a | ||
| /// chosen witness. Unseeded, one offline grind yields node hashes sharing a bucket for every block | ||
| /// and payload, and nothing here rejects unreachable witness nodes. See | ||
| /// <see cref="SpanExtensions.SeedHashes"/>. | ||
|
Comment on lines
+114
to
+118
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Medium — this is the one container where the payload-root seed is not a fixed point, so the protection is weaker than the remark implies. The change is right and strictly better than the leading-bytes hash. But the threat model recorded here reads like the one that covers the storage-slot maps, and for this store it does not hold, because the seed's preimage excludes the witness: // InputDecoder.cs:47-51
NewPayloadRequest<TExecutionPayload>.Merkleize(input.NewPayloadRequest, out UInt256 root);
SpanExtensions.SeedHashes(in root);
Cost of that grind, with So the honest statement is that seeding converts a one-time universal grind into a per-payload |
||
| /// </remarks> | ||
| private readonly struct NodeKey(in ValueHash256 hash) : IEquatable<NodeKey> | ||
| { | ||
|
|
@@ -132,6 +134,7 @@ public bool Equals(NodeKey other) | |
|
|
||
| public override bool Equals(object? obj) => obj is NodeKey other && Equals(other); | ||
|
|
||
| public override int GetHashCode() => Unsafe.As<ValueHash256, int>(ref Unsafe.AsRef(in _hash)); | ||
| public override int GetHashCode() => | ||
| (int)SpanExtensions.FastHash64For32Bytes(ref Unsafe.As<ValueHash256, byte>(ref Unsafe.AsRef(in _hash))); | ||
|
Comment on lines
+137
to
+138
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Medium — the cheapest hash in the guest's largest dictionary becomes the most expensive one, and the line this replaced carried a measurement. Before:
What makes this worth a number rather than a shrug is the sentence the diff deletes: "Reading the leading word and truncating it instead measured 0.15% worse." The team measured a variation far smaller than this one. The PR body says native RISC-V execution was not rerun, and this commit post-dates the body's description entirely. A guest step count for |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Low — the attribute is right; the reason given is the one that does not apply to this build.
Ethereum.Blockchain.Pyspec.Testbuilds withoutEnableZkEvm, soSpanExtensions.SeedHasheshere is thestdpartial with an empty body (SpanExtensions.std.cs:28).InputDecoder.Decodeinstalls nothing in this assembly, and the described reseed-mid-hash race cannot occur.What is live is the other process-wide static this method touches:
Two
StatelessExecutorOutputMatchesFixturecases running concurrently under the inheritedParallelScope.Allwere racing on it. Not asserted today, so nothing was failing — but it is a genuine shared-mutable static in a parallel fixture, and[NonParallelizable]fixes it.Everything else in the commit message checks out:
parallel: falsereally isParallelExecutionOverride(PyspecTestFixture.cs:23), the[Parallelizable(ParallelScope.All)]really is onPyspecBlockchainFixtureBase(line 20) and inherited, and NUnit's shift dispatcher does drain the parallel shift before running a non-parallel work item. Just worth pointing the comment atFailureOutput, or saying the seed part is forward-looking for a guest-configuration build.