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.

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

Allow to disable gap creation during block import #5343

Merged
19 changes: 19 additions & 0 deletions prdoc/pr_5343.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
title: Allow to disable gap creation during block import

doc:
- audience: Node Dev
description: |
New property `BlockImportParams::create_gap` allows to change whether to create block gap in case block
has no parent (defaults to `true` keeping existing behavior), which is helpful for sync protocols that do not need
to sync the gap after this happens. `BlockImportOperation::create_gap()` method was also introduced, though in
most cases `BlockImportParams::create_gap` will be used.

crates:
- name: sc-client-api
bump: major
- name: sc-consensus
bump: minor
- name: sc-client-db
bump: minor
- name: sc-service
bump: minor
3 changes: 3 additions & 0 deletions substrate/client/api/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,9 @@ pub trait BlockImportOperation<Block: BlockT> {
/// Add a transaction index operation.
fn update_transaction_index(&mut self, index: Vec<IndexOperation>)
-> sp_blockchain::Result<()>;

/// Configure whether to create a block gap if newly imported block is missing parent
fn create_gap(&mut self, create_gap: bool);
}

/// Interface for performing operations on the backend.
Expand Down
2 changes: 2 additions & 0 deletions substrate/client/api/src/in_mem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,8 @@ impl<Block: BlockT> backend::BlockImportOperation<Block> for BlockImportOperatio
) -> sp_blockchain::Result<()> {
Ok(())
}

fn create_gap(&mut self, _create_gap: bool) {}
}

/// In-memory backend. Keeps all states and blocks in memory.
Expand Down
3 changes: 3 additions & 0 deletions substrate/client/consensus/common/src/block_import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,8 @@ pub struct BlockImportParams<Block: BlockT> {
pub fork_choice: Option<ForkChoiceStrategy>,
/// Re-validate existing block.
pub import_existing: bool,
/// Whether to create "block gap" in case this block doesn't have parent.
pub create_gap: bool,
/// Cached full header hash (with post-digests applied).
pub post_hash: Option<Block::Hash>,
}
Expand All @@ -234,6 +236,7 @@ impl<Block: BlockT> BlockImportParams<Block> {
auxiliary: Vec::new(),
fork_choice: None,
import_existing: false,
create_gap: true,
post_hash: None,
}
}
Expand Down
11 changes: 9 additions & 2 deletions substrate/client/db/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -841,6 +841,7 @@ pub struct BlockImportOperation<Block: BlockT> {
finalized_blocks: Vec<(Block::Hash, Option<Justification>)>,
set_head: Option<Block::Hash>,
commit_state: bool,
create_gap: bool,
index_ops: Vec<IndexOperation>,
}

Expand Down Expand Up @@ -995,6 +996,10 @@ impl<Block: BlockT> sc_client_api::backend::BlockImportOperation<Block>
self.index_ops = index_ops;
Ok(())
}

fn create_gap(&mut self, create_gap: bool) {
nazar-pc marked this conversation as resolved.
Show resolved Hide resolved
self.create_gap = create_gap;
}
}

struct StorageDb<Block: BlockT> {
Expand Down Expand Up @@ -1707,8 +1712,9 @@ impl<Block: BlockT> Backend<Block> {
&(start, end).encode(),
);
}
} else if number > best_num + One::one() &&
number > One::one() && self.blockchain.header(parent_hash)?.is_none()
} else if operation.create_gap &&
Copy link
Contributor

Choose a reason for hiding this comment

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

cc #4984

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly sure how it is related necessarily, it still had to pass through consensus successfully before ending up here IIUC.

I do see those "block has an unknown parent" in some cases though and subscribed to linked issue a while ago.

number > best_num + One::one() &&
nazar-pc marked this conversation as resolved.
Show resolved Hide resolved
self.blockchain.header(parent_hash)?.is_none()
{
let gap = (best_num + One::one(), number - One::one());
transaction.set(columns::META, meta_keys::BLOCK_GAP, &gap.encode());
Expand Down Expand Up @@ -2060,6 +2066,7 @@ impl<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
finalized_blocks: Vec::new(),
set_head: None,
commit_state: false,
create_gap: true,
index_ops: Default::default(),
})
}
Expand Down
3 changes: 3 additions & 0 deletions substrate/client/service/src/client/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,7 @@ where
fork_choice,
intermediates,
import_existing,
create_gap,
..
} = import_block;

Expand All @@ -537,6 +538,8 @@ where

*self.importing_block.write() = Some(hash);

operation.op.create_gap(create_gap);

let result = self.execute_and_import_block(
operation,
origin,
Expand Down
Loading