Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable non-sequential block finalization. #21

Closed
wants to merge 1 commit into from
Closed

Enable non-sequential block finalization. #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions substrate/client/api/src/backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,7 @@ pub trait Backend<Block: BlockT>: AuxStore + Send + Sync {
&self,
hash: Block::Hash,
justification: Option<Justification>,
ensure_sequential_finalization: bool,
) -> sp_blockchain::Result<()>;

/// Append justification to the block with the given `hash`.
Expand Down
1 change: 1 addition & 0 deletions substrate/client/api/src/in_mem.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,7 @@ impl<Block: BlockT> backend::Backend<Block> for Backend<Block> {
&self,
hash: Block::Hash,
justification: Option<Justification>,
_ensure_sequesntial_finalization: bool,
) -> sp_blockchain::Result<()> {
self.blockchain.finalize_header(hash, justification)
}
Expand Down
8 changes: 7 additions & 1 deletion substrate/client/db/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1380,10 +1380,13 @@ impl<Block: BlockT> Backend<Block> {
justification: Option<Justification>,
current_transaction_justifications: &mut HashMap<Block::Hash, Justification>,
remove_displaced: bool,
ensure_sequential_finalization: bool,
) -> ClientResult<MetaUpdate<Block>> {
// TODO: ensure best chain contains this block.
let number = *header.number();
self.ensure_sequential_finalization(header, last_finalized)?;
if ensure_sequential_finalization {
self.ensure_sequential_finalization(header, last_finalized)?;
}
let with_state = sc_client_api::Backend::have_state_at(self, hash, number);

self.note_finalized(
Expand Down Expand Up @@ -1481,6 +1484,7 @@ impl<Block: BlockT> Backend<Block> {
justification,
&mut current_transaction_justifications,
finalized_blocks.peek().is_none(),
true,
)?);
last_finalized_hash = block_hash;
last_finalized_num = *block_header.number();
Expand Down Expand Up @@ -2114,6 +2118,7 @@ impl<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
&self,
hash: Block::Hash,
justification: Option<Justification>,
ensure_sequential_finalization: bool,
) -> ClientResult<()> {
let mut transaction = Transaction::new();
let header = self.blockchain.expect_header(hash)?;
Expand All @@ -2127,6 +2132,7 @@ impl<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
justification,
&mut current_transaction_justifications,
true,
ensure_sequential_finalization,
)?;

self.storage.db.commit(transaction)?;
Expand Down
4 changes: 4 additions & 0 deletions substrate/client/service/src/client/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,10 @@ where
fn clear_block_gap(&self) -> sp_blockchain::Result<()> {
self.backend.blockchain().clear_block_gap()
}

fn finalize_block(&self, hash: Block::Hash) -> sp_blockchain::Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

There is already apply_finality in Substrate, why do we have to add another API on top of this instead of fixing the API we already have?

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, apply_finality relies on the tree_route which demands existing headers from the genesis in our case. Changing the method seems much more invasive than preparing conditions for its correct work - I'm open to suggestions here though.

self.backend.finalize_block(hash, None, false)
}
}

/// Used in importing a block, where additional changes are made after the runtime
Expand Down
3 changes: 3 additions & 0 deletions substrate/client/service/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ pub struct RpcHandlers(Arc<RpcModule<()>>);
pub trait ClientExt<Block: BlockT, B: backend::Backend<Block>> {
/// Clear block gap after initial block insertion.
fn clear_block_gap(&self) -> sp_blockchain::Result<()>;

/// Finalize block disabling sequential order check.
fn finalize_block(&self, hash: Block::Hash) -> sp_blockchain::Result<()>;
}

impl RpcHandlers {
Expand Down
13 changes: 9 additions & 4 deletions substrate/primitives/blockchain/src/backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

//! Substrate blockchain trait

use log::warn;
use log::{debug, warn};
use parking_lot::RwLock;
use sp_runtime::{
generic::BlockId,
Expand Down Expand Up @@ -289,9 +289,14 @@ pub trait Backend<Block: BlockT>:
match finalized_chain.iter().rev().nth(distance_from_finalized as usize) {
Some(header) => (header.number, header.hash),
None => {
let header = self.header_metadata(
finalized_chain.front().expect("Not empty; qed").parent,
)?;
let parent = finalized_chain.front().expect("Not empty; qed").parent;
let header = match self.header_metadata(parent) {
Ok(header) => header,
Err(_) => {
debug!("Can't find obtain header metatadata: {parent:?}");
break;
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we never remove one of the blocks from the database?

Copy link
Author

Choose a reason for hiding this comment

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

If I understand the code correctly we skip only a missing block.

},
};
let result = (header.number, header.hash);
finalized_chain.push_front(header);
result
Expand Down
Loading