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

[Multisig V2] Implemented the implicit voting for multisig transaction execution and rejection #11941

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

junkil-park
Copy link
Contributor

@junkil-park junkil-park commented Feb 7, 2024

Description

To simplify the Multisig v2 transaction execution workflow and minimize the required number of transactions, this implemented implicit voting as follows:

  • Count it as an implicit approval upon transaction execution
  • Count it as an implicit rejection upon "rejection execution"

This resolves #11011.

Test Plan

aptos move test

Copy link

trunk-io bot commented Feb 7, 2024

⏱️ 4h 8m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-move-unit-coverage 1h 9m 🟩🟩
rust-unit-tests 58m 🟩🟩
windows-build 39m 🟩🟩
rust-move-tests 35m 🟩🟩
rust-lints 14m 🟩🟩
run-tests-main-branch 12m 🟥🟥
check 8m 🟩🟩
general-lints 5m 🟩🟩
check-dynamic-deps 4m 🟩🟩
semgrep/ci 57s 🟩🟩
file_change_determinator 22s 🟩🟩
file_change_determinator 18s 🟩🟩
permission-check 7s 🟩🟩
permission-check 6s 🟩🟩
permission-check 5s 🟩🟩
permission-check 2s 🟩🟩

🚨 1 job on the last run was significantly faster/slower than expected

Job Duration vs 7d avg Delta
run-tests-main-branch 6m 4m +45%

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link

codecov bot commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.0%. Comparing base (ebbe720) to head (93d4766).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11941   +/-   ##
=======================================
  Coverage    64.0%    64.0%           
=======================================
  Files         803      803           
  Lines      178181   178181           
=======================================
  Hits       114043   114043           
  Misses      64138    64138           

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

alnoki

This comment was marked as resolved.

@junkil-park junkil-park force-pushed the jpark/multisig-implicit-approval-rejection branch from caeb7db to c47876f Compare February 12, 2024 22:40
@junkil-park
Copy link
Contributor Author

Essentially I'm proposing that regardless of if/how an owner has voted in the past, they express their intent to vote yes or no, respectively, through the act of invoking approve or rejection functions, respectively. E.g. if someone votes no but later calls execute their vote should be implicitly re-cast to yes. You might need to update the tests if you incorporate the logic I suggest

I updated the code to override any previous votes and included such a scenario in the unit test.

@junkil-park
Copy link
Contributor Author

junkil-park commented Feb 12, 2024

Additionally, I'm not sure if voting records are available after execution, but if they are, then I think this PR does not accurately capture the final voting state for implicit votes

That's a good point! Voting records are removed from the Table:

let transaction = table::remove(&mut multisig_account_resource.transactions, sequence_number);

However, events will remain. So, I updated the code to correctly emit VoteEvent and the events for transaction execution/rejection especially for the num_approvals and num_rejections fields.

alnoki

This comment was marked as resolved.

Copy link
Contributor

@movekevin movekevin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you see if we can do this for implicit rejection as well?

@junkil-park junkil-park force-pushed the jpark/multisig-implicit-approval-rejection branch from c47876f to 6795736 Compare March 4, 2024 23:20
Comment on lines +362 to +371
if (!has_voted_for_rejection(multisig_account, sequence_number, owner)) {
num_rejections = num_rejections + 1;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this saying if the owner has not voted for rejection, increment number of rejections?

Copy link
Contributor Author

@junkil-park junkil-park Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnChangUK , right, "+1" is the implicit vote. The "execute" operation will involve an implicit vote for approval, and the "execute-reject" operation will involve an implicit vote for rejection. This feature is requested by #11011.

@junkil-park
Copy link
Contributor Author

junkil-park commented Mar 5, 2024

Can you see if we can do this for implicit rejection as well?

@movekevin , this line is doing implicit rejections: https://github.com/aptos-labs/aptos-core/pull/11941/files#diff-cded52b0ab3d00d8edcfab232ce1f48c8f3b5a1f1485b2b8c40f6e49147967c9R896

@junkil-park junkil-park force-pushed the jpark/multisig-implicit-approval-rejection branch from 6795736 to af02259 Compare March 6, 2024 21:37
…d rejection

To simplify the Multisig v2 transaction execution workflow and minimize the required number of transactions, this implemented the implicit voting as follows:
- Count it as an implicit approval upon transaction execution
- Count it as an implicit rejection upon "rejection execution"
@junkil-park junkil-park force-pushed the jpark/multisig-implicit-approval-rejection branch from af02259 to 93d4766 Compare March 9, 2024 01:50
@junkil-park junkil-park enabled auto-merge (squash) March 11, 2024 20:55

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.9.5 ==> 93d47668a3677d127c2372698dfb3ea4cbbec64e

Compatibility test results for aptos-node-v1.9.5 ==> 93d47668a3677d127c2372698dfb3ea4cbbec64e (PR)
1. Check liveness of validators at old version: aptos-node-v1.9.5
compatibility::simple-validator-upgrade::liveness-check : committed: 6175 txn/s, latency: 5336 ms, (p50: 4800 ms, p90: 8700 ms, p99: 16000 ms), latency samples: 216140
2. Upgrading first Validator to new version: 93d47668a3677d127c2372698dfb3ea4cbbec64e
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 694 txn/s, latency: 34047 ms, (p50: 35200 ms, p90: 53100 ms, p99: 55100 ms), latency samples: 56980
3. Upgrading rest of first batch to new version: 93d47668a3677d127c2372698dfb3ea4cbbec64e
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 72 txn/s, submitted: 599 txn/s, expired: 526 txn/s, latency: 23713 ms, (p50: 21400 ms, p90: 39500 ms, p99: 40700 ms), latency samples: 4951
4. upgrading second batch to new version: 93d47668a3677d127c2372698dfb3ea4cbbec64e
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 2377 txn/s, latency: 12731 ms, (p50: 15800 ms, p90: 18400 ms, p99: 19200 ms), latency samples: 111760
5. check swarm health
Compatibility test for aptos-node-v1.9.5 ==> 93d47668a3677d127c2372698dfb3ea4cbbec64e passed
Test Ok

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 93d47668a3677d127c2372698dfb3ea4cbbec64e

two traffics test: inner traffic : committed: 7626 txn/s, latency: 5140 ms, (p50: 5100 ms, p90: 6000 ms, p99: 9900 ms), latency samples: 3294440
two traffics test : committed: 100 txn/s, latency: 1834 ms, (p50: 1800 ms, p90: 2100 ms, p99: 2400 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.244, avg: 0.205", "QsPosToProposal: max: 0.297, avg: 0.276", "ConsensusProposalToOrdered: max: 0.443, avg: 0.425", "ConsensusOrderedToCommit: max: 0.331, avg: 0.317", "ConsensusProposalToCommit: max: 0.757, avg: 0.742"]
Max round gap was 1 [limit 4] at version 1575475. Max no progress secs was 4.063221 [limit 15] at version 1575475.
Test Ok

@junkil-park junkil-park merged commit e492ecd into main Mar 11, 2024
86 checks passed
@junkil-park junkil-park deleted the jpark/multisig-implicit-approval-rejection branch March 11, 2024 21:27
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.

[Feature Request][CLI][Multisig v2] Enable implicit yes/no vote for multisig execution/rejection
4 participants