Merge branch 'main' into clean_testing_framework

.github/CONTRIBUTING.md

@@ -1,7 +1,13 @@
 # Contribution Guidelines
 
 Hello! Thanks for your interest in joining the mission to accelerate the mass adoption of crypto for personal
-sovereignty! We welcome contributions from anyone on the internet, and are grateful for even the smallest of fixes!
+sovereignty! We welcome contributions from anyone on the internet.
+
+Note, however, that all the contributions are subject to review, and not every contribution is guaranteed to be merged.
+It is highly advised to reach out to developers (for example, by creating an issue) before preparing a significant
+change in the codebase, and explicitly confirm that this contribution will be considered for merge. Otherwise, it is
+possible to discover that a feature you have spent some time on does not align with the core team vision or capacity to
+maintain a high quality of given submission long term.
 
 ## Ways to contribute
 

node/Cargo.toml

@@ -81,7 +81,6 @@ test-casing = "0.1.0"
 thiserror = "1.0.40"
 time = "0.3.23"
 tokio = { version = "1.34.0", features = ["full"] }
-tokio-retry = "0.3.0"
 tracing = { version = "0.1.37", features = ["attributes"] }
 tracing-subscriber = { version = "0.3.16", features = ["env-filter", "fmt"] }
 kube = { version = "0.88.1", features = ["runtime", "derive"] }
@@ -157,7 +156,5 @@ wildcard_dependencies = "warn"
 redundant_locals = "allow"
 needless_pass_by_ref_mut = "allow"
 box_default = "allow"
-# remove once fix to https://github.com/rust-lang/rust-clippy/issues/11764 is available on CI.
-map_identity = "allow"
 # &*x is not equivalent to x, because it affects borrowing in closures.
 borrow_deref_ref = "allow"

node/actors/bft/src/leader/state_machine.rs

@@ -167,9 +167,9 @@ impl StateMachine {
             // The previous block was finalized, so we can propose a new block.
             _ => {
                 let fork = &cfg.genesis().fork;
-                let (parent, number) = match high_qc {
-                    Some(qc) => (Some(qc.header().hash()), qc.header().number.next()),
-                    None => (fork.first_parent, fork.first_block),
+                let number = match high_qc {
+                    Some(qc) => qc.header().number.next(),
+                    None => fork.first_block,
                 };
                 // Defensively assume that PayloadManager cannot propose until the previous block is stored.
                 if let Some(prev) = number.prev() {
@@ -189,7 +189,6 @@ impl StateMachine {
                     .observe(payload.0.len());
                 let proposal = validator::BlockHeader {
                     number,
-                    parent,
                     payload: payload.hash(),
                 };
                 (proposal, Some(payload))

node/actors/bft/src/leader/tests.rs

@@ -37,13 +37,6 @@ async fn replica_prepare_sanity_yield_leader_prepare() {
             .unwrap()
             .unwrap();
         assert_eq!(leader_prepare.msg.view(), &replica_prepare.view);
-        assert_eq!(
-            leader_prepare.msg.proposal.parent,
-            replica_prepare
-                .high_vote
-                .as_ref()
-                .map(|v| v.proposal.hash()),
-        );
         assert_eq!(
             leader_prepare.msg.justification,
             util.new_prepare_qc(|msg| *msg = replica_prepare)

node/actors/bft/src/replica/block.rs

@@ -39,7 +39,7 @@ impl StateMachine {
         tracing::info!(
             "Finalized block {}: {:#?}",
             block.header().number,
-            block.header().hash(),
+            block.header().payload,
         );
         self.config
             .block_store

node/actors/bft/src/replica/leader_prepare.rs

@@ -42,8 +42,9 @@ pub(crate) enum Error {
     /// Invalid message.
     #[error("invalid message: {0:#}")]
     InvalidMessage(#[source] validator::LeaderPrepareVerifyError),
-    /// Previous proposal was not finalized.
-
+    /// Leader proposed a block that was already pruned from replica's storage.
+    #[error("leader proposed a block that was already pruned from replica's storage")]
+    ProposalAlreadyPruned,
     /// Oversized payload.
     #[error("block proposal with an oversized payload (payload size: {payload_size})")]
     ProposalOversizedPayload {
@@ -110,6 +111,14 @@ impl StateMachine {
             });
         }
 
+        // Replica MUSTN'T vote for blocks which have been already pruned for storage.
+        // (because it won't be able to persist and broadcast them once finalized).
+        // TODO(gprusak): it should never happen, we should add safety checks to prevent
+        // pruning blocks not known to be finalized.
+        if message.proposal.number < self.config.block_store.subscribe().borrow().first {
+            return Err(Error::ProposalAlreadyPruned);
+        }
+
         // ----------- Checking the message --------------
 
         signed_message.verify().map_err(Error::InvalidSignature)?;

node/actors/bft/src/replica/tests.rs

@@ -7,7 +7,7 @@ use assert_matches::assert_matches;
 use rand::Rng;
 use zksync_concurrency::{ctx, scope};
 use zksync_consensus_roles::validator::{
-    self, CommitQC, Payload, PrepareQC, ReplicaCommit, ReplicaPrepare, ViewNumber,
+    self, CommitQC, Payload, PrepareQC, ReplicaCommit, ReplicaPrepare,
 };
 
 /// Sanity check of the happy path.
@@ -101,10 +101,6 @@ async fn leader_prepare_invalid_leader() {
         let (mut util, runner) = UTHarness::new(ctx, 2).await;
         s.spawn_bg(runner.run(ctx));
 
-        let view = ViewNumber(2);
-        util.set_view(view);
-        assert_eq!(util.view_leader(view), util.keys[0].public());
-
         let replica_prepare = util.new_replica_prepare();
         assert!(util
             .process_replica_prepare(ctx, util.sign(replica_prepare.clone()))
@@ -167,6 +163,35 @@ async fn leader_prepare_old_view() {
     .unwrap();
 }
 
+#[tokio::test]
+async fn leader_prepare_pruned_block() {
+    zksync_concurrency::testonly::abort_on_panic();
+    let ctx = &ctx::test_root(&ctx::RealClock);
+    scope::run!(ctx, |ctx, s| async {
+        let (mut util, runner) = UTHarness::new(ctx, 1).await;
+        s.spawn_bg(runner.run(ctx));
+
+        let mut leader_prepare = util.new_leader_prepare(ctx).await;
+        // We assume default replica state and nontrivial `genesis.fork.first_block` here.
+        leader_prepare.proposal.number = util
+            .replica
+            .config
+            .block_store
+            .subscribe()
+            .borrow()
+            .first
+            .prev()
+            .unwrap();
+        let res = util
+            .process_leader_prepare(ctx, util.sign(leader_prepare))
+            .await;
+        assert_matches!(res, Err(leader_prepare::Error::ProposalAlreadyPruned));
+        Ok(())
+    })
+    .await
+    .unwrap();
+}
+
 /// Tests that `WriteBlockStore::verify_payload` is applied before signing a vote.
 #[tokio::test]
 async fn leader_prepare_invalid_payload() {
@@ -338,33 +363,6 @@ async fn leader_prepare_proposal_when_previous_not_finalized() {
     .unwrap();
 }
 
-#[tokio::test]
-async fn leader_prepare_bad_parent_hash() {
-    zksync_concurrency::testonly::abort_on_panic();
-    let ctx = &ctx::test_root(&ctx::RealClock);
-    scope::run!(ctx, |ctx, s| async {
-        let (mut util, runner) = UTHarness::new(ctx, 1).await;
-        s.spawn_bg(runner.run(ctx));
-
-        tracing::info!("Produce initial block.");
-        util.produce_block(ctx).await;
-        tracing::info!("Make leader propose the next block.");
-        let mut leader_prepare = util.new_leader_prepare(ctx).await;
-        tracing::info!("Modify the proposal.parent so that it doesn't match the previous block");
-        leader_prepare.proposal.parent = Some(ctx.rng().gen());
-        let res = util.process_leader_prepare(ctx, util.sign(leader_prepare.clone())).await;
-        assert_matches!(res, Err(leader_prepare::Error::InvalidMessage(
-            validator::LeaderPrepareVerifyError::BadParentHash { got, want }
-        )) => {
-            assert_eq!(want, Some(leader_prepare.justification.high_qc().unwrap().message.proposal.hash()));
-            assert_eq!(got, leader_prepare.proposal.parent);
-        });
-        Ok(())
-    })
-    .await
-    .unwrap();
-}
-
 #[tokio::test]
 async fn leader_prepare_bad_block_number() {
     zksync_concurrency::testonly::abort_on_panic();
@@ -500,7 +498,7 @@ async fn leader_prepare_reproposal_invalid_block() {
     .unwrap();
 }
 
-/// Check that replica provides expecte high_vote and high_qc after finalizing a block.
+/// Check that replica provides expected high_vote and high_qc after finalizing a block.
 #[tokio::test]
 async fn leader_commit_sanity_yield_replica_prepare() {
     zksync_concurrency::testonly::abort_on_panic();

node/actors/bft/src/testonly/fuzz.rs

@@ -174,11 +174,9 @@ impl Fuzz for validator::Payload {
 
 impl Fuzz for validator::BlockHeader {
     fn mutate(&mut self, rng: &mut impl Rng) {
-        match rng.gen_range(0..3) {
-            0 => self.parent = rng.gen(),
-            1 => self.number = rng.gen(),
-            2 => self.payload = rng.gen(),
-            _ => unreachable!(),
+        match rng.gen_range(0..2) {
+            0 => self.number = rng.gen(),
+            _ => self.payload = rng.gen(),
         }
     }
 }

node/actors/bft/src/testonly/ut_harness.rs

@@ -137,19 +137,6 @@ impl UTHarness {
         self.replica.view = view;
     }
 
-    pub(crate) fn set_view(&mut self, view: ViewNumber) {
-        self.set_replica_view(view);
-        self.set_leader_view(view);
-    }
-
-    pub(crate) fn set_leader_view(&mut self, view: ViewNumber) {
-        self.leader.view = view
-    }
-
-    pub(crate) fn set_replica_view(&mut self, view: ViewNumber) {
-        self.replica.view = view
-    }
-
     pub(crate) fn replica_view(&self) -> validator::View {
         validator::View {
             protocol_version: self.protocol_version(),