Skip to content
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

faster hash for Address #688

Merged
merged 1 commit into from
Jun 5, 2024
Merged

faster hash for Address #688

merged 1 commit into from
Jun 5, 2024

Conversation

arnetheduck
Copy link
Member

@arnetheduck arnetheduck commented Jun 4, 2024


# Addresses are more or less random so we should not need a fancy mixing
# function
copyMem(addr a0, unsafeAddr a[0], sizeof(a0))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will be more efficient if EthAddress also is required to be aligned, which nim-eth does not require and array[20, byte] might not automatically trigger since its base type has sizeof() == 1. But that'd have to be changed in another repo, and it's cleaner to keep the changes separate. Both can help, but this works and is useful on its own.

@jangko
Copy link
Contributor

jangko commented Jun 5, 2024

nimbus-eth1 also using a lot of Hash256 as lookup key, maybe add one hash function for Hash256 too?

@arnetheduck
Copy link
Member Author

nimbus-eth1 also using a lot of Hash256 as lookup key, maybe add one hash function for Hash256 too?

One problem here would be that we use the same alias of MDigest in eth2 as well:
https://github.com/status-im/nimbus-eth2/blob/d2a07514541ffe6ee02a2ec7272ce7a315131e04/beacon_chain/spec/digest.nim#L134 - there, we've already fixed this silly problem - with an even more simple implementation.

One thing I wasn't sure of was whether to use all bytes of EthAddress - on the one hand, it should be random, on the other, I'm not sure it has uniform distribution - for hash on the other hand, it already has uniform distribution so any bytes will do.

cc @cheatfate wdyt of adding the hash from beacon chain to nimcrypto?

@cheatfate
Copy link
Contributor

cheatfate commented Jun 5, 2024

Such trick should not be used for SHA256 because you significantly lowering its chance for uniform distribution while taking only first 8 bytes. Such trick could work with 3rd generation of digests like KECCAK/BLAKE2 (this algorithms has such function), but not with second generation like SHA256.

256 bits of cryptographic hash will have uniform distribution but 64 bits of 256 bits will not have such property.

@arnetheduck
Copy link
Member Author

For the purpose of Hash, does that matter though? Ie I'm guessing you're referring to the lack of proof that it is sufficiently random in a security context, but for the purpose of selecting a bucket in Table, it seems to me like the bar is much lower?

That said, I don't think it's the end of the world if we use this xor variant on MDigest - it'll have to read a bit more memory but that's likely not going to change anything significant since it's likely going to be sitting in the same cache line.

The question is more whether we can add it to nimcrypto?

@arnetheduck
Copy link
Member Author

arnetheduck commented Jun 5, 2024

cheatfate/nimcrypto#79 adds some infrastructure so we can unroll an xor implementation for MDigest that does all bits - then we would remove the one in nimbus-eth2 as well

@arnetheduck arnetheduck merged commit 68bd675 into master Jun 5, 2024
12 checks passed
@arnetheduck arnetheduck deleted the fix-address-hash branch June 5, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants