Why request block body for fast sync?

[liuchengxu]

Is there an existing issue?

• I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

• This is not a support question.

Description of bug

If I read correctly, the docs https://github.com/paritytech/polkadot-sdk/blob/3fe22d17c4904c5ae016034b6ec2c335464b4165/substrate/client/network/README.md#fast-sync indicates that the fast sync is expected only to download and verify the header chain, and the subsequent state snapshot.

But the inline docs are contradictory, saying Downloading blocks:

polkadot-sdk/substrate/client/cli/src/arg_enums.rs

Lines 275 to 278 in 3fe22d1

	/// Download blocks without executing them. Download latest state with proofs.
	Fast,
	/// Download blocks without executing them. Download latest state without proofs.
	FastUnsafe,

and the block body is indeed requested for --sync fast and --sync fast-unsafe.

polkadot-sdk/substrate/client/network/sync/src/strategy/chain_sync.rs

Lines 1220 to 1225 in 3fe22d1

	ChainSyncMode::LightState { storage_chain_mode: false, .. } =>
	BlockAttributes::HEADER | BlockAttributes::JUSTIFICATION | BlockAttributes::BODY,
	ChainSyncMode::LightState { storage_chain_mode: true, .. } =>
	BlockAttributes::HEADER |
	BlockAttributes::JUSTIFICATION |
	BlockAttributes::INDEXED_BODY,

I'm looking for a sync mode that only downloads the headers and the state snapshot. I initially assumed that the --sync fast/fast-unsafe options would fulfill this requirement. This looks like a bug to me, could you please clarify this confusion?

Steps to reproduce

No response

[liuchengxu]

After looking into the original PR of fast sync paritytech/substrate#8884, I found this comment paritytech/substrate#8884 (comment), so it was intentionally to request the body in fast sync. However, this is a mismatch between the implementation and spec IMHO. The existing documentation always mentions downloading the header chain, but the block history is downloaded silently. While it may not be a big problem for the chain without a huge block history, for a project like subcoin, it matters about downloading hundreds of MiB or hundreds of GiB data before downloading the state. We should fix this mismatch in the implementation, what do you think? @arkpar

[arkpar]

Indeed, fast sync is supposed to download and verify the header chain first, then the state, then block bodies in the background.

[liuchengxu]

Great that we're on the same page! Any insights on the implementation? If I read the code correctly, the gap sync will be started automatically to download the block history once the block gap is detected in client.info().block_gap within the chain sync restart function. How about reusing the gap sync to download the block bodies by updating the block_gap for fast sync? @arkpar

[arkpar]

Right, the backgroudn block body download should utilize the same mechanism as the warp sync. The gap in this case is in block bodies, rather than whole block. The Client should have no issue importing a body for an existing block header.
@dmitry-markin can give you further hints, as I have not touched that code for a while.

[dmitry-markin]

Going through Backend::try_commit_operation() it looks like if block header is imported, it is not considered to be a block gap anymore. So gap sync probably won't run after fast sync because no gap will be detected. Makes sense double checking by not requesting block bodies during fast sync and checking if gap sync was performed.

[liuchengxu]

So I guess we need to either change the logic of detecting the block gap within Backend::try_commit_operation() or update the block gap manually after the state sync is finished (and ensure the Client can import the body with an existing header) so that the gap sync can be started as expected. The former seems to be more straightforward.

[dmitry-markin]

Yes, the first approach is better. Worth checking where else the block gap is used and if it won't break anything.

[liuchengxu]

We need a formal definition of a block gap before implementation. The concept of a block gap has evolved. Initially, it could only be created during warp sync; now, it can be created by both warp sync and fast sync. Warp sync creates a block gap by first downloading the finality proof and state at a specific height and then proceeding to download the block history. In contrast, fast sync downloads the header chain and state at a specific height before downloading the block history.

Block Gap Creation and Updates

• Warp Sync
  • Creating a Block Gap
    • A block gap is created when the warp block is imported and the following condition is met: !parent_header_exists && number > best_num + 1

      polkadot-sdk/substrate/client/db/src/lib.rs

      Lines 1710 to 1711 in da11c7e

      	} else if number > best_num + One::one() &&
      	number > One::one() && self.blockchain.header(parent_hash)?.is_none()

  • Updating a Block Gap (During Block History Download)
    • The block gap is decreased when a block in the history is downloaded
      • Condition: number == block_gap.start

        polkadot-sdk/substrate/client/db/src/lib.rs

        Line 1688 in da11c7e

        if number == start {

      • Action: gap.start += 1 => gap decreased

        polkadot-sdk/substrate/client/db/src/lib.rs

        Line 1689 in da11c7e

        start += One::one();

• Fast Sync
  • Creating a Block Gap
    • A block gap is created when a new header is imported and the following condition is met: parent_header_exists && !body_exists && number == best_num + 1
  • Updating a Block Gap (During Header Chain and Block History Download)
    • Header Chain Download: The block gap is increased when a new header is downloaded:
      • Condition: number == gap.end + 1 && !body_exists
      • Action: gap.end += 1 => gap increased
    • Block History Download: The block gap is decreased when a block in the history is downloaded:
      • Condition number == block_gap.start
      • Action: gap.start += 1 => gap decreased

Block Gap Check

• Previous check: !existing_header.

  polkadot-sdk/substrate/client/db/src/lib.rs

  Line 1658 in da11c7e

  if !existing_header {

• New check: !existing_header || !body_exists.

The logic for decreasing a block gap is consistent across both warp sync and fast sync, but fast sync can increase the gap during the header chain download.

Implementation Plan

1. Refactor BlockGap.

The current BlockGap is represented as Option<(NumberFor<Block>, NumberFor<Block>)>. To improve extensibility, this tuple should be refactored into a struct. Compatibility concerns should be addressed by upgrading the old gap storage to the new structure.

pub enum BlockGapType {
    // Gap created by warp sync.
    MissingHeaderAndBody,
    // Gap created by fast sync.
    MissingBody,
}

pub struct BlockGap<N> {
    start: N,
    end: N,
    gap_type: BlockGapType,
}

2. Add Block Gap Support for Fast Sync.

The primary changes will occur in try_commit_operation, with adjustments to the ChainSync logic as well.

I've implemented a working prototype locally based on the ideas above, and it appears to be functioning well. I would appreciate it if you could review the changes and provide feedback in case I missed something important. @arkpar @dmitry-markin

[dmitry-markin]

@liuchengxu sure, feel free to publish your local changes as a PR / draft PR.

[liuchengxu]

Please have a look at #5540 before I send a new PR @dmitry-markin .