Ed25519 PK validation in Move should return `None` instead of aborting on wrong length #7043

mstraka100 · 2023-03-10T01:40:19Z

This PR updates the native_public_key_validate native Move function to return false when a key of the wrong length is passed in, rather than aborting.

This change in behavior is feature-gated via a new Ed25519PubkeyValidateReturnFalseWrongLength feature flag which is checked inside the Rust native implementation itself.

Testing

Ran replay tests here to see if they trigger any problems in past TXNs. All passed.

alinush · 2023-03-10T16:54:56Z

@mstraka100, I updated your PR's description to be a little more precise.

Also, can you change the base branch of this PR to be https://github.com/aptos-labs/aptos-core/tree/alin/native-feature-flag-context?

This way, only your commits & changes will show up in "Files Changed."

aptos-move/aptos-release-builder/src/components/feature_flags.rs

aptos-move/aptos-release-builder/data/release.yaml

alinush · 2023-03-11T00:52:45Z

Hey Michael, as you can see once my stuff merged, this PR got rebased to main. You will somehow need to make sure only your changes show up in "Files changed"... not sure how... 🤔

aptos-move/vm-genesis/src/lib.rs

movekevin

Approval pending removing the unused getter functions in features.move

aptos-move/aptos-release-builder/src/components/feature_flags.rs

alinush

LGTM, but fix the errors, since it seems you might be missing a clause in a match.

github-actions · 2023-03-20T18:43:08Z

✅ Forge suite `land_blocking` success on `d48c4a53ab7c3705b5c685bfd73b940b2eb01692`

performance benchmark with full nodes : 5893 TPS, 6746 ms latency, 10200 ms p99 latency,no expired txns
Test Ok

github-actions · 2023-03-20T18:45:29Z

✅ Forge suite `framework_upgrade` success on `cb4ba0a57c998c60cbab65af31a64875d2588ca5` ==> `d48c4a53ab7c3705b5c685bfd73b940b2eb01692`

Compatibility test results for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> d48c4a53ab7c3705b5c685bfd73b940b2eb01692 (PR)
Upgrade the nodes to version: d48c4a53ab7c3705b5c685bfd73b940b2eb01692
framework_upgrade::framework-upgrade::full-framework-upgrade : 6514 TPS, 5962 ms latency, 9900 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> d48c4a53ab7c3705b5c685bfd73b940b2eb01692 passed
Test Ok

github-actions · 2023-03-20T18:45:32Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `d48c4a53ab7c3705b5c685bfd73b940b2eb01692`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> d48c4a53ab7c3705b5c685bfd73b940b2eb01692 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7930 TPS, 4829 ms latency, 6800 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: d48c4a53ab7c3705b5c685bfd73b940b2eb01692
compatibility::simple-validator-upgrade::single-validator-upgrade : 4901 TPS, 8217 ms latency, 10100 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: d48c4a53ab7c3705b5c685bfd73b940b2eb01692
compatibility::simple-validator-upgrade::half-validator-upgrade : 5067 TPS, 7960 ms latency, 11600 ms p99 latency,no expired txns
4. upgrading second batch to new version: d48c4a53ab7c3705b5c685bfd73b940b2eb01692
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6839 TPS, 5642 ms latency, 10100 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> d48c4a53ab7c3705b5c685bfd73b940b2eb01692 passed
Test Ok

mstraka100 and others added 6 commits March 9, 2023 14:27

Timed feature flag for ed25519 abort

e1934c0

[natives] expose all feature flags inside NativeContext as an extension

272bce7

started normal feature ed25519 abort

95bde37

merge with alin/native-feature-flag-context

dd00ce9

ed25519 abort as normal feature flag start

824795d

more code for adding ed25519 abort feature flag

1e58a1f

mstraka100 requested review from davidiw, movekevin, alinush, wrwg and zekun000 as code owners March 10, 2023 01:40

[natives] expose all feature flags inside NativeContext as an extension

fef208a

alinush marked this pull request as draft March 10, 2023 16:57

alinush added 3 commits March 10, 2023 09:07

actually, undo the NativeContext extension approach

34e47bc

pass in an Arc<Features> to each SafeNativeContext

215663d

expose method to access feature flags in SafeNativeContext

030f587

mstraka100 changed the base branch from main to alin/native-feature-flag-context March 10, 2023 18:23

alinush and others added 6 commits March 10, 2023 10:44

oops, remove last .clone() calls

3b17b86

merge with alin/native-feature-flag-context

d8b033d

use new api from alin/native-feature-flag-context

1a0984f

add move function to access ed25519 abort feature flag

a46de7b

merge with alin/native-feature-flag-context

