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] more useful abort codes for secp256k1::ecdsa_recover #15989

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

alinush
Copy link
Contributor

@alinush alinush commented Feb 20, 2025

Description

Developers often get confused about how to use ECDSA(-secp256k1)'s pubkey recovery algorithm in Move via secp256k1::ecdsa_recover because they are typically verifying Ethereum $(r,s,v)$ signatures and using the $v$ value as the recovery ID.

But ECDSA recovery IDs and Ethereum recovery IDs are different.

As a result, ecdsa_recover which expects a 2-bit recovery ID, aborts with a deserialization error when it sees an Ethereum recovery ID.

This PR returns a more useful abort code to developers when it encounters a bad recovery ID.

It also adds some comments to ecdsa_recovery to help people better use it.

(Tagging @zjma and @rex1fernando for visibility and stamps.)

How Has This Been Tested?

  • Unit tests that verify hardcoded signature still pass.
  • Added unit test to ensure bad recovery IDs lead to an abort with the right code

Key Areas to Review

  • Backwards compatibility
  • Re-check that the Rust secp256k1 library does indeed use only 0, 1, 2 or 3 for recovery IDs (see here also)

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
  • Move Compiler
  • Other (specify)

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

@alinush alinush requested a review from rex1fernando February 20, 2025 23:41
@alinush alinush requested a review from zjma as a code owner February 20, 2025 23:41
Copy link

trunk-io bot commented Feb 20, 2025

⏱️ 1h 49m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 48m 🟥🟩
check-dynamic-deps 14m 🟩🟩🟩🟩🟩 (+1 more)
execution-performance / test-target-determinator 10m 🟩🟩
test-target-determinator 9m 🟩
rust-cargo-deny 7m 🟩🟩🟩🟩
rust-doc-tests 7m 🟩
rust-doc-tests 4m
fetch-last-released-docker-image-tag 3m 🟩🟩
semgrep/ci 2m 🟩🟩🟩🟩🟩 (+1 more)
general-lints 2m 🟩🟩🟩🟩
file_change_determinator 57s 🟩🟩🟩🟩 (+1 more)
file_change_determinator 26s 🟩🟩
permission-check 19s 🟩🟩🟩🟩🟩 (+1 more)
permission-check 14s 🟩🟩🟩🟩🟩 (+1 more)
permission-check 5s 🟩🟩

settingsfeedbackdocs ⋅ learn more about trunk.io

@alinush alinush force-pushed the alin/secp256k1-more-helpful-comments branch from d4f174e to 7ed316f Compare February 20, 2025 23:41
@alinush alinush force-pushed the alin/secp256k1-more-helpful-comments branch from ce65718 to 59aeba7 Compare February 21, 2025 16:32
@alinush alinush enabled auto-merge (squash) February 21, 2025 16:33

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite framework_upgrade success on 51e3afbaf4645c7a8dd03b94e47555c0dbed0366 ==> 45ac29f4d1ffa6d84176ba8bf48dfefeb0066280

Compatibility test results for 51e3afbaf4645c7a8dd03b94e47555c0dbed0366 ==> 45ac29f4d1ffa6d84176ba8bf48dfefeb0066280 (PR)
Upgrade the nodes to version: 45ac29f4d1ffa6d84176ba8bf48dfefeb0066280
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1674.97 txn/s, submitted: 1679.37 txn/s, failed submission: 4.40 txn/s, expired: 4.40 txn/s, latency: 1717.41 ms, (p50: 1500 ms, p70: 1800, p90: 2500 ms, p99: 4200 ms), latency samples: 152423
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1862.99 txn/s, submitted: 1868.76 txn/s, failed submission: 5.78 txn/s, expired: 5.78 txn/s, latency: 1572.87 ms, (p50: 1500 ms, p70: 1800, p90: 2200 ms, p99: 3000 ms), latency samples: 167682
5. check swarm health
Compatibility test for 51e3afbaf4645c7a8dd03b94e47555c0dbed0366 ==> 45ac29f4d1ffa6d84176ba8bf48dfefeb0066280 passed
Upgrade the remaining nodes to version: 45ac29f4d1ffa6d84176ba8bf48dfefeb0066280
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1821.98 txn/s, submitted: 1827.90 txn/s, failed submission: 5.92 txn/s, expired: 5.92 txn/s, latency: 1633.69 ms, (p50: 1500 ms, p70: 1800, p90: 2200 ms, p99: 3400 ms), latency samples: 166100
Test Ok

