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

Sha3 refactor unsafe #338

Merged
merged 3 commits into from Dec 15, 2021
Merged

Conversation

aewag
Copy link
Contributor

@aewag aewag commented Dec 15, 2021

I was looking into the sha3 implementation and would like to propose this PR to avoid the use of unsafe.

While the implementation differentiated between little & big endian, I ended for the little endian path with more or less similar code than it was existing in the big endian path already. Thus, I assume that there is no differentiation needed. Unfortunately, I could only test it on my machine, which is LE.

Performance-wise it does not really change anything, see below first without unsafe as it is now and second as it was before:

850bcc2 sha3: Add forbid(unsafe_code) crate attribute

test sha3_224_10    ... bench:          38 ns/iter (+/- 3) = 263 MB/s
test sha3_224_100   ... bench:         370 ns/iter (+/- 19) = 270 MB/s
test sha3_224_1000  ... bench:       3,706 ns/iter (+/- 582) = 269 MB/s
test sha3_224_10000 ... bench:      36,332 ns/iter (+/- 2,908) = 275 MB/s
test sha3_256_10    ... bench:          39 ns/iter (+/- 3) = 256 MB/s
test sha3_256_1000  ... bench:       3,894 ns/iter (+/- 125) = 256 MB/s
test sha3_256_10000 ... bench:      38,367 ns/iter (+/- 4,029) = 260 MB/s
test sha3_265_100   ... bench:         398 ns/iter (+/- 76) = 251 MB/s
test sha3_384_10    ... bench:          52 ns/iter (+/- 31) = 192 MB/s
test sha3_384_100   ... bench:         515 ns/iter (+/- 77) = 194 MB/s
test sha3_384_1000  ... bench:       5,151 ns/iter (+/- 1,037) = 194 MB/s
test sha3_384_10000 ... bench:      50,180 ns/iter (+/- 2,378) = 199 MB/s
test sha3_512_10    ... bench:          74 ns/iter (+/- 4) = 135 MB/s
test sha3_512_100   ... bench:         730 ns/iter (+/- 69) = 136 MB/s
test sha3_512_1000  ... bench:       7,212 ns/iter (+/- 269) = 138 MB/s
test sha3_512_10000 ... bench:      73,020 ns/iter (+/- 8,302) = 136 MB/s
test shake128_10    ... bench:          33 ns/iter (+/- 2) = 303 MB/s
test shake128_100   ... bench:         319 ns/iter (+/- 10) = 313 MB/s
test shake128_1000  ... bench:       3,234 ns/iter (+/- 187) = 309 MB/s
test shake128_10000 ... bench:      32,770 ns/iter (+/- 4,745) = 305 MB/s
test shake256_10    ... bench:          42 ns/iter (+/- 8) = 238 MB/s
test shake256_100   ... bench:         452 ns/iter (+/- 280) = 221 MB/s
test shake256_1000  ... bench:       4,175 ns/iter (+/- 2,144) = 239 MB/s
test shake256_10000 ... bench:      38,911 ns/iter (+/- 4,559) = 256 MB/s

894f62f build(deps): bump digest from 0.10.0 to 0.10.1 (#337

test sha3_224_10    ... bench:          37 ns/iter (+/- 1) = 270 MB/s       
test sha3_224_100   ... bench:         370 ns/iter (+/- 16) = 270 MB/s
test sha3_224_1000  ... bench:       3,776 ns/iter (+/- 702) = 264 MB/s
test sha3_224_10000 ... bench:      36,316 ns/iter (+/- 2,393) = 275 MB/s     
test sha3_256_10    ... bench:          40 ns/iter (+/- 1) = 250 MB/s
test sha3_256_1000  ... bench:       3,933 ns/iter (+/- 274) = 254 MB/s
test sha3_256_10000 ... bench:      38,828 ns/iter (+/- 4,400) = 257 MB/s
test sha3_265_100   ... bench:         391 ns/iter (+/- 14) = 255 MB/s
test sha3_384_10    ... bench:          52 ns/iter (+/- 2) = 192 MB/s
test sha3_384_100   ... bench:         510 ns/iter (+/- 17) = 196 MB/s                                   
test sha3_384_1000  ... bench:       5,088 ns/iter (+/- 271) = 196 MB/s
test sha3_384_10000 ... bench:      50,486 ns/iter (+/- 3,347) = 198 MB/s                                
test sha3_512_10    ... bench:          73 ns/iter (+/- 1) = 136 MB/s
test sha3_512_100   ... bench:         735 ns/iter (+/- 92) = 136 MB/s
test sha3_512_1000  ... bench:       7,297 ns/iter (+/- 261) = 137 MB/s
test sha3_512_10000 ... bench:      72,539 ns/iter (+/- 2,847) = 137 MB/s
test shake128_10    ... bench:          32 ns/iter (+/- 1) = 312 MB/s
test shake128_100   ... bench:         317 ns/iter (+/- 12) = 315 MB/s
test shake128_1000  ... bench:       3,189 ns/iter (+/- 316) = 313 MB/s                                  
test shake128_10000 ... bench:      31,223 ns/iter (+/- 1,740) = 320 MB/s
test shake256_10    ... bench:          40 ns/iter (+/- 1) = 250 MB/s                                    
test shake256_100   ... bench:         390 ns/iter (+/- 10) = 256 MB/s      
test shake256_1000  ... bench:       3,864 ns/iter (+/- 240) = 258 MB/s    
test shake256_10000 ... bench:      38,826 ns/iter (+/- 2,433) = 257 MB/s

@newpavlov
Copy link
Member

Thank you!

@newpavlov newpavlov merged commit cbd6849 into RustCrypto:master Dec 15, 2021
@tarcieri
Copy link
Member

Unfortunately, I could only test it on my machine, which is LE.

Perhaps we should have cross-based tests for big endian architectures like we have for sha2

@newpavlov
Copy link
Member

I think the author meant performance difference. This PR has removed different endianness-dependent code paths, so such tests would be less important.

@aewag aewag deleted the sha3-refactor-unsafe branch December 16, 2021 08:28
@aewag
Copy link
Contributor Author

aewag commented Dec 16, 2021

I was concerned wrt the functionality. As the code in the LE path matched the BE path I was quite confident to not break anything.

Looking into the sha2 github actions I stumbled across the cross test. I just ran the tests for mips-unknown-linux-gnu and it succeeded:

$ cross test --no-default-features --target mips-unknown-linux-gnu
    Finished test [unoptimized + debuginfo] target(s) in 0.02s
     Running unittests (/target/mips-unknown-linux-gnu/debug/deps/sha3-b931d17dc83aced4)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/mod.rs (/target/mips-unknown-linux-gnu/debug/deps/mod-901f5c5c71251583)

running 11 tests
test keccak_224 ... ok
test keccak_256 ... ok
test keccak_256_full ... ok
test keccak_384 ... ok
test keccak_512 ... ok
test sha3_224 ... ok
test sha3_256 ... ok
test sha3_384 ... ok
test sha3_512 ... ok
test shake128 ... ok
test shake256 ... ok

test result: ok. 11 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 23.87s

Nevertheless, having cross-based tests for all crates would be nice to catch any errors in advance. If wanted, I could create a PR to add them.

@tarcieri
Copy link
Member

I'd be in favor of them, especially as sha3 is one of our most widely used crates

@aewag aewag mentioned this pull request Dec 16, 2021
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.

None yet

3 participants