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

Do not re-sequence replayed transactions #1947

Merged
merged 11 commits into from
May 16, 2022
Merged

Do not re-sequence replayed transactions #1947

merged 11 commits into from
May 16, 2022

Conversation

asonnino
Copy link
Contributor

The authority currently sequences every shared-object transaction it sees. This is fine, but we can skip consensus in the following cases:

  • we already executed the transaction, we can thus immeditaly return the effects without first going through consensus
  • we already assigned locks to the transaction but failed to execute it. This scenario happens when the authority missed some of the transaction's dependencies; we can thus try to re-execute it now.

@asonnino asonnino marked this pull request as ready for review May 13, 2022 17:17
@asonnino asonnino added Priority: Low Wishlist feature or bug with very infrequent occurrence and/or severity Status: Needs Review PR is ready for review sui-node Type: Enhancement New feature or request labels May 13, 2022
@asonnino asonnino self-assigned this May 13, 2022
@asonnino asonnino enabled auto-merge (squash) May 13, 2022 17:21
Copy link
Contributor

@huitseeker huitseeker 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 nits, but this looks great!

ConsensusNarwhalSerializationError(String),
#[error("Only shared object transactions need to be sequenced")]
OnlySharedObjectTransactionNeedSequencing,
Copy link
Contributor

Choose a reason for hiding this comment

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

NotASharedObjectTransaction maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea

Comment on lines 219 to 222
let ConsensusTransaction::UserTransaction(certificate) = &transaction;
let info = match self
.state
.try_skip_consensus(certificate)
Copy link
Contributor

Choose a reason for hiding this comment

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

You own the transaction, consider having try_skip_consensus take ownership of it.
See C-CALLER-CONTROL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this is a good read (should have done it earlier): https://rust-lang.github.io/api-guidelines/about.html

@asonnino asonnino merged commit 4990b2f into main May 16, 2022
@asonnino asonnino deleted the dos-checks branch May 16, 2022 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low Wishlist feature or bug with very infrequent occurrence and/or severity Status: Needs Review PR is ready for review sui-node Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants