Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

BlockId removal: refactor: Backend::append_justification #12551

Merged
merged 5 commits into from
Oct 30, 2022
Merged

BlockId removal: refactor: Backend::append_justification #12551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c43f308
BlockId removal: refactor: Backend::append_justification
michalkucharczyk Oct 20, 2022
fd55fb7
Error message improved
michalkucharczyk Oct 24, 2022
4930349
single error message in beefy::finalize
michalkucharczyk Oct 28, 2022
0ae7f17
println removed
michalkucharczyk Oct 30, 2022
389e3b1
Merge remote-tracking branch 'origin/master' into mku-blockid-append-…
michalkucharczyk Oct 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions client/api/src/backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ use sp_blockchain;
use sp_consensus::BlockOrigin;
use sp_core::offchain::OffchainStorage;
use sp_runtime::{
generic::BlockId,
traits::{Block as BlockT, HashFor, NumberFor},
Justification, Justifications, StateVersion, Storage,
};
Expand Down Expand Up @@ -485,12 +484,12 @@ pub trait Backend<Block: BlockT>: AuxStore + Send + Sync {
justification: Option<Justification>,
) -> sp_blockchain::Result<()>;

/// Append justification to the block with the given Id.
/// Append justification to the block with the given `hash`.
///
/// This should only be called for blocks that are already finalized.
fn append_justification(
&self,
block: BlockId<Block>,
hash: &Block::Hash,
justification: Justification,
) -> sp_blockchain::Result<()>;

