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.

Sign up for GitHub

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

Jump to bottom

[shared-object] Implement mock sequencer #632

Merged
merged 19 commits into from
Mar 7, 2022
Merged

[shared-object] Implement mock sequencer #632

merged 19 commits into from
Mar 7, 2022

Conversation

asonnino
Copy link
Contributor

@asonnino asonnino commented Mar 3, 2022 •
edited
Loading

This PR

  • Mock sequencer emulating consensus
  • New task taking an Arc of AuthorityState, receiving consensus outputs, and assigning locks to shared objects

Next PR

Questions to address (issues #633)

  • What is the best way to allow clients to submit certificates to consensus?
  • What is the best way to send consensus outputs to the core?

@oxade
Copy link
Contributor

oxade commented Mar 3, 2022

This is nice @asonnino
Is it possible to include tests? This way we can see it operation and understand how it works better

@asonnino
Copy link
Contributor Author

asonnino commented Mar 3, 2022

This is nice @asonnino
Is it possible to include tests? This way we can see it operation and understand how it works better

Absolutely, would you like it in another PR or do I extend this one?

@gdanezis
Copy link
Collaborator

gdanezis commented Mar 3, 2022

Absolutely, would you like it in another PR or do I extend this one?

Tests go in the same PR :)

gdanezis
gdanezis reviewed Mar 4, 2022
View reviewed changes
Copy link
Collaborator

@gdanezis gdanezis left a comment

Choose a reason for hiding this comment

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

A few comments inline, overall:

  • How to separate the mocks from our production code.
  • How to handle the gap between consensus and processing, where messages may be lost as we start late / crash.

sui_core/src/consensus_handler.rs
};

// Process the certificate to set the locks on the shared objects.
let certificate = &confirmation.certificate;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here I was expecting to see logic that processes the certificate then somehow tells the consensus (per cert or in batches) that it may delete the information because it has been processed. Overall, how do we handle lifecycle? When we start the authority or when we stop/crash the authority. How do we ensure that messages that are sequenced are always processed despite these, and then garbage collected from consensus?

Copy link
Contributor Author

@asonnino asonnino Mar 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The code to process the certificate is located in authority_state(to keep all dangerous things in the same file). I think GC of consensus depends on the consensus protocol (consensus nodes need to keep the state to allow other slow consensus nodes to sync). I think to handle the crash/stop of the Sui authority with respect to the consensus node we need to implement a similar logic used to interface FastPay with a smart contract. The problem is that the logic depends on the smart contract implementation and consensus protocol. It will also depend on how we approach #633

@asonnino
Copy link
Contributor Author

asonnino commented Mar 4, 2022

Absolutely, would you like it in another PR or do I extend this one?

Tests go in the same PR :)

Added unit tests (but not end-to-end tests to keep the PR small)

@asonnino asonnino mentioned this pull request Mar 7, 2022
Merged
@asonnino asonnino self-assigned this Mar 7, 2022
@gdanezis
Copy link
Collaborator

gdanezis commented Mar 7, 2022

Lets talk about the open question at the sync today, and we can review and merge.

gdanezis
gdanezis approved these changes Mar 7, 2022
View reviewed changes
Copy link
Collaborator

@gdanezis gdanezis left a comment

Choose a reason for hiding this comment

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

A few tactical comments.

sui_core/src/consensus_handler.rs
}
// Log the errors that are the client's fault (not ours). This is
// only for debug purposes: all correct authorities will do the same.
Err(e) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

At this point processing a certificate should be infallible no? What kind of errors can the client induce?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The certificate may not be valid (eg. it is not signed by a quorum of authorities)

@asonnino asonnino merged commit b67e0a0 into MystenLabs:main Mar 7, 2022
@asonnino asonnino deleted the mock-sequencer branch March 7, 2022 16:31
@huitseeker huitseeker mentioned this pull request Apr 15, 2022
Merged
mwtian pushed a commit to mwtian/sui that referenced this pull request Sep 29, 2022
@dependabot
chore(deps): bump lycheeverse/lychee-action from 1.5.0 to 1.5.1 (Myst…
9c02fa6 
…enLabs#632)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gdanezis gdanezis gdanezis approved these changes

Assignees

@asonnino asonnino

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@asonnino @oxade @gdanezis