Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the Brahms discovery protocol #867

Closed
wants to merge 38 commits into from
Closed

Conversation

tomaka
Copy link
Member

@tomaka tomaka commented Jan 17, 2019

@ghost ghost assigned tomaka Jan 17, 2019
@ghost ghost added the in progress label Jan 17, 2019
@tomaka
Copy link
Member Author

tomaka commented Jan 23, 2019

Since we don't have to conform to any existing protocol, we probably want to make Brahms generic in order to be able to pass additional metadata for peers.

The user would have to pass a protocol name if they don't use the default type. This can look something like:

impl Brahms<()> {
    pub fn new() -> Self {
        Self::with_protocol_name("/brahms/1.0.0")
    }
}

impl<T> Brahms<T> {
     pub fn with_protocol_name(name: ...) -> Self { ... }
}

This metadata would be useful for example for Substrate to pass information such as the authority node ID or whether a node has full/light client capacity.
(disclaimer for anyone reading this: the idea of Substrate using Brahms is just an experiment)

@tomaka
Copy link
Member Author

tomaka commented Jan 23, 2019

This idea of metadata causes a big problem, which is how to "merge" metadata if it is diverging (ie. multiple nodes report multiple different metadata for the same target).
Let's scrap this for now.

@burdges
Copy link

burdges commented Jan 28, 2019

Any idea what role the metadata serves?

@tomaka
Copy link
Member Author

tomaka commented Jan 29, 2019

@burdges This metadata thing is an invention of mine. I thought it would be easy to do, but it turns out that not. We will see later if it could be useful.

As for this PR, what remains to be done is:

@tomaka
Copy link
Member Author

tomaka commented Feb 2, 2019

Integrates #910

@tomaka
Copy link
Member Author

tomaka commented Feb 3, 2019

Here are some difficulties I've encountered:

Duplicates in pull responses

Imagine we perform 10 pull requests, and each request returns up to 10 nodes. According to the original, we are supposed to randomly pick 10 entries within these 100 results.
What the paper may not take into account is that these 100 nodes can contain duplicates, sometimes a lot.
Instead of picking 10 elements, we instead want to insert 10 entries in the view.
Should the nodes that have been reported multiple times have a higher weight in the random process? In this PR, no, but maybe it should be the case.

Bootstrapping

The original Brahms paper mentions that a round should happen only if we received any push request, or received any response to one of our pull requests.

This creates two issues:

  • If a node has just joined the network, it will not received any push request during the first round and thus will waste the responses of its pull requests. It is only once the bootstrap nodes have decided to send a push request to the new node that the Brahms process will truly start, as the new node will now be able to discover other nodes. However, since the picking is random, bootstrap nodes may decide to ignore the new node, and further delay the start of the process.

  • If one only has one bootstrap node, this bootstrap node will never send any pull request, thus never receive any response, thus will ignore all the push requests that have been sent to it. This means that in practice Brahms never starts.

@burdges
Copy link

burdges commented Feb 25, 2019

@tomaka tomaka changed the title [WIP] Implement the Brahms discovery protocol Implement the Brahms discovery protocol May 18, 2020
@tomaka tomaka marked this pull request as draft May 18, 2020 08:13
@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2023

This pull request has merge conflicts. Could you please resolve them @tomaka? 🙏

@thomaseizinger
Copy link
Contributor

Closing as stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants