Skip to content

Commit

Permalink
[narwhal] swap PublicKey usage with index approach (MystenLabs#9576)
Browse files Browse the repository at this point in the history
## Description 

Resolves: MystenLabs#9348

Given the latest findings that dictate significant
serialisation/deserialisation cost for Narwhal entities (Certificate,
Header etc) that bear `PublicKey` values (as the EC point conversions
appear to be costly) , this PR is swapping the use of them with
something much more lightweight that will still allow us to
deterministically identify the authorities. Basically wherever we would
use a `PublicKey` we instead use an `AuthorityIdentifier` . This
uniquely identifies an authority but is cheap is terms of ser/des. For
the Narwhal purposes we chose `u16` for this type which is large enough
to identify the authorities in our network , but also small an simple
enough to serialisation costs.

As this is a quite large PR (although mostly swap changes have taken
place - not much of new functionality) I would advise to look first at:

* The Committee changes . It has been extracted from the
`narwhal/config/src/lib.rs` and is now on an independent file
[narwhal/config/src/committee.rs](https://github.com/MystenLabs/sui/pull/9576/files#r1144796971).
I've made both the `Authority` and `Committee` properties `private` and
now everything should be accessed via public methods. This was a
conscious decision to ensure safety since some values of both Authority
and Committee get initialised after construction.
* Introduced the
[CommitteeBuilder](https://github.com/MystenLabs/sui/pull/9576/files#diff-bf39a5ebab429076e198116328c16f4d2d6176154e4d649250ff9cc00e93c8c3R406)
which from now on should be the de-facto way of building a Committee.
Again for safety reasons - to ensure that when a user accesses an
Authority it will be properly initialised.
* The
[AuthorityIdentifier](https://github.com/MystenLabs/sui/pull/9576/files#diff-bf39a5ebab429076e198116328c16f4d2d6176154e4d649250ff9cc00e93c8c3R143)
is chosen to be a simple `u16` value. We follow the natural sorting of
the `PublicKey` to assign the corresponding incremental id value. It has
to be noted that the implementation is generic enough, so if we need in
the future to change the type we can easily do without touching any
other part of the code (ex making the `AuthorityIdentifier =
AuthorityPublicKeyBytes` - it would just work). It's easy-peasy to
derive the authority's details by just querying the
[committee](https://github.com/MystenLabs/sui/pull/9576/files#diff-bf39a5ebab429076e198116328c16f4d2d6176154e4d649250ff9cc00e93c8c3R160)
by whatever identifier we have defined.
* I've deliberately not touched the `WorkerCache` where we still
identify the workers by the authority's `PublicKey` - to note that there
is not conversion taken place here - I might address as separate PR to
ensure uniformity, avoided doing for now to reduce the scope of the PR
* I've done a small refactoring on the way we used the `name` before -
now we are using the term `authority_id` to decouple the semantics
(feedback is needed here). Also in some places , like the
[primary.rs](https://github.com/MystenLabs/sui/pull/9576/files#diff-ea7fbf16c23e1a2466afa04456a57c35115d6b406eb2897821be4bfc83673a3aR102)
,
[worker.rs](https://github.com/MystenLabs/sui/pull/9576/files#diff-6fec58b362511100ea42543a8030e7f11bbc476202f47ebd9589ef690463cc32R54)
I am now passing "our" whole `Authority` object which bears the full set
of details - so we have access to variety of properties and we can
easily derive things like the authority's `id` , or `public_key` etc. We
could keep this as a practice across our narwhal codebase instead of
just injecting the `id` - no additional cost here.
* Things might be a bit ugly on the interface with SUI
([authority_scoring](https://github.com/MystenLabs/sui/pull/9576/files#diff-3ed6074eee064ff17c26fa5b98012562d6f430beeb739cf93ed45d855bad246fR57)
,
[consensus_handler](https://github.com/MystenLabs/sui/pull/9576/files#diff-8ccde7467e1b7899685cdb5469dc530bc63b1945339660881ad1267287d4e2e7R204)
as we need to do the necessary conversions by consulting the `narwhal
Comittee` in order to translate the `AuthorityIdentifier` to the
`AuthorityName` that SUI uses - but I tried to make things as nice as
possible.

## Test Plan 

Updated all the existing unit & integration tests.

---
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)

- [ ] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [x] 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
  • Loading branch information
akichidis authored Mar 23, 2023
1 parent 3822cb4 commit e20cef1
Show file tree
Hide file tree
Showing 77 changed files with 1,937 additions and 1,575 deletions.
3 changes: 3 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/sui-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ mysten-metrics = { path = "../mysten-metrics" }
narwhal-config = { path = "../../narwhal/config" }
narwhal-executor = { path = "../../narwhal/executor" }
narwhal-node = { path = "../../narwhal/node" }
narwhal-crypto = { path = "../../narwhal/crypto" }
narwhal-types = { path = "../../narwhal/types" }
narwhal-worker = { path = "../../narwhal/worker" }
telemetry-subscribers.workspace = true
Expand Down
8 changes: 4 additions & 4 deletions crates/sui-core/src/authority/authority_per_epoch_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1514,7 +1514,8 @@ impl AuthorityPerEpochStore {
) -> SuiResult<Option<VerifiedExecutableTransaction>> {
let _scope = monitored_scope("HandleConsensusTransaction");
let VerifiedSequencedConsensusTransaction(SequencedConsensusTransaction {
certificate: consensus_output,
certificate: _consensus_output,
certificate_author,
consensus_index,
transaction,
}) = transaction;
Expand All @@ -1533,13 +1534,12 @@ impl AuthorityPerEpochStore {
);
return Ok(None);
}
let authority = (&consensus_output.header.author).into();
if self.has_sent_end_of_publish(&authority)? {
if self.has_sent_end_of_publish(&certificate_author)? {
// This can not happen with valid authority
// With some edge cases narwhal might sometimes resend previously seen certificate after EndOfPublish
// However this certificate will be filtered out before this line by `consensus_message_processed` call in `verify_consensus_transaction`
// If we see some new certificate here it means authority is byzantine and sent certificate after EndOfPublish (or we have some bug in ConsensusAdapter)
warn!("[Byzantine authority] Authority {:?} sent a new, previously unseen certificate {:?} after it sent EndOfPublish message to consensus", authority.concise(), certificate.digest());
warn!("[Byzantine authority] Authority {:?} sent a new, previously unseen certificate {:?} after it sent EndOfPublish message to consensus", certificate_author.concise(), certificate.digest());
return Ok(None);
}
// Safe because signatures are verified when VerifiedSequencedConsensusTransaction
Expand Down
27 changes: 20 additions & 7 deletions crates/sui-core/src/consensus_handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@ use crate::scoring_decision::update_low_scoring_authorities;
use crate::transaction_manager::TransactionManager;
use arc_swap::ArcSwap;
use async_trait::async_trait;
use fastcrypto::traits::ToFromBytes;
use lru::LruCache;
use mysten_metrics::{monitored_scope, spawn_monitored_task};
use narwhal_config::Committee;
use narwhal_executor::{ExecutionIndices, ExecutionState};
use narwhal_types::ConsensusOutput;
use parking_lot::Mutex;
Expand All @@ -23,7 +25,6 @@ use std::hash::{Hash, Hasher};
use std::num::NonZeroUsize;
use std::sync::Arc;
use sui_types::base_types::{AuthorityName, EpochId, TransactionDigest};
use sui_types::committee::Committee;
use sui_types::messages::{
ConsensusTransaction, ConsensusTransactionKey, ConsensusTransactionKind,
VerifiedExecutableTransaction, VerifiedTransaction,
Expand All @@ -44,10 +45,10 @@ pub struct ConsensusHandler<T> {
parent_sync_store: T,
/// Reputation scores used by consensus adapter that we update, forwarded from consensus
low_scoring_authorities: Arc<ArcSwap<HashMap<AuthorityName, u64>>>,
/// The committee used to do stake computations for deciding set of low scoring authorities
committee: Arc<Committee>,
/// Mappings used for logging and metrics
authority_names_to_peer_ids: Arc<HashMap<AuthorityName, PeerId>>,
/// The narwhal committee used to do stake computations for deciding set of low scoring authorities
committee: Committee,
// TODO: ConsensusHandler doesn't really share metrics with AuthorityState. We could define
// a new metrics type here if we want to.
metrics: Arc<AuthorityMetrics>,
Expand All @@ -65,8 +66,8 @@ impl<T> ConsensusHandler<T> {
transaction_manager: Arc<TransactionManager>,
parent_sync_store: T,
low_scoring_authorities: Arc<ArcSwap<HashMap<AuthorityName, u64>>>,
committee: Arc<Committee>,
authority_names_to_peer_ids: Arc<HashMap<AuthorityName, PeerId>>,
committee: Committee,
metrics: Arc<AuthorityMetrics>,
) -> Self {
let last_seen = Mutex::new(Default::default());
Expand Down Expand Up @@ -144,7 +145,7 @@ impl<T: ParentSync + Send + Sync> ExecutionState for ConsensusHandler<T> {
// TODO: spawn a separate task for this as an optimization
update_low_scoring_authorities(
self.low_scoring_authorities.clone(),
self.committee.clone(),
&self.committee,
consensus_output.sub_dag.reputation_score.clone(),
self.authority_names_to_peer_ids.clone(),
&self.metrics,
Expand All @@ -155,7 +156,7 @@ impl<T: ParentSync + Send + Sync> ExecutionState for ConsensusHandler<T> {
.with_label_values(&[&consensus_output.sub_dag.leader.header.author.to_string()])
.inc();
for (cert, batches) in consensus_output.batches {
let author = cert.header.author.clone();
let author = cert.header.author;
self.metrics
.consensus_committed_certificates
.with_label_values(&[&author.to_string()])
Expand Down Expand Up @@ -207,8 +208,18 @@ impl<T: ParentSync + Send + Sync> ExecutionState for ConsensusHandler<T> {
}
};

let certificate_author = AuthorityName::from_bytes(
self.committee
.authority_safe(&output_cert.header.author)
.protocol_key_bytes()
.0
.as_ref(),
)
.unwrap();

sequenced_transactions.push(SequencedConsensusTransaction {
certificate: output_cert.clone(),
certificate_author,
consensus_index: index_with_hash,
transaction,
});
Expand Down Expand Up @@ -341,6 +352,7 @@ fn classify(transaction: &ConsensusTransaction) -> &'static str {

pub struct SequencedConsensusTransaction {
pub certificate: Arc<narwhal_types::Certificate>,
pub certificate_author: AuthorityName,
pub consensus_index: ExecutionIndicesWithHash,
pub transaction: SequencedConsensusTransactionKind,
}
Expand Down Expand Up @@ -385,7 +397,7 @@ impl SequencedConsensusTransactionKind {

impl SequencedConsensusTransaction {
pub fn sender_authority(&self) -> AuthorityName {
(&self.certificate.header.author).into()
self.certificate_author
}

pub fn key(&self) -> SequencedConsensusTransactionKey {
Expand All @@ -408,6 +420,7 @@ impl SequencedConsensusTransaction {
Self {
transaction: SequencedConsensusTransactionKind::External(transaction),
certificate: Default::default(),
certificate_author: AuthorityName::ZERO,
consensus_index: Default::default(),
}
}
Expand Down
Loading

0 comments on commit e20cef1

Please sign in to comment.