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

Add QuorumDriver #2515

Merged
merged 9 commits into from
Jun 14, 2022
Merged

Add QuorumDriver #2515

merged 9 commits into from
Jun 14, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Jun 10, 2022

This PR adds a new component called QuorumDriver.
It provides 3 interface functions:

  1. execute_transaction. This API takes a transaction and tries to execute it through the authority aggregator. It accepts 3 different request types, one returns immediately, one waits for the cert and one waits for the effects cert. In the first two cases, it schedules the transaction execution by sending a task to an event queue, which would be processed by an event processor.
  2. next_effects. This allows a node to subscribe effects certs coming out of the quorum driver. A fullnode may want this ti get certs faster than waiting from validators.
  3. update_validators. This can be called at epoch boundaries, when the committee and validator set changes.

pub enum ExecuteTransactionResponse {
ImmediateReturn,
TxCert(Box<CertifiedTransaction>),
EffectsCert(Box<(CertifiedTransaction, TransactionEffects)>),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit misleadingly named, isn't it? The TransactionEffects here is unsigned, so the client can't use this as a proof of finality. An effects cert would actually be a TransactionEffectsEnvelope<AuthorityQuorumSignInfo>, right?

FWIW I may look at forming effects certificates in full node, since I'm already going to be doing the work of waiting for finality before syncing a cert. I say "may" because fullnode can get away with merely proving finality to itself by observed 2f+1 idential effects digests - forming an fx cert involves further fetching 2f+1 signed effects.

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 plan here is to make it return TransactionEffectsEnvelope eventually.
Currently since authority_aggregator only returns effects, here I am leaving it as it is. I plan to send future PRs to actually make authority_aggregator return effects certs.

.instrument(tracing::debug_span!("process_cert"))
.await?;
Ok(ExecuteTransactionResponse::EffectsCert(Box::new(response)))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Evan built a very similar pattern for sending commands over a channel for lock_service.rs, I wonder if its worth abstracting that mechanism so we don't keep re-inventing it.

Copy link
Contributor

@patrickkuo patrickkuo left a comment

Choose a reason for hiding this comment

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

objects.json, owned_objects.json and transactions.json doesn't have real changes, all changes are due to random ids and transaction hash, we can revert the changes.

crates/sui-quorum-driver/src/lib.rs Outdated Show resolved Hide resolved
crates/sui-quorum-driver/src/lib.rs Outdated Show resolved Hide resolved
@lxfind
Copy link
Contributor Author

lxfind commented Jun 13, 2022

Address all feedback:

  • Rebase
  • Remove all mutex
  • Added comments/clarifications on some items

@lxfind lxfind requested a review from patrickkuo June 13, 2022 20:02
Copy link
Contributor

@patrickkuo patrickkuo left a comment

Choose a reason for hiding this comment

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

LGTM!

@lxfind lxfind merged commit e83495a into main Jun 14, 2022
@lxfind lxfind deleted the quorum-driver branch June 14, 2022 00:35
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.

5 participants