Skip to content

Commit

Permalink
a few more revisions
Browse files Browse the repository at this point in the history
  • Loading branch information
@timwu20
timwu20 committed Feb 11, 2025
1 parent eaccfb4 commit 5a4e20f
Showing 1 changed file with 5 additions and 5 deletions.
10 changes: 5 additions & 5 deletions docs/docs/design/grandpa-client.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ The following high level types, `BlockImport` trait implementation, and integrat

### `Environment`

[`Environment`] is a type that utilizes already introduced dependencies (`VoterSet`, `SharedAuthoritySet`, etc.) to provide functionality to the [`VoterWork`] type. It is a self contained type that encapsulates all dependencies of the [`VoterWork`] type. The [`Environment`] contains a client dependency which is a subset of all the traits that the substrate `Client` implements. This subset of traits is defined as `ClientForGrandpa` and is described as:
[`Environment`] is a type that utilizes already introduced dependencies (`VoterSet`, `SharedAuthoritySet`, etc.) to provide functionality to the [`VoterWork`] type. It is a self contained type that encapsulates all dependencies of the [`VoterWork`] type. The [`Environment`] contains a client dependency which is a subset of the traits that the substrate `Client` implements. This subset of traits is defined as `ClientForGrandpa` and is described as:

```rust
/// A trait that includes all the client functionalities grandpa requires.
Expand Down Expand Up @@ -60,7 +60,7 @@ Every trait except `Finalizer`, `ProvideRuntimeApi`, `ExecutorProvider` has been
### `NetworkBridge`
[`NetworkBridge`] is the bridge between the underlying network service, gossiping consensus messages and GRANDPA. It contains an implementation of `Network` trait and an implementation of `Syncing` trait though these are just stored to instantiate [`GossipEngine`] which is a dependency. There is also a [`GossipValidator`](https://github.com/paritytech/polkadot-sdk/blob/d2fd53645654d3b8e12cbf735b67b93078d70113/substrate/client/consensus/grandpa/src/communication/gossip.rs#L1486) type used as a dependency.

I propose replicating [`NetworkBridge`] given it's a good abstraction and a defined dependency of `VoterWork`. We will also need to replicate [`GossipEngine`] given it's a depency of [`NetworkBridge`].
I propose replicating [`NetworkBridge`] given it's a good abstraction and a defined dependency of `VoterWork`. We will also need to replicate [`GossipEngine`] given it is a dependancy of [`NetworkBridge`].

### `GossipEngine`

Expand Down Expand Up @@ -222,7 +222,7 @@ pub trait NetworkEventStream {
fn event_stream(&self, name: &'static str) -> Pin<Box<dyn Stream<Item = Event> + Send>>;
}
```
[`GossipEngine`] uses `add_set_reserved` and `remove_set_reserved`. Looks like Gossamer already support adding and removing reserved peers. In substrate from what I gather, there are authorized peers, and there are reserved peers. Reserved peers are peers that are in the authorized pool, that are assigned to a set for a given `ProtocolName`. We should be able to add/remove from the reserved set. Removing from the authorized set is done via peer reputation and I assume will be removed from any reserved sets if disconnected. `peer_role()` and `ObservedRole` are used qutie a bit in [`GossipEngine`]. `report_peer` is also used in [`GossipEngine`]. Doesn't seem like `NetworkEventStream` is actually used in GRANDPA client integration.
[`GossipEngine`] uses `add_set_reserved` and `remove_set_reserved`. Looks like Gossamer already supports adding and removing reserved peers. In substrate from what I gather, there are authorized peers, and there are reserved peers. Reserved peers are peers that are in the authorized pool, that are assigned to a set for a given `ProtocolName`. We should be able to add/remove from the reserved set. Removing from the authorized set is done via peer reputation and I assume will be removed from any reserved sets if disconnected. `peer_role()` and `ObservedRole` are used qutie a bit in [`GossipEngine`]. `report_peer` is also used in [`GossipEngine`]. Doesn't seem like `NetworkEventStream` is actually used in GRANDPA client.

#### `Syncing` trait

Expand Down Expand Up @@ -276,11 +276,11 @@ pub trait SyncEventStream: Send + Sync {

`set_sync_fork_request` is called by [`GossipEngine`]. `announce_block` is also called through `GossipEngine.announce()` via `OutgoingMessages` sink type in `communication` crate. Doesn't look like `new_best_block_imported` is actually used. `event_stream` is used in [`GossipEngine`] constructor.

I propose introducing interfaces for `Network` and `Syncing` to the codebase. Mocks will be used in unit tests and integration tests to test GRANDPA client functionality. The actual implementation used to fulfill `Network` and `Syncing` interfaces using existing Gossamer peerset, synching, and network modules will be outlined in another design document.
I propose introducing interfaces for `Network` and `Syncing` to the codebase. Mocks will be used in unit tests and integration tests to test GRANDPA client functionality. The actual implementation used to fulfill `Network` and `Syncing` interfaces using existing Gossamer peerset, synching, and network modules will be outlined in another design document specifically focused on integrating the GRANDPA client into the rest of the Gossamer codebase.

### 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 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](https://github.com/ChainSafe/gossamer/issues/4466), to call the new pipeline as well as remove any existing finality domain specific logic in BABE and in sync.
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 we have already begun replicating the `Client` type. This does imply that we alter the translation shim work of `BlockState.AddBlock` in [#4466](https://github.com/ChainSafe/gossamer/issues/4466), to call the new pipeline as well as remove any existing finality domain specific logic in BABE and in sync.


### Integration tests
Expand Down

0 comments on commit 5a4e20f

Please sign in to comment.