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

BaseCommitter declarative tests #21154

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

Conversation

lexfrl
Copy link
Contributor

@lexfrl lexfrl commented Feb 10, 2025

Description

While reviewing base_committer_tests, I noticed that that it seems to have been written before DagBuilder and DagParser were introduced by @arun-koshy. Using these tools allows to describe DAG state in a declaratively, making tests both easier to understand and maintain.

By covering key DAG patterns, this PR aims to:

  • serve as a useful learning reference for those exploring Sui’s consensus mechanism
  • provide a more declarative and maintainable approach to testing

Next steps

The plan is to continue to cover more cases and also to do the same for UniversalCommitter in the subsequent PRs.

Test plan

not applicable


Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol:
  • Nodes (Validators and Full nodes):
  • gRPC:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:

@lexfrl lexfrl requested a review from a team as a code owner February 10, 2025 12:45
Copy link

vercel bot commented Feb 10, 2025

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 Feb 12, 2025 0:34am
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Feb 12, 2025 0:34am
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Feb 12, 2025 0:34am

Copy link
Contributor

@mwtian mwtian left a comment

Choose a reason for hiding this comment

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

Nice examples for the commit rules. Some constructed DAG may not be realistic: each round must have >= 3 blocks and each block must have >=3 parents. But doing that will obscure the commit rule.

Copy link
Contributor

@akichidis akichidis left a comment

Choose a reason for hiding this comment

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

Nice tests @lexfrl ! Similar comment with @mwtian - I would recommend to try and build a complete DAG , even if that makes the tests a bit more convoluted.

Comment on lines 79 to 80
Round 1 : { },
Round 2 : { },
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend to still try and express the DAG in the most complete/valid way . Although the dag builder allows us to do this (which we might want to revisit) , I believe it would be better to have rounds complete

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment applies for the rest of the tests.

Copy link
Contributor Author

@lexfrl lexfrl Feb 11, 2025

Choose a reason for hiding this comment

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

Thank you for a feedback @akichidis

The reason why I decided to omit those, because those initial rounds are kinda strange: it doesn't matter if I define connections or not, the leader of wave 0 is always Undecided. So, if I do:

DAG {
        Round 0 : { 4 },
        Round 1 : { * },
        Round 2 : { * },
};

The leader status appears as an Undecided anyway. I'm going to investigate it if it's intentional..

Copy link
Contributor Author

@lexfrl lexfrl Feb 11, 2025

Choose a reason for hiding this comment

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

I checked the builder state and found that it does not create blocks at round 0..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I experimented with this a bit and added blocks on the genesis. The dag_state doesn't like it:

thread 'base_committer::base_committer_declarative_tests::direct_commit' panicked at consensus/core/src/dag_state.rs:294:9:
assertion `left != right` failed: Genesis block should not be accepted into DAG.

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.

3 participants