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

Fix the order of signer and non-signer tx arg validation to maintain backward compatibility #8649

Merged
merged 1 commit into from
Jun 14, 2023

Conversation

movekevin
Copy link
Contributor

@movekevin movekevin commented Jun 13, 2023

Description

This commit broke compatibility in the past because transactions with mismatching numbers of senders vs what the number of signers the invoked function expects would result in mismatching senders error before the change and failed to deserialize error (from constructing non-signer args) after.

Test Plan

Replay tests

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> c2a89e5fe9a8f52c29be7c65a1b7e9fbef663d20

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> c2a89e5fe9a8f52c29be7c65a1b7e9fbef663d20 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : committed: 9086 txn/s, latency: 3680 ms, (p50: 3600 ms, p90: 5100 ms, p99: 6100 ms), latency samples: 308940
2. Upgrading first Validator to new version: c2a89e5fe9a8f52c29be7c65a1b7e9fbef663d20
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 4438 txn/s, latency: 7215 ms, (p50: 7800 ms, p90: 9000 ms, p99: 9900 ms), latency samples: 168680
3. Upgrading rest of first batch to new version: c2a89e5fe9a8f52c29be7c65a1b7e9fbef663d20
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 4587 txn/s, latency: 6984 ms, (p50: 7300 ms, p90: 9200 ms, p99: 9700 ms), latency samples: 169720
4. upgrading second batch to new version: c2a89e5fe9a8f52c29be7c65a1b7e9fbef663d20
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 7108 txn/s, latency: 4619 ms, (p50: 4500 ms, p90: 6400 ms, p99: 8900 ms), latency samples: 248800
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> c2a89e5fe9a8f52c29be7c65a1b7e9fbef663d20 passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on c2a89e5fe9a8f52c29be7c65a1b7e9fbef663d20

performance benchmark : committed: 5748 txn/s, latency: 6889 ms, (p50: 5500 ms, p90: 10400 ms, p99: 27100 ms), latency samples: 2454520
Max round gap was 1 [limit 4] at version 771834. Max no progress secs was 3.463849 [limit 10] at version 1200322.
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> c2a89e5fe9a8f52c29be7c65a1b7e9fbef663d20

Compatibility test results for aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> c2a89e5fe9a8f52c29be7c65a1b7e9fbef663d20 (PR)
Upgrade the nodes to version: c2a89e5fe9a8f52c29be7c65a1b7e9fbef663d20
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 4734 txn/s, latency: 6931 ms, (p50: 7300 ms, p90: 9900 ms, p99: 11900 ms), latency samples: 175180
5. check swarm health
Compatibility test for aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> c2a89e5fe9a8f52c29be7c65a1b7e9fbef663d20 passed
Test Ok

@movekevin movekevin merged commit c9a3d5c into main Jun 14, 2023
@movekevin movekevin deleted the fix-validation branch June 14, 2023 01:03
movekevin added a commit that referenced this pull request Jun 14, 2023
movekevin added a commit that referenced this pull request Jun 14, 2023
movekevin added a commit that referenced this pull request Jun 14, 2023
movekevin added a commit that referenced this pull request Jun 14, 2023
vusirikala pushed a commit that referenced this pull request Jun 21, 2023
banool pushed a commit that referenced this pull request Jul 7, 2023
xbtmatt pushed a commit to xbtmatt/aptos-core that referenced this pull request Jul 25, 2023
xbtmatt pushed a commit to xbtmatt/aptos-core that referenced this pull request Jul 25, 2023
gedigi pushed a commit that referenced this pull request Aug 2, 2023
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