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

[move] fix MultiEd25519 PK validation small bug #5822

Merged
merged 1 commit into from
Dec 19, 2022

Conversation

alinush
Copy link
Contributor

@alinush alinush commented Dec 8, 2022

Description

Our MultiEd25519 Move module allowed for some invalid MultiEd25519 PKs to be deserialized as ValidatedPublicKey structs. Such incorrectly-deserialized structs would've been caught later on during signature verification. Nonetheless, we will fix this bug to guarantee correct semantics of MultiEd25519 validated PKs.

Details

Our native implementation of the public_key_validate_internal Move function did not check that a MultiEd25519 PK had >= 1 sub-PKs.

Furthermore, this implementation did not minimize the gas costs charged to the user, when aborting early.

Fix

This PR introduces a new public_key_validate_v2_internal API that addresses these two issues. As a result, it deprecates two public functions that used the old version:

  • Deprecates new_validated_public_key_from_bytes for new_validated_public_key_from_bytes_v2
  • Deprecates public_key_validate for public_key_validate_v2

Our public_key_validate_v2_internal API uses a new MultiEd25519PublicKey::validate_bytes_and_count_checks in the aptos_crypto crate to check the well-formedness of a public key.

Test Plan

  • Added a test case with a MultiEd25519 PK with 1 sub-PKs but no byte encoding the threshold
  • Added a test case with a MultiEd25519 PK with 0 sub-PKs
  • Added a test case for a 1-out-of-1 MultiEd25519 PK where the sub-PK is of small order
  • Tested validate_bytes_and_count_checks against MultiEd25519PublicKey::try_from

@alinush alinush requested a review from zjma December 8, 2022 04:28
@alinush alinush force-pushed the alin/multied25519-pk-validation-v2-fixes branch from a91f460 to c65dcd7 Compare December 8, 2022 21:06
@alinush alinush force-pushed the alin/multied25519-pk-validation-v2-fixes branch from c65dcd7 to e956b9b Compare December 19, 2022 15:44
@alinush alinush enabled auto-merge (squash) December 19, 2022 16:28
@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 e956b9b83c4433a3b4a21ff3a0449d448ee85e13

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

@github-actions
Copy link
Contributor

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> e956b9b83c4433a3b4a21ff3a0449d448ee85e13

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> e956b9b83c4433a3b4a21ff3a0449d448ee85e13 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7471 TPS, 5190 ms latency, 7200 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: e956b9b83c4433a3b4a21ff3a0449d448ee85e13
compatibility::simple-validator-upgrade::single-validator-upgrade : 4659 TPS, 8818 ms latency, 11600 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: e956b9b83c4433a3b4a21ff3a0449d448ee85e13
compatibility::simple-validator-upgrade::half-validator-upgrade : 4552 TPS, 9063 ms latency, 12500 ms p99 latency,no expired txns
4. upgrading second batch to new version: e956b9b83c4433a3b4a21ff3a0449d448ee85e13
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6395 TPS, 6182 ms latency, 11100 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> e956b9b83c4433a3b4a21ff3a0449d448ee85e13 passed
Test Ok

@alinush alinush merged commit 29f352f into main Dec 19, 2022
@alinush alinush deleted the alin/multied25519-pk-validation-v2-fixes branch December 19, 2022 17:16
@Markuze Markuze mentioned this pull request Dec 26, 2022
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.

3 participants