Refactor ConsensusHandler in preparation for parallel signature verification #5344
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
asonnino
approved these changes
Oct 18, 2022
| if self | ||
| .database | ||
| .consensus_message_processed(certificate.digest()) | ||
| .expect("Storage error") |
There was a problem hiding this comment.
Should we add some comments about why we panic upon this storage error?
There was a problem hiding this comment.
Just to be sure: panicking here ensures that the consensus transaction will not be skipped (ie. validator stops/crashes and the execution indices do not update with future transactions), right? (because we cannot skip that transaction)
There was a problem hiding this comment.
I think in general when we have storage error there is not much we can do. Yes, this will not skip transaction, but essentially will shutdown the node
akichidis
approved these changes
Oct 18, 2022
| @@ -1,3 +1,4 @@ | |||
| use std::sync::Arc; | |||
There was a problem hiding this comment.
probably the license header checker will complain as well, this will need to be after the header comments
| @@ -1114,7 +1114,7 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> SuiDataStore<S> { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| pub async fn consensus_message_processed( | |||
| pub fn consensus_message_processed( | |||
…ication This PR refactors ConsensusHandler in order to prepare for parallel verification of the transaction signatures. On Sui side: (1) Break down consensus handling into two independent functions on the `AuthorityState` - one for (potentially parallel) verification and one for sequential handling of the transaction. (2) Move `ConsensusHandler` into separate file - `authority.rs` is growing big and parallel signature verification will add even more code to ConsensusHandler. (3) Abolish `NarwhalHandlerError` - after separating `handle_consensus_transaction` and `verify_consensus_transaction`, `ConsensusHandler` can now decide what to do based on what function has failed. Note that in practice nothing has changed in how errors are processed by this PR - previously we were mapping errors from `verify_narwhal_transaction` into `NarwhalHandlerError::Skip`, and now we just call it directly from `ConsensusHandler`. (4) Two utilization metrics on the `ConsensusHandler` side, namely `verify_narwhal_transaction_duration_mcs` and `handle_consensus_duration_mcs` slightly changed what they measure - previously verification metric was fraction of handle_consensus timer, now they are two independent steps. The total consensus utilization can now be measured as sum of those two metrics (until parallel verification is introduced). On Narwhal side: (1) Minor interface change - pass `Arc<ConsensusOutput>` instead of `&ConsensusOutput` from nw executor - this will allow to caller to cheaply clone ConsensusOutput when it needs to pass it to verification thread
mystenmark
approved these changes
Oct 18, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors ConsensusHandler in order to prepare for parallel verification of the transaction signatures.
On Sui side:
(1) Break down consensus handling into two independent functions on the
AuthorityState- one for (potentially parallel) verification and one for sequential handling of the transaction.(2) Move
ConsensusHandlerinto separate file -authority.rsis growing big and parallel signature verification will add even more code to ConsensusHandler.(3) Abolish
NarwhalHandlerError- after separatinghandle_consensus_transactionandverify_consensus_transaction,ConsensusHandlercan now decide what to do based on what function has failed. Note that in practice nothing has changed in how errors are processed by this PR - previously we were mapping errors fromverify_narwhal_transactionintoNarwhalHandlerError::Skip, and now we just call it directly fromConsensusHandler.(4) Two utilization metrics on the
ConsensusHandlerside, namelyverify_narwhal_transaction_duration_mcsandhandle_consensus_duration_mcsslightly changed what they measure - previously verification metric was fraction of handle_consensus timer, now they are two independent steps. The total consensus utilization can now be measured as sum of those two metrics (until parallel verification is introduced).On Narwhal side:
(1) Minor interface change - pass
Arc<ConsensusOutput>instead of&ConsensusOutputfrom the narwhal executor - this will allow caller to cheaply clone Arc when it needs to pass it to verification thread