docs(design): Authority Discovery design #4577
Conversation
| - Sign the addresses with the keys. | ||
| - Put addresses and signature as a record with the authority id as a key on the Kademlia DHT. | ||
|
|
||
| ### `Role::Discover` |
There was a problem hiding this comment.
It looks like we can skip Role::Discover since it is only for collators, cumulus and other occasions where software is not an authority, which is not a case for Gossamer
There was a problem hiding this comment.
This section describes behaviour that needs to be run whether or not the node is an authority or not. It still needs to fetch and validate DHT requests based on the authority records.
|
|
||
| The [`AuthorityDiscovery`] trait contains only two methods: `authorities` and `best_hash`. | ||
|
|
||
| The `authorities` is the same method found in [`ProvideRuntimeApi`] trait, implying a runtime call is expected to be made to get the authorities at a given block hash. |
There was a problem hiding this comment.
As I understood we would need to implement this host function?
There was a problem hiding this comment.
We already have this host function.
|
|
||
| #### Implementing `NetworkDhtProvider` and `DhtEventStream` | ||
|
|
||
| I believe will we need to fork `go-libp2p-kad-dht` or look for viable alternatives DHT packages to support the `put_record_to` functionality as well emit the `DhtEvent` variants from the package itself. Without this functionality we will not be able to run the Substrate Authority Discovery protocol as expected by Substrate based nodes. |
There was a problem hiding this comment.
I have concerns regarding forking. put_record_to can be implemented on our side by wrapping original interface.
Regarding events I have a feeling they do not have rather because of different architectures. Moreover I checked handle_dht_event function (not sure if it is explicit) but it is mostly about metrics and though we would need them, I beleive we have control over whole events that happening, eg ValuePut, ValueNotFound are just resutls of the operations that we do, hence I we really need to have vent based architecture we can also wrap kad-dht-go with our implementation and add events streaming on top of each action
There was a problem hiding this comment.
I have concerns regarding forking. put_record_to can be implemented on our side by wrapping original interface.
I'm not sure if we can replicate the put_record_to functionality without forking the repo. If we look at the IpfsDHT type, all the attributes are private, so we wouldn't necessarily be able to extend it. There may be a possibility where we expose certain attributes of IpfsDHT through receiver methods, and add the put_record_to functionality on a type that embeds IpfsDHT and extends it. When I create the issue to investigate implementing put_record_to to, we should consider all options that mitigate the need to maintain a fork.
Regarding events I have a feeling they do not have rather because of different architectures. Moreover I checked handle_dht_event function (not sure if it is explicit) but it is mostly about metrics and though we would need them, I beleive we have control over whole events that happening, eg ValuePut, ValueNotFound are just resutls of the operations that we do, hence I we really need to have vent based architecture we can also wrap kad-dht-go with our implementation and add events streaming on top of each action
There is quite a bit of business logic related to the ValueFound event which calls handle_dht_value_found_event. handle_put_record_requested is called from handling PutRecordRequest event, which also contains quite a bit of business logic. I agree we should attempt to wrap IpfsDHT first to emit these events instead of completely forking it. I'll mention this in a design issue to implement our own version of DhtEventStream.
There was a problem hiding this comment.
I also was reveiwing GossipEnginer and I only see that DHTEvents are defined within substrate not within the libp2p, or am I missing something?
There was a problem hiding this comment.
Their DHT produces errors that are translated through an intermediary DiscoveryOut enum to eventually become DhtEvent. libp2p::kad::GetRecordError -> DiscoveryOut::ValueNotFound -> DhtEvent::ValueNotFound see https://github.com/paritytech/polkadot-sdk/blob/c88128832f95bb916c8b1b5eb45a34bf78ec998a/substrate/client/network/src/discovery.rs#L987
Changes
Issues