87dbb68

cargo +nightly fmt

e8982ec

alinush reviewed Mar 10, 2023

View reviewed changes

aptos-move/aptos-release-builder/src/components/feature_flags.rs Outdated Show resolved Hide resolved

movekevin reviewed Mar 10, 2023

View reviewed changes

aptos-move/aptos-release-builder/data/release.yaml Outdated Show resolved Hide resolved

alinush requested a review from zjma March 10, 2023 23:29

Base automatically changed from alin/native-feature-flag-context to main March 11, 2023 00:05

merge with main

9684d26

alinush changed the title ~~Add feature flag for ed25519 abort on wrong length~~ ed25519 PK validation in Move should return none instead of aborting on wrong length Mar 16, 2023

alinush changed the title ~~ed25519 PK validation in Move should return none instead of aborting on wrong length~~ Ed25519 PK validation in Move should return None instead of aborting on wrong length Mar 16, 2023

alinush reviewed Mar 16, 2023

View reviewed changes

aptos-move/vm-genesis/src/lib.rs Outdated Show resolved Hide resolved

movekevin approved these changes Mar 16, 2023

View reviewed changes

mstraka100 added 2 commits March 16, 2023 14:57

remove getter functions for flag

7e228c8

add ED25519 prefix to flag

6ad7817

alinush reviewed Mar 16, 2023

View reviewed changes

aptos-move/aptos-release-builder/src/components/feature_flags.rs Outdated Show resolved Hide resolved

mstraka100 added 2 commits March 16, 2023 16:20

fix names

03dfecd

Merge branch 'main' into michael/native_public_key_validate

380e3da

alinush marked this pull request as ready for review March 16, 2023 22:39

alinush requested review from clay-aptos and saharct as code owners March 16, 2023 22:39

alinush approved these changes Mar 16, 2023

View reviewed changes

mstraka100 added 7 commits March 17, 2023 09:42

added missing match clause

e01811d

merge with main

936d627

fix merge conflict

b38630c

merge conflict

5884c4e

Merge branch 'main' into michael/native_public_key_validate

7527146

Merge branch 'main' into michael/native_public_key_validate

fb37768

Merge branch 'main' into michael/native_public_key_validate

d48c4a5

mstraka100 enabled auto-merge (squash) March 20, 2023 17:58

This comment has been minimized.

Sign in to view

mstraka100 merged commit 99b44af into main Mar 20, 2023

mstraka100 deleted the michael/native_public_key_validate branch March 20, 2023 18:45

thepomeranian mentioned this pull request Apr 18, 2023

[AIP-23][Discussion] Make Ed25519 Public Key Validation Return False if Key Is the Wrong Length aptos-foundation/AIPs#102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ed25519 PK validation in Move should return `None` instead of aborting on wrong length #7043

Ed25519 PK validation in Move should return `None` instead of aborting on wrong length #7043

mstraka100 commented Mar 10, 2023 •

edited by alinush

Loading

alinush commented Mar 10, 2023

alinush commented Mar 11, 2023

movekevin left a comment

alinush left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Mar 20, 2023

github-actions bot commented Mar 20, 2023

github-actions bot commented Mar 20, 2023

Ed25519 PK validation in Move should return None instead of aborting on wrong length #7043

Ed25519 PK validation in Move should return None instead of aborting on wrong length #7043

Conversation

mstraka100 commented Mar 10, 2023 • edited by alinush Loading

Testing

alinush commented Mar 10, 2023

alinush commented Mar 11, 2023

movekevin left a comment

Choose a reason for hiding this comment

alinush left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Mar 20, 2023

✅ Forge suite land_blocking success on d48c4a53ab7c3705b5c685bfd73b940b2eb01692

github-actions bot commented Mar 20, 2023

✅ Forge suite framework_upgrade success on cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> d48c4a53ab7c3705b5c685bfd73b940b2eb01692

github-actions bot commented Mar 20, 2023

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> d48c4a53ab7c3705b5c685bfd73b940b2eb01692

Ed25519 PK validation in Move should return `None` instead of aborting on wrong length #7043

Ed25519 PK validation in Move should return `None` instead of aborting on wrong length #7043

mstraka100 commented Mar 10, 2023 •

edited by alinush

Loading

✅ Forge suite `land_blocking` success on `d48c4a53ab7c3705b5c685bfd73b940b2eb01692`

✅ Forge suite `framework_upgrade` success on `cb4ba0a57c998c60cbab65af31a64875d2588ca5` ==> `d48c4a53ab7c3705b5c685bfd73b940b2eb01692`

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `d48c4a53ab7c3705b5c685bfd73b940b2eb01692`