[consensus] Disable multi leader per round in universal committer #18206

arun-koshy · 2024-06-12T01:33:46Z

Description

On the boundaries of Leader Schedule change it is possible to elect a new leader for the same round resulting in multiple commits for the same round. This was unintended output from consensus as we have set the parameters to only commit 1 leader per round. Sui also only expects one commit per round, because of this the second commit of the same round is ignored by consensus handler. This led to an error in simtests where the EndOfPublish transaction was part of this ignored commit and we could not form a quorum to close the epoch without this node.

Changes

Explicitly set number of leaders per round to 1 for Mysticeti commits. I wanted to try and maintain this but it would be a much more intrusive change and we will probably have to do a larger refactor to fully support it anyways, so better off doing the cleaner mitigation for now.
Pass back the block that the transaction was included in as part of the tx acknowledgment and include that in the logging to help with debugging future issues.
Also a minor change to print if all tests pass during simtest seed-search

Test plan

❯ scripts/simtest/seed-search.py simtest --test test_reconfig_with_committee_change_basic --num-seeds 1000
    Updating git repository `https://github.com/MystenLabs/mysten-sim.git`
   Compiling consensus-core v0.1.0 (/Users/arunkoshy/Documents/GitHub/sui/consensus/core)
   ...
   All tests passed successfully!
  Killing child processes
  [1]    391 killed     scripts/simtest/seed-search.py simtest --test  --num-seeds 1000

Release notes

vercel · 2024-06-12T01:33:49Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 13, 2024 3:15pm

3 Ignored Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Jun 13, 2024 3:15pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Jun 13, 2024 3:15pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Jun 13, 2024 3:15pm

akichidis

Great find and thanks for the fix @arun-koshy ! If indeed you won't try to do accounting based on the committer index then this PR looks good

Personally I would like us to have though a test that will specifically test this case - when schedule changes and different leader that the last_decided is elected. Since we do have the flag we can confirm that with the flag enabled/disabled we get either a double commit for same round and single with the fix. Do you believe we can add this?

crates/sui-core/src/mysticeti_adapter.rs

crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Testnet_version_50.snap

akichidis · 2024-06-12T12:58:19Z

consensus/core/src/transaction.rs

