diff --git a/Cargo.lock b/Cargo.lock index eb02b1007a..49426b4f4d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2187,7 +2187,6 @@ dependencies = [ "proc-macro2", "quote", "syn 2.0.85", - "unicode-xid", ] [[package]] @@ -3440,7 +3439,6 @@ dependencies = [ "committable", "custom_debug", "derivative", - "derive_more 1.0.0", "digest 0.10.7", "displaydoc", "dyn-clone 1.0.17 (git+https://github.com/dtolnay/dyn-clone?tag=1.0.17)", diff --git a/crates/example-types/src/block_types.rs b/crates/example-types/src/block_types.rs index 05ff656ecf..dc5d5f2045 100644 --- a/crates/example-types/src/block_types.rs +++ b/crates/example-types/src/block_types.rs @@ -370,7 +370,7 @@ impl< } fn payload_commitment(&self) -> VidCommitment { - self.payload_commitment.clone() + self.payload_commitment } fn metadata(&self) -> &>::Metadata { @@ -395,7 +395,9 @@ impl Committable for TestBlockHeader { ) .constant_str("payload commitment") .fixed_size_bytes( - >::payload_commitment(self).as_ref(), + >::payload_commitment(self) + .as_ref() + .as_ref(), ) .finalize() } diff --git a/crates/task-impls/src/da.rs b/crates/task-impls/src/da.rs index c9fab1c7e3..bada6f4ddf 100644 --- a/crates/task-impls/src/da.rs +++ b/crates/task-impls/src/da.rs @@ -200,7 +200,7 @@ impl, V: Versions> DaTaskState HandleDepOutput for ProposalDependencyHandle< auction_result, ) => { commit_and_metadata = Some(CommitmentAndMetadata { - commitment: payload_commitment.clone(), + commitment: *payload_commitment, builder_commitment: builder_commitment.clone(), metadata: metadata.clone(), fees: fees.clone(), diff --git a/crates/task-impls/src/quorum_vote/mod.rs b/crates/task-impls/src/quorum_vote/mod.rs index a25b6d2213..f78bf15b59 100644 --- a/crates/task-impls/src/quorum_vote/mod.rs +++ b/crates/task-impls/src/quorum_vote/mod.rs @@ -310,7 +310,7 @@ impl + 'static, V: Versions> Handl return; } } else { - payload_commitment = Some(cert_payload_comm.clone()); + payload_commitment = Some(*cert_payload_comm); } } HotShotEvent::VidShareValidated(share) => { @@ -322,7 +322,7 @@ impl + 'static, V: Versions> Handl return; } } else { - payload_commitment = Some(vid_payload_commitment.clone()); + payload_commitment = Some(*vid_payload_commitment); } } _ => {} diff --git a/crates/task-impls/src/transactions.rs b/crates/task-impls/src/transactions.rs index 0e82920930..69ea07f1a5 100644 --- a/crates/task-impls/src/transactions.rs +++ b/crates/task-impls/src/transactions.rs @@ -297,9 +297,7 @@ impl, V: Versions> TransactionTask self.builder_timeout.saturating_sub(start.elapsed()), async { let client = BuilderClientMarketplace::new(url); - client - .bundle(*parent_view, parent_hash.clone(), *block_view) - .await + client.bundle(*parent_view, parent_hash, *block_view).await }, )); } @@ -539,7 +537,7 @@ impl, V: Versions> TransactionTask match &view_data.view_inner { ViewInner::Da { payload_commitment } => { - return Ok((target_view, payload_commitment.clone())) + return Ok((target_view, *payload_commitment)) } ViewInner::Leaf { leaf: leaf_commitment, @@ -590,7 +588,7 @@ impl, V: Versions> TransactionTask match async_timeout( self.builder_timeout .saturating_sub(task_start_time.elapsed()), - self.block_from_builder(parent_comm.clone(), parent_view, &parent_comm_sig), + self.block_from_builder(parent_comm, parent_view, &parent_comm_sig), ) .await { @@ -631,23 +629,20 @@ impl, V: Versions> TransactionTask .builder_clients .iter() .enumerate() - .map(|(builder_idx, client)| { - let parent_comm = parent_comm.clone(); - async move { - client - .available_blocks( - parent_comm, - view_number.u64(), - self.public_key.clone(), - parent_comm_sig, - ) - .await - .map(move |blocks| { - blocks - .into_iter() - .map(move |block_info| (block_info, builder_idx)) - }) - } + .map(|(builder_idx, client)| async move { + client + .available_blocks( + parent_comm, + view_number.u64(), + self.public_key.clone(), + parent_comm_sig, + ) + .await + .map(move |blocks| { + blocks + .into_iter() + .map(move |block_info| (block_info, builder_idx)) + }) }) .collect::>(); let mut results = Vec::with_capacity(self.builder_clients.len()); diff --git a/crates/task-impls/src/vid.rs b/crates/task-impls/src/vid.rs index 500170cc9c..68afabf6d0 100644 --- a/crates/task-impls/src/vid.rs +++ b/crates/task-impls/src/vid.rs @@ -87,7 +87,7 @@ impl> VidTaskState { vid_precompute.clone(), ) .await; - let payload_commitment = vid_disperse.payload_commitment.clone(); + let payload_commitment = vid_disperse.payload_commitment; let shares = VidDisperseShare::from_vid_disperse(vid_disperse.clone()); let mut consensus_writer = self.consensus.write().await; for share in shares { diff --git a/crates/testing/tests/tests_1/da_task.rs b/crates/testing/tests/tests_1/da_task.rs index 28a35bbe0b..ad417a0431 100644 --- a/crates/testing/tests/tests_1/da_task.rs +++ b/crates/testing/tests/tests_1/da_task.rs @@ -69,13 +69,8 @@ async fn test_da_task() { proposals.push(view.da_proposal.clone()); leaders.push(view.leader_public_key); votes.push( - view.create_da_vote( - DaData { - payload_commit: payload_commit.clone(), - }, - &handle, - ) - .await, + view.create_da_vote(DaData { payload_commit }, &handle) + .await, ); dacs.push(view.da_certificate.clone()); vids.push(view.vid_proposal.clone()); @@ -87,13 +82,8 @@ async fn test_da_task() { proposals.push(view.da_proposal.clone()); leaders.push(view.leader_public_key); votes.push( - view.create_da_vote( - DaData { - payload_commit: payload_commit.clone(), - }, - &handle, - ) - .await, + view.create_da_vote(DaData { payload_commit }, &handle) + .await, ); dacs.push(view.da_certificate.clone()); vids.push(view.vid_proposal.clone()); @@ -180,13 +170,8 @@ async fn test_da_task_storage_failure() { proposals.push(view.da_proposal.clone()); leaders.push(view.leader_public_key); votes.push( - view.create_da_vote( - DaData { - payload_commit: payload_commit.clone(), - }, - &handle, - ) - .await, + view.create_da_vote(DaData { payload_commit }, &handle) + .await, ); dacs.push(view.da_certificate.clone()); vids.push(view.vid_proposal.clone()); @@ -198,13 +183,8 @@ async fn test_da_task_storage_failure() { proposals.push(view.da_proposal.clone()); leaders.push(view.leader_public_key); votes.push( - view.create_da_vote( - DaData { - payload_commit: payload_commit.clone(), - }, - &handle, - ) - .await, + view.create_da_vote(DaData { payload_commit }, &handle) + .await, ); dacs.push(view.da_certificate.clone()); vids.push(view.vid_proposal.clone()); diff --git a/crates/testing/tests/tests_1/vid_task.rs b/crates/testing/tests/tests_1/vid_task.rs index b5ef8335f7..c2ca9aec09 100644 --- a/crates/testing/tests/tests_1/vid_task.rs +++ b/crates/testing/tests/tests_1/vid_task.rs @@ -69,7 +69,7 @@ async fn test_vid_task() { let encoded_transactions = Arc::from(TestTransaction::encode(&transactions)); let vid_disperse = vid.disperse(&encoded_transactions).unwrap(); let (_, vid_precompute) = vid.commit_only_precompute(&encoded_transactions).unwrap(); - let payload_commitment = vid_disperse.commit.clone(); + let payload_commitment = vid_disperse.commit; let signature = ::SignatureKey::sign( handle.private_key(), diff --git a/crates/types/Cargo.toml b/crates/types/Cargo.toml index 25d47d7d9e..e9ad6368b5 100644 --- a/crates/types/Cargo.toml +++ b/crates/types/Cargo.toml @@ -38,7 +38,6 @@ time = { workspace = true } tracing = { workspace = true } typenum = { workspace = true } derivative = "2" -derive_more = { workspace = true, features = ["display"] } # TODO promote display feature to workspace? jf-vid = { workspace = true } jf-pcs = { workspace = true } jf-signature = { workspace = true, features = ["schnorr"] } diff --git a/crates/types/src/data.rs b/crates/types/src/data.rs index 5528a1835c..db891c8a0b 100644 --- a/crates/types/src/data.rs +++ b/crates/types/src/data.rs @@ -282,7 +282,7 @@ impl VidDisperseShare { recipient_key, view_number: vid_disperse.view_number, common: vid_disperse.common.clone(), - payload_commitment: vid_disperse.payload_commitment.clone(), + payload_commitment: vid_disperse.payload_commitment, }) .collect() } @@ -345,7 +345,7 @@ impl VidDisperseShare { recipient_key, view_number: vid_disperse_proposal.data.view_number, common: vid_disperse_proposal.data.common.clone(), - payload_commitment: vid_disperse_proposal.data.payload_commitment.clone(), + payload_commitment: vid_disperse_proposal.data.payload_commitment, }, signature: vid_disperse_proposal.signature.clone(), _pd: vid_disperse_proposal._pd, diff --git a/crates/types/src/utils.rs b/crates/types/src/utils.rs index 699d1a3830..e3d19a8286 100644 --- a/crates/types/src/utils.rs +++ b/crates/types/src/utils.rs @@ -58,7 +58,7 @@ impl Clone for ViewInner { fn clone(&self) -> Self { match self { Self::Da { payload_commitment } => Self::Da { - payload_commitment: payload_commitment.clone(), + payload_commitment: *payload_commitment, }, Self::Leaf { leaf, state, delta } => Self::Leaf { leaf: *leaf, @@ -123,7 +123,7 @@ impl ViewInner { #[must_use] pub fn payload_commitment(&self) -> Option { if let Self::Da { payload_commitment } = self { - Some(payload_commitment.clone()) + Some(*payload_commitment) } else { None } diff --git a/crates/types/src/vid.rs b/crates/types/src/vid.rs index 8e936f7416..7d3bfb21a7 100644 --- a/crates/types/src/vid.rs +++ b/crates/types/src/vid.rs @@ -4,20 +4,20 @@ // You should have received a copy of the MIT License // along with the HotShot repository. If not, see . -//! This module provides an opaque constructor [`vid_scheme`] that returns a new -//! instance of a VID scheme. +//! This module provides: +//! - an opaque constructor [`vid_scheme`] that returns a new instance of a +//! VID scheme. +//! - type aliases [`VidCommitment`], [`VidCommon`], [`VidShare`] +//! for [`VidScheme`] assoc types. //! -//! Purpose: the specific choice of VID scheme is an implementation detail. We -//! want all communication with the VID scheme to occur only via the API exposed -//! in the [`VidScheme`] trait, but limitations of Rust make it difficult to -//! achieve this level of abstraction. Hence, there's a lot of boilerplate code -//! in this module. +//! Purpose: the specific choice of VID scheme is an implementation detail. +//! This crate and all downstream crates should talk to the VID scheme only +//! via the traits exposed here. #![allow(missing_docs)] use std::{fmt::Debug, ops::Range}; use ark_bn254::Bn254; -use derive_more::Display; use jf_pcs::{ prelude::{UnivariateKzgPCS, UnivariateUniversalParams}, PolynomialCommitmentScheme, @@ -34,7 +34,6 @@ use jf_vid::{ use lazy_static::lazy_static; use serde::{Deserialize, Serialize}; use sha2::Sha256; -use tagged_base64::TaggedBase64; use crate::{ constants::SRS_DEGREE, @@ -104,6 +103,14 @@ pub fn vid_scheme_for_test(num_storage_nodes: usize) -> VidSchemeType { ) } +/// VID commitment type +pub type VidCommitment = ::Commit; +/// VID common type +pub type VidCommon = ::Common; +/// VID share type +pub type VidShare = ::Share; +/// VID PrecomputeData type +pub type VidPrecomputeData = ::PrecomputeData; /// VID proposal type pub type VidProposal = ( Proposal>, @@ -122,22 +129,6 @@ type Advz = advz::AdvzGPU<'static, E, H>; #[derive(Clone)] pub struct VidSchemeType(Advz); -/// Newtype wrapper for assoc type [`VidScheme::Commit`]. -#[derive(Clone, Debug, Deserialize, Display, Eq, PartialEq, Hash, Serialize)] -pub struct VidCommitment(::Commit); - -/// Newtype wrapper for assoc type [`VidScheme::Common`]. -#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Hash, Serialize)] -pub struct VidCommon(::Common); - -/// Newtype wrapper for assoc type [`VidScheme::Share`]. -#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Hash, Serialize)] -pub struct VidShare(::Share); - -/// Newtype wrapper for assoc type [`Precomputable::PrecomputeData`]. -#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Hash, Serialize)] -pub struct VidPrecomputeData(::PrecomputeData); - /// Newtype wrapper for a large payload range proof. /// /// Useful for namespace proofs. @@ -207,15 +198,15 @@ type H = Sha256; // type alias impl trait (TAIT): // [rfcs/text/2515-type_alias_impl_trait.md at master ยท rust-lang/rfcs](https://github.com/rust-lang/rfcs/blob/master/text/2515-type_alias_impl_trait.md) impl VidScheme for VidSchemeType { - type Commit = VidCommitment; - type Share = VidShare; - type Common = VidCommon; + type Commit = ::Commit; + type Share = ::Share; + type Common = ::Common; fn commit_only(&mut self, payload: B) -> VidResult where B: AsRef<[u8]>, { - self.0.commit_only(payload).map(VidCommitment) + self.0.commit_only(payload) } fn disperse(&mut self, payload: B) -> VidResult> @@ -231,32 +222,27 @@ impl VidScheme for VidSchemeType { common: &Self::Common, commit: &Self::Commit, ) -> VidResult> { - self.0.verify_share(&share.0, &common.0, &commit.0) + self.0.verify_share(share, common, commit) } fn recover_payload(&self, shares: &[Self::Share], common: &Self::Common) -> VidResult> { - // TODO costly Vec copy. Is the compiler smart enough to optimize away - // this Vec, or shall we use unsafe code to cast `shares`? - // It's only `recover_payload` so who cares? - let shares: Vec<_> = shares.iter().map(|s| s.0.clone()).collect(); - - self.0.recover_payload(&shares, &common.0) + self.0.recover_payload(shares, common) } fn is_consistent(commit: &Self::Commit, common: &Self::Common) -> VidResult<()> { - ::is_consistent(&commit.0, &common.0) + ::is_consistent(commit, common) } fn get_payload_byte_len(common: &Self::Common) -> u32 { - ::get_payload_byte_len(&common.0) + ::get_payload_byte_len(common) } fn get_num_storage_nodes(common: &Self::Common) -> u32 { - ::get_num_storage_nodes(&common.0) + ::get_num_storage_nodes(common) } fn get_multiplicity(common: &Self::Common) -> u32 { - ::get_multiplicity(&common.0) + ::get_multiplicity(common) } /// Helper function for testing only @@ -266,33 +252,6 @@ impl VidScheme for VidSchemeType { } } -impl From for TaggedBase64 { - fn from(value: VidCommitment) -> Self { - TaggedBase64::from(value.0) - } -} - -impl<'a> TryFrom<&'a TaggedBase64> for VidCommitment { - type Error = <::Commit as TryFrom<&'a TaggedBase64>>::Error; - - fn try_from(value: &'a TaggedBase64) -> Result { - ::Commit::try_from(value).map(Self) - } -} - -// TODO add AsRef trait bound upstream. -// -// We impl `AsRef<[u8; _]>` instead of `AsRef<[u8]>` to maintain backward -// compatibility for downstream code that re-hashes the `VidCommitment`. -// Specifically, if we support only `AsRef<[u8]>` then code that uses -// `Committable` must switch `fixed_size_bytes` to `var_size_bytes`, which -// prepends length data into the hash, thus changing the hash. -impl AsRef<[u8; 32]> for VidCommitment { - fn as_ref(&self) -> &[u8; 32] { - self.0.as_ref().as_ref() - } -} - impl PayloadProver for VidSchemeType { fn payload_proof(&self, payload: B, range: Range) -> VidResult where @@ -332,7 +291,7 @@ impl PayloadProver for VidSchemeType { } impl Precomputable for VidSchemeType { - type PrecomputeData = VidPrecomputeData; + type PrecomputeData = ::PrecomputeData; fn commit_only_precompute( &self, @@ -341,9 +300,7 @@ impl Precomputable for VidSchemeType { where B: AsRef<[u8]>, { - self.0 - .commit_only_precompute(payload) - .map(|r| (VidCommitment(r.0), VidPrecomputeData(r.1))) + self.0.commit_only_precompute(payload) } fn disperse_precompute( @@ -355,7 +312,7 @@ impl Precomputable for VidSchemeType { B: AsRef<[u8]>, { self.0 - .disperse_precompute(payload, &data.0) + .disperse_precompute(payload, data) .map(vid_disperse_conversion) } } @@ -368,14 +325,10 @@ impl Precomputable for VidSchemeType { /// and similarly for `Statement`. /// Thus, we accomplish type conversion via functions. fn vid_disperse_conversion(vid_disperse: VidDisperse) -> VidDisperse { - // TODO costly Vec copy. Is the compiler smart enough to optimize away - // this Vec, or shall we use unsafe code to cast `shares`? - let shares = vid_disperse.shares.into_iter().map(VidShare).collect(); - VidDisperse { - shares, - common: VidCommon(vid_disperse.common), - commit: VidCommitment(vid_disperse.commit), + shares: vid_disperse.shares, + common: vid_disperse.common, + commit: vid_disperse.commit, } } @@ -384,7 +337,7 @@ fn stmt_conversion(stmt: Statement<'_, VidSchemeType>) -> Statement<'_, Advz> { Statement { payload_subslice: stmt.payload_subslice, range: stmt.range, - commit: &stmt.commit.0, - common: &stmt.common.0, + commit: stmt.commit, + common: stmt.common, } }