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

add permissioned signer move code #15735

Merged
merged 2 commits into from
Jan 15, 2025
Merged

Conversation

runtian-zhou
Copy link
Contributor

@runtian-zhou runtian-zhou commented Jan 14, 2025

Description

Clone of #14469 due to rebase error

Implemented aptos framework code for the permissioned signer.

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Other (specify)

How Has This Been Tested?

Implemented tests to cover the entire lifecycle of a permission handle.

Key Areas to Review

Whether the api is safe to use.

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

Copy link

trunk-io bot commented Jan 14, 2025

⏱️ 2h 25m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 45m 🟩🟩
check-dynamic-deps 30m 🟩🟩🟩🟩🟩 (+7 more)
rust-cargo-deny 21m 🟩🟩🟩🟩🟩 (+7 more)
test-target-determinator 9m 🟩🟩
execution-performance / test-target-determinator 9m 🟩🟩
rust-doc-tests 6m 🟩
rust-doc-tests 6m 🟩
general-lints 6m 🟩🟩🟩🟩🟩 (+7 more)
semgrep/ci 4m 🟩🟩🟩🟩🟩 (+7 more)
fetch-last-released-docker-image-tag 3m 🟩🟩
file_change_determinator 2m 🟩🟩🟩🟩🟩 (+7 more)
permission-check 34s 🟩🟩🟩🟩🟩 (+7 more)
permission-check 32s 🟩🟩🟩🟩🟩 (+7 more)
file_change_determinator 21s 🟩🟩
permission-check 7s 🟩🟩

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link
Contributor Author

runtian-zhou commented Jan 14, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@runtian-zhou runtian-zhou marked this pull request as ready for review January 14, 2025 23:47
@runtian-zhou runtian-zhou requested review from igor-aptos and removed request for davidiw, lightmark, msmouse, grao1991, junkil-park and movekevin January 14, 2025 23:47
@runtian-zhou runtian-zhou force-pushed the runtianz/permissioned_signer_move branch from 39bee7e to b2eadad Compare January 14, 2025 23:58
@runtian-zhou runtian-zhou force-pushed the 09-04-implement_rust_logics_for_permissioned_signer branch from fd6ef05 to 6c56a97 Compare January 15, 2025 00:02
@runtian-zhou runtian-zhou force-pushed the runtianz/permissioned_signer_move branch from b2eadad to 4aff032 Compare January 15, 2025 00:02
@runtian-zhou runtian-zhou force-pushed the 09-04-implement_rust_logics_for_permissioned_signer branch from 6c56a97 to 95d2640 Compare January 15, 2025 00:34
@runtian-zhou runtian-zhou force-pushed the runtianz/permissioned_signer_move branch from 4aff032 to dbb1cda Compare January 15, 2025 00:35
Base automatically changed from 09-04-implement_rust_logics_for_permissioned_signer to main January 15, 2025 01:07
@runtian-zhou runtian-zhou force-pushed the runtianz/permissioned_signer_move branch 2 times, most recently from ce32584 to bb58864 Compare January 15, 2025 01:25
// Native Functions
///
/// Check whether this is a permissioned signer.
public native fun is_permissioned_signer(s: &signer): bool;
Copy link
Contributor

Choose a reason for hiding this comment

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

this cannot be public in a first release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this needs to be public so that 3rd party can at least abort on permissioned signer if needed

Copy link
Contributor

Choose a reason for hiding this comment

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

@runtian-zhou is that needed for first release?

this is not backward compatible - you cannot add new public native function in one release.

