[Multisig V2] Aborts the multisig transaction if the provided payload does not match the payload stored onchain #13160

junkil-park · 2024-05-01T20:00:30Z

Description

This PR update the validate_multisig_transaction function, so that the multisig transaction fails if the provided payload does not match to the onchain payload. This fixes #12929.

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Tested on the localnet
Added API tests

trunk-io · 2024-05-01T20:00:33Z

⏱️ 2h 47m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-move-unit-coverage	1h 7m	🟩 🟩 🟩 🟩
rust-targeted-unit-tests	45m	🟥 🟥
rust-move-tests	23m	🟩 🟩
run-tests-main-branch	13m	🟩 🟩 🟩
rust-lints	11m	🟥 🟥
general-lints	4m	🟩 🟩
check-dynamic-deps	2m	🟩 🟩
semgrep/ci	1m	🟩 🟩 🟩
file_change_determinator	30s	🟩 🟩
file_change_determinator	19s	🟩 🟩
permission-check	6s	🟩 🟩
permission-check	5s	🟩 🟩
permission-check	5s	🟩 🟩
permission-check	4s	🟩 🟩

🚨 2 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
rust-targeted-unit-tests	23m	16m
rust-move-tests	12m	9m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

codecov · 2024-05-01T20:21:12Z

Codecov Report

Attention: Patch coverage is 6.66667% with 14 lines in your changes missing coverage. Please review.

Project coverage is 58.6%. Comparing base (5ad91bc) to head (ebd1b45).

Files	Patch %	Lines
aptos-move/aptos-vm/src/aptos_vm.rs	0.0%	6 Missing ⚠️
aptos-move/aptos-vm/src/transaction_validation.rs	0.0%	4 Missing ⚠️
types/src/on_chain_config/aptos_features.rs	25.0%	3 Missing ⚠️
aptos-move/aptos-vm/src/errors.rs	0.0%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #13160     +/-   ##
=========================================
- Coverage    58.6%    58.6%   -0.1%     
=========================================
  Files         823      823             
  Lines      198440   198453     +13     
=========================================
+ Hits       116350   116351      +1     
- Misses      82090    82102     +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aptos-move/framework/aptos-framework/sources/multisig_account.move

georgemitenkov · 2024-05-28T18:43:56Z

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

@@ -116,6 +116,7 @@ pub enum FeatureFlag {
 DispatchableFungibleAsset,
 NewAccountsDefaultToFaAptStore,
 OperationsDefaultToFaAptStore,
+ MultisigV2Fix,


nit: maybe a more descriptive name rather than "fix"? Would be nice to have some context for the future

Done renaming it. Thanks!

aptos-move/aptos-vm/src/aptos_vm.rs

georgemitenkov

LGTM, see comments for some questions I have

aptos-move/aptos-vm/src/transaction_validation.rs

georgemitenkov · 2024-06-11T16:07:08Z

aptos-move/framework/move-stdlib/sources/configs/features.move

+ /// abort if the provided payload does not match the payload stored on-chain.
+ ///
+ /// Lifetime: transient
+ const MULTISIG_V2_FIX: u64 = 69;


nit: based on the description, isn't it better to call the flag (and in Rust code) ABORT_IF_MISMATCHED_MULTISIG_PAYLOAD? Then feature reads easily like abort_if_mismatched_multisig_payload_enabled() and it is way clearer what the features is above without having to look up much?

Done renaming it! Thank you!

… onchain payloads Updates the `validate_multisig_transaction ` function, so that the multisig transaction fails if the provided payload does not match to the onchain payload. This fixes #12929.

github-actions · 2024-06-12T22:26:37Z

✅ Forge suite `compat` success on `a68e71c05caebf01504d4499110f3fba213fb53d` ==> `ebd1b45c3ba64d3193b31bd6e75828388666c2b3`

Compatibility test results for a68e71c05caebf01504d4499110f3fba213fb53d ==> ebd1b45c3ba64d3193b31bd6e75828388666c2b3 (PR)
1. Check liveness of validators at old version: a68e71c05caebf01504d4499110f3fba213fb53d
compatibility::simple-validator-upgrade::liveness-check : committed: 10673.105396101895 txn/s, latency: 3175.406548622822 ms, (p50: 2500 ms, p90: 5900 ms, p99: 12100 ms), latency samples: 355800
2. Upgrading first Validator to new version: ebd1b45c3ba64d3193b31bd6e75828388666c2b3
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6829.57945517944 txn/s, latency: 4455.250817427386 ms, (p50: 4500 ms, p90: 6100 ms, p99: 6400 ms), latency samples: 241000
3. Upgrading rest of first batch to new version: ebd1b45c3ba64d3193b31bd6e75828388666c2b3
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6775.995271832782 txn/s, latency: 4522.436786469345 ms, (p50: 4500 ms, p90: 6200 ms, p99: 6500 ms), latency samples: 236500
4. upgrading second batch to new version: ebd1b45c3ba64d3193b31bd6e75828388666c2b3
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10750.312052306039 txn/s, latency: 2847.379375740814 ms, (p50: 2400 ms, p90: 4700 ms, p99: 6100 ms), latency samples: 354340
5. check swarm health
Compatibility test for a68e71c05caebf01504d4499110f3fba213fb53d ==> ebd1b45c3ba64d3193b31bd6e75828388666c2b3 passed
Test Ok

github-actions · 2024-06-12T22:29:12Z

✅ Forge suite `realistic_env_max_load` success on `ebd1b45c3ba64d3193b31bd6e75828388666c2b3`

two traffics test: inner traffic : committed: 9380.861662270743 txn/s, latency: 4191.713111078147 ms, (p50: 4200 ms, p90: 4500 ms, p99: 5700 ms), latency samples: 4044900
two traffics test : committed: 99.95952860432475 txn/s, latency: 1994.4017441860465 ms, (p50: 1900 ms, p90: 2200 ms, p99: 3600 ms), latency samples: 1720
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.242, avg: 0.217", "QsPosToProposal: max: 1.988, avg: 1.883", "ConsensusProposalToOrdered: max: 0.324, avg: 0.286", "ConsensusOrderedToCommit: max: 0.367, avg: 0.355", "ConsensusProposalToCommit: max: 0.651, avg: 0.641"]
Max round gap was 1 [limit 4] at version 1943611. Max no progress secs was 5.401813 [limit 15] at version 1943611.
Test Ok

github-actions · 2024-06-12T22:29:54Z

✅ Forge suite `framework_upgrade` success on `a68e71c05caebf01504d4499110f3fba213fb53d` ==> `ebd1b45c3ba64d3193b31bd6e75828388666c2b3`

Compatibility test results for a68e71c05caebf01504d4499110f3fba213fb53d ==> ebd1b45c3ba64d3193b31bd6e75828388666c2b3 (PR)
Upgrade the nodes to version: ebd1b45c3ba64d3193b31bd6e75828388666c2b3
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1204.7958930803609 txn/s, submitted: 1207.778629056374 txn/s, failed submission: 2.982735976013082 txn/s, expired: 2.982735976013082 txn/s, latency: 2715.401771091221 ms, (p50: 1800 ms, p90: 4700 ms, p99: 12100 ms), latency samples: 105020
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1141.1137110076654 txn/s, submitted: 1142.865564718673 txn/s, failed submission: 1.7518537110077381 txn/s, expired: 1.7518537110077381 txn/s, latency: 2591.3668201880637 ms, (p50: 2100 ms, p90: 4500 ms, p99: 8700 ms), latency samples: 104220
5. check swarm health
Compatibility test for a68e71c05caebf01504d4499110f3fba213fb53d ==> ebd1b45c3ba64d3193b31bd6e75828388666c2b3 passed
Upgrade the remaining nodes to version: ebd1b45c3ba64d3193b31bd6e75828388666c2b3
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1167.0528105845967 txn/s, submitted: 1168.6044477496666 txn/s, failed submission: 1.5516371650697394 txn/s, expired: 1.5516371650697394 txn/s, latency: 2657.7768850902185 ms, (p50: 2100 ms, p90: 4500 ms, p99: 10200 ms), latency samples: 105300
Test Ok

junkil-park marked this pull request as ready for review May 2, 2024 21:16

junkil-park requested review from davidiw, movekevin, wrwg, zekun000, vgao1996 and georgemitenkov as code owners May 2, 2024 21:16

junkil-park force-pushed the jpark/payload-mismatch branch from 9838644 to c7b80fc Compare May 2, 2024 21:17

junkil-park force-pushed the jpark/payload-mismatch branch 7 times, most recently from e0271b4 to 9a4cc41 Compare May 24, 2024 10:21

movekevin reviewed May 24, 2024

View reviewed changes

aptos-move/framework/aptos-framework/sources/multisig_account.move Outdated Show resolved Hide resolved

movekevin reviewed May 24, 2024

View reviewed changes

aptos-move/framework/aptos-framework/sources/multisig_account.move Outdated Show resolved Hide resolved

movekevin approved these changes May 24, 2024

View reviewed changes

junkil-park force-pushed the jpark/payload-mismatch branch from 9a4cc41 to d22a42c Compare May 28, 2024 18:27

georgemitenkov approved these changes May 28, 2024

View reviewed changes

junkil-park force-pushed the jpark/payload-mismatch branch from d22a42c to 001a924 Compare May 29, 2024 17:02

junkil-park requested review from banool, gregnazario and 0xmaayan as code owners May 29, 2024 17:02

junkil-park force-pushed the jpark/payload-mismatch branch from 001a924 to 27aff2c Compare May 29, 2024 19:31

junkil-park force-pushed the jpark/payload-mismatch branch 2 times, most recently from 453b5cc to d694c34 Compare June 10, 2024 17:10

georgemitenkov approved these changes Jun 11, 2024

View reviewed changes

junkil-park force-pushed the jpark/payload-mismatch branch from d694c34 to edb757e Compare June 11, 2024 16:58

Fails the multisig transaction upon the mismatch between provided and…

ebd1b45

… onchain payloads Updates the `validate_multisig_transaction ` function, so that the multisig transaction fails if the provided payload does not match to the onchain payload. This fixes #12929.

junkil-park force-pushed the jpark/payload-mismatch branch from edb757e to ebd1b45 Compare June 12, 2024 21:15

junkil-park changed the title ~~[Multisig V2] Fails the multisig transaction upon the mismatch between provided and onchain payloads~~ [Multisig V2] Aborts the multisig transaction if the provided payload does not match the payload stored onchain Jun 12, 2024

junkil-park enabled auto-merge (squash) June 12, 2024 22:02

This comment has been minimized.

Sign in to view

junkil-park merged commit 2b01095 into main Jun 12, 2024
89 checks passed

junkil-park deleted the jpark/payload-mismatch branch June 12, 2024 22:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Multisig V2] Aborts the multisig transaction if the provided payload does not match the payload stored onchain #13160