@@ -172,7 +174,7 @@ impl TransactionClient {

    /// Submits a list of transactions to be sequenced. The method returns when all the transactions have been successfully included
    /// to next proposed blocks.
-    pub async fn submit(&self, transactions: Vec<Vec<u8>>) -> Result<(), ClientError> {
+    pub async fn submit(&self, transactions: Vec<Vec<u8>>) -> Result<BlockRef, ClientError> {


There is a small edge case here as we don't have a guarantee that all the provided transactions will be included in the same block - they might span across multiple blocks. So based on the current functionality this will return the BlockRef of the transactions that got included in the last block. We can ignore for now as this will consider only the soft bundle transactions, but maybe leave a TODO to potentially refactor in the future

Good point! Left a todo but wanted to ensure my understanding is correct here, if we have too many transactions for one block we will acknowledge that the transactions were included in 'Block A' and then create a new TransactionGuard with the remaining transactions. The remaining transactions should get included in the subsequent block(s) but their acknowledgment will not be received by Sui? So like you said Sui will print out that all transactions were included in 'Block A' but may span 'Block B, C, etc.'

So on the above example the Block A transactions will not get acknowledged as the ack channel will only get passed with the last set of transactions. Let's assume that the transactions are enough to span across three blocks, Block A, B & C . The ack channel will only be provided to the closure that performs the acknowledgment only when the last transactions have been included to Block C. So the last block is the one that performs the acknowledgment

Personally I think just returning the last block is fine. When the last block is committed, blocks before should usually be committed. If we try to be precise here, we will need to return the mapping between transactions and blocks.

Yeah I guess we could be as precise/less precise want to. As I said, this only concerns soft bundles so wouldn't prioritise any time soon if needs to change.

consensus/core/src/universal_committer.rs

arun-koshy

Spoke offline with the team and decided to stick with the simpler solution for now and will investigate multi leader per round in the future. Also added some tests for this case

arun-koshy · 2024-06-13T10:09:34Z

consensus/core/src/transaction.rs

@@ -172,7 +174,7 @@ impl TransactionClient {

    /// Submits a list of transactions to be sequenced. The method returns when all the transactions have been successfully included
    /// to next proposed blocks.
-    pub async fn submit(&self, transactions: Vec<Vec<u8>>) -> Result<(), ClientError> {
+    pub async fn submit(&self, transactions: Vec<Vec<u8>>) -> Result<BlockRef, ClientError> {


Good point! Left a todo but wanted to ensure my understanding is correct here, if we have too many transactions for one block we will acknowledge that the transactions were included in 'Block A' and then create a new TransactionGuard with the remaining transactions. The remaining transactions should get included in the subsequent block(s) but their acknowledgment will not be received by Sui? So like you said Sui will print out that all transactions were included in 'Block A' but may span 'Block B, C, etc.'

consensus/core/src/universal_committer.rs

…stenLabs#18206) ## Description On the boundaries of Leader Schedule change it is possible to elect a new leader for the same round resulting in multiple commits for the same round. This was unintended output from consensus as we have set the parameters to only commit 1 leader per round. Sui also only expects one commit per round, because of this the second commit of the same round is ignored by consensus handler. This led to an error in simtests where the `EndOfPublish` transaction was part of this ignored commit and we could not form a quorum to close the epoch without this node. Changes - Explicitly set number of leaders per round to 1 for Mysticeti commits. I wanted to try and maintain this but it would be a much more intrusive change and we will probably have to do a larger refactor to fully support it anyways, so better off doing the cleaner mitigation for now. - Pass back the block that the transaction was included in as part of the tx acknowledgment and include that in the logging to help with debugging future issues. - Also a minor change to print if all tests pass during simtest seed-search ## Test plan ``` ❯ scripts/simtest/seed-search.py simtest --test test_reconfig_with_committee_change_basic --num-seeds 1000 Updating git repository `https://github.com/MystenLabs/mysten-sim.git` Compiling consensus-core v0.1.0 (/Users/arunkoshy/Documents/GitHub/sui/consensus/core) ... All tests passed successfully! Killing child processes [1] 391 killed scripts/simtest/seed-search.py simtest --test --num-seeds 1000 ``` --- ## Release notes - [X] Protocol: Version 50 - Explicitly set number of leaders per round to 1 for Mysticeti commits - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: --------- Co-authored-by: Mark Logan <mark@mystenlabs.com>

arun-koshy requested review from akichidis and mwtian June 12, 2024 01:33

arun-koshy requested a review from mystenmark as a code owner June 12, 2024 01:33

akichidis approved these changes Jun 12, 2024

View reviewed changes

vercel bot deployed to Preview – sui-docs June 12, 2024 17:20 View deployment

mwtian approved these changes Jun 12, 2024

View reviewed changes

consensus/core/src/universal_committer.rs Outdated Show resolved Hide resolved

arun-koshy and others added 3 commits June 13, 2024 05:04

Disable multi leader per round in universal committer

4d31114

Update protocol config tests

074e39c

Strengthen the assert that there is one commit per round

92070e0

arun-koshy force-pushed the ak/simtest-investigation branch from fa49949 to 8c6df9d Compare June 13, 2024 10:16

arun-koshy commented Jun 13, 2024

View reviewed changes

vercel bot deployed to Preview – sui-docs June 13, 2024 10:18 View deployment

Address review comments

a36a1a4

arun-koshy force-pushed the ak/simtest-investigation branch from 8c6df9d to a36a1a4 Compare June 13, 2024 10:22

vercel bot deployed to Preview – sui-docs June 13, 2024 10:23 View deployment

Fix unit test

13d5406

vercel bot deployed to Preview – sui-docs June 13, 2024 15:15 View deployment

arun-koshy enabled auto-merge (squash) June 13, 2024 15:19

arun-koshy merged commit 81d0217 into main Jun 13, 2024
47 checks passed

arun-koshy deleted the ak/simtest-investigation branch June 13, 2024 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[consensus] Disable multi leader per round in universal committer #18206

[consensus] Disable multi leader per round in universal committer #18206

arun-koshy commented Jun 12, 2024 •

edited

Loading

vercel bot commented Jun 12, 2024 •

edited

Loading

akichidis left a comment •

edited

Loading

akichidis Jun 12, 2024

arun-koshy Jun 13, 2024 •

edited

Loading

akichidis Jun 13, 2024

mwtian Jun 13, 2024

akichidis Jun 14, 2024

arun-koshy left a comment

arun-koshy Jun 13, 2024 •

edited

Loading

[consensus] Disable multi leader per round in universal committer #18206

[consensus] Disable multi leader per round in universal committer #18206

Conversation

arun-koshy commented Jun 12, 2024 • edited Loading

Description

Test plan

Release notes

vercel bot commented Jun 12, 2024 • edited Loading

akichidis left a comment • edited Loading

Choose a reason for hiding this comment

akichidis Jun 12, 2024

Choose a reason for hiding this comment

arun-koshy Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

akichidis Jun 13, 2024

Choose a reason for hiding this comment

mwtian Jun 13, 2024

Choose a reason for hiding this comment

akichidis Jun 14, 2024

Choose a reason for hiding this comment

arun-koshy left a comment

Choose a reason for hiding this comment

arun-koshy Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

arun-koshy commented Jun 12, 2024 •

edited

Loading

vercel bot commented Jun 12, 2024 •

edited

Loading

akichidis left a comment •

edited

Loading

arun-koshy Jun 13, 2024 •

edited

Loading

arun-koshy Jun 13, 2024 •

edited

Loading