Skip to content

Commit

Permalink
add more details about BlockImport, small fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
timwu20 committed Feb 7, 2025
1 parent 3e72f8d commit d7bd44c
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions docs/docs/design/grandpa-client.md
Original file line number Diff line number Diff line change
Expand Up @@ -256,11 +256,12 @@ pub trait SyncEventStream: Send + Sync {

### Implementation of `BlockImport`

There is a [`BlockImport`](https://github.com/paritytech/polkadot-sdk/blob/030cb4a71b0b390626a586bfe7117b7c66b4700c/substrate/client/consensus/common/src/block_import.rs#L308) trait that `sc_consensus_grandpa` implements. In substrate, the `BlockImport` trait is called through a pipelining type called [`BasicQueue`]. An instance of [`BasicQueue`] creates a pipeline of calls to a sequence of implementations of `BlockImport`. [`BlockImportParams`](https://github.com/paritytech/polkadot-sdk/blob/030cb4a71b0b390626a586bfe7117b7c66b4700c/substrate/client/consensus/common/src/block_import.rs#L170) are provided to each implementation as it progresses through the pipeline for `BlockImport::import_block` function, and is expected to return an `ImportResult` which an enum where the [`ImportResult::Imported`](https://github.com/paritytech/polkadot-sdk/blob/030cb4a71b0b390626a586bfe7117b7c66b4700c/substrate/client/consensus/common/src/block_import.rs#L34) variant is of type [`ImportedAux`](https://github.com/paritytech/polkadot-sdk/blob/030cb4a71b0b390626a586bfe7117b7c66b4700c/substrate/client/consensus/common/src/block_import.rs#L47), which contains data associated with the imported block. Given that this is the process in substrate, both BEEFY and GRANDPA client implementations currently implement this, and the final implementation in the pipeline is `Client`, I think it makes sense to replicate this functionality given we already already begun replicating the `Client` type.
There is a [`BlockImport`](https://github.com/paritytech/polkadot-sdk/blob/030cb4a71b0b390626a586bfe7117b7c66b4700c/substrate/client/consensus/common/src/block_import.rs#L308) trait that `sc_consensus_grandpa` implements. In substrate, the `BlockImport` trait is called through a pipelining type called [`BasicQueue`]. An instance of [`BasicQueue`] creates a pipeline of calls to a sequence of implementations of `BlockImport`. [`BlockImportParams`](https://github.com/paritytech/polkadot-sdk/blob/030cb4a71b0b390626a586bfe7117b7c66b4700c/substrate/client/consensus/common/src/block_import.rs#L170) are provided to each implementation as it progresses through the pipeline for `BlockImport::import_block` function, and is expected to return an `ImportResult` which is an enum where the [`ImportResult::Imported`](https://github.com/paritytech/polkadot-sdk/blob/030cb4a71b0b390626a586bfe7117b7c66b4700c/substrate/client/consensus/common/src/block_import.rs#L34) variant is of type [`ImportedAux`](https://github.com/paritytech/polkadot-sdk/blob/030cb4a71b0b390626a586bfe7117b7c66b4700c/substrate/client/consensus/common/src/block_import.rs#L47) contains data associated with the imported block. Given that this is the process in substrate, both BEEFY and GRANDPA client implementations currently implement this, and the final implementation in the pipeline is `Client`, I think it makes sense to replicate this functionality given have already begun replicating the `Client` type. This does imply that we alter the translation shim work of `BlockState.AddBlock` in #4466, to call the new pipeline as well as remove any existing finality domain specific logic in BABE and in sync.


### Integration tests

There are tests found in `sc_consensus_grandpa` ([link](https://github.com/paritytech/polkadot-sdk/blob/feac7a521092c599d47df3e49084e6bff732c7db/substrate/client/consensus/grandpa/src/tests.rs#L19)) that create a mock network to test general functionality. There are both startup, shutdown tests, as well as testing finalization of blocks and persistence of voter state. I think it would be prudent to replicate the same tests minus the ones that utilize the `ObvserverWork`.
There are tests found in `sc_consensus_grandpa` ([link](https://github.com/paritytech/polkadot-sdk/blob/feac7a521092c599d47df3e49084e6bff732c7db/substrate/client/consensus/grandpa/src/tests.rs#L19)) that create a mock network to test general functionality. There are both startup, shutdown tests, as well as testing finalization of blocks and persistence of voter state. I think it would be prudent to replicate the same tests minus the ones that utilize `ObserverWork`.


[`VoterWork`]:(https://github.com/paritytech/polkadot-sdk/blob/d2fd53645654d3b8e12cbf735b67b93078d70113/substrate/client/consensus/grandpa/src/lib.rs#L867)
Expand Down

0 comments on commit d7bd44c

Please sign in to comment.