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

[crypto+move] added support for SHA2-512, SHA3-512 and RIPEMD-160 in Move #4181

Merged
merged 2 commits into from
Nov 1, 2022

Conversation

alinush
Copy link
Contributor

@alinush alinush commented Sep 14, 2022

Description

Added support for SHA2-512, SHA3-512 and RIPEMD-160, to support folks write bridge contracts to other platforms, as per conversations with @JoshLind.

This PR should be good to go:

  • it is feature-gated
  • it has (reasonable) gas costs for the new hash functions, marked with optional
  • it bumps up the gas feature version

Test Plan

This PR merely exports existing hash function crates, which are either already used throughout aptos-core/ or are believed to be trustworthy implementations. Therefore, we only made sure the native implementation give the expected hash values for the empty string "" and for b"testing".


This change is Reviewable

@alinush alinush added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Sep 14, 2022
@github-actions
Copy link
Contributor

Forge is running suite land_blocking on 3b016bcbd163f3fd4b26069fe2b36e4176df0bb1

✅ Forge suite land_blocking success on 3b016bcbd163f3fd4b26069fe2b36e4176df0bb1

performance benchmark with full nodes : 8020 TPS, 4950 ms latency, 7500 ms p99 latency,no expired txns
Test Ok

@github-actions
Copy link
Contributor

Forge is running suite compat on testnet ==> 3b016bcbd163f3fd4b26069fe2b36e4176df0bb1

✅ Forge suite compat success on testnet ==> 3b016bcbd163f3fd4b26069fe2b36e4176df0bb1

Compatibility test results for testnet ==> 3b016bcbd163f3fd4b26069fe2b36e4176df0bb1 (PR)
1. Check liveness of validators at old version: testnet
compatibility::simple-validator-upgrade::liveness-check : 8564 TPS, 4319 ms latency, 6400 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 3b016bcbd163f3fd4b26069fe2b36e4176df0bb1
compatibility::simple-validator-upgrade::single-validator-upgrade : 5783 TPS, 6307 ms latency, 8000 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 3b016bcbd163f3fd4b26069fe2b36e4176df0bb1
compatibility::simple-validator-upgrade::half-validator-upgrade : 5742 TPS, 6435 ms latency, 7800 ms p99 latency,no expired txns
4. upgrading second batch to new version: 3b016bcbd163f3fd4b26069fe2b36e4176df0bb1
compatibility::simple-validator-upgrade::rest-validator-upgrade : 7799 TPS, 4815 ms latency, 7900 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet ==> 3b016bcbd163f3fd4b26069fe2b36e4176df0bb1 passed
Test Ok

Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

pretty trivial PR.

two questions...
what happens if we use this software with the current network? would it load fine without the gas schedule available?

could you validate that you can connect to testnet as a fullnode and submit a transaction?

looks good otherwise, will approve with those validations and a little more time.

aptos-move/aptos-gas/src/aptos_framework.rs Outdated Show resolved Hide resolved
aptos-move/framework/src/natives/mod.rs Show resolved Hide resolved
@alinush
Copy link
Contributor Author

alinush commented Oct 3, 2022

The main blocker for this PR, besides @davidiw's blessing, is a generic way of estimating gas costs for any native function. Will work with @vgao1996 on that and then we can push this through.

But yes, other than that, it's a trivial PR.

@vgao1996
Copy link
Contributor

vgao1996 commented Oct 3, 2022

@alinush yeah as we discussed offline, I'm currently busy with some urgent hotfixes... Let's try not to land this before launch. We'll get back to it later

aptos-move/framework/aptos-stdlib/sources/hash.move Outdated Show resolved Hide resolved
aptos-move/framework/src/natives/mod.rs Show resolved Hide resolved
@alinush
Copy link
Contributor Author

alinush commented Oct 28, 2022

@davidiw, this PR is ready to be approved now:

  • it is feature-gated
  • it has the (reasonable) gas costs, marked with optional
  • it bumps up the gas version

...whenever you are ready!

@alinush alinush requested a review from davidiw October 28, 2022 21:05
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on fa58ea74244849a620d12153f618b9b98f8d6bdb

performance benchmark with full nodes : 6905 TPS, 5748 ms latency, 8100 ms p99 latency,no expired txns
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite compat success on 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> fa58ea74244849a620d12153f618b9b98f8d6bdb

Compatibility test results for 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> fa58ea74244849a620d12153f618b9b98f8d6bdb (PR)
1. Check liveness of validators at old version: 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7163 TPS, 5413 ms latency, 7200 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: fa58ea74244849a620d12153f618b9b98f8d6bdb
compatibility::simple-validator-upgrade::single-validator-upgrade : 4327 TPS, 9502 ms latency, 13100 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: fa58ea74244849a620d12153f618b9b98f8d6bdb
compatibility::simple-validator-upgrade::half-validator-upgrade : 4710 TPS, 8698 ms latency, 12100 ms p99 latency,no expired txns
4. upgrading second batch to new version: fa58ea74244849a620d12153f618b9b98f8d6bdb
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6541 TPS, 5946 ms latency, 10800 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for 2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> fa58ea74244849a620d12153f618b9b98f8d6bdb passed
Test Ok

@davidiw davidiw dismissed their stale review October 28, 2022 23:33

good for me!

@alinush alinush merged commit ba0d5dc into main Nov 1, 2022
@alinush alinush deleted the alin/move-hashing branch November 1, 2022 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants