`Murmur3` should not be cryptographic hash algorithm #3668

nan01ab · 2025-01-12T16:21:30Z

Description

Murmur3 is not a secure hash algorithm, so it should not implement System.Security.Cryptography.HashAlgorithm, as this can lead to misunderstanding and misuse.

Murmur3 probably shouldn't be under the namespace Neo.Cryptography , but this PR doesn't change that.

Type of change

Optimization (the change is only an optimization)
Style (the change is only a code style for better maintenance or standard purpose)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

shargon

What's the problem?

cschuchardt88 · 2025-01-13T12:13:03Z

The class has nothing to do with security.

Hash functions are fundamental to modern cryptography. These functions map binary strings of an arbitrary length to small binary strings of a fixed length, known as hash values. A cryptographic hash function has the property that it is computationally infeasible to find two distinct inputs that hash to the same value. Hash functions are commonly used with digital signatures and for data integrity.

The hash is used as a unique value of fixed size representing a large amount of data. Hashes of two sets of data should match if the corresponding data also matches. Small changes to the data result in large unpredictable changes in the hash.

Dictionary:
cryp·tog·ra·phy
/kripˈtäɡrəfē/

the art of writing or solving codes.

nan01ab · 2025-01-13T13:56:42Z

The class has nothing to do with security.

Hash functions are fundamental to modern cryptography. These functions map binary strings of an arbitrary length to small binary strings of a fixed length, known as hash values. A cryptographic hash function has the property that it is computationally infeasible to find two distinct inputs that hash to the same value. Hash functions are commonly used with digital signatures and for data integrity.

The hash is used as a unique value of fixed size representing a large amount of data. Hashes of two sets of data should match if the corresponding data also matches. Small changes to the data result in large unpredictable changes in the hash.

Dictionary: cryp·tog·ra·phy /kripˈtäɡrəfē/

the art of writing or solving codes.

Great explanation.

But:

Hash functions are fundamental to modern cryptography.

These hash functions refer to the secure hash functions in cryptography(SHA -> Secure Hash Algorithm).
Such as SHA2, SHA3, RIPMD-160.