[Multisig V2] Aborts the multisig transaction if the provided payload does not match the payload stored onchain #13160

junkil-park commented May 1, 2024 •

edited

Loading

trunk-io bot commented May 1, 2024 •

edited

Loading

codecov bot commented May 1, 2024 •

edited

Loading

georgemitenkov May 28, 2024

junkil-park Jun 12, 2024

georgemitenkov left a comment

georgemitenkov Jun 11, 2024

junkil-park Jun 11, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jun 12, 2024

github-actions bot commented Jun 12, 2024

github-actions bot commented Jun 12, 2024

[Multisig V2] Aborts the multisig transaction if the provided payload does not match the payload stored onchain #13160

[Multisig V2] Aborts the multisig transaction if the provided payload does not match the payload stored onchain #13160

Conversation

junkil-park commented May 1, 2024 • edited Loading

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

trunk-io bot commented May 1, 2024 • edited Loading

codecov bot commented May 1, 2024 • edited Loading

Codecov Report

georgemitenkov May 28, 2024

Choose a reason for hiding this comment

junkil-park Jun 12, 2024

Choose a reason for hiding this comment

georgemitenkov left a comment

Choose a reason for hiding this comment

georgemitenkov Jun 11, 2024

Choose a reason for hiding this comment

junkil-park Jun 11, 2024

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 Jun 12, 2024

✅ Forge suite compat success on a68e71c05caebf01504d4499110f3fba213fb53d ==> ebd1b45c3ba64d3193b31bd6e75828388666c2b3

github-actions bot commented Jun 12, 2024

✅ Forge suite realistic_env_max_load success on ebd1b45c3ba64d3193b31bd6e75828388666c2b3

github-actions bot commented Jun 12, 2024

✅ Forge suite framework_upgrade success on a68e71c05caebf01504d4499110f3fba213fb53d ==> ebd1b45c3ba64d3193b31bd6e75828388666c2b3

junkil-park commented May 1, 2024 •

edited

Loading

trunk-io bot commented May 1, 2024 •

edited

Loading

codecov bot commented May 1, 2024 •

edited

Loading

✅ Forge suite `compat` success on `a68e71c05caebf01504d4499110f3fba213fb53d` ==> `ebd1b45c3ba64d3193b31bd6e75828388666c2b3`

✅ Forge suite `realistic_env_max_load` success on `ebd1b45c3ba64d3193b31bd6e75828388666c2b3`

✅ Forge suite `framework_upgrade` success on `a68e71c05caebf01504d4499110f3fba213fb53d` ==> `ebd1b45c3ba64d3193b31bd6e75828388666c2b3`