diff --git a/client/api/src/client.rs b/client/api/src/client.rs index 670c1711db948..d0053afeba97b 100644 --- a/client/api/src/client.rs +++ b/client/api/src/client.rs @@ -131,7 +131,7 @@ pub trait BlockBackend { -> sp_blockchain::Result; /// Get block justifications for the block with the given id. - fn justifications(&self, id: &BlockId) -> sp_blockchain::Result>; + fn justifications(&self, hash: &Block::Hash) -> sp_blockchain::Result>; /// Get block hash by number. fn block_hash(&self, number: NumberFor) -> sp_blockchain::Result>; diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs index 7b0e6f7b72575..6f33695fe4bf4 100644 --- a/client/api/src/in_mem.rs +++ b/client/api/src/in_mem.rs @@ -415,10 +415,8 @@ impl blockchain::Backend for Blockchain { .and_then(|b| b.extrinsics().map(|x| x.to_vec()))) } - fn justifications(&self, id: BlockId) -> sp_blockchain::Result> { - Ok(self.id(id).and_then(|hash| { - self.storage.read().blocks.get(&hash).and_then(|b| b.justifications().cloned()) - })) + fn justifications(&self, hash: &Block::Hash) -> sp_blockchain::Result> { + Ok(self.storage.read().blocks.get(&hash).and_then(|b| b.justifications().cloned())) } fn last_finalized(&self) -> sp_blockchain::Result { @@ -816,7 +814,7 @@ pub fn check_genesis_storage(storage: &Storage) -> sp_blockchain::Result<()> { #[cfg(test)] mod tests { use crate::{in_mem::Blockchain, NewBlockState}; - use sp_api::{BlockId, HeaderT}; + use sp_api::HeaderT; use sp_blockchain::Backend; use sp_runtime::{ConsensusEngineId, Justifications}; use substrate_test_runtime::{Block, Header, H256}; @@ -870,10 +868,7 @@ mod tests { just.append((ID2, vec![4])); just }; - assert_eq!( - blockchain.justifications(BlockId::Hash(last_finalized)).unwrap(), - Some(justifications) - ); + assert_eq!(blockchain.justifications(&last_finalized).unwrap(), Some(justifications)); } #[test] diff --git a/client/beefy/src/communication/request_response/incoming_requests_handler.rs b/client/beefy/src/communication/request_response/incoming_requests_handler.rs index c0910a60fba3b..3affbbe870ad7 100644 --- a/client/beefy/src/communication/request_response/incoming_requests_handler.rs +++ b/client/beefy/src/communication/request_response/incoming_requests_handler.rs @@ -26,7 +26,7 @@ use log::{debug, trace}; use sc_client_api::BlockBackend; use sc_network::{config as netconfig, config::RequestResponseConfig, PeerId, ReputationChange}; use sc_network_common::protocol::ProtocolName; -use sp_runtime::{generic::BlockId, traits::Block}; +use sp_runtime::traits::Block; use std::{marker::PhantomData, sync::Arc}; use crate::communication::request_response::{ @@ -149,13 +149,18 @@ where fn handle_request(&self, request: IncomingRequest) -> Result<(), Error> { // TODO (issue #12293): validate `request` and change peer reputation for invalid requests. - let maybe_encoded_proof = self - .client - .justifications(&BlockId::Number(request.payload.begin)) - .map_err(Error::Client)? - .and_then(|justifs| justifs.get(BEEFY_ENGINE_ID).cloned()) - // No BEEFY justification present. - .ok_or(()); + let maybe_encoded_proof = if let Some(hash) = + self.client.block_hash(request.payload.begin).map_err(Error::Client)? + { + self.client + .justifications(&hash) + .map_err(Error::Client)? + .and_then(|justifs| justifs.get(BEEFY_ENGINE_ID).cloned()) + // No BEEFY justification present. + .ok_or(()) + } else { + Err(()) + }; request .pending_response diff --git a/client/beefy/src/tests.rs b/client/beefy/src/tests.rs index b0a1433fafec6..62d8a45f4471c 100644 --- a/client/beefy/src/tests.rs +++ b/client/beefy/src/tests.rs @@ -741,9 +741,9 @@ fn beefy_importing_blocks() { let full_client = client.as_client(); let parent_id = BlockId::Number(0); - let block_id = BlockId::Number(1); let builder = full_client.new_block_at(&parent_id, Default::default(), false).unwrap(); let block = builder.build().unwrap().block; + let hashof1 = block.header.hash(); // Import without justifications. let mut justif_recv = justif_stream.subscribe(); @@ -760,7 +760,7 @@ fn beefy_importing_blocks() { // none in backend, assert_eq!( full_client - .justifications(&block_id) + .justifications(&hashof1) .unwrap() .and_then(|j| j.get(BEEFY_ENGINE_ID).cloned()), None @@ -784,6 +784,7 @@ fn beefy_importing_blocks() { let builder = full_client.new_block_at(&parent_id, Default::default(), false).unwrap(); let block = builder.build().unwrap().block; + let hashof2 = block.header.hash(); let mut justif_recv = justif_stream.subscribe(); assert_eq!( block_on(block_import.import_block(params(block, justif), HashMap::new())).unwrap(), @@ -798,7 +799,7 @@ fn beefy_importing_blocks() { // still not in backend (worker is responsible for appending to backend), assert_eq!( full_client - .justifications(&block_id) + .justifications(&hashof2) .unwrap() .and_then(|j| j.get(BEEFY_ENGINE_ID).cloned()), None @@ -826,6 +827,7 @@ fn beefy_importing_blocks() { let builder = full_client.new_block_at(&parent_id, Default::default(), false).unwrap(); let block = builder.build().unwrap().block; + let hashof3 = block.header.hash(); let mut justif_recv = justif_stream.subscribe(); assert_eq!( block_on(block_import.import_block(params(block, justif), HashMap::new())).unwrap(), @@ -841,7 +843,7 @@ fn beefy_importing_blocks() { // none in backend, assert_eq!( full_client - .justifications(&BlockId::Number(block_num)) + .justifications(&hashof3) .unwrap() .and_then(|j| j.get(BEEFY_ENGINE_ID).cloned()), None diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 305b92d696975..ea9e0d0c33999 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -709,7 +709,7 @@ where // a BEEFY justification, or at this session's boundary; voter will resume from there. loop { if let Some(true) = blockchain - .justifications(BlockId::hash(header.hash())) + .justifications(&header.hash()) .ok() .flatten() .map(|justifs| justifs.get(BEEFY_ENGINE_ID).is_some()) @@ -1401,7 +1401,7 @@ pub(crate) mod tests { })); // check BEEFY justifications are also appended to backend - let justifs = backend.blockchain().justifications(BlockId::number(2)).unwrap().unwrap(); + let justifs = backend.blockchain().justifications(&hashof2).unwrap().unwrap(); assert!(justifs.get(BEEFY_ENGINE_ID).is_some()) } diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 9d9a8d268dd81..af45e7c961fd3 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -642,8 +642,13 @@ impl sc_client_api::blockchain::Backend for BlockchainDb) -> ClientResult> { - match read_db(&*self.db, columns::KEY_LOOKUP, columns::JUSTIFICATIONS, id)? { + fn justifications(&self, hash: &Block::Hash) -> ClientResult> { + match read_db( + &*self.db, + columns::KEY_LOOKUP, + columns::JUSTIFICATIONS, + BlockId::::Hash(*hash), + )? { Some(justifications) => match Decode::decode(&mut &justifications[..]) { Ok(justifications) => Ok(Some(justifications)), Err(err) => @@ -2039,7 +2044,7 @@ impl sc_client_api::backend::Backend for Backend { } let justifications = if let Some(mut stored_justifications) = - self.blockchain.justifications(BlockId::Hash(*hash))? + self.blockchain.justifications(hash)? { if !stored_justifications.append(justification) { return Err(ClientError::BadJustification("Duplicate consensus engine ID".into())) @@ -3017,7 +3022,7 @@ pub(crate) mod tests { backend.finalize_block(&block1, justification.clone()).unwrap(); assert_eq!( - backend.blockchain().justifications(BlockId::Number(1)).unwrap(), + backend.blockchain().justifications(&block1).unwrap(), justification.map(Justifications::from), ); } @@ -3048,10 +3053,7 @@ pub(crate) mod tests { just.append(just1); just }; - assert_eq!( - backend.blockchain().justifications(BlockId::Number(1)).unwrap(), - Some(justifications), - ); + assert_eq!(backend.blockchain().justifications(&block1).unwrap(), Some(justifications),); } #[test] diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index a7042e26b1a71..3070581350662 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -183,10 +183,12 @@ where } }, AuthoritySetChangeId::Set(_, last_block_for_set) => { - let last_block_for_set_id = BlockId::Number(last_block_for_set); + let last_block_for_set_id = backend + .blockchain() + .expect_block_hash_from_id(&BlockId::Number(last_block_for_set))?; let justification = if let Some(grandpa_justification) = backend .blockchain() - .justifications(last_block_for_set_id)? + .justifications(&last_block_for_set_id)? .and_then(|justifications| justifications.into_justification(GRANDPA_ENGINE_ID)) { grandpa_justification diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 39379e69c9157..d2db1feea0fef 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -363,9 +363,11 @@ fn finalize_3_voters_no_observers() { runtime.spawn(initialize_grandpa(&mut net, peers)); net.peer(0).push_blocks(20, false); net.block_until_sync(); + let hashof20 = net.peer(0).client().info().best_hash; for i in 0..3 { assert_eq!(net.peer(i).client().info().best_number, 20, "Peer #{} failed to sync", i); + assert_eq!(net.peer(i).client().info().best_hash, hashof20, "Peer #{} failed to sync", i); } let net = Arc::new(Mutex::new(net)); @@ -373,12 +375,7 @@ fn finalize_3_voters_no_observers() { // normally there's no justification for finalized blocks assert!( - net.lock() - .peer(0) - .client() - .justifications(&BlockId::Number(20)) - .unwrap() - .is_none(), + net.lock().peer(0).client().justifications(&hashof20).unwrap().is_none(), "Extra justification for block#1", ); } @@ -616,19 +613,15 @@ fn justification_is_generated_periodically() { net.peer(0).push_blocks(32, false); net.block_until_sync(); + let hashof32 = net.peer(0).client().info().best_hash; + let net = Arc::new(Mutex::new(net)); run_to_completion(&mut runtime, 32, net.clone(), peers); // when block#32 (justification_period) is finalized, justification // is required => generated for i in 0..3 { - assert!(net - .lock() - .peer(i) - .client() - .justifications(&BlockId::Number(32)) - .unwrap() - .is_some()); + assert!(net.lock().peer(i).client().justifications(&hashof32).unwrap().is_some()); } } @@ -648,7 +641,7 @@ fn sync_justifications_on_change_blocks() { net.peer(0).push_blocks(20, false); // at block 21 we do add a transition which is instant - net.peer(0).generate_blocks(1, BlockOrigin::File, |builder| { + let hashof21 = net.peer(0).generate_blocks(1, BlockOrigin::File, |builder| { let mut block = builder.build().unwrap().block; add_scheduled_change( &mut block, @@ -672,25 +665,12 @@ fn sync_justifications_on_change_blocks() { // the first 3 peers are grandpa voters and therefore have already finalized // block 21 and stored a justification for i in 0..3 { - assert!(net - .lock() - .peer(i) - .client() - .justifications(&BlockId::Number(21)) - .unwrap() - .is_some()); + assert!(net.lock().peer(i).client().justifications(&hashof21).unwrap().is_some()); } // the last peer should get the justification by syncing from other peers futures::executor::block_on(futures::future::poll_fn(move |cx| { - if net - .lock() - .peer(3) - .client() - .justifications(&BlockId::Number(21)) - .unwrap() - .is_none() - { + if net.lock().peer(3).client().justifications(&hashof21).unwrap().is_none() { net.lock().poll(cx); Poll::Pending } else { @@ -1686,7 +1666,7 @@ fn imports_justification_for_regular_blocks_on_import() { ); // the justification should be imported and available from the client - assert!(client.justifications(&BlockId::Hash(block_hash)).unwrap().is_some()); + assert!(client.justifications(&block_hash).unwrap().is_some()); } #[test] diff --git a/client/finality-grandpa/src/warp_proof.rs b/client/finality-grandpa/src/warp_proof.rs index 10d02f790a0dc..786dfacf8b0b9 100644 --- a/client/finality-grandpa/src/warp_proof.rs +++ b/client/finality-grandpa/src/warp_proof.rs @@ -130,7 +130,7 @@ impl WarpSyncProof { } let justification = blockchain - .justifications(BlockId::Number(*last_block))? + .justifications(&header.hash())? .and_then(|just| just.into_justification(GRANDPA_ENGINE_ID)) .expect( "header is last in set and contains standard change signal; \ diff --git a/client/network/sync/src/block_request_handler.rs b/client/network/sync/src/block_request_handler.rs index dbde5fc5d8c22..08b8cffa38714 100644 --- a/client/network/sync/src/block_request_handler.rs +++ b/client/network/sync/src/block_request_handler.rs @@ -331,11 +331,8 @@ where let number = *header.number(); let hash = header.hash(); let parent_hash = *header.parent_hash(); - let justifications = if get_justification { - self.client.justifications(&BlockId::Hash(hash))? - } else { - None - }; + let justifications = + if get_justification { self.client.justifications(&hash)? } else { None }; let (justifications, justification, is_empty_justification) = if support_multiple_justifications { diff --git a/client/network/test/src/block_import.rs b/client/network/test/src/block_import.rs index a2bd5276c31d6..a1d42f1e60440 100644 --- a/client/network/test/src/block_import.rs +++ b/client/network/test/src/block_import.rs @@ -40,7 +40,7 @@ fn prepare_good_block() -> (TestClient, Hash, u64, PeerId, IncomingBlock) let (hash, number) = (client.block_hash(1).unwrap().unwrap(), 1); let header = client.header(&BlockId::Number(1)).unwrap(); - let justifications = client.justifications(&BlockId::Number(1)).unwrap(); + let justifications = client.justifications(&hash).unwrap(); let peer_id = PeerId::random(); ( client, diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index a6d73b53efdf0..f348d5cf94c43 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -176,8 +176,11 @@ impl PeersClient { self.backend.have_state_at(&header.hash(), *header.number()) } - pub fn justifications(&self, block: &BlockId) -> ClientResult> { - self.client.justifications(block) + pub fn justifications( + &self, + hash: &::Hash, + ) -> ClientResult> { + self.client.justifications(hash) } pub fn finality_notification_stream(&self) -> FinalityNotifications { diff --git a/client/network/test/src/sync.rs b/client/network/test/src/sync.rs index 399062a88d269..9ae3014e497ce 100644 --- a/client/network/test/src/sync.rs +++ b/client/network/test/src/sync.rs @@ -246,15 +246,16 @@ fn sync_justifications() { net.peer(0).push_blocks(20, false); net.block_until_sync(); - // there's currently no justification for block #10 - assert_eq!(net.peer(0).client().justifications(&BlockId::Number(10)).unwrap(), None); - assert_eq!(net.peer(1).client().justifications(&BlockId::Number(10)).unwrap(), None); - - // we finalize block #10, #15 and #20 for peer 0 with a justification let backend = net.peer(0).client().as_backend(); let hashof10 = backend.blockchain().expect_block_hash_from_id(&BlockId::Number(10)).unwrap(); let hashof15 = backend.blockchain().expect_block_hash_from_id(&BlockId::Number(15)).unwrap(); let hashof20 = backend.blockchain().expect_block_hash_from_id(&BlockId::Number(20)).unwrap(); + + // there's currently no justification for block #10 + assert_eq!(net.peer(0).client().justifications(&hashof10).unwrap(), None); + assert_eq!(net.peer(1).client().justifications(&hashof10).unwrap(), None); + + // we finalize block #10, #15 and #20 for peer 0 with a justification let just = (*b"FRNK", Vec::new()); net.peer(0) .client() @@ -269,25 +270,25 @@ fn sync_justifications() { .finalize_block(&hashof20, Some(just.clone()), true) .unwrap(); - let h1 = net.peer(1).client().header(&BlockId::Number(10)).unwrap().unwrap(); - let h2 = net.peer(1).client().header(&BlockId::Number(15)).unwrap().unwrap(); - let h3 = net.peer(1).client().header(&BlockId::Number(20)).unwrap().unwrap(); + let hashof10 = net.peer(1).client().header(&BlockId::Number(10)).unwrap().unwrap().hash(); + let hashof15 = net.peer(1).client().header(&BlockId::Number(15)).unwrap().unwrap().hash(); + let hashof20 = net.peer(1).client().header(&BlockId::Number(20)).unwrap().unwrap().hash(); // peer 1 should get the justifications from the network - net.peer(1).request_justification(&h1.hash().into(), 10); - net.peer(1).request_justification(&h2.hash().into(), 15); - net.peer(1).request_justification(&h3.hash().into(), 20); + net.peer(1).request_justification(&hashof10, 10); + net.peer(1).request_justification(&hashof15, 15); + net.peer(1).request_justification(&hashof20, 20); block_on(futures::future::poll_fn::<(), _>(|cx| { net.poll(cx); - for height in (10..21).step_by(5) { - if net.peer(0).client().justifications(&BlockId::Number(height)).unwrap() != + for hash in [hashof10, hashof15, hashof20] { + if net.peer(0).client().justifications(&hash).unwrap() != Some(Justifications::from((*b"FRNK", Vec::new()))) { return Poll::Pending } - if net.peer(1).client().justifications(&BlockId::Number(height)).unwrap() != + if net.peer(1).client().justifications(&hash).unwrap() != Some(Justifications::from((*b"FRNK", Vec::new()))) { return Poll::Pending @@ -321,9 +322,9 @@ fn sync_justifications_across_forks() { block_on(futures::future::poll_fn::<(), _>(|cx| { net.poll(cx); - if net.peer(0).client().justifications(&BlockId::Number(10)).unwrap() == + if net.peer(0).client().justifications(&f1_best).unwrap() == Some(Justifications::from((*b"FRNK", Vec::new()))) && - net.peer(1).client().justifications(&BlockId::Number(10)).unwrap() == + net.peer(1).client().justifications(&f1_best).unwrap() == Some(Justifications::from((*b"FRNK", Vec::new()))) { Poll::Ready(()) @@ -952,14 +953,14 @@ fn multiple_requests_are_accepted_as_long_as_they_are_not_fulfilled() { net.peer(0).push_blocks(10, false); net.block_until_sync(); - // there's currently no justification for block #10 - assert_eq!(net.peer(0).client().justifications(&BlockId::Number(10)).unwrap(), None); - assert_eq!(net.peer(1).client().justifications(&BlockId::Number(10)).unwrap(), None); + let hashof10 = net.peer(1).client().header(&BlockId::Number(10)).unwrap().unwrap().hash(); - let h1 = net.peer(1).client().header(&BlockId::Number(10)).unwrap().unwrap(); + // there's currently no justification for block #10 + assert_eq!(net.peer(0).client().justifications(&hashof10).unwrap(), None); + assert_eq!(net.peer(1).client().justifications(&hashof10).unwrap(), None); // Let's assume block 10 was finalized, but we still need the justification from the network. - net.peer(1).request_justification(&h1.hash().into(), 10); + net.peer(1).request_justification(&hashof10, 10); // Let's build some more blocks and wait always for the network to have synced them for _ in 0..5 { @@ -987,7 +988,7 @@ fn multiple_requests_are_accepted_as_long_as_they_are_not_fulfilled() { block_on(futures::future::poll_fn::<(), _>(|cx| { net.poll(cx); - if net.peer(1).client().justifications(&BlockId::Number(10)).unwrap() != + if net.peer(1).client().justifications(&hashof10).unwrap() != Some(Justifications::from((*b"FRNK", Vec::new()))) { return Poll::Pending diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index b5ab06a0318c5..6d463f337aabc 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -1948,7 +1948,7 @@ where Ok(match self.header(id)? { Some(header) => { let hash = header.hash(); - match (self.body(&hash)?, self.justifications(id)?) { + match (self.body(&hash)?, self.justifications(&hash)?) { (Some(extrinsics), justifications) => Some(SignedBlock { block: Block::new(header, extrinsics), justifications }), _ => None, @@ -1962,8 +1962,8 @@ where Client::block_status(self, id) } - fn justifications(&self, id: &BlockId) -> sp_blockchain::Result> { - self.backend.blockchain().justifications(*id) + fn justifications(&self, hash: &Block::Hash) -> sp_blockchain::Result> { + self.backend.blockchain().justifications(hash) } fn block_hash(&self, number: NumberFor) -> sp_blockchain::Result> { diff --git a/client/service/test/src/client/mod.rs b/client/service/test/src/client/mod.rs index b9aec421802c9..c60ff4dd09d7b 100644 --- a/client/service/test/src/client/mod.rs +++ b/client/service/test/src/client/mod.rs @@ -884,11 +884,11 @@ fn import_with_justification() { assert_eq!(client.chain_info().finalized_hash, a3.hash()); - assert_eq!(client.justifications(&BlockId::Hash(a3.hash())).unwrap(), Some(justification)); + assert_eq!(client.justifications(&a3.hash()).unwrap(), Some(justification)); - assert_eq!(client.justifications(&BlockId::Hash(a1.hash())).unwrap(), None); + assert_eq!(client.justifications(&a1.hash()).unwrap(), None); - assert_eq!(client.justifications(&BlockId::Hash(a2.hash())).unwrap(), None); + assert_eq!(client.justifications(&a2.hash()).unwrap(), None); finality_notification_check(&mut finality_notifications, &[a1.hash(), a2.hash()], &[]); finality_notification_check(&mut finality_notifications, &[a3.hash()], &[]); diff --git a/primitives/blockchain/src/backend.rs b/primitives/blockchain/src/backend.rs index 610ceb0cf9323..e5764354e90f3 100644 --- a/primitives/blockchain/src/backend.rs +++ b/primitives/blockchain/src/backend.rs @@ -91,7 +91,7 @@ pub trait Backend: /// Get block body. Returns `None` if block is not found. fn body(&self, hash: &Block::Hash) -> Result::Extrinsic>>>; /// Get block justifications. Returns `None` if no justification exists. - fn justifications(&self, id: BlockId) -> Result>; + fn justifications(&self, hash: &Block::Hash) -> Result>; /// Get last finalized block hash. fn last_finalized(&self) -> Result;