docs(design): GRANDPA client implementation design #4519
Conversation
|
|
||
| ### `GossipEngine` | ||
|
|
||
| [`GossipEngine`] is the main long running parallel process that provides generalized gossip functinonality in substrate. It will also be required for BEEFY implementation. [`GossipEngine`] requires an implementation of the `Network` trait as well as the `Syncing` trait. |
There was a problem hiding this comment.
Is there any evidence showing we need BEEFY to propose a block? I personally have not seen any, so if there is not I think we need not worry to much about BEEFY atm
There was a problem hiding this comment.
🤔 The GossipEngine sub-dependency is essentially what is used to drive protocol specific gossip functionality. Nothing to do with proposing blocks.
docs/docs/design/grandpa-client.md
Outdated
|
|
||
| ### 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 was a problem hiding this comment.
You are proposing we fully refactor how gossamer imports blocks? This would quite a big refactor right?
There was a problem hiding this comment.
Not exactly. Currently we have design work and issues for implementing BlockState.AddBlock functionality #4466 and introducing and implementing the BlockImport interface into Client #4458. By the time we implement GRANDPA into development, we should refactor this translation method AddBlock to use the introduced pipeline instead of calling Client.ImportBlock directly. High level the logic currently exists in sync, and in BABE to do these finality related checks and additions to the block. I'm proposing we move this domain specific logic to their appropriate spots in the pipeline, and remove the calling of said logic in BABE and in sync.
There was a problem hiding this comment.
Added some more notes to this section in d7bd44c
|
|
||
| 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 was a problem hiding this comment.
This design does a good job describing how we will finish adding the remaining parts of the grandpa client. However it would be good to have more details into how this actually integrates into the current gossamer code base.
Specifically details around building the syncing and network traits and if/how we can use current gossamer code there. If we can use the code then details on what this looks like would be useful, and if we are not using it then not only would I like to see a description of the additions, but then we would need to highlight what integrating this into gossamer would look like.
Additionally, if we are going to refactor how we import blocks in gossamer, this goes beyond the scope of grandpa and we need to highlight the needed changes and what they look like specifically in gossamer. I am quite skeptical of this given the size of the refactor, but maybe it is not to big. Either way it should be fully described here
There was a problem hiding this comment.
This design does a good job describing how we will finish adding the remaining parts of the grandpa client. However it would be good to have more details into how this actually integrates into the current gossamer code base.
Specifically details around building the syncing and network traits and if/how we can use current gossamer code there. If we can use the code then details on what this looks like would be useful, and if we are not using it then not only would I like to see a description of the additions, but then we would need to highlight what integrating this into gossamer would look like.
This client integration design is outlined in #4519 . This GRANDPA implementation is essentially recreating the interfaces in the environment that GRANDPA operates in given we have already proceeded with this approach in previous work. Implementation of the Syncing and Network traits within the Gossamer codebase is not fully fleshed out I agree. This does imply that there will be some extra code to interface with our existing networking stack and sync package. Given that I'm juggling multiple priorities at the moment, I propose we do that in a separate design document, so we can move forward with development on the GRANDPA client implementation.
Additionally, if we are going to refactor how we import blocks in gossamer, this goes beyond the scope of grandpa and we need to highlight the needed changes and what they look like specifically in gossamer. I am quite skeptical of this given the size of the refactor, but maybe it is not to big. Either way it should be fully described here.
I think I talked about this in #4519 (comment). I was also working off of the previous issue you created #3642.
Changes
Issues
closes #4306