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

[Consensus] Make consensus pipeline self-sending optional. #14662

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JoshLind
Copy link
Contributor

Description

This PR makes self-sending in the consensus pipeline optional, such that it does not occur for consensus observer (CO). This is prompted by a spurious error log for CO that self sends are failing (see: #14559), however, self-sends are not required for CO.

Testing Plan

Existing test infrastructure.

Copy link

trunk-io bot commented Sep 17, 2024

@JoshLind JoshLind 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 17, 2024

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

@zekun000 zekun000 left a comment

Choose a reason for hiding this comment

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

which messages trigger the errors? it might be useful to listen to this message to make epoch switch smoother

.send_epoch_change(EpochChangeProof::new(vec![commit_proof], false))

currently we rely on upstream to send a message from a new epoch and call into state sync to enter new epoch but we should be able to avoid that

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 4beb28710a8ca7e24bb90c7dfcce8e74aa3d0f35

two traffics test: inner traffic : committed: 13581.60 txn/s, submitted: 13582.84 txn/s, failed submission: 1.24 txn/s, expired: 1.24 txn/s, latency: 2930.02 ms, (p50: 2700 ms, p70: 3000, p90: 3300 ms, p99: 5200 ms), latency samples: 5164010
two traffics test : committed: 98.67 txn/s, submitted: 100.02 txn/s, failed submission: 1.35 txn/s, expired: 1.35 txn/s, latency: 2400.30 ms, (p50: 2300 ms, p70: 2400, p90: 2600 ms, p99: 8100 ms), latency samples: 1780
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.243, avg: 0.221", "QsPosToProposal: max: 0.578, avg: 0.385", "ConsensusProposalToOrdered: max: 0.330, avg: 0.306", "ConsensusOrderedToCommit: max: 0.442, avg: 0.423", "ConsensusProposalToCommit: max: 0.762, avg: 0.729"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.65s no progress at version 2878664 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 9.45s no progress at version 2877313 (avg 9.45s) [limit 15].
Test Ok

Copy link
Contributor

✅ Forge suite compat success on 70806ff543496aa6de7807feff49e7e1370efd20 ==> 4beb28710a8ca7e24bb90c7dfcce8e74aa3d0f35

Compatibility test results for 70806ff543496aa6de7807feff49e7e1370efd20 ==> 4beb28710a8ca7e24bb90c7dfcce8e74aa3d0f35 (PR)
1. Check liveness of validators at old version: 70806ff543496aa6de7807feff49e7e1370efd20
compatibility::simple-validator-upgrade::liveness-check : committed: 11662.43 txn/s, latency: 2426.54 ms, (p50: 2000 ms, p70: 2400, p90: 3100 ms, p99: 16200 ms), latency samples: 475760
2. Upgrading first Validator to new version: 4beb28710a8ca7e24bb90c7dfcce8e74aa3d0f35
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 8057.60 txn/s, latency: 3506.71 ms, (p50: 3800 ms, p70: 4000, p90: 4100 ms, p99: 4300 ms), latency samples: 147720
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 8042.35 txn/s, latency: 3933.44 ms, (p50: 4100 ms, p70: 4200, p90: 5700 ms, p99: 6200 ms), latency samples: 265580
3. Upgrading rest of first batch to new version: 4beb28710a8ca7e24bb90c7dfcce8e74aa3d0f35
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6087.84 txn/s, latency: 4765.12 ms, (p50: 4000 ms, p70: 6400, p90: 6800 ms, p99: 7000 ms), latency samples: 112880
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7306.63 txn/s, latency: 4342.50 ms, (p50: 4400 ms, p70: 4700, p90: 6300 ms, p99: 6800 ms), latency samples: 243300
4. upgrading second batch to new version: 4beb28710a8ca7e24bb90c7dfcce8e74aa3d0f35
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 10373.29 txn/s, latency: 2373.20 ms, (p50: 2500 ms, p70: 2700, p90: 3100 ms, p99: 3800 ms), latency samples: 206060
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10458.12 txn/s, latency: 2936.65 ms, (p50: 2600 ms, p70: 2700, p90: 5900 ms, p99: 7700 ms), latency samples: 340040
5. check swarm health
Compatibility test for 70806ff543496aa6de7807feff49e7e1370efd20 ==> 4beb28710a8ca7e24bb90c7dfcce8e74aa3d0f35 passed
Test Ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants