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

simplify transaction list verify code #14783

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

msmouse
Copy link
Contributor

@msmouse msmouse commented Sep 27, 2024

Description

How Has This Been Tested?

Key Areas to Review

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

Copy link

trunk-io bot commented Sep 27, 2024

⏱️ 11h 44m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
indexer-grpc-e2e-tests / test-indexer-grpc-docker-compose 6h 🟥
execution-performance / single-node-performance 53m 🟥🟥🟥🟥🟥 (+1 more)
rust-cargo-deny 30m 🟩🟩🟩 (+14 more)
check-dynamic-deps 27m 🟩🟩🟩🟩🟩 (+15 more)
execution-performance / test-target-determinator 27m 🟩🟩🟩🟩🟩 (+1 more)
test-target-determinator 20m 🟩🟩🟩 (+1 more)
check 17m 🟩🟩🟩 (+1 more)
rust-move-unit-coverage 11m 🟩
rust-move-tests 10m 🟩
rust-move-tests 10m 🟩
rust-move-tests 10m 🟩
rust-move-tests 10m 🟩
rust-move-tests 10m 🟩
rust-move-tests 10m 🟩
rust-move-tests 10m 🟩

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

Job Duration vs 7d avg Delta
indexer-grpc-e2e-tests / test-indexer-grpc-docker-compose 6h 1m +24238%
execution-performance / test-target-determinator 5m 4m +31%

settingsfeedbackdocs ⋅ learn more about trunk.io

@msmouse msmouse force-pushed the 0927-alden-executor-workflow branch 9 times, most recently from a6413a0 to b85f4f4 Compare September 28, 2024 01:03
Comment on lines 276 to 290
let num_txns = txn_list_with_proof.transactions.len();
ensure!(num_txns != 0, "Empty transaction list!");
let first_version_in_request = txn_list_with_proof
.first_transaction_version
.ok_or_else(|| anyhow!("Non-empty chunk with first_version == None."))?;
let parent_state = self.commit_queue.lock().latest_state();
ensure!(
first_version_in_request == parent_state.next_version(),
"Unexpected chunk. version in request: {}, current_version: {:?}",
first_version_in_request,
parent_state.current_version,
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are guaranteed if list.verify() passes with Some(expected_version) passed in, specifically, it's guranteed that 1. the list is not empty and 2. the list starts from expected_version

@msmouse msmouse added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Sep 30, 2024
Copy link
Contributor

Forge is running suite compat on 28abb7db64af8de81a6bc76c6b2d6600181ac578 ==> ae6b08f38a0f99e9aff16d82afe08313213123b3

This comment has been minimized.

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on ae6b08f38a0f99e9aff16d82afe08313213123b3

two traffics test: inner traffic : committed: 13917.48 txn/s, latency: 2855.21 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 12600 ms), latency samples: 5291820
two traffics test : committed: 99.93 txn/s, latency: 1814.71 ms, (p50: 1500 ms, p70: 1600, p90: 2400 ms, p99: 9500 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.241, avg: 0.225", "QsPosToProposal: max: 1.106, avg: 1.001", "ConsensusProposalToOrdered: max: 0.309, avg: 0.294", "ConsensusOrderedToCommit: max: 0.431, avg: 0.417", "ConsensusProposalToCommit: max: 0.726, avg: 0.711"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.95s no progress at version 1254852 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.79s no progress at version 5549130 (avg 8.61s) [limit 15].
Test Ok

@msmouse msmouse force-pushed the 0927-alden-executor-workflow branch 3 times, most recently from 8a098a6 to 88b65de Compare October 1, 2024 19:56
@msmouse msmouse changed the base branch from main to 1001-alden-combine-execute-apply October 1, 2024 19:57
@msmouse msmouse force-pushed the 0927-alden-executor-workflow branch from 88b65de to 5a3af0e Compare October 1, 2024 19:57
Base automatically changed from 1001-alden-combine-execute-apply to main October 1, 2024 21:50
@msmouse msmouse force-pushed the 0927-alden-executor-workflow branch 2 times, most recently from eca2910 to a8b15b2 Compare October 2, 2024 02:50
@msmouse msmouse removed the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Oct 2, 2024
Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.

Project coverage is 59.7%. Comparing base (67f7ee6) to head (be1b350).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
types/src/transaction/mod.rs 0.0% 19 Missing ⚠️
types/src/proof/accumulator/mod.rs 0.0% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14783   +/-   ##
=======================================
  Coverage    59.7%    59.7%           
=======================================
  Files         853      853           
  Lines      208283   208300   +17     
=======================================
+ Hits       124530   124548   +18     
+ Misses      83753    83752    -1     

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

@msmouse msmouse force-pushed the 0927-alden-executor-workflow branch from a8b15b2 to 73351ba Compare October 7, 2024 20:22
@msmouse msmouse force-pushed the 0927-alden-executor-workflow branch from 73351ba to a1ebce8 Compare October 8, 2024 21:38
@msmouse msmouse force-pushed the 0927-alden-executor-workflow branch from a1ebce8 to be1b350 Compare October 8, 2024 23:00
@msmouse msmouse added CICD:run-execution-performance-test Run execution performance test CICD:run-execution-performance-full-test Run execution performance test (full version) labels Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:run-execution-performance-full-test Run execution performance test (full version) CICD:run-execution-performance-test Run execution performance test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant