From 1097d48d6f217d2662e497408570c1adf4df0557 Mon Sep 17 00:00:00 2001 From: guy-starkware Date: Sun, 10 Nov 2024 01:50:50 -0800 Subject: [PATCH] chore: unify the two types of ProposalInit structs (#1792) --- crates/papyrus_protobuf/src/consensus.rs | 32 +++++++++---------- .../src/converters/consensus.rs | 6 ++-- .../src/converters/test_instances.rs | 4 +-- .../papyrus_consensus/src/manager.rs | 16 ++++------ .../papyrus_consensus/src/manager_test.rs | 11 ++----- .../src/single_height_consensus.rs | 3 +- .../src/single_height_consensus_test.rs | 4 +-- .../papyrus_consensus/src/test_utils.rs | 11 ++----- .../sequencing/papyrus_consensus/src/types.rs | 17 +--------- .../src/papyrus_consensus_context.rs | 3 +- .../src/papyrus_consensus_context_test.rs | 4 +-- .../src/sequencer_consensus_context.rs | 13 ++------ .../src/sequencer_consensus_context_test.rs | 3 +- 13 files changed, 43 insertions(+), 84 deletions(-) diff --git a/crates/papyrus_protobuf/src/consensus.rs b/crates/papyrus_protobuf/src/consensus.rs index 7dbd661ab9..d4156ce270 100644 --- a/crates/papyrus_protobuf/src/consensus.rs +++ b/crates/papyrus_protobuf/src/consensus.rs @@ -61,10 +61,10 @@ pub struct StreamMessage> + TryFrom, Error = ProtobufCon } /// This message must be sent first when proposing a new block. -#[derive(Debug, Clone, PartialEq)] +#[derive(Default, Debug, Clone, PartialEq)] pub struct ProposalInit { /// The height of the consensus (block number). - pub height: u64, + pub height: BlockNumber, /// The current round of the consensus. pub round: u32, /// The last round that was valid. @@ -129,16 +129,16 @@ where pub struct ProposalWrapper(pub Proposal); impl From - for ( - (BlockNumber, u32, ContractAddress, Option), - mpsc::Receiver, - oneshot::Receiver, - ) + for (ProposalInit, mpsc::Receiver, oneshot::Receiver) { fn from(val: ProposalWrapper) -> Self { let transactions: Vec = val.0.transactions.into_iter().collect(); - let proposal_init = - (BlockNumber(val.0.height), val.0.round, val.0.proposer, val.0.valid_round); + let proposal_init = ProposalInit { + height: BlockNumber(val.0.height), + round: val.0.round, + proposer: val.0.proposer, + valid_round: val.0.valid_round, + }; let (mut content_sender, content_receiver) = mpsc::channel(transactions.len()); for tx in transactions { content_sender.try_send(tx).expect("Send should succeed"); @@ -153,15 +153,15 @@ impl From } impl From - for ( - (BlockNumber, u32, ContractAddress, Option), - mpsc::Receiver>, - oneshot::Receiver, - ) + for (ProposalInit, mpsc::Receiver>, oneshot::Receiver) { fn from(val: ProposalWrapper) -> Self { - let proposal_init = - (BlockNumber(val.0.height), val.0.round, val.0.proposer, val.0.valid_round); + let proposal_init = ProposalInit { + height: BlockNumber(val.0.height), + round: val.0.round, + proposer: val.0.proposer, + valid_round: val.0.valid_round, + }; let (_, content_receiver) = mpsc::channel(0); // This should only be used for Milestone 1, and then removed once streaming is supported. diff --git a/crates/papyrus_protobuf/src/converters/consensus.rs b/crates/papyrus_protobuf/src/converters/consensus.rs index 72d2c5858e..ad80418e65 100644 --- a/crates/papyrus_protobuf/src/converters/consensus.rs +++ b/crates/papyrus_protobuf/src/converters/consensus.rs @@ -4,7 +4,7 @@ mod consensus_test; use std::convert::{TryFrom, TryInto}; use prost::Message; -use starknet_api::block::BlockHash; +use starknet_api::block::{BlockHash, BlockNumber}; use starknet_api::hash::StarkHash; use starknet_api::transaction::Transaction; @@ -209,14 +209,14 @@ impl TryFrom for ProposalInit { .proposer .ok_or(ProtobufConversionError::MissingField { field_description: "proposer" })? .try_into()?; - Ok(ProposalInit { height, round, valid_round, proposer }) + Ok(ProposalInit { height: BlockNumber(height), round, valid_round, proposer }) } } impl From for protobuf::ProposalInit { fn from(value: ProposalInit) -> Self { protobuf::ProposalInit { - height: value.height, + height: value.height.0, round: value.round, valid_round: value.valid_round, proposer: Some(value.proposer.into()), diff --git a/crates/papyrus_protobuf/src/converters/test_instances.rs b/crates/papyrus_protobuf/src/converters/test_instances.rs index 57a787320d..742349b973 100644 --- a/crates/papyrus_protobuf/src/converters/test_instances.rs +++ b/crates/papyrus_protobuf/src/converters/test_instances.rs @@ -1,6 +1,6 @@ use papyrus_test_utils::{auto_impl_get_test_instance, get_number_of_variants, GetTestInstance}; use rand::Rng; -use starknet_api::block::BlockHash; +use starknet_api::block::{BlockHash, BlockNumber}; use starknet_api::core::ContractAddress; use starknet_api::transaction::Transaction; @@ -42,7 +42,7 @@ auto_impl_get_test_instance! { Precommit = 1, } pub struct ProposalInit { - pub height: u64, + pub height: BlockNumber, pub round: u32, pub valid_round: Option, pub proposer: ContractAddress, diff --git a/crates/sequencing/papyrus_consensus/src/manager.rs b/crates/sequencing/papyrus_consensus/src/manager.rs index 38da35c1f7..5dfa5c7ad5 100644 --- a/crates/sequencing/papyrus_consensus/src/manager.rs +++ b/crates/sequencing/papyrus_consensus/src/manager.rs @@ -12,9 +12,8 @@ use futures::stream::FuturesUnordered; use futures::{Stream, StreamExt}; use papyrus_common::metrics::{PAPYRUS_CONSENSUS_HEIGHT, PAPYRUS_CONSENSUS_SYNC_COUNT}; use papyrus_network::network_manager::BroadcastTopicClientTrait; -use papyrus_protobuf::consensus::{ConsensusMessage, ProposalWrapper}; +use papyrus_protobuf::consensus::{ConsensusMessage, ProposalInit, ProposalWrapper}; use starknet_api::block::{BlockHash, BlockNumber}; -use starknet_api::core::ContractAddress; use tracing::{debug, info, instrument}; use crate::config::TimeoutsConfig; @@ -43,11 +42,8 @@ pub async fn run_consensus( where ContextT: ConsensusContext, SyncReceiverT: Stream + Unpin, - ProposalWrapper: Into<( - (BlockNumber, u32, ContractAddress, Option), - mpsc::Receiver, - oneshot::Receiver, - )>, + ProposalWrapper: + Into<(ProposalInit, mpsc::Receiver, oneshot::Receiver)>, { info!( "Running consensus, start_height={}, validator_id={}, consensus_delay={}, timeouts={:?}", @@ -114,7 +110,7 @@ impl MultiHeightManager { where ContextT: ConsensusContext, ProposalWrapper: Into<( - (BlockNumber, u32, ContractAddress, Option), + ProposalInit, mpsc::Receiver, oneshot::Receiver, )>, @@ -171,7 +167,7 @@ impl MultiHeightManager { where ContextT: ConsensusContext, ProposalWrapper: Into<( - (BlockNumber, u32, ContractAddress, Option), + ProposalInit, mpsc::Receiver, oneshot::Receiver, )>, @@ -193,7 +189,7 @@ impl MultiHeightManager { let (proposal_init, content_receiver, fin_receiver) = ProposalWrapper(proposal).into(); let res = shc - .handle_proposal(context, proposal_init.into(), content_receiver, fin_receiver) + .handle_proposal(context, proposal_init, content_receiver, fin_receiver) .await?; Ok(res) } diff --git a/crates/sequencing/papyrus_consensus/src/manager_test.rs b/crates/sequencing/papyrus_consensus/src/manager_test.rs index e40de91845..3f94d78fb4 100644 --- a/crates/sequencing/papyrus_consensus/src/manager_test.rs +++ b/crates/sequencing/papyrus_consensus/src/manager_test.rs @@ -13,7 +13,7 @@ use papyrus_network::network_manager::test_utils::{ TestSubscriberChannels, }; use papyrus_network_types::network_types::BroadcastedMessageMetadata; -use papyrus_protobuf::consensus::{ConsensusMessage, Vote}; +use papyrus_protobuf::consensus::{ConsensusMessage, ProposalInit, Vote}; use papyrus_test_utils::{get_rng, GetTestInstance}; use starknet_api::block::{BlockHash, BlockNumber}; use starknet_api::transaction::Transaction; @@ -22,14 +22,7 @@ use starknet_types_core::felt::Felt; use super::{run_consensus, MultiHeightManager}; use crate::config::TimeoutsConfig; use crate::test_utils::{precommit, prevote, proposal}; -use crate::types::{ - ConsensusContext, - ConsensusError, - ProposalContentId, - ProposalInit, - Round, - ValidatorId, -}; +use crate::types::{ConsensusContext, ConsensusError, ProposalContentId, Round, ValidatorId}; lazy_static! { static ref PROPOSER_ID: ValidatorId = 0_u32.into(); diff --git a/crates/sequencing/papyrus_consensus/src/single_height_consensus.rs b/crates/sequencing/papyrus_consensus/src/single_height_consensus.rs index af5318b4f4..f2ee23c7d3 100644 --- a/crates/sequencing/papyrus_consensus/src/single_height_consensus.rs +++ b/crates/sequencing/papyrus_consensus/src/single_height_consensus.rs @@ -9,7 +9,7 @@ use std::time::Duration; #[cfg(test)] use enum_as_inner::EnumAsInner; use futures::channel::{mpsc, oneshot}; -use papyrus_protobuf::consensus::{ConsensusMessage, Vote, VoteType}; +use papyrus_protobuf::consensus::{ConsensusMessage, ProposalInit, Vote, VoteType}; use starknet_api::block::BlockNumber; use tracing::{debug, info, instrument, trace, warn}; @@ -20,7 +20,6 @@ use crate::types::{ ConsensusError, Decision, ProposalContentId, - ProposalInit, Round, ValidatorId, }; diff --git a/crates/sequencing/papyrus_consensus/src/single_height_consensus_test.rs b/crates/sequencing/papyrus_consensus/src/single_height_consensus_test.rs index 6758d09072..deb0d937f8 100644 --- a/crates/sequencing/papyrus_consensus/src/single_height_consensus_test.rs +++ b/crates/sequencing/papyrus_consensus/src/single_height_consensus_test.rs @@ -1,6 +1,6 @@ use futures::channel::{mpsc, oneshot}; use lazy_static::lazy_static; -use papyrus_protobuf::consensus::ConsensusMessage; +use papyrus_protobuf::consensus::{ConsensusMessage, ProposalInit}; use starknet_api::block::{BlockHash, BlockNumber}; use starknet_types_core::felt::Felt; use test_case::test_case; @@ -11,7 +11,7 @@ use crate::config::TimeoutsConfig; use crate::single_height_consensus::{ShcEvent, ShcReturn, ShcTask}; use crate::state_machine::StateMachineEvent; use crate::test_utils::{precommit, prevote, MockTestContext, TestBlock}; -use crate::types::{ConsensusError, ProposalInit, ValidatorId}; +use crate::types::{ConsensusError, ValidatorId}; lazy_static! { static ref PROPOSER_ID: ValidatorId = 0_u32.into(); diff --git a/crates/sequencing/papyrus_consensus/src/test_utils.rs b/crates/sequencing/papyrus_consensus/src/test_utils.rs index 21443a4be3..303e30a049 100644 --- a/crates/sequencing/papyrus_consensus/src/test_utils.rs +++ b/crates/sequencing/papyrus_consensus/src/test_utils.rs @@ -3,18 +3,11 @@ use std::time::Duration; use async_trait::async_trait; use futures::channel::{mpsc, oneshot}; use mockall::mock; -use papyrus_protobuf::consensus::{ConsensusMessage, Proposal, Vote, VoteType}; +use papyrus_protobuf::consensus::{ConsensusMessage, Proposal, ProposalInit, Vote, VoteType}; use starknet_api::block::{BlockHash, BlockNumber}; use starknet_types_core::felt::Felt; -use crate::types::{ - ConsensusContext, - ConsensusError, - ProposalContentId, - ProposalInit, - Round, - ValidatorId, -}; +use crate::types::{ConsensusContext, ConsensusError, ProposalContentId, Round, ValidatorId}; /// Define a consensus block which can be used to enable auto mocking Context. #[derive(Debug, PartialEq, Clone)] diff --git a/crates/sequencing/papyrus_consensus/src/types.rs b/crates/sequencing/papyrus_consensus/src/types.rs index d773df1ab4..6e31c011c6 100644 --- a/crates/sequencing/papyrus_consensus/src/types.rs +++ b/crates/sequencing/papyrus_consensus/src/types.rs @@ -9,7 +9,7 @@ use papyrus_network::network_manager::{ GenericReceiver, }; use papyrus_network_types::network_types::BroadcastedMessageMetadata; -use papyrus_protobuf::consensus::{ConsensusMessage, Vote}; +use papyrus_protobuf::consensus::{ConsensusMessage, ProposalInit, Vote}; use papyrus_protobuf::converters::ProtobufConversionError; use starknet_api::block::{BlockHash, BlockNumber}; use starknet_api::core::ContractAddress; @@ -118,21 +118,6 @@ impl Debug for Decision { } } -#[derive(PartialEq, Debug, Default, Clone)] -pub struct ProposalInit { - pub height: BlockNumber, - pub round: Round, - pub proposer: ValidatorId, - pub valid_round: Option, -} - -// TODO(Guy): Remove after implementing broadcast streams. -impl From<(BlockNumber, u32, ContractAddress, Option)> for ProposalInit { - fn from(val: (BlockNumber, u32, ContractAddress, Option)) -> Self { - ProposalInit { height: val.0, round: val.1, proposer: val.2, valid_round: val.3 } - } -} - pub struct BroadcastConsensusMessageChannel { pub broadcasted_messages_receiver: GenericReceiver<( Result, diff --git a/crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context.rs b/crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context.rs index 8092ce4207..5779d8a156 100644 --- a/crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context.rs +++ b/crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context.rs @@ -19,12 +19,11 @@ use papyrus_consensus::types::{ ConsensusContext, ConsensusError, ProposalContentId, - ProposalInit, Round, ValidatorId, }; use papyrus_network::network_manager::{BroadcastTopicClient, BroadcastTopicClientTrait}; -use papyrus_protobuf::consensus::{ConsensusMessage, Proposal, Vote}; +use papyrus_protobuf::consensus::{ConsensusMessage, Proposal, ProposalInit, Vote}; use papyrus_storage::body::BodyStorageReader; use papyrus_storage::header::HeaderStorageReader; use papyrus_storage::{StorageError, StorageReader}; diff --git a/crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context_test.rs b/crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context_test.rs index 4f4b82276a..9c7e5b9f96 100644 --- a/crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context_test.rs +++ b/crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context_test.rs @@ -2,12 +2,12 @@ use std::time::Duration; use futures::channel::{mpsc, oneshot}; use futures::StreamExt; -use papyrus_consensus::types::{ConsensusContext, ProposalInit}; +use papyrus_consensus::types::ConsensusContext; use papyrus_network::network_manager::test_utils::{ mock_register_broadcast_topic, BroadcastNetworkMock, }; -use papyrus_protobuf::consensus::{ConsensusMessage, Vote}; +use papyrus_protobuf::consensus::{ConsensusMessage, ProposalInit, Vote}; use papyrus_storage::body::BodyStorageWriter; use papyrus_storage::header::HeaderStorageWriter; use papyrus_storage::test_utils::get_test_storage; diff --git a/crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs b/crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs index 6bdb75cb51..40e23fe204 100644 --- a/crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs +++ b/crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs @@ -16,14 +16,13 @@ use papyrus_consensus::types::{ ConsensusContext, ConsensusError, ProposalContentId, - ProposalInit, Round, ValidatorId, }; use papyrus_network::network_manager::{BroadcastTopicClient, BroadcastTopicClientTrait}; use papyrus_protobuf::consensus::{ ConsensusMessage, - ProposalInit as ProtobufProposalInit, + ProposalInit, ProposalPart, TransactionBatch, Vote, @@ -125,15 +124,9 @@ impl ConsensusContext for SequencerConsensusContext { .build_proposal(build_proposal_input) .await .expect("Failed to initiate proposal build"); - let protobuf_consensus_init = ProtobufProposalInit { - height: proposal_init.height.0, - round: proposal_init.round, - proposer: proposal_init.proposer, - valid_round: proposal_init.valid_round, - }; - debug!("Broadcasting proposal init: {protobuf_consensus_init:?}"); + debug!("Broadcasting proposal init: {proposal_init:?}"); self.network_broadcast_client - .broadcast_message(ProposalPart::Init(protobuf_consensus_init)) + .broadcast_message(ProposalPart::Init(proposal_init.clone())) .await .expect("Failed to broadcast proposal init"); let broadcast_client = self.network_broadcast_client.clone(); diff --git a/crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context_test.rs b/crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context_test.rs index f1d36e8b7e..0006955f65 100644 --- a/crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context_test.rs +++ b/crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context_test.rs @@ -5,12 +5,13 @@ use std::vec; use futures::channel::mpsc; use futures::SinkExt; use lazy_static::lazy_static; -use papyrus_consensus::types::{ConsensusContext, ProposalInit}; +use papyrus_consensus::types::ConsensusContext; use papyrus_network::network_manager::test_utils::{ mock_register_broadcast_topic, TestSubscriberChannels, }; use papyrus_network::network_manager::BroadcastTopicChannels; +use papyrus_protobuf::consensus::ProposalInit; use starknet_api::block::{BlockHash, BlockNumber}; use starknet_api::core::{ContractAddress, StateDiffCommitment}; use starknet_api::executable_transaction::{AccountTransaction, Transaction};