Add AppContext switch to allow hash seed to be deterministic#79
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes the hash seed mutable to enable zkVM compatibility by allowing external control over the hash seed used in GetHashCode(). The change removes the automatic random initialization and replaces it with a mutable static field that can be set via a new public API.
- Replaces readonly random hash seed initialization with a mutable static field
- Adds a public
SetHashSeedmethod to allow external configuration - Updates hash code computation to use the mutable seed
src/Nethermind.Int256/UInt256.cs
Outdated
| return true; | ||
| } | ||
|
|
||
| public static void SetHashSeed(uint seed) => _hashSeed = seed; |
There was a problem hiding this comment.
Changing the hash seed after UInt256 instances have been added to hash-based collections (Dictionary, HashSet, etc.) will break those collections, as the hash codes will change. The API design should either document this critical constraint clearly, or consider making the seed settable only once (e.g., check if it's already been set and throw an exception). Additionally, there's no validation of the seed parameter, and allowing a seed value of 0 may reduce hash distribution quality compared to the previous random initialization.
There was a problem hiding this comment.
@LukaszRozmej I thought about this. Better to make init-once.
benaadams
left a comment
benaadams
left a comment
There was a problem hiding this comment.
Since UInt256 are essentially user supplied keys to internal dictionaries mean a bad actor could offline generate a hash flood DoS attack once which would knock every node offline
A random seed means they can't generate it offline (but need to do it against a specific node) and if they find a DoS collision it will only effect that node and not any others
I'm not sure what you're getting at. |
benaadams
changed the title
benaadams
left a comment
benaadams
left a comment
There was a problem hiding this comment.
Wait shouldn't this be inverted?
src/Nethermind.Int256/UInt256.cs
Outdated
| private static readonly uint s_instanceRandom = (uint)System.Security.Cryptography.RandomNumberGenerator.GetInt32(int.MinValue, int.MaxValue); | ||
| // Constant by default for zkVM-compatibility. | ||
| private static readonly uint _hashSeed = | ||
| AppContext.TryGetSwitch("Nethermind.Int256.UseRandomSeed", out var useRandomSeed) && useRandomSeed |
There was a problem hiding this comment.
Shouldn't this be inverted so is random by default and can be switched off; rather than off by default?
src/Nethermind.Int256/UInt256.cs
Outdated
| private static readonly uint s_instanceRandom = (uint)System.Security.Cryptography.RandomNumberGenerator.GetInt32(int.MinValue, int.MaxValue); | ||
| // Constant by default for zkVM-compatibility. | ||
| private static readonly uint _hashSeed = | ||
| AppContext.TryGetSwitch("Nethermind.Int256.UseRandomSeed", out var useRandomSeed) && useRandomSeed |
There was a problem hiding this comment.
You probably want it behind a static property with FeatureSwitchDefinitionAttribute on it so it can be resolved at compile time: https://medium.com/@malarsharmila/feature-switches-in-net-9-73b1cb96c13f
|
Unfortunately, AppContext switch doesn't work with our bflat approach, hitting the devirtualization issue. This said, it won't be possible to keep things intact for regular .NET and have a switch for the zkVM. Thus, the approach is the opposite -- having the default for zkVM and a switch to set for regulat .NET runtime. This will hopefully change if we solve the devirtualization issue. |
LukaszRozmej
left a comment
LukaszRozmej
left a comment
There was a problem hiding this comment.
Does it work in our zkVM compiler?
Sure it does. Otherwise this PR wouldn't make sense. |
3 participants
Added AppContext switch to control hash seed randomization for zkVM-compatibility.