Copy link
Contributor

✅ Forge suite compat success on 51e3afbaf4645c7a8dd03b94e47555c0dbed0366 ==> 45ac29f4d1ffa6d84176ba8bf48dfefeb0066280

Compatibility test results for 51e3afbaf4645c7a8dd03b94e47555c0dbed0366 ==> 45ac29f4d1ffa6d84176ba8bf48dfefeb0066280 (PR)
1. Check liveness of validators at old version: 51e3afbaf4645c7a8dd03b94e47555c0dbed0366
compatibility::simple-validator-upgrade::liveness-check : committed: 11476.63 txn/s, latency: 2764.43 ms, (p50: 2800 ms, p70: 3000, p90: 3500 ms, p99: 4900 ms), latency samples: 381840
2. Upgrading first Validator to new version: 45ac29f4d1ffa6d84176ba8bf48dfefeb0066280
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 3055.77 txn/s, latency: 9584.70 ms, (p50: 10200 ms, p70: 11800, p90: 12500 ms, p99: 12700 ms), latency samples: 67120
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3139.89 txn/s, latency: 10824.87 ms, (p50: 11800 ms, p70: 12500, p90: 12800 ms, p99: 13000 ms), latency samples: 121140
3. Upgrading rest of first batch to new version: 45ac29f4d1ffa6d84176ba8bf48dfefeb0066280
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 3350.01 txn/s, latency: 8892.15 ms, (p50: 9500 ms, p70: 10600, p90: 11600 ms, p99: 11700 ms), latency samples: 75160
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3357.28 txn/s, latency: 10270.28 ms, (p50: 11100 ms, p70: 11700, p90: 11800 ms, p99: 11800 ms), latency samples: 127920
4. upgrading second batch to new version: 45ac29f4d1ffa6d84176ba8bf48dfefeb0066280
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 1501.29 txn/s, submitted: 1501.44 txn/s, expired: 0.15 txn/s, latency: 5256.48 ms, (p50: 6100 ms, p70: 6400, p90: 6800 ms, p99: 7100 ms), latency samples: 110169
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3262.19 txn/s, submitted: 3262.36 txn/s, expired: 0.18 txn/s, latency: 5694.43 ms, (p50: 6200 ms, p70: 6500, p90: 6700 ms, p99: 6900 ms), latency samples: 203469
5. check swarm health
Compatibility test for 51e3afbaf4645c7a8dd03b94e47555c0dbed0366 ==> 45ac29f4d1ffa6d84176ba8bf48dfefeb0066280 passed
Test Ok

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 45ac29f4d1ffa6d84176ba8bf48dfefeb0066280

two traffics test: inner traffic : committed: 12224.31 txn/s, submitted: 12238.20 txn/s, expired: 13.89 txn/s, latency: 3176.42 ms, (p50: 3000 ms, p70: 3300, p90: 3500 ms, p99: 3900 ms), latency samples: 4647940
two traffics test : committed: 99.99 txn/s, latency: 1668.21 ms, (p50: 1600 ms, p70: 1700, p90: 1800 ms, p99: 2200 ms), latency samples: 1760
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.814, avg: 1.597", "ConsensusProposalToOrdered: max: 0.335, avg: 0.328", "ConsensusOrderedToCommit: max: 0.757, avg: 0.611", "ConsensusProposalToCommit: max: 1.085, avg: 0.938"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.92s no progress at version 38961 (avg 0.23s) [limit 15].
Max epoch-change gap was: 1 rounds at version 3069914 (avg 1.00) [limit 4], 2.77s no progress at version 3069914 (avg 2.77s) [limit 16].
Test Ok

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.

2 participants