if you think this is critical , you need to make this not be native, and have is_permissioned_signer_impl be private and native (and you can register both in rust code, so that you can later remove is_permissioned_signer_impl and change is_permissioned_signer to native

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the natives.

@rahxephon89 rahxephon89 self-requested a review January 15, 2025 01:35
@runtian-zhou runtian-zhou force-pushed the runtianz/permissioned_signer_move branch from bb58864 to 7279808 Compare January 15, 2025 02:23
Comment on lines +98 to +99
"is_permissioned_signer_impl",
native_is_permissioned_signer_impl as RawSafeNative,
Copy link
Contributor

Choose a reason for hiding this comment

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

you should put both in the list here (pointing to the same native_is_permissioned_signer_impl, so you can deprecate is_permissioned_signer_impl later

}

/// initialize permission storage by putting an empty storage under the address.
inline fun initialize_permission_address(permissions_storage_addr: address) {
Copy link
Contributor

Choose a reason for hiding this comment

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

move this function below, in the private section.

@runtian-zhou runtian-zhou force-pushed the runtianz/permissioned_signer_move branch from 7279808 to da38949 Compare January 15, 2025 03:03
@runtian-zhou runtian-zhou force-pushed the runtianz/permissioned_signer_move branch from 55cbb90 to f928323 Compare January 15, 2025 05:01
@runtian-zhou runtian-zhou enabled auto-merge (squash) January 15, 2025 06:13
@runtian-zhou runtian-zhou force-pushed the runtianz/permissioned_signer_move branch from f928323 to 05937d0 Compare January 15, 2025 06:20

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 05937d07905442bab72dc27f73425a56d2f5431e

two traffics test: inner traffic : committed: 14985.18 txn/s, latency: 2651.50 ms, (p50: 2700 ms, p70: 2700, p90: 2700 ms, p99: 3000 ms), latency samples: 5697720
two traffics test : committed: 100.00 txn/s, latency: 1347.21 ms, (p50: 1300 ms, p70: 1400, p90: 1500 ms, p99: 1800 ms), latency samples: 1820
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.534, avg: 1.515", "ConsensusProposalToOrdered: max: 0.289, avg: 0.286", "ConsensusOrderedToCommit: max: 0.297, avg: 0.290", "ConsensusProposalToCommit: max: 0.583, avg: 0.576"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.99s no progress at version 36504 (avg 0.19s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.52s no progress at version 2975131 (avg 0.52s) [limit 16].
Test Ok

Copy link
Contributor

✅ Forge suite compat success on 6593fb81261f25490ffddc2252a861c994234c2a ==> 05937d07905442bab72dc27f73425a56d2f5431e

Compatibility test results for 6593fb81261f25490ffddc2252a861c994234c2a ==> 05937d07905442bab72dc27f73425a56d2f5431e (PR)
1. Check liveness of validators at old version: 6593fb81261f25490ffddc2252a861c994234c2a
compatibility::simple-validator-upgrade::liveness-check : committed: 16252.61 txn/s, latency: 2000.60 ms, (p50: 1600 ms, p70: 1700, p90: 1900 ms, p99: 20800 ms), latency samples: 570440
2. Upgrading first Validator to new version: 05937d07905442bab72dc27f73425a56d2f5431e
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 5020.72 txn/s, latency: 6129.29 ms, (p50: 7000 ms, p70: 7400, p90: 7700 ms, p99: 7800 ms), latency samples: 99620
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 4910.18 txn/s, latency: 7029.95 ms, (p50: 7700 ms, p70: 7900, p90: 8100 ms, p99: 8200 ms), latency samples: 174760
3. Upgrading rest of first batch to new version: 05937d07905442bab72dc27f73425a56d2f5431e
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 5128.59 txn/s, latency: 6001.01 ms, (p50: 6800 ms, p70: 7300, p90: 7400 ms, p99: 7500 ms), latency samples: 101000
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 5053.13 txn/s, latency: 6819.04 ms, (p50: 7500 ms, p70: 7600, p90: 7900 ms, p99: 7900 ms), latency samples: 177100
4. upgrading second batch to new version: 05937d07905442bab72dc27f73425a56d2f5431e
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 8339.51 txn/s, latency: 3582.65 ms, (p50: 3800 ms, p70: 4100, p90: 5200 ms, p99: 5400 ms), latency samples: 153940
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 8289.58 txn/s, latency: 4087.06 ms, (p50: 3900 ms, p70: 5000, p90: 5200 ms, p99: 5300 ms), latency samples: 277320
5. check swarm health
Compatibility test for 6593fb81261f25490ffddc2252a861c994234c2a ==> 05937d07905442bab72dc27f73425a56d2f5431e passed
Test Ok

@runtian-zhou runtian-zhou merged commit 817f444 into main Jan 15, 2025
46 checks passed
@runtian-zhou runtian-zhou deleted the runtianz/permissioned_signer_move branch January 15, 2025 06:53
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.

5 participants