From b144c958cda63a6a793cc3ffc4342abfb62e6463 Mon Sep 17 00:00:00 2001 From: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> Date: Wed, 19 Oct 2022 10:57:00 +0200 Subject: [PATCH 1/3] BlockId removal: refactor: BlockImportOperation+Bknd::finalize_block It changes the arguments of methods of `BlockImportOperation` trait from: block: `BlockId` to: hash: `&Block::Hash` `Backend::finalize_block` was also changed. This PR is part of BlockId::Number refactoring analysis (paritytech/substrate#11292) --- client/api/src/backend.rs | 8 +-- client/api/src/in_mem.rs | 31 ++++----- client/beefy/src/worker.rs | 6 +- client/db/src/lib.rs | 97 +++++++++++++++-------------- client/service/src/client/client.rs | 6 +- 5 files changed, 73 insertions(+), 75 deletions(-) diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs index 5b4acb0be8bda..864a9af5685d8 100644 --- a/client/api/src/backend.rs +++ b/client/api/src/backend.rs @@ -216,13 +216,13 @@ pub trait BlockImportOperation { /// Mark a block as finalized. fn mark_finalized( &mut self, - id: BlockId, + hash: &Block::Hash, justification: Option, ) -> sp_blockchain::Result<()>; /// Mark a block as new head. If both block import and set head are specified, set head /// overrides block import's best block rule. - fn mark_head(&mut self, id: BlockId) -> sp_blockchain::Result<()>; + fn mark_head(&mut self, hash: &Block::Hash) -> sp_blockchain::Result<()>; /// Add a transaction index operation. fn update_transaction_index(&mut self, index: Vec) @@ -476,12 +476,12 @@ pub trait Backend: AuxStore + Send + Sync { transaction: Self::BlockImportOperation, ) -> sp_blockchain::Result<()>; - /// Finalize block with given Id. + /// Finalize block with given `hash`. /// /// This should only be called if the parent of the given block has been finalized. fn finalize_block( &self, - block: BlockId, + hash: &Block::Hash, justification: Option, ) -> sp_blockchain::Result<()>; diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs index 99efdd3bf1d22..dac02c893b1dc 100644 --- a/client/api/src/in_mem.rs +++ b/client/api/src/in_mem.rs @@ -223,9 +223,9 @@ impl Blockchain { } /// Set an existing block as head. - pub fn set_head(&self, id: BlockId) -> sp_blockchain::Result<()> { + pub fn set_head(&self, id: Block::Hash) -> sp_blockchain::Result<()> { let header = self - .header(id)? + .header(BlockId::Hash(id))? .ok_or_else(|| sp_blockchain::Error::UnknownBlock(format!("{}", id)))?; self.apply_head(&header) @@ -271,21 +271,16 @@ impl Blockchain { fn finalize_header( &self, - id: BlockId, + block: &Block::Hash, justification: Option, ) -> sp_blockchain::Result<()> { - let hash = match self.header(id)? { - Some(h) => h.hash(), - None => return Err(sp_blockchain::Error::UnknownBlock(format!("{}", id))), - }; - let mut storage = self.storage.write(); - storage.finalized_hash = hash; + storage.finalized_hash = *block; if justification.is_some() { let block = storage .blocks - .get_mut(&hash) + .get_mut(block) .expect("hash was fetched from a block in the db; qed"); let block_justifications = match block { @@ -500,8 +495,8 @@ pub struct BlockImportOperation { new_state: Option<> as StateBackend>>::Transaction>, aux: Vec<(Vec, Option>)>, - finalized_blocks: Vec<(BlockId, Option)>, - set_head: Option>, + finalized_blocks: Vec<(Block::Hash, Option)>, + set_head: Option, } impl BlockImportOperation @@ -605,16 +600,16 @@ where fn mark_finalized( &mut self, - block: BlockId, + hash: &Block::Hash, justification: Option, ) -> sp_blockchain::Result<()> { - self.finalized_blocks.push((block, justification)); + self.finalized_blocks.push((*hash, justification)); Ok(()) } - fn mark_head(&mut self, block: BlockId) -> sp_blockchain::Result<()> { + fn mark_head(&mut self, hash: &Block::Hash) -> sp_blockchain::Result<()> { assert!(self.pending_block.is_none(), "Only one set block per operation is allowed"); - self.set_head = Some(block); + self.set_head = Some(*hash); Ok(()) } @@ -710,7 +705,7 @@ where fn commit_operation(&self, operation: Self::BlockImportOperation) -> sp_blockchain::Result<()> { if !operation.finalized_blocks.is_empty() { for (block, justification) in operation.finalized_blocks { - self.blockchain.finalize_header(block, justification)?; + self.blockchain.finalize_header(&block, justification)?; } } @@ -743,7 +738,7 @@ where fn finalize_block( &self, - block: BlockId, + block: &Block::Hash, justification: Option, ) -> sp_blockchain::Result<()> { self.blockchain.finalize_header(block, justification) diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 6efebd131d6da..9b331b73ed093 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -1371,8 +1371,10 @@ pub(crate) mod tests { let mut best_block_stream = best_block_streams.drain(..).next().unwrap(); net.peer(0).push_blocks(2, false); // finalize 1 and 2 without justifications - backend.finalize_block(BlockId::number(1), None).unwrap(); - backend.finalize_block(BlockId::number(2), None).unwrap(); + let hashof1 = backend.blockchain().expect_block_hash_from_id(&BlockId::Number(1)).unwrap(); + let hashof2 = backend.blockchain().expect_block_hash_from_id(&BlockId::Number(2)).unwrap(); + backend.finalize_block(&hashof1, None).unwrap(); + backend.finalize_block(&hashof2, None).unwrap(); let justif = create_finality_proof(2); // create new session at block #2 diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 1a80c16c4e59d..70d9ceec9d607 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -756,8 +756,8 @@ pub struct BlockImportOperation { offchain_storage_updates: OffchainChangesCollection, pending_block: Option>, aux_ops: Vec<(Vec, Option>)>, - finalized_blocks: Vec<(BlockId, Option)>, - set_head: Option>, + finalized_blocks: Vec<(Block::Hash, Option)>, + set_head: Option, commit_state: bool, index_ops: Vec, } @@ -897,16 +897,16 @@ impl sc_client_api::backend::BlockImportOperation fn mark_finalized( &mut self, - block: BlockId, + block: &Block::Hash, justification: Option, ) -> ClientResult<()> { - self.finalized_blocks.push((block, justification)); + self.finalized_blocks.push((*block, justification)); Ok(()) } - fn mark_head(&mut self, block: BlockId) -> ClientResult<()> { + fn mark_head(&mut self, block: &Block::Hash) -> ClientResult<()> { assert!(self.set_head.is_none(), "Only one set head per operation is allowed"); - self.set_head = Some(block); + self.set_head = Some(*block); Ok(()) } @@ -1351,8 +1351,7 @@ impl Backend { (meta.best_number, meta.finalized_hash, meta.finalized_number, meta.block_gap) }; - for (block, justification) in operation.finalized_blocks { - let block_hash = self.blockchain.expect_block_hash_from_id(&block)?; + for (block_hash, justification) in operation.finalized_blocks { let block_header = self.blockchain.expect_header(BlockId::Hash(block_hash))?; meta_updates.push(self.finalize_block_with_transaction( &mut transaction, @@ -1624,9 +1623,10 @@ impl Backend { }; if let Some(set_head) = operation.set_head { - if let Some(header) = - sc_client_api::blockchain::HeaderBackend::header(&self.blockchain, set_head)? - { + if let Some(header) = sc_client_api::blockchain::HeaderBackend::header( + &self.blockchain, + BlockId::Hash(set_head), + )? { let number = header.number(); let hash = header.hash(); @@ -1992,17 +1992,16 @@ impl sc_client_api::backend::Backend for Backend { fn finalize_block( &self, - block: BlockId, + block: &Block::Hash, justification: Option, ) -> ClientResult<()> { let mut transaction = Transaction::new(); - let hash = self.blockchain.expect_block_hash_from_id(&block)?; - let header = self.blockchain.expect_header(block)?; + let header = self.blockchain.expect_header(BlockId::Hash(*block))?; let mut displaced = None; let m = self.finalize_block_with_transaction( &mut transaction, - &hash, + block, &header, None, justification, @@ -2605,13 +2604,12 @@ pub(crate) mod tests { header.state_root = root.into(); op.update_storage(storage, Vec::new()).unwrap(); - op.set_block_data(header, Some(vec![]), None, None, NewBlockState::Best) + op.set_block_data(header.clone(), Some(vec![]), None, None, NewBlockState::Best) .unwrap(); db.commit_operation(op).unwrap(); - let hash = db.blockchain().expect_block_hash_from_id(&BlockId::Number(1)).unwrap(); - let state = db.state_at(&hash).unwrap(); + let state = db.state_at(&header.hash()).unwrap(); assert_eq!(state.storage(&[1, 3, 5]).unwrap(), None); assert_eq!(state.storage(&[1, 2, 3]).unwrap(), Some(vec![9, 9, 9])); @@ -2665,7 +2663,7 @@ pub(crate) mod tests { hash }; - let hash = { + let hashof1 = { let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, BlockId::Number(0)).unwrap(); let mut header = Header { @@ -2702,12 +2700,12 @@ pub(crate) mod tests { hash }; - let hash = { + let hashof2 = { let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, BlockId::Number(1)).unwrap(); let mut header = Header { number: 2, - parent_hash: hash, + parent_hash: hashof1, state_root: Default::default(), digest: Default::default(), extrinsics_root: Default::default(), @@ -2736,12 +2734,12 @@ pub(crate) mod tests { hash }; - { + let hashof3 = { let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, BlockId::Number(2)).unwrap(); let mut header = Header { number: 3, - parent_hash: hash, + parent_hash: hashof2, state_root: Default::default(), digest: Default::default(), extrinsics_root: Default::default(), @@ -2754,6 +2752,7 @@ pub(crate) mod tests { .storage_root(storage.iter().cloned().map(|(x, y)| (x, Some(y))), state_version) .0 .into(); + let hash = header.hash(); op.set_block_data(header, Some(vec![]), None, None, NewBlockState::Best) .unwrap(); @@ -2764,11 +2763,12 @@ pub(crate) mod tests { .db .get(columns::STATE, &sp_trie::prefixed_key::(&key, EMPTY_PREFIX)) .is_none()); - } + hash + }; - backend.finalize_block(BlockId::Number(1), None).unwrap(); - backend.finalize_block(BlockId::Number(2), None).unwrap(); - backend.finalize_block(BlockId::Number(3), None).unwrap(); + backend.finalize_block(&hashof1, None).unwrap(); + backend.finalize_block(&hashof2, None).unwrap(); + backend.finalize_block(&hashof3, None).unwrap(); assert!(backend .storage .db @@ -2991,8 +2991,8 @@ pub(crate) mod tests { vec![block2_a, block2_b, block2_c, block1_c] ); - backend.finalize_block(BlockId::hash(block1_a), None).unwrap(); - backend.finalize_block(BlockId::hash(block2_a), None).unwrap(); + backend.finalize_block(&block1_a, None).unwrap(); + backend.finalize_block(&block2_a, None).unwrap(); // leaves at same height stay. Leaves at lower heights pruned. assert_eq!(backend.blockchain().leaves().unwrap(), vec![block2_a, block2_b, block2_c]); @@ -3016,10 +3016,10 @@ pub(crate) mod tests { let backend = Backend::::new_test(10, 10); let block0 = insert_header(&backend, 0, Default::default(), None, Default::default()); - let _ = insert_header(&backend, 1, block0, None, Default::default()); + let block1 = insert_header(&backend, 1, block0, None, Default::default()); let justification = Some((CONS0_ENGINE_ID, vec![1, 2, 3])); - backend.finalize_block(BlockId::Number(1), justification.clone()).unwrap(); + backend.finalize_block(&block1, justification.clone()).unwrap(); assert_eq!( backend.blockchain().justifications(BlockId::Number(1)).unwrap(), @@ -3034,10 +3034,10 @@ pub(crate) mod tests { let backend = Backend::::new_test(10, 10); let block0 = insert_header(&backend, 0, Default::default(), None, Default::default()); - let _ = insert_header(&backend, 1, block0, None, Default::default()); + let block1 = insert_header(&backend, 1, block0, None, Default::default()); let just0 = (CONS0_ENGINE_ID, vec![1, 2, 3]); - backend.finalize_block(BlockId::Number(1), Some(just0.clone().into())).unwrap(); + backend.finalize_block(&block1, Some(just0.clone().into())).unwrap(); let just1 = (CONS1_ENGINE_ID, vec![4, 5]); backend.append_justification(BlockId::Number(1), just1.clone()).unwrap(); @@ -3071,15 +3071,15 @@ pub(crate) mod tests { { let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, BlockId::Hash(block0)).unwrap(); - op.mark_finalized(BlockId::Hash(block1), None).unwrap(); - op.mark_finalized(BlockId::Hash(block2), None).unwrap(); + op.mark_finalized(&block1, None).unwrap(); + op.mark_finalized(&block2, None).unwrap(); backend.commit_operation(op).unwrap(); } { let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, BlockId::Hash(block2)).unwrap(); - op.mark_finalized(BlockId::Hash(block3), None).unwrap(); - op.mark_finalized(BlockId::Hash(block4), None).unwrap(); + op.mark_finalized(&block3, None).unwrap(); + op.mark_finalized(&block4, None).unwrap(); backend.commit_operation(op).unwrap(); } } @@ -3181,7 +3181,7 @@ pub(crate) mod tests { { let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, BlockId::Hash(block0)).unwrap(); - op.mark_finalized(BlockId::Hash(block2), None).unwrap(); + op.mark_finalized(&block2, None).unwrap(); backend.commit_operation(op).unwrap_err(); } } @@ -3210,7 +3210,7 @@ pub(crate) mod tests { let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, BlockId::Hash(blocks[4])).unwrap(); for i in 1..5 { - op.mark_finalized(BlockId::Hash(blocks[i]), None).unwrap(); + op.mark_finalized(&blocks[i], None).unwrap(); } backend.commit_operation(op).unwrap(); } @@ -3245,7 +3245,7 @@ pub(crate) mod tests { let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, BlockId::Hash(blocks[4])).unwrap(); for i in 1..3 { - op.mark_finalized(BlockId::Hash(blocks[i]), None).unwrap(); + op.mark_finalized(&blocks[i], None).unwrap(); } backend.commit_operation(op).unwrap(); @@ -3301,7 +3301,7 @@ pub(crate) mod tests { let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, BlockId::Hash(blocks[4])).unwrap(); - op.mark_head(BlockId::Hash(blocks[4])).unwrap(); + op.mark_head(&blocks[4]).unwrap(); backend.commit_operation(op).unwrap(); let bc = backend.blockchain(); @@ -3310,7 +3310,7 @@ pub(crate) mod tests { for i in 1..5 { let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, BlockId::Hash(blocks[i])).unwrap(); - op.mark_finalized(BlockId::Hash(blocks[i]), None).unwrap(); + op.mark_finalized(&blocks[i], None).unwrap(); backend.commit_operation(op).unwrap(); } @@ -3370,13 +3370,13 @@ pub(crate) mod tests { .unwrap(); let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, BlockId::Hash(blocks[4])).unwrap(); - op.mark_head(BlockId::Hash(blocks[4])).unwrap(); + op.mark_head(&blocks[4]).unwrap(); backend.commit_operation(op).unwrap(); for i in 1..5 { let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, BlockId::Hash(blocks[4])).unwrap(); - op.mark_finalized(BlockId::Hash(blocks[i]), None).unwrap(); + op.mark_finalized(&blocks[i], None).unwrap(); backend.commit_operation(op).unwrap(); } @@ -3423,8 +3423,9 @@ pub(crate) mod tests { assert_eq!(bc.indexed_transaction(&x1_hash).unwrap().unwrap(), &x1[1..]); // Push one more blocks and make sure block is pruned and transaction index is cleared. - insert_block(&backend, 1, hash, None, Default::default(), vec![], None).unwrap(); - backend.finalize_block(BlockId::Number(1), None).unwrap(); + let block1 = + insert_block(&backend, 1, hash, None, Default::default(), vec![], None).unwrap(); + backend.finalize_block(&block1, None).unwrap(); assert_eq!(bc.body(BlockId::Number(0)).unwrap(), None); assert_eq!(bc.indexed_transaction(&x0_hash).unwrap(), None); assert_eq!(bc.indexed_transaction(&x1_hash).unwrap(), None); @@ -3501,7 +3502,7 @@ pub(crate) mod tests { for i in 1..10 { let mut op = backend.begin_operation().unwrap(); backend.begin_state_operation(&mut op, BlockId::Hash(blocks[4])).unwrap(); - op.mark_finalized(BlockId::Hash(blocks[i]), None).unwrap(); + op.mark_finalized(&blocks[i], None).unwrap(); backend.commit_operation(op).unwrap(); let bc = backend.blockchain(); if i < 6 { @@ -3676,7 +3677,7 @@ pub(crate) mod tests { let block1_a = insert_header(&backend, 1, block0, None, Default::default()); let block2_a = insert_header(&backend, 2, block1_a, None, Default::default()); - backend.finalize_block(BlockId::hash(block1_a), None).unwrap(); + backend.finalize_block(&block1_a, None).unwrap(); assert_eq!(backend.blockchain().leaves().unwrap(), vec![block2_a]); // Insert a fork prior to finalization point. Leave should not be created. diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index 5d73ef4911dcb..b18c6d226706b 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -878,17 +878,17 @@ where // plugable we cannot make a better choice here. usages that need // an accurate "best" block need to go through `SelectChain` // instead. - operation.op.mark_head(BlockId::Hash(block))?; + operation.op.mark_head(&block)?; } let enacted = route_from_finalized.enacted(); assert!(enacted.len() > 0); for finalize_new in &enacted[..enacted.len() - 1] { - operation.op.mark_finalized(BlockId::Hash(finalize_new.hash), None)?; + operation.op.mark_finalized(&finalize_new.hash, None)?; } assert_eq!(enacted.last().map(|e| e.hash), Some(block)); - operation.op.mark_finalized(BlockId::Hash(block), justification)?; + operation.op.mark_finalized(&block, justification)?; if notify { let finalized = From 54eff79b57a0de50793c9bccebed533da68d7979 Mon Sep 17 00:00:00 2001 From: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> Date: Thu, 20 Oct 2022 10:01:15 +0200 Subject: [PATCH 2/3] Review suggestion applied thx to @davxy --- client/api/src/in_mem.rs | 10 +++++----- client/db/src/lib.rs | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs index dac02c893b1dc..1cb61ba1a0b0a 100644 --- a/client/api/src/in_mem.rs +++ b/client/api/src/in_mem.rs @@ -223,10 +223,10 @@ impl Blockchain { } /// Set an existing block as head. - pub fn set_head(&self, id: Block::Hash) -> sp_blockchain::Result<()> { + pub fn set_head(&self, hash: Block::Hash) -> sp_blockchain::Result<()> { let header = self - .header(BlockId::Hash(id))? - .ok_or_else(|| sp_blockchain::Error::UnknownBlock(format!("{}", id)))?; + .header(BlockId::Hash(hash))? + .ok_or_else(|| sp_blockchain::Error::UnknownBlock(format!("{}", hash)))?; self.apply_head(&header) } @@ -738,10 +738,10 @@ where fn finalize_block( &self, - block: &Block::Hash, + hash: &Block::Hash, justification: Option, ) -> sp_blockchain::Result<()> { - self.blockchain.finalize_header(block, justification) + self.blockchain.finalize_header(hash, justification) } fn append_justification( diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 70d9ceec9d607..14ebafa01bec1 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -904,9 +904,9 @@ impl sc_client_api::backend::BlockImportOperation Ok(()) } - fn mark_head(&mut self, block: &Block::Hash) -> ClientResult<()> { + fn mark_head(&mut self, hash: &Block::Hash) -> ClientResult<()> { assert!(self.set_head.is_none(), "Only one set head per operation is allowed"); - self.set_head = Some(*block); + self.set_head = Some(*hash); Ok(()) } @@ -1992,16 +1992,16 @@ impl sc_client_api::backend::Backend for Backend { fn finalize_block( &self, - block: &Block::Hash, + hash: &Block::Hash, justification: Option, ) -> ClientResult<()> { let mut transaction = Transaction::new(); - let header = self.blockchain.expect_header(BlockId::Hash(*block))?; + let header = self.blockchain.expect_header(BlockId::Hash(*hash))?; let mut displaced = None; let m = self.finalize_block_with_transaction( &mut transaction, - block, + hash, &header, None, justification, From 860e253568f48de213c87e43d2bb0e5f9bb07aee Mon Sep 17 00:00:00 2001 From: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> Date: Thu, 20 Oct 2022 17:24:06 +0200 Subject: [PATCH 3/3] trigger CI job