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

remove rust-cypto dependency, fix issuse 2404 3254 #3609

Merged
merged 5 commits into from
Aug 4, 2022

Conversation

nkysg
Copy link
Collaborator

@nkysg nkysg commented Aug 3, 2022

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

fix issue
#2404
#3254

Issue Number: N/A

What is the new behavior?

Other information

@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Merging #3609 (23b1aa1) into master (8aa6bf2) will decrease coverage by 0.29%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3609      +/-   ##
==========================================
- Coverage   29.29%   29.00%   -0.28%     
==========================================
  Files         589      589              
  Lines       49864    49864              
  Branches    23450    23450              
==========================================
- Hits        14601    14458     -143     
+ Misses      21264    21257       -7     
- Partials    13999    14149     +150     
Flag Coverage Δ
unittests 29.00% <ø> (-0.28%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/merkle-generator/src/lib.rs 25.00% <ø> (ø)
commons/accumulator/src/tree.rs 53.54% <0.00%> (-13.27%) ⬇️
chain/open-block/src/lib.rs 31.79% <0.00%> (-11.92%) ⬇️
vm/natives/src/lib.rs 80.00% <0.00%> (-10.00%) ⬇️
executor/src/block_executor.rs 25.59% <0.00%> (-9.30%) ⬇️
vm/types/src/on_chain_config/mod.rs 41.10% <0.00%> (-8.21%) ⬇️
vm/natives/src/token.rs 22.23% <0.00%> (-6.66%) ⬇️
chain/src/chain.rs 33.34% <0.00%> (-5.79%) ⬇️
commons/forkable-jellyfish-merkle/src/lib.rs 37.94% <0.00%> (-4.98%) ⬇️
executor/src/executor.rs 45.84% <0.00%> (-4.16%) ⬇️
... and 78 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8aa6bf2...23b1aa1. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Aug 4, 2022

Benchmark for 99d2ef1

Click to view benchmark
Test Base PR %
accumulator_append 614.0±30.89µs 610.5±31.63µs -0.57%
block_apply/block_apply_10 634.0±4.46ms 633.5±5.98ms -0.08%
block_apply/block_apply_1000 64.5±0.04s 64.4±0.06s -0.16%
get_with_proof/db_store 37.0±0.27µs 36.7±0.47µs -0.81%
get_with_proof/mem_store 31.6±0.26µs 31.4±0.67µs -0.63%
put_and_commit/db_store/1 98.3±5.56µs 96.0±5.21µs -2.34%
put_and_commit/db_store/10 868.7±38.78µs 864.4±41.80µs -0.49%
put_and_commit/db_store/100 7.4±0.32ms 7.5±0.43ms +1.35%
put_and_commit/db_store/5 448.8±23.33µs 445.5±25.10µs -0.74%
put_and_commit/db_store/50 3.9±0.17ms 3.8±0.17ms -2.56%
put_and_commit/mem_store/1 62.4±5.98µs 61.6±6.06µs -1.28%
put_and_commit/mem_store/10 585.4±48.95µs 574.7±49.87µs -1.83%
put_and_commit/mem_store/100 5.7±0.86ms 5.7±0.87ms 0.00%
put_and_commit/mem_store/5 292.6±26.10µs 290.1±26.49µs -0.85%
put_and_commit/mem_store/50 2.9±0.20ms 2.8±0.19ms -3.45%
query_block/query_block_in(10)_times(100) 5.5±0.19ms 5.4±0.22ms -1.82%
query_block/query_block_in(10)_times(1000) 54.5±2.65ms 53.7±1.78ms -1.47%
query_block/query_block_in(10)_times(10000) 536.6±12.20ms 532.0±15.84ms -0.86%
query_block/query_block_in(1000)_times(100) 956.7±10.15µs 989.4±23.15µs +3.42%
query_block/query_block_in(1000)_times(1000) 9.6±0.05ms 10.0±0.10ms +4.17%
query_block/query_block_in(1000)_times(10000) 95.5±0.85ms 97.4±1.07ms +1.99%
storage_transaction 1094.3±315.93µs 1093.9±294.85µs -0.04%
vm/transaction_execution/1 711.0±2.25ms 707.5±2.65ms -0.49%
vm/transaction_execution/10 177.8±1.30ms 176.6±1.94ms -0.67%
vm/transaction_execution/20 152.8±0.51ms 152.2±0.88ms -0.39%
vm/transaction_execution/5 234.8±2.70ms 233.8±2.05ms -0.43%
vm/transaction_execution/50 168.6±1.60ms 166.5±0.78ms -1.25%

Copy link
Contributor

@JerryKwan JerryKwan left a comment

Choose a reason for hiding this comment

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

@nkysg
Seems like you are using tiny-keccak to replace rust-crypto, but the latest commit to tini-keccak is 2 years ago. Not sure if it is maintained.
When reading the codes in Move language, we noticed that they are using crate sha2 to do some hash algorithms.
image

https://docs.rs/sha2/latest/sha2/
https://github.com/RustCrypto/hashes

@nkysg
Copy link
Collaborator Author

nkysg commented Aug 4, 2022

@nkysg Seems like you are using tiny-keccak to replace rust-crypto, but the latest commit to tini-keccak is 2 years ago. Not sure if it is maintained. When reading the codes in Move language, we noticed that they are using crate sha2 to do some hash algorithms. image

https://docs.rs/sha2/latest/sha2/ https://github.com/RustCrypto/hashes

@JerryKwan we should use lib sha3 or tiny-keccak, our HashValue compute hash implement sha3 algo。
https://github.com/starcoinorg/starcoin-crypto/blob/2c95387a3e5ebfbff544e0044f039ce071125dc5/crates/diem-crypto/src/hash.rs#L199-L203

    #[cfg(not(feature = "avx512f"))]
    pub fn sha3_256_of(buffer: &[u8]) -> Self {
        let mut sha3 = Sha3::v256();
        sha3.update(buffer);
        HashValue::from_keccak(sha3)
    }

HashValue code is port from diem-crypto

@github-actions
Copy link

github-actions bot commented Aug 4, 2022

Benchmark for 49c0531

Click to view benchmark
Test Base PR %
accumulator_append 608.9±28.74µs 606.3±29.83µs -0.43%
block_apply/block_apply_10 635.5±2.89ms 637.5±1.33ms +0.31%
block_apply/block_apply_1000 64.5±0.14s 65.2±0.07s +1.09%
get_with_proof/db_store 36.5±0.26µs 37.8±0.31µs +3.56%
get_with_proof/mem_store 31.6±0.20µs 32.5±0.43µs +2.85%
put_and_commit/db_store/1 95.5±4.83µs 95.6±5.03µs +0.10%
put_and_commit/db_store/10 866.4±40.74µs 853.9±39.12µs -1.44%
put_and_commit/db_store/100 7.3±0.32ms 7.4±0.32ms +1.37%
put_and_commit/db_store/5 437.8±22.05µs 440.6±23.25µs +0.64%
put_and_commit/db_store/50 3.8±0.17ms 3.8±0.16ms 0.00%
put_and_commit/mem_store/1 61.7±5.73µs 61.8±5.68µs +0.16%
put_and_commit/mem_store/10 579.6±47.58µs 582.2±48.80µs +0.45%
put_and_commit/mem_store/100 5.7±0.36ms 5.7±0.84ms 0.00%
put_and_commit/mem_store/5 291.0±24.43µs 293.0±24.99µs +0.69%
put_and_commit/mem_store/50 2.8±0.18ms 2.8±0.19ms 0.00%
query_block/query_block_in(10)_times(100) 5.4±0.22ms 5.5±0.10ms +1.85%
query_block/query_block_in(10)_times(1000) 54.3±2.24ms 54.8±2.66ms +0.92%
query_block/query_block_in(10)_times(10000) 548.3±14.38ms 554.7±15.75ms +1.17%
query_block/query_block_in(1000)_times(100) 1009.5±8.94µs 1001.7±11.66µs -0.77%
query_block/query_block_in(1000)_times(1000) 10.1±0.12ms 10.0±0.13ms -0.99%
query_block/query_block_in(1000)_times(10000) 100.8±0.69ms 99.4±0.90ms -1.39%
storage_transaction 1070.5±275.14µs 1100.0±343.11µs +2.76%
vm/transaction_execution/1 708.4±1.14ms 712.5±3.97ms +0.58%
vm/transaction_execution/10 177.8±1.50ms 176.9±0.73ms -0.51%
vm/transaction_execution/20 152.7±0.49ms 152.8±0.95ms +0.07%
vm/transaction_execution/5 234.2±2.26ms 233.6±0.49ms -0.26%
vm/transaction_execution/50 167.2±0.61ms 167.2±0.45ms 0.00%

@github-actions
Copy link

github-actions bot commented Aug 4, 2022

Benchmark for 9712801

Click to view benchmark
Test Base PR %
accumulator_append 606.2±29.32µs 602.4±28.53µs -0.63%
block_apply/block_apply_10 633.9±0.63ms 631.7±2.73ms -0.35%
block_apply/block_apply_1000 65.3±0.12s 64.4±0.10s -1.38%
get_with_proof/db_store 36.3±0.20µs 36.8±0.23µs +1.38%
get_with_proof/mem_store 31.5±0.29µs 31.5±0.30µs 0.00%
put_and_commit/db_store/1 96.2±4.77µs 95.8±4.81µs -0.42%
put_and_commit/db_store/10 859.6±40.22µs 863.9±39.09µs +0.50%
put_and_commit/db_store/100 7.4±0.30ms 7.3±0.28ms -1.35%
put_and_commit/db_store/5 437.2±21.83µs 442.8±22.09µs +1.28%
put_and_commit/db_store/50 3.8±0.17ms 3.8±0.17ms 0.00%
put_and_commit/mem_store/1 61.6±5.70µs 61.8±5.81µs +0.32%
put_and_commit/mem_store/10 580.4±48.95µs 577.1±47.11µs -0.57%
put_and_commit/mem_store/100 5.7±0.83ms 5.6±0.34ms -1.75%
put_and_commit/mem_store/5 291.3±24.79µs 291.5±24.58µs +0.07%
put_and_commit/mem_store/50 2.9±0.19ms 2.8±0.18ms -3.45%
query_block/query_block_in(10)_times(100) 5.5±0.07ms 5.3±0.22ms -3.64%
query_block/query_block_in(10)_times(1000) 55.0±2.34ms 55.4±2.21ms +0.73%
query_block/query_block_in(10)_times(10000) 547.1±15.61ms 550.1±14.18ms +0.55%
query_block/query_block_in(1000)_times(100) 1012.5±11.67µs 1015.7±6.69µs +0.32%
query_block/query_block_in(1000)_times(1000) 10.0±0.12ms 10.1±0.05ms +1.00%
query_block/query_block_in(1000)_times(10000) 99.6±0.99ms 101.7±1.34ms +2.11%
storage_transaction 1095.4±322.57µs 1113.1±351.24µs +1.62%
vm/transaction_execution/1 709.7±1.20ms 706.2±1.18ms -0.49%
vm/transaction_execution/10 177.1±0.89ms 176.2±0.98ms -0.51%
vm/transaction_execution/20 152.3±0.42ms 151.8±0.38ms -0.33%
vm/transaction_execution/5 234.3±1.89ms 233.7±3.81ms -0.26%
vm/transaction_execution/50 167.0±0.43ms 166.8±0.54ms -0.12%

@jolestar jolestar merged commit d216459 into master Aug 4, 2022
@jolestar jolestar deleted the remove_rust_crypto branch August 4, 2022 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants