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

Clean up ConsensusHandler #14126

Merged
merged 1 commit into from
Oct 10, 2023
Merged

Clean up ConsensusHandler #14126

merged 1 commit into from
Oct 10, 2023

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Oct 5, 2023

Description

This PR does a few cleanups for ConsensusHandler:

  1. Move all the code that are consensus-independent into the function inside epoch_store. This allows us to get rid of this strange CommitBatch thing, as well as opens up an interface for other code to call (e.g. if we want to benchmark the system with a dummy consensus)
  2. Move the tx oredering code into its own file for clarity
  3. Insert roots at the end to reduce dup entries.

Test Plan

CI


If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

  • protocol change
  • user-visible impact
  • breaking change for a client SDKs
  • breaking change for FNs (FN binary must upgrade)
  • breaking change for validators or node operators (must upgrade binaries)
  • breaking change for on-chain data layout
  • necessitate either a data wipe or data migration

Release notes

@vercel
Copy link

vercel bot commented Oct 5, 2023

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

Name Status Preview Comments Updated (UTC)
mysten-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 10, 2023 6:53pm
sui-typescript-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 10, 2023 6:53pm
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
explorer ⬜️ Ignored (Inspect) Visit Preview Oct 10, 2023 6:53pm
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Oct 10, 2023 6:53pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Oct 10, 2023 6:53pm

crates/sui-core/src/authority.rs Outdated Show resolved Hide resolved

pub struct PostConsensusTxReorder {}

impl PostConsensusTxReorder {
Copy link
Member

Choose a reason for hiding this comment

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

TBH I'm not sure if it is worth it to pull this into a separate file. But either is fine.

lock_and_final_round,

// The last block in this function notifies about new checkpoint if needed
let final_checkpoint_round = lock_and_final_round.map(|(_, r)| r);
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference between this and the previous way of getting final_checkpoint_round?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lock_and_final_round contains a lock besides the round

@lxfind lxfind force-pushed the consensus-handler-restructure branch from 04e5624 to 7ff723e Compare October 10, 2023 18:53
@vercel vercel bot temporarily deployed to Preview – mysten-ui October 10, 2023 18:53 Inactive
@lxfind lxfind merged commit 5d99127 into main Oct 10, 2023
31 checks passed
@lxfind lxfind deleted the consensus-handler-restructure branch October 10, 2023 22:08
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.

2 participants