From 01359f1e936a37278e6624c9dd5ac584e5493d4e Mon Sep 17 00:00:00 2001 From: Anca Zamfir Date: Wed, 12 Feb 2025 09:42:01 -0500 Subject: [PATCH] chore(code/staknet): Enforce parts-only configuration and mode for starknet app (#848) * Enforce parts-only configuration and mode for starknet app * Revert spawn.bash change * Small cleanup * Post-merge fixes * Remove vote extensions from starknet app --------- Co-authored-by: Romain Ruetschi --- code/crates/starknet/host/src/actor.rs | 57 +++++----------- .../crates/starknet/host/src/host/proposal.rs | 3 - .../crates/starknet/host/src/host/starknet.rs | 24 ------- code/crates/starknet/host/src/spawn.rs | 33 ++++----- code/crates/starknet/test/src/lib.rs | 4 +- code/crates/starknet/test/src/tests/mod.rs | 1 - .../test/src/tests/n3f0_consensus_mode.rs | 51 -------------- .../starknet/test/src/tests/value_sync.rs | 39 +---------- .../starknet/test/src/tests/vote_sync.rs | 39 +---------- code/crates/starknet/test/src/tests/wal.rs | 68 ++----------------- code/crates/test/framework/src/lib.rs | 2 +- code/crates/test/tests/it/main.rs | 2 +- 12 files changed, 43 insertions(+), 280 deletions(-) delete mode 100644 code/crates/starknet/test/src/tests/n3f0_consensus_mode.rs diff --git a/code/crates/starknet/host/src/actor.rs b/code/crates/starknet/host/src/actor.rs index e722516d0..eb12bedfb 100644 --- a/code/crates/starknet/host/src/actor.rs +++ b/code/crates/starknet/host/src/actor.rs @@ -11,9 +11,7 @@ use tokio::time::Instant; use tracing::{debug, error, info, trace, warn}; use malachitebft_core_consensus::{PeerId, VoteExtensionError}; -use malachitebft_core_types::{ - CommitCertificate, Round, Validity, ValueId, ValueOrigin, VoteExtensions, -}; +use malachitebft_core_types::{CommitCertificate, Round, Validity, ValueId, ValueOrigin}; use malachitebft_engine::consensus::{ConsensusMsg, ConsensusRef}; use malachitebft_engine::host::{LocallyProposedValue, ProposedValue}; use malachitebft_engine::network::{NetworkMsg, NetworkRef}; @@ -186,19 +184,9 @@ impl Host { HostMsg::Decided { certificate, - extensions, consensus, - } => { - on_decided( - state, - &consensus, - &self.mempool, - certificate, - extensions, - &self.metrics, - ) - .await - } + .. + } => on_decided(state, &consensus, &self.mempool, certificate, &self.metrics).await, HostMsg::GetDecidedValue { height, reply_to } => { on_get_decided_block(height, state, reply_to).await @@ -341,24 +329,20 @@ async fn on_get_value( while let Some(part) = rx_part.recv().await { state.host.part_store.store(height, round, part.clone()); - if state.host.params.value_payload.include_parts() { - debug!(%stream_id, %sequence, "Broadcasting proposal part"); + debug!(%stream_id, %sequence, "Broadcasting proposal part"); - let msg = StreamMessage::new( - stream_id.clone(), - sequence, - StreamContent::Data(part.clone()), - ); - network.cast(NetworkMsg::PublishProposalPart(msg))?; - } + let msg = StreamMessage::new( + stream_id.clone(), + sequence, + StreamContent::Data(part.clone()), + ); + network.cast(NetworkMsg::PublishProposalPart(msg))?; sequence += 1; } - if state.host.params.value_payload.include_parts() { - let msg = StreamMessage::new(stream_id, sequence, StreamContent::Fin); - network.cast(NetworkMsg::PublishProposalPart(msg))?; - } + let msg = StreamMessage::new(stream_id, sequence, StreamContent::Fin); + network.cast(NetworkMsg::PublishProposalPart(msg))?; let block_hash = rx_hash.await?; debug!(%block_hash, "Assembled block"); @@ -478,16 +462,13 @@ async fn on_restream_proposal( state.host.part_store.store(height, round, new_part.clone()); - if state.host.params.value_payload.include_parts() { - debug!(%stream_id, %sequence, "Broadcasting proposal part"); + debug!(%stream_id, %sequence, "Broadcasting proposal part"); - let msg = - StreamMessage::new(stream_id.clone(), sequence, StreamContent::Data(new_part)); + let msg = StreamMessage::new(stream_id.clone(), sequence, StreamContent::Data(new_part)); - network.cast(NetworkMsg::PublishProposalPart(msg))?; + network.cast(NetworkMsg::PublishProposalPart(msg))?; - sequence += 1; - } + sequence += 1; } Ok(()) @@ -633,7 +614,6 @@ async fn on_decided( consensus: &ConsensusRef, mempool: &MempoolRef, certificate: CommitCertificate, - extensions: VoteExtensions, metrics: &Metrics, ) -> Result<(), ActorProcessingErr> { let (height, round) = (certificate.height, certificate.round); @@ -659,9 +639,7 @@ async fn on_decided( // Update metrics let tx_count: usize = all_parts.iter().map(|p| p.tx_count()).sum(); - let parts_size: usize = all_parts.iter().map(|p| p.size_bytes()).sum(); - let extensions_size = extensions.size_bytes(); - let block_size = parts_size + extensions_size; + let block_size: usize = all_parts.iter().map(|p| p.size_bytes()).sum(); metrics.block_tx_count.observe(tx_count as f64); metrics.block_size_bytes.observe(block_size as f64); @@ -686,7 +664,6 @@ async fn on_decided( mempool.cast(MempoolMsg::Update { tx_hashes })?; // Notify Starknet Host of the decision - // TODO: Pass extensions along as well? state.host.decision(certificate).await; // Start the next height diff --git a/code/crates/starknet/host/src/host/proposal.rs b/code/crates/starknet/host/src/host/proposal.rs index 23b3fa9b6..e790b0952 100644 --- a/code/crates/starknet/host/src/host/proposal.rs +++ b/code/crates/starknet/host/src/host/proposal.rs @@ -22,7 +22,6 @@ pub async fn build_proposal_task( round: Round, proposer: Address, private_key: PrivateKey, - // vote_extensions: VoteExtensions, params: StarknetParams, deadline: Instant, mempool: MempoolRef, @@ -34,7 +33,6 @@ pub async fn build_proposal_task( round, proposer, private_key, - // vote_extensions, params, deadline, mempool, @@ -52,7 +50,6 @@ async fn run_build_proposal_task( round: Round, proposer: Address, _private_key: PrivateKey, - // vote_extensions: VoteExtensions, params: StarknetParams, deadline: Instant, mempool: MempoolRef, diff --git a/code/crates/starknet/host/src/host/starknet.rs b/code/crates/starknet/host/src/host/starknet.rs index 535ec5831..812a04d43 100644 --- a/code/crates/starknet/host/src/host/starknet.rs +++ b/code/crates/starknet/host/src/host/starknet.rs @@ -7,8 +7,6 @@ use tokio::sync::{mpsc, oneshot}; use tokio::time::Instant; use tracing::Instrument; -// use malachitebft_config::VoteExtensionsConfig; -use malachitebft_core_consensus::ValuePayload; use malachitebft_core_types::{CommitCertificate, Round, SignedVote}; use crate::host::Host; @@ -21,13 +19,11 @@ use super::proposal::{build_proposal_task, repropose_task}; #[derive(Copy, Clone, Debug)] pub struct StarknetParams { pub max_block_size: ByteSize, - pub value_payload: ValuePayload, pub tx_size: ByteSize, pub txs_per_part: usize, pub time_allowance_factor: f32, pub exec_time_per_tx: Duration, pub max_retain_blocks: usize, - // pub vote_extensions: VoteExtensionsConfig, } pub struct StarknetHost { @@ -37,7 +33,6 @@ pub struct StarknetHost { pub private_key: PrivateKey, pub validator_set: ValidatorSet, pub part_store: PartStore, - // pub vote_extensions: HashMap>, } impl StarknetHost { @@ -55,25 +50,8 @@ impl StarknetHost { private_key, validator_set, part_store: Default::default(), - // vote_extensions: Default::default(), } } - - // pub fn generate_vote_extension(&self, _height: Height, _round: Round) -> Option { - // use rand::RngCore; - // - // if !self.params.vote_extensions.enabled { - // return None; - // } - // - // let size = self.params.vote_extensions.size.as_u64() as usize; - // debug!(%size, "Vote extensions are enabled"); - // - // let mut bytes = vec![0u8; size]; - // rand::thread_rng().fill_bytes(&mut bytes); - // - // Some(Bytes::from(bytes)) - // } } #[async_trait] @@ -100,8 +78,6 @@ impl Host for StarknetHost { let (tx_part, rx_content) = mpsc::channel(self.params.txs_per_part); let (tx_block_hash, rx_block_hash) = oneshot::channel(); - // let vote_extensions = self.vote_extensions.remove(&height).unwrap_or_default(); - tokio::spawn( build_proposal_task( height, diff --git a/code/crates/starknet/host/src/spawn.rs b/code/crates/starknet/host/src/spawn.rs index 676745719..1b52b31ae 100644 --- a/code/crates/starknet/host/src/spawn.rs +++ b/code/crates/starknet/host/src/spawn.rs @@ -2,23 +2,23 @@ use std::path::{Path, PathBuf}; use std::time::Duration; use libp2p_identity::ecdsa; -use malachitebft_engine::util::events::TxEvent; -use malachitebft_engine::wal::{Wal, WalRef}; -use malachitebft_starknet_p2p_types::EcdsaProvider; use tokio::task::JoinHandle; +use tracing::warn; use malachitebft_config::{ self as config, Config as NodeConfig, MempoolConfig, SyncConfig, TestConfig, TransportProtocol, }; -use malachitebft_core_consensus::ValuePayload; +use malachitebft_core_types::ValuePayload; use malachitebft_engine::consensus::{Consensus, ConsensusParams, ConsensusRef}; use malachitebft_engine::host::HostRef; use malachitebft_engine::network::{Network, NetworkRef}; use malachitebft_engine::node::{Node, NodeRef}; use malachitebft_engine::sync::{Params as SyncParams, Sync, SyncRef}; -use malachitebft_metrics::Metrics; -use malachitebft_metrics::SharedRegistry; +use malachitebft_engine::util::events::TxEvent; +use malachitebft_engine::wal::{Wal, WalRef}; +use malachitebft_metrics::{Metrics, SharedRegistry}; use malachitebft_network::Keypair; +use malachitebft_starknet_p2p_types::EcdsaProvider; use malachitebft_sync as sync; use malachitebft_test_mempool::Config as MempoolNetworkConfig; @@ -165,18 +165,12 @@ async fn spawn_consensus_actor( tx_event: TxEvent, span: &tracing::Span, ) -> ConsensusRef { - let value_payload = match cfg.test.value_payload { - malachitebft_config::ValuePayload::PartsOnly => ValuePayload::PartsOnly, - malachitebft_config::ValuePayload::ProposalOnly => ValuePayload::ProposalOnly, - malachitebft_config::ValuePayload::ProposalAndParts => ValuePayload::ProposalAndParts, - }; - let consensus_params = ConsensusParams { initial_height, initial_validator_set, address, threshold_params: Default::default(), - value_payload, + value_payload: ValuePayload::PartsOnly, }; Consensus::spawn( @@ -321,21 +315,20 @@ async fn spawn_host_actor( metrics: Metrics, span: &tracing::Span, ) -> HostRef { - let value_payload = match cfg.test.value_payload { - malachitebft_config::ValuePayload::PartsOnly => ValuePayload::PartsOnly, - malachitebft_config::ValuePayload::ProposalOnly => ValuePayload::ProposalOnly, - malachitebft_config::ValuePayload::ProposalAndParts => ValuePayload::ProposalAndParts, - }; + if cfg.test.value_payload != config::ValuePayload::PartsOnly { + warn!( + "`value_payload` must be set to `PartsOnly` for Starknet app, ignoring current configuration `{:?}`", + cfg.test.value_payload + ); + } let mock_params = StarknetParams { - value_payload, max_block_size: cfg.test.max_block_size, tx_size: cfg.test.tx_size, txs_per_part: cfg.test.txs_per_part, time_allowance_factor: cfg.test.time_allowance_factor, exec_time_per_tx: cfg.test.exec_time_per_tx, max_retain_blocks: cfg.test.max_retain_blocks, - // vote_extensions: cfg.test.vote_extensions, }; let mock_host = StarknetHost::new( diff --git a/code/crates/starknet/test/src/lib.rs b/code/crates/starknet/test/src/lib.rs index 39a2aec37..25a8421d2 100644 --- a/code/crates/starknet/test/src/lib.rs +++ b/code/crates/starknet/test/src/lib.rs @@ -15,9 +15,7 @@ use malachitebft_test_framework::HasTestRunner; use malachitebft_test_framework::{NodeRunner, TestNode}; pub use malachitebft_test_framework::TestBuilder as GenTestBuilder; -pub use malachitebft_test_framework::{ - init_logging, EngineHandle, HandlerResult, Node, NodeId, TestParams, -}; +pub use malachitebft_test_framework::{EngineHandle, HandlerResult, Node, NodeId, TestParams}; use tempfile::TempDir; diff --git a/code/crates/starknet/test/src/tests/mod.rs b/code/crates/starknet/test/src/tests/mod.rs index 0e43f75c8..61444ce1c 100644 --- a/code/crates/starknet/test/src/tests/mod.rs +++ b/code/crates/starknet/test/src/tests/mod.rs @@ -1,6 +1,5 @@ pub mod full_nodes; pub mod n3f0; -pub mod n3f0_consensus_mode; pub mod n3f0_pubsub_protocol; pub mod n3f1; pub mod value_sync; diff --git a/code/crates/starknet/test/src/tests/n3f0_consensus_mode.rs b/code/crates/starknet/test/src/tests/n3f0_consensus_mode.rs deleted file mode 100644 index e14e8c369..000000000 --- a/code/crates/starknet/test/src/tests/n3f0_consensus_mode.rs +++ /dev/null @@ -1,51 +0,0 @@ -use std::time::Duration; - -use malachitebft_config::ValuePayload; - -use crate::{TestBuilder, TestParams}; - -async fn run_test(params: TestParams) { - const HEIGHT: u64 = 5; - - let mut test = TestBuilder::<()>::new(); - - test.add_node().start().wait_until(HEIGHT).success(); - test.add_node().start().wait_until(HEIGHT).success(); - test.add_node().start().wait_until(HEIGHT).success(); - - test.build() - .run_with_params(Duration::from_secs(30), params) - .await -} - -#[tokio::test] -pub async fn parts_only() { - let params = TestParams { - value_payload: ValuePayload::PartsOnly, - ..Default::default() - }; - - run_test(params).await -} - -#[tokio::test] -#[ignore] // Starknet app only supports parts only mode -pub async fn proposal_only() { - let params = TestParams { - value_payload: ValuePayload::ProposalOnly, - ..Default::default() - }; - - run_test(params).await -} - -#[tokio::test] -#[ignore] // Starknet app only supports parts only mode -pub async fn proposal_and_parts() { - let params = TestParams { - value_payload: ValuePayload::ProposalAndParts, - ..Default::default() - }; - - run_test(params).await -} diff --git a/code/crates/starknet/test/src/tests/value_sync.rs b/code/crates/starknet/test/src/tests/value_sync.rs index 57446abf0..e630685b7 100644 --- a/code/crates/starknet/test/src/tests/value_sync.rs +++ b/code/crates/starknet/test/src/tests/value_sync.rs @@ -1,10 +1,9 @@ use std::time::Duration; -use malachitebft_config::ValuePayload; - use crate::{TestBuilder, TestParams}; -pub async fn crash_restart_from_start(params: TestParams) { +#[tokio::test] +pub async fn crash_restart_from_start() { const HEIGHT: u64 = 10; let mut test = TestBuilder::<()>::new(); @@ -47,44 +46,12 @@ pub async fn crash_restart_from_start(params: TestParams) { Duration::from_secs(60), // Timeout for the whole test TestParams { enable_sync: true, // Enable Sync - ..params + ..Default::default() }, ) .await } -#[tokio::test] -pub async fn crash_restart_from_start_parts_only() { - let params = TestParams { - value_payload: ValuePayload::PartsOnly, - ..Default::default() - }; - - crash_restart_from_start(params).await -} - -#[tokio::test] -#[ignore] // Starknet app only supports parts only mode -pub async fn crash_restart_from_start_proposal_only() { - let params = TestParams { - value_payload: ValuePayload::ProposalOnly, - ..Default::default() - }; - - crash_restart_from_start(params).await -} - -#[tokio::test] -#[ignore] // Starknet app only supports parts only mode -pub async fn crash_restart_from_start_proposal_and_parts() { - let params = TestParams { - value_payload: ValuePayload::ProposalAndParts, - ..Default::default() - }; - - crash_restart_from_start(params).await -} - #[tokio::test] pub async fn crash_restart_from_latest() { const HEIGHT: u64 = 10; diff --git a/code/crates/starknet/test/src/tests/vote_sync.rs b/code/crates/starknet/test/src/tests/vote_sync.rs index 01d163e4a..3ae8f37e7 100644 --- a/code/crates/starknet/test/src/tests/vote_sync.rs +++ b/code/crates/starknet/test/src/tests/vote_sync.rs @@ -1,13 +1,12 @@ use std::time::Duration; -use malachitebft_config::ValuePayload; - use crate::{TestBuilder, TestParams}; // NOTE: These tests are very similar to the Sync tests, with the difference that // all nodes have the same voting power and therefore get stuck when one of them dies. -pub async fn crash_restart_from_start(params: TestParams) { +#[tokio::test] +pub async fn crash_restart_from_start() { const HEIGHT: u64 = 10; let mut test = TestBuilder::<()>::new(); @@ -38,44 +37,12 @@ pub async fn crash_restart_from_start(params: TestParams) { TestParams { enable_sync: true, // Enable Sync timeout_step: Duration::from_secs(5), - ..params + ..Default::default() }, ) .await } -#[tokio::test] -pub async fn crash_restart_from_start_parts_only() { - let params = TestParams { - value_payload: ValuePayload::PartsOnly, - ..Default::default() - }; - - crash_restart_from_start(params).await -} - -#[tokio::test] -#[ignore] // Starknet app only supports parts only mode -pub async fn crash_restart_from_start_proposal_only() { - let params = TestParams { - value_payload: ValuePayload::ProposalOnly, - ..Default::default() - }; - - crash_restart_from_start(params).await -} - -#[tokio::test] -#[ignore] // Starknet app only supports parts only mode -pub async fn crash_restart_from_start_proposal_and_parts() { - let params = TestParams { - value_payload: ValuePayload::ProposalAndParts, - ..Default::default() - }; - - crash_restart_from_start(params).await -} - #[tokio::test] pub async fn crash_restart_from_latest() { const HEIGHT: u64 = 10; diff --git a/code/crates/starknet/test/src/tests/wal.rs b/code/crates/starknet/test/src/tests/wal.rs index d6fb6e42a..e384247ea 100644 --- a/code/crates/starknet/test/src/tests/wal.rs +++ b/code/crates/starknet/test/src/tests/wal.rs @@ -1,10 +1,8 @@ use std::time::Duration; use eyre::bail; -use malachitebft_test_framework::init_logging; use tracing::info; -use malachitebft_config::ValuePayload; use malachitebft_core_consensus::LocallyProposedValue; use malachitebft_core_types::SignedVote; use malachitebft_engine::util::events::Event; @@ -13,35 +11,7 @@ use malachitebft_starknet_host::types::MockContext; use crate::{HandlerResult, TestBuilder, TestParams}; #[tokio::test] -async fn proposer_crashes_after_proposing_parts_only() { - proposer_crashes_after_proposing(TestParams { - value_payload: ValuePayload::PartsOnly, - ..TestParams::default() - }) - .await -} - -#[tokio::test] -#[ignore] // Starknet app only supports parts only mode -async fn proposer_crashes_after_proposing_proposal_and_parts() { - proposer_crashes_after_proposing(TestParams { - value_payload: ValuePayload::ProposalAndParts, - ..TestParams::default() - }) - .await -} - -#[tokio::test] -#[ignore] // Starknet app only supports parts only mode -async fn proposer_crashes_after_proposing_proposal_only() { - proposer_crashes_after_proposing(TestParams { - value_payload: ValuePayload::ProposalOnly, - ..TestParams::default() - }) - .await -} - -async fn proposer_crashes_after_proposing(params: TestParams) { +async fn proposer_crashes_after_proposing() { #[derive(Clone, Debug, Default)] struct State { first_proposed_value: Option>, @@ -98,42 +68,14 @@ async fn proposer_crashes_after_proposing(params: TestParams) { Duration::from_secs(60), TestParams { enable_sync: false, - ..params + ..TestParams::default() }, ) .await } #[tokio::test] -async fn non_proposer_crashes_after_voting_parts_only() { - non_proposer_crashes_after_voting(TestParams { - value_payload: ValuePayload::PartsOnly, - ..TestParams::default() - }) - .await -} - -#[tokio::test] -#[ignore] // Starknet app only supports parts only mode -async fn non_proposer_crashes_after_voting_proposal_and_parts() { - non_proposer_crashes_after_voting(TestParams { - value_payload: ValuePayload::ProposalAndParts, - ..TestParams::default() - }) - .await -} - -#[tokio::test] -#[ignore] // Starknet app only supports parts only mode -async fn non_proposer_crashes_after_voting_proposal_only() { - non_proposer_crashes_after_voting(TestParams { - value_payload: ValuePayload::ProposalOnly, - ..TestParams::default() - }) - .await -} - -async fn non_proposer_crashes_after_voting(params: TestParams) { +async fn non_proposer_crashes_after_voting() { #[derive(Clone, Debug, Default)] struct State { first_vote: Option>, @@ -188,7 +130,7 @@ async fn non_proposer_crashes_after_voting(params: TestParams) { Duration::from_secs(60), TestParams { enable_sync: false, - ..params + ..TestParams::default() }, ) .await @@ -196,8 +138,6 @@ async fn non_proposer_crashes_after_voting(params: TestParams) { #[tokio::test] pub async fn node_crashes_after_vote_set_request() { - init_logging(); - const HEIGHT: u64 = 3; let mut test = TestBuilder::<()>::new(); diff --git a/code/crates/test/framework/src/lib.rs b/code/crates/test/framework/src/lib.rs index 76b7cd798..a6608d062 100644 --- a/code/crates/test/framework/src/lib.rs +++ b/code/crates/test/framework/src/lib.rs @@ -15,7 +15,7 @@ pub use malachitebft_app::{EngineHandle, Node, NodeHandle}; pub use malachitebft_engine::util::events::{Event, RxEvent, TxEvent}; mod logging; -pub use logging::init_logging; +use logging::init_logging; mod node; pub use node::{HandlerResult, NodeId, TestNode}; diff --git a/code/crates/test/tests/it/main.rs b/code/crates/test/tests/it/main.rs index 6d4f90d3c..dbebcd4d9 100644 --- a/code/crates/test/tests/it/main.rs +++ b/code/crates/test/tests/it/main.rs @@ -26,7 +26,7 @@ use malachitebft_test_framework::HasTestRunner; use malachitebft_test_framework::{NodeRunner, TestNode}; pub use malachitebft_test_framework::TestBuilder as GenTestBuilder; -pub use malachitebft_test_framework::{init_logging, HandlerResult, NodeId, TestParams}; +pub use malachitebft_test_framework::{HandlerResult, NodeId, TestParams}; use informalsystems_malachitebft_test::{Height, TestContext, Validator, ValidatorSet};