Expand Down
20 changes: 10 additions & 10 deletions client/api/src/in_mem.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,10 +295,9 @@ impl<Block: BlockT> Blockchain<Block> {

fn append_justification(
&self,
id: BlockId<Block>,
hash: &Block::Hash,
justification: Justification,
) -> sp_blockchain::Result<()> {
let hash = self.expect_block_hash_from_id(&id)?;
let mut storage = self.storage.write();

let block = storage
Expand Down Expand Up @@ -745,10 +744,10 @@ where

fn append_justification(
&self,
block: BlockId<Block>,
hash: &Block::Hash,
justification: Justification,
) -> sp_blockchain::Result<()> {
self.blockchain.append_justification(block, justification)
self.blockchain.append_justification(hash, justification)
}

fn blockchain(&self) -> &Self::Blockchain {
Expand Down Expand Up @@ -865,26 +864,27 @@ mod tests {
fn append_and_retrieve_justifications() {
let blockchain = test_blockchain();
let last_finalized = blockchain.last_finalized().unwrap();
let block = BlockId::Hash(last_finalized);

blockchain.append_justification(block, (ID2, vec![4])).unwrap();
blockchain.append_justification(&last_finalized, (ID2, vec![4])).unwrap();
let justifications = {
let mut just = Justifications::from((ID1, vec![3]));
just.append((ID2, vec![4]));
just
};
assert_eq!(blockchain.justifications(block).unwrap(), Some(justifications));
assert_eq!(
blockchain.justifications(BlockId::Hash(last_finalized)).unwrap(),
Some(justifications)
);
}

#[test]
fn store_duplicate_justifications_is_forbidden() {
let blockchain = test_blockchain();
let last_finalized = blockchain.last_finalized().unwrap();
let block = BlockId::Hash(last_finalized);

blockchain.append_justification(block, (ID2, vec![0])).unwrap();
blockchain.append_justification(&last_finalized, (ID2, vec![0])).unwrap();
assert!(matches!(
blockchain.append_justification(block, (ID2, vec![1])),
blockchain.append_justification(&last_finalized, (ID2, vec![1])),
Err(sp_blockchain::Error::BadJustification(_)),
));
}
Expand Down
32 changes: 19 additions & 13 deletions client/beefy/src/worker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,20 +500,22 @@ where

self.on_demand_justifications.cancel_requests_older_than(block_num);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something for @acatangiu. Why do we use the block number here as part of the commitment and not the block hash?

Copy link
Contributor

@acatangiu acatangiu Oct 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All BEEFY voting and justifications/proofs happen on GRANDPA-finalized blocks, therefore block numbers are enough to uniquely identify blocks. (My best guess) block numbers were initially used instead of hashes to lower cognitive complexity when following BEEFY proofs/progress.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

therefore block numbers are enough to uniquely identify blocks.

Yeah I know.

(My best guess) block numbers were initially used instead of hashes to lower cognitive complexity when following BEEFY proofs/progress

Hmm okay :P


if let Err(e) = self.backend.append_justification(
BlockId::Number(block_num),
(BEEFY_ENGINE_ID, finality_proof.encode()),
) {
if let Err(e) = self
.backend
.blockchain()
.expect_block_hash_from_id(&BlockId::Number(block_num))
.and_then(|hash| {
self.links
.to_rpc_best_block_sender
.notify(|| Ok::<_, ()>(hash))
.expect("forwards closure result; the closure always returns Ok; qed.");

self.backend
.append_justification(&hash, (BEEFY_ENGINE_ID, finality_proof.encode()))
}) {
error!(target: "beefy", "🥩 Error {:?} on appending justification: {:?}", e, finality_proof);
}

self.backend.blockchain().hash(block_num).ok().flatten().map(|hash| {
self.links
.to_rpc_best_block_sender
.notify(|| Ok::<_, ()>(hash))
.expect("forwards closure result; the closure always returns Ok; qed.")
});

self.links
.to_rpc_justif_sender
.notify(|| Ok::<_, ()>(finality_proof))
Expand Down Expand Up @@ -1546,8 +1548,10 @@ pub(crate) mod tests {
commitment,
signatures: vec![None],
});
let hashof10 =
backend.blockchain().expect_block_hash_from_id(&BlockId::Number(10)).unwrap();
backend
.append_justification(BlockId::Number(10), (BEEFY_ENGINE_ID, justif.encode()))
.append_justification(&hashof10, (BEEFY_ENGINE_ID, justif.encode()))
.unwrap();

// initialize voter at block 13, expect rounds initialized at last beefy finalized 10
Expand Down Expand Up @@ -1580,8 +1584,10 @@ pub(crate) mod tests {
commitment,
signatures: vec![None],
});
let hashof12 =
backend.blockchain().expect_block_hash_from_id(&BlockId::Number(12)).unwrap();
backend
.append_justification(BlockId::Number(12), (BEEFY_ENGINE_ID, justif.encode()))
.append_justification(&hashof12, (BEEFY_ENGINE_ID, justif.encode()))
.unwrap();

// initialize voter at block 13, expect rounds initialized at last beefy finalized 12
Expand Down
15 changes: 7 additions & 8 deletions client/db/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2013,12 +2013,11 @@ impl<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {

fn append_justification(
&self,
block: BlockId<Block>,
hash: &Block::Hash,
justification: Justification,
) -> ClientResult<()> {
let mut transaction: Transaction<DbHash> = 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(*hash))?;
let number = *header.number();

// Check if the block is finalized first.
Expand All @@ -2027,13 +2026,13 @@ impl<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {

// We can do a quick check first, before doing a proper but more expensive check
if number > self.blockchain.info().finalized_number ||
(hash != last_finalized && !is_descendent_of(&hash, &last_finalized)?)
(*hash != last_finalized && !is_descendent_of(hash, &last_finalized)?)
{
return Err(ClientError::NotInFinalizedChain)
}

let justifications = if let Some(mut stored_justifications) =
self.blockchain.justifications(block)?
self.blockchain.justifications(BlockId::Hash(*hash))?
{
if !stored_justifications.append(justification) {
return Err(ClientError::BadJustification("Duplicate consensus engine ID".into()))
Expand All @@ -2045,7 +2044,7 @@ impl<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {

transaction.set_from_vec(
columns::JUSTIFICATIONS,
&utils::number_and_hash_to_lookup_key(number, hash)?,
&utils::number_and_hash_to_lookup_key(number, *hash)?,
justifications.encode(),
);

Expand Down Expand Up @@ -3029,11 +3028,11 @@ pub(crate) mod tests {
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();
backend.append_justification(&block1, just1.clone()).unwrap();

let just2 = (CONS1_ENGINE_ID, vec![6, 7]);
assert!(matches!(
backend.append_justification(BlockId::Number(1), just2),
backend.append_justification(&block1, just2),
Err(ClientError::BadJustification(_))
));

Expand Down