However, murmur32(as well as murmur128) is a NonCryptographicHashAlgorithm(https://learn.microsoft.com/en-us/dotnet/api/system.io.hashing.noncryptographichashalgorithm?view=net-9.0-pp, https://www.nuget.org/packages/System.IO.Hashing/ ).

It is wrong to implement murmur32 as System.Security.Cryptography.HashAlgorithm.

nan01ab · 2025-01-13T13:58:07Z

What's the problem?

Explained above.

nan01ab · 2025-01-13T16:01:28Z

src/Neo/Cryptography/Murmur32.cs

+        /// Append data to murmur computation
+        /// </summary>
+        /// <param name="source">Source</param>
+        public void Append(ReadOnlySpan<byte> source)


There is a problem with the implementation of murmur3 in neo.
So append is not supported yet.

Then we can remove also Initialize

Then we can remove also Initialize

Yes. Initialize can be removed.
I keep Initialize because ComputeHash can be called multiple times for a Murmur3 instance.

src/Neo/Cryptography/Murmur32.cs

Co-authored-by: Shargon <[email protected]>

cschuchardt88 · 2025-01-15T00:41:51Z

src/Neo/Cryptography/Murmur32.cs

    /// </summary>
-    public sealed class Murmur32 : System.Security.Cryptography.HashAlgorithm
+    public sealed class Murmur32


Use NonCryptographicHashAlgorithm as inherit class

More updates are needed, because Append not supported.

What do you mean? You can still use append.

What do you mean? You can still use append.

The current murmur32 impl in neo doesn’t support multiple Append operations

So you can't use it in NonCryptographicHashAlgorithm?

If the current logic does not allow this interface, feel free to create a new PR for this purpose

Well then I don't find this as a fix. Using these interfaces makes the hash a lot faster to process data. I want to see a benchmark of this new implementation.

The logic is the same, should be faster but never slower, why do you think it will be slower?

The logic is the same, should be faster but never slower, why do you think it will be slower?

Because of the class interfaces use an engine for fast hashing for huge amounts of data up to 2GB.

Well then I don't find this as a fix. Using these interfaces makes the hash a lot faster to process data. I want to see a benchmark of this new implementation.

No significant performance difference.

src/Neo/SmartContract/Native/CryptoLib.cs

* master: (43 commits) Fix `GetAndChange` warnings (neo-project#3702) `Murmur3` should not be cryptographic hash algorithm (neo-project#3668) Test: add tests for native contract id (neo-project#3697) Update nugets (neo-project#3692) [Core P2P] fix the bug (neo-project#3695) Add hardfork HF_Echidna (neo-project#3454) Fix: add lock for RocksDbStore.Snapshot to keep same behavior as MemoryStore and LevelDbStore (neo-project#3689) Nullable rocks db (neo-project#3686) Nullable leveldb (neo-project#3685) Enforcement Compiler Warnings (neo-project#3687) [`Update`] Dotnet & Compiler Version (neo-project#3684) [`Add`]: LevelDB Benchmarks (neo-project#3667) [`Fix`]: Behavior when `keyPrefix` is null in different `IStore.Seek` impls. (neo-project#3682) Improve calculatenetworkfee (neo-project#3674) more 2025 (neo-project#3678) Nullable in Storage classes (neo-project#3670) readonly (neo-project#3676) [Fix] Set max entries for `VerifyProof` in `statePlugin` (neo-project#3675) Neo.json.benchmarks (neo-project#3673) Happy new year 2025 (neo-project#3677) ... # Conflicts: # src/Neo/Neo.csproj # src/Neo/ProtocolSettings.cs # src/Neo/SmartContract/ApplicationEngine.cs # src/Neo/SmartContract/Native/NeoToken.cs # src/Neo/SmartContract/Native/RoleManagement.cs # tests/Neo.UnitTests/SmartContract/Native/UT_NativeContract.cs

optimize: Murmur3 should be no cryptographic hash algorithm

e88a1ea

nan01ab changed the title ~~Murmur3 should not no cryptographic hash algorithm~~ Murmur3 should not be cryptographic hash algorithm Jan 12, 2025

shargon reviewed Jan 13, 2025

View reviewed changes

shargon added 3 commits January 13, 2025 15:31

Avoid double initialize when only call once ComputeHash

4868dc8

Rename

c2fc2e5

Allow to append

a8855d2

shargon previously approved these changes Jan 13, 2025

View reviewed changes

nan01ab commented Jan 13, 2025

View reviewed changes

src/Neo/Cryptography/Murmur32.cs Outdated Show resolved Hide resolved

Jim8y previously approved these changes Jan 14, 2025

View reviewed changes

Merge branch 'master' into feat.murmur3-non-cryptographic-hash

9c5e956

Jim8y added Waiting for Review Ready to Merge labels Jan 14, 2025

Update src/Neo/Cryptography/Murmur32.cs

dfbfb95

nan01ab dismissed stale reviews from shargon and Jim8y via dfbfb95 January 14, 2025 05:51

Jim8y previously approved these changes Jan 14, 2025

View reviewed changes

shargon previously approved these changes Jan 14, 2025

View reviewed changes

src/Neo/Cryptography/Murmur32.cs Outdated Show resolved Hide resolved

Update src/Neo/Cryptography/Murmur32.cs

4894dfc

Co-authored-by: Shargon <[email protected]>

nan01ab dismissed stale reviews from shargon and Jim8y via 4894dfc January 14, 2025 07:42

shargon approved these changes Jan 14, 2025

View reviewed changes

cschuchardt88 requested changes Jan 15, 2025

View reviewed changes

shargon added 3 commits January 15, 2025 23:43

Merge branch 'master' into feat.murmur3-non-cryptographic-hash

24991ed

Merge branch 'master' into feat.murmur3-non-cryptographic-hash

8acc8f5

Merge branch 'master' into feat.murmur3-non-cryptographic-hash

90dee11

AnnaShaleva reviewed Jan 20, 2025

View reviewed changes

src/Neo/SmartContract/Native/CryptoLib.cs Show resolved Hide resolved

shargon added 2 commits January 20, 2025 11:13

Merge branch 'master' into feat.murmur3-non-cryptographic-hash

25df0aa

Merge branch 'master' into feat.murmur3-non-cryptographic-hash

8273b9a

cschuchardt88 approved these changes Jan 21, 2025

View reviewed changes

Merge branch 'master' into feat.murmur3-non-cryptographic-hash

6536483

cschuchardt88 removed the Waiting for Review label Jan 23, 2025

cschuchardt88 and others added 2 commits January 23, 2025 05:03

Merge branch 'master' into feat.murmur3-non-cryptographic-hash

ee87f06

Merge branch 'master' into feat.murmur3-non-cryptographic-hash

d933edd

shargon requested a review from Jim8y January 26, 2025 10:14

Jim8y approved these changes Jan 27, 2025

View reviewed changes

shargon merged commit 806091d into neo-project:master Jan 27, 2025
6 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Murmur3` should not be cryptographic hash algorithm #3668

`Murmur3` should not be cryptographic hash algorithm #3668

nan01ab commented Jan 12, 2025 •

edited

Loading

shargon left a comment

cschuchardt88 commented Jan 13, 2025

nan01ab commented Jan 13, 2025 •

edited

Loading

nan01ab commented Jan 13, 2025

nan01ab Jan 13, 2025 •

edited

Loading

shargon Jan 13, 2025

nan01ab Jan 14, 2025 •

edited

Loading

cschuchardt88 Jan 15, 2025

nan01ab Jan 15, 2025 •

edited

Loading

cschuchardt88 Jan 15, 2025

nan01ab Jan 15, 2025

cschuchardt88 Jan 15, 2025

shargon Jan 17, 2025 •

edited

Loading

cschuchardt88 Jan 17, 2025 •

edited

Loading

shargon Jan 17, 2025

cschuchardt88 Jan 17, 2025 •

edited

Loading

nan01ab Jan 20, 2025

Murmur3 should not be cryptographic hash algorithm #3668

Murmur3 should not be cryptographic hash algorithm #3668

Conversation

nan01ab commented Jan 12, 2025 • edited Loading

Description

Type of change

Checklist:

shargon left a comment

Choose a reason for hiding this comment

cschuchardt88 commented Jan 13, 2025

nan01ab commented Jan 13, 2025 • edited Loading

nan01ab commented Jan 13, 2025

nan01ab Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

shargon Jan 13, 2025

Choose a reason for hiding this comment

nan01ab Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

cschuchardt88 Jan 15, 2025

Choose a reason for hiding this comment

nan01ab Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

cschuchardt88 Jan 15, 2025

Choose a reason for hiding this comment

nan01ab Jan 15, 2025

Choose a reason for hiding this comment

cschuchardt88 Jan 15, 2025

Choose a reason for hiding this comment

shargon Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

cschuchardt88 Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

shargon Jan 17, 2025

Choose a reason for hiding this comment

cschuchardt88 Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

nan01ab Jan 20, 2025

Choose a reason for hiding this comment

`Murmur3` should not be cryptographic hash algorithm #3668

`Murmur3` should not be cryptographic hash algorithm #3668

nan01ab commented Jan 12, 2025 •

edited

Loading

nan01ab commented Jan 13, 2025 •

edited

Loading

nan01ab Jan 13, 2025 •

edited

Loading

nan01ab Jan 14, 2025 •

edited

Loading

nan01ab Jan 15, 2025 •

edited

Loading

shargon Jan 17, 2025 •

edited

Loading

cschuchardt88 Jan 17, 2025 •

edited

Loading

cschuchardt88 Jan 17, 2025 •

edited

Loading