refactor: Make SignedBlock immutable (#4620)

Signed-off-by: Dmitry Murzin <diralik@yandex.ru>

core/src/block.rs

@@ -235,15 +235,8 @@ mod chained {
     impl BlockBuilder<Chained> {
         /// Sign this block and get [`SignedBlock`].
         pub fn sign(self, key_pair: &KeyPair) -> WithEvents<ValidBlock> {
-            let signature = SignatureOf::new(key_pair, &self.0 .0);
-
-            WithEvents::new(ValidBlock(
-                SignedBlockV1 {
-                    payload: self.0 .0,
-                    signatures: SignaturesOf::from(signature),
-                }
-                .into(),
-            ))
+            let signed_block = SignedBlockV1::new(self.0 .0, key_pair);
+            WithEvents::new(ValidBlock(signed_block.into()))
         }
     }
 }
@@ -352,14 +345,7 @@ mod valid {
                 return WithEvents::new(Err((block, error.into())));
             }
 
-            let SignedBlock::V1(block) = block;
-            WithEvents::new(Ok(ValidBlock(
-                SignedBlockV1 {
-                    payload: block.payload,
-                    signatures: block.signatures,
-                }
-                .into(),
-            )))
+            WithEvents::new(Ok(ValidBlock(block)))
         }
 
         fn validate_transactions(
@@ -492,7 +478,12 @@ mod valid {
 
         #[cfg(test)]
         pub(crate) fn new_dummy() -> Self {
-            BlockBuilder(Chained(BlockPayload {
+            Self::new_dummy_and_modify_payload(|_| {})
+        }
+
+        #[cfg(test)]
+        pub(crate) fn new_dummy_and_modify_payload(f: impl FnOnce(&mut BlockPayload)) -> Self {
+            let mut payload = BlockPayload {
                 header: BlockHeader {
                     height: 2,
                     previous_block_hash: None,
@@ -507,9 +498,11 @@ mod valid {
                 transactions: Vec::new(),
                 commit_topology: UniqueVec::new(),
                 event_recommendations: Vec::new(),
-            }))
-            .sign(&KeyPair::random())
-            .unpack(|_| {})
+            };
+            f(&mut payload);
+            BlockBuilder(Chained(payload))
+                .sign(&KeyPair::random())
+                .unpack(|_| {})
         }
 
         /// Check if block's signatures meet requirements for given topology.
@@ -576,7 +569,7 @@ mod valid {
 
         fn payload(block: &ValidBlock) -> &BlockPayload {
             let SignedBlock::V1(signed) = &block.0;
-            &signed.payload
+            &signed.payload()
         }
 
         #[test]

core/src/state.rs

@@ -1773,31 +1773,29 @@ mod tests {
     };
 
     /// Used to inject faulty payload for testing
-    fn payload_mut(block: &mut CommittedBlock) -> &mut BlockPayload {
-        let SignedBlock::V1(signed) = block.as_mut();
-        &mut signed.payload
+    fn new_dummy_block_with_payload(f: impl FnOnce(&mut BlockPayload)) -> CommittedBlock {
+        let topology = Topology::new(UniqueVec::new());
+        ValidBlock::new_dummy_and_modify_payload(f)
+            .commit(&topology)
+            .unpack(|_| {})
+            .unwrap()
     }
 
     #[tokio::test]
     async fn get_block_hashes_after_hash() {
         const BLOCK_CNT: usize = 10;
 
-        let topology = Topology::new(UniqueVec::new());
-        let block = ValidBlock::new_dummy()
-            .commit(&topology)
-            .unpack(|_| {})
-            .unwrap();
         let kura = Kura::blank_kura_for_testing();
         let query_handle = LiveQueryStore::test().start();
         let state = State::new(World::default(), kura, query_handle);
         let mut state_block = state.block();
 
         let mut block_hashes = vec![];
         for i in 1..=BLOCK_CNT {
-            let mut block = block.clone();
-
-            payload_mut(&mut block).header.height = i as u64;
-            payload_mut(&mut block).header.previous_block_hash = block_hashes.last().copied();
+            let block = new_dummy_block_with_payload(|payload| {
+                payload.header.height = i as u64;
+                payload.header.previous_block_hash = block_hashes.last().copied();
+            });
 
             block_hashes.push(block.as_ref().hash());
             let _events = state_block.apply(&block).unwrap();
@@ -1813,19 +1811,15 @@ mod tests {
     async fn get_blocks_from_height() {
         const BLOCK_CNT: usize = 10;
 
-        let topology = Topology::new(UniqueVec::new());
-        let block = ValidBlock::new_dummy()
-            .commit(&topology)
-            .unpack(|_| {})
-            .unwrap();
         let kura = Kura::blank_kura_for_testing();
         let query_handle = LiveQueryStore::test().start();
         let state = State::new(World::default(), kura.clone(), query_handle);
         let mut state_block = state.block();
 
         for i in 1..=BLOCK_CNT {
-            let mut block = block.clone();
-            payload_mut(&mut block).header.height = i as u64;
+            let block = new_dummy_block_with_payload(|payload| {
+                payload.header.height = i as u64;
+            });
 
             let _events = state_block.apply(&block).unwrap();
             kura.store_block(block);

core/src/sumeragi/main_loop.rs

@@ -1209,9 +1209,16 @@ mod tests {
     use crate::{query::store::LiveQueryStore, smartcontracts::Registrable};
 
     /// Used to inject faulty payload for testing
-    fn payload_mut(block: &mut SignedBlock) -> &mut BlockPayload {
+    fn clone_and_modify_payload(
+        block: &SignedBlock,
+        key_pair: &KeyPair,
+        f: impl FnOnce(&mut BlockPayload),
+    ) -> SignedBlock {
         let SignedBlock::V1(signed) = block;
-        &mut signed.payload
+        let mut payload = signed.payload().clone();
+        f(&mut payload);
+
+        SignedBlock::V1(SignedBlockV1::new(payload, &key_pair))
     }
 
     fn create_data_for_test(
@@ -1312,11 +1319,13 @@ mod tests {
             "127.0.0.1:8080".parse().unwrap(),
             leader_key_pair.public_key().clone(),
         )]);
-        let (state, _, mut block, genesis_public_key) =
+        let (state, _, block, genesis_public_key) =
             create_data_for_test(&chain_id, &topology, &leader_key_pair);
 
         // Malform block to make it invalid
-        payload_mut(&mut block).commit_topology.clear();
+        let block = clone_and_modify_payload(&block, &leader_key_pair, |payload| {
+            payload.commit_topology.clear();
+        });
 
         let result = handle_block_sync(&chain_id, &genesis_public_key, block, &state, &|_| {});
         assert!(matches!(result, Err((_, BlockSyncError::BlockNotValid(_)))))
@@ -1331,7 +1340,7 @@ mod tests {
             "127.0.0.1:8080".parse().unwrap(),
             leader_key_pair.public_key().clone(),
         )]);
-        let (state, kura, mut block, genesis_public_key) =
+        let (state, kura, block, genesis_public_key) =
             create_data_for_test(&chain_id, &topology, &leader_key_pair);
 
         let mut state_block = state.block();
@@ -1352,8 +1361,10 @@ mod tests {
         kura.store_block(committed_block);
 
         // Malform block to make it invalid
-        payload_mut(&mut block).commit_topology.clear();
-        payload_mut(&mut block).header.view_change_index = 1;
+        let block = clone_and_modify_payload(&block, &leader_key_pair, |payload| {
+            payload.commit_topology.clear();
+            payload.header.view_change_index = 1;
+        });
 
         let result = handle_block_sync(&chain_id, &genesis_public_key, block, &state, &|_| {});
         assert!(matches!(
@@ -1369,11 +1380,13 @@ mod tests {
 
         let topology = Topology::new(UniqueVec::new());
         let leader_key_pair = KeyPair::random();
-        let (state, _, mut block, genesis_public_key) =
+        let (state, _, block, genesis_public_key) =
             create_data_for_test(&chain_id, &topology, &leader_key_pair);
 
         // Change block height
-        payload_mut(&mut block).header.height = 42;
+        let block = clone_and_modify_payload(&block, &leader_key_pair, |payload| {
+            payload.header.height = 42;
+        });
 
         let result = handle_block_sync(&chain_id, &genesis_public_key, block, &state, &|_| {});
         assert!(matches!(
@@ -1413,7 +1426,7 @@ mod tests {
             "127.0.0.1:8080".parse().unwrap(),
             leader_key_pair.public_key().clone(),
         )]);
-        let (state, kura, mut block, genesis_public_key) =
+        let (state, kura, block, genesis_public_key) =
             create_data_for_test(&chain_id, &topology, &leader_key_pair);
 
         let mut state_block = state.block();
@@ -1436,7 +1449,9 @@ mod tests {
         assert_eq!(state.view().latest_block_view_change_index(), 0);
 
         // Increase block view change index
-        payload_mut(&mut block).header.view_change_index = 42;
+        let block = clone_and_modify_payload(&block, &leader_key_pair, |payload| {
+            payload.header.view_change_index = 42;
+        });
 
         let result = handle_block_sync(&chain_id, &genesis_public_key, block, &state, &|_| {});
         assert!(matches!(result, Ok(BlockSyncOk::ReplaceTopBlock(_, _))))
@@ -1451,11 +1466,13 @@ mod tests {
             "127.0.0.1:8080".parse().unwrap(),
             leader_key_pair.public_key().clone(),
         )]);
-        let (state, kura, mut block, genesis_public_key) =
+        let (state, kura, block, genesis_public_key) =
             create_data_for_test(&chain_id, &topology, &leader_key_pair);
 
         // Increase block view change index
-        payload_mut(&mut block).header.view_change_index = 42;
+        let block = clone_and_modify_payload(&block, &leader_key_pair, |payload| {
+            payload.header.view_change_index = 42;
+        });
 
         let mut state_block = state.block();
         let committed_block = ValidBlock::validate(
@@ -1476,7 +1493,9 @@ mod tests {
         assert_eq!(state.view().latest_block_view_change_index(), 42);
 
         // Decrease block view change index back
-        payload_mut(&mut block).header.view_change_index = 0;
+        let block = clone_and_modify_payload(&block, &leader_key_pair, |payload| {
+            payload.header.view_change_index = 0;
+        });
 
         let result = handle_block_sync(&chain_id, &genesis_public_key, block, &state, &|_| {});
         assert!(matches!(
@@ -1498,13 +1517,15 @@ mod tests {
 
         let topology = Topology::new(UniqueVec::new());
         let leader_key_pair = KeyPair::random();
-        let (state, _, mut block, genesis_public_key) =
+        let (state, _, block, genesis_public_key) =
             create_data_for_test(&chain_id, &topology, &leader_key_pair);
 
         // Change block height and view change index
         // Soft-fork on genesis block is not possible
-        payload_mut(&mut block).header.view_change_index = 42;
-        payload_mut(&mut block).header.height = 1;
+        let block = clone_and_modify_payload(&block, &leader_key_pair, |payload| {
+            payload.header.view_change_index = 42;
+            payload.header.height = 1;
+        });
 
         let result = handle_block_sync(&chain_id, &genesis_public_key, block, &state, &|_| {});
         assert!(matches!(

data_model/src/block.rs

@@ -107,9 +107,9 @@ mod model {
     #[ffi_type]
     pub struct SignedBlockV1 {
         /// Signatures of peers which approved this block.
-        pub signatures: SignaturesOf<BlockPayload>,
+        pub(super) signatures: SignaturesOf<BlockPayload>,
         /// Block payload
-        pub payload: BlockPayload,
+        pub(super) payload: BlockPayload,
     }
 }
 
@@ -133,6 +133,23 @@ impl BlockHeader {
 }
 
 impl SignedBlockV1 {
+    /// Create new signed block, using `key_pair` to sign `payload`
+    #[cfg(feature = "transparent_api")]
+    pub fn new(payload: BlockPayload, key_pair: &iroha_crypto::KeyPair) -> SignedBlockV1 {
+        let signature = iroha_crypto::SignatureOf::new(key_pair, &payload);
+        let signatures = SignaturesOf::from(signature);
+        SignedBlockV1 {
+            signatures,
+            payload,
+        }
+    }
+
+    /// Block payload. Used for tests
+    #[cfg(feature = "transparent_api")]
+    pub fn payload(&self) -> &BlockPayload {
+        &self.payload
+    }
+
     #[cfg(feature = "std")]
     fn hash(&self) -> iroha_crypto::HashOf<SignedBlock> {
         iroha_crypto::HashOf::from_untyped_unchecked(iroha_crypto::HashOf::new(self).into())