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

Network peering discovery - Seed mode - Addrbook.json #2308

Open
albttx opened this issue Jun 7, 2024 · 8 comments · May be fixed by #2852
Open

Network peering discovery - Seed mode - Addrbook.json #2308

albttx opened this issue Jun 7, 2024 · 8 comments · May be fixed by #2852
Assignees

Comments

@albttx
Copy link
Member

albttx commented Jun 7, 2024

Peering and network discovery

Description

During my setup of a multi-node cluster, i realised there is not (as in tendermint 1):

  • "self discovery peering"
  • Storage of peers in the addrbook.json

The only peering solution is persistent_peers. Which is known by tendermint node runner to node be a best practice to set because it could cause network issue leading to missing blocks.

To setup a 3 nodes cluster, what i would like to do is :

  • validator 1: seed_mode = true
  • validator 2: seeds = id@validator1:26656
  • validator 3: seeds = id@validator1:26656

thanks to that, all 3 nodes will be connected.

If a new node arrive

  • validator 4: it can just set the same seeds and it would works.

We would need to add all nodes are persistent peers... :/

Seed Mode

Some people run a "Seed node", which is a node only to help people to connect to the network and gather peers, this setting is mostly set to false.

In tendermint, there is a seed_mode = {{ true || false }}.

Here is a part default tendermint config.toml file.

# Seed mode, in which node constantly crawls the network and looks for
# peers. If another node asks it for addresses, it responds and disconnects.
#
# Does not work if the peer-exchange reactor is disabled.
seed_mode = false

FYI, validators and infra people mostly run binaryholdings/tenderseed which is like a seed node, without block check.

Addrbook.json

in tendermint, there is a $DAEMON_HOME/config/addrbook.json file, which contains a list of peers, so everytime the node restart, it will read from this file the list of peers.


This 2 features has been removed by @jaekwon from 2021, in the beginning of tm2. Is it wanted ? Temporary removed ? Does he have a different solutions in mind ?


I would be happy to work in this and get my hands inside of tm2 if it's accepted :)

@zivkovicmilos
Copy link
Member

I don't think it's a question of whether we need this, but how do we not already have this?

Bootnodes are essential to bootstrapping a chain / network pockets, and making sure discovery happens out of the box, continuously.

@albttx, I see the addrbook addition as an easy addition (periodic peer info dump to disk). I'm just curious whether we need this kind of functionality in the first place if the size of the cluster that ultimately needs to be discovered is not even large, and rediscovering it upon boot shouldn't take too much time. What do you think? How much time is actually saved in practice, through this "optimization"?

I'd say full steam ahead on the bootnode (seeds) implementation

@mazzy89
Copy link
Contributor

mazzy89 commented Jun 19, 2024

@albttx does this apply also for seeds field? in other words, setting a list of peers under seeds has any impact? Or all the seeding functionalities are completely removed?

@albttx
Copy link
Member Author

albttx commented Jun 20, 2024

@albttx does this apply also for seeds field? in other words, setting a list of peers under seeds has any impact? Or all the seeding functionalities are completely removed?

@mazzy89 since seed_mode functionnality is dead, setting seeds has no impact :/

@zivkovicmilos
Copy link
Member

@albttx
I'm taking this up as part of a bigger cleanup of p2p 🍻

@moul
Copy link
Member

moul commented Oct 3, 2024

The reason for removing Addrbook is to replace it later with a contract-based approach: a r/sys/nodes realm. This realm will enable us to reference validators and full nodes that provide RPC services, which will also incentivize non-validator node operators.

Before we implement r/sys/nodes, which will not be a priority before the launch, we should probably refrain from developing the Addrbook module since it will not be used. Instead, we can temporarily rely on the existing whitelisting configuration option and expect users to coordinate on the configuration file using a GitHub repository.

@moul moul moved this from 📥 Inbox to 👀 Watching in 😎 Manfred's Board Oct 3, 2024
@moul
Copy link
Member

moul commented Oct 15, 2024

Fixing the networking configuration for the launch is essential, but we shouldn't implement AddrBook. We can either rename the title and description or close this issue in favor of a new one.

Remember, it's not just about writing code—there may not even be any code to write. It's also about coordinating the configuration on an external repository.

@Kouteki
Copy link
Contributor

Kouteki commented Oct 16, 2024

@zivkovicmilos please reuse this issue when you have time

@albttx
Copy link
Member Author

albttx commented Oct 16, 2024

I'm not sure to understand @moul ,

Fixing the networking configuration for the launch is essential

Do you means by that fixing p2p seeds and pex ?

In that case, where are you planning to store the discovered peers ?

the cool things with the addrbook.json is that you can easily share the file, from a server to another for a quicker sync, lot of cosmos provider provide this kind of services (eg: Polkachu https://polkachu.com/addrbooks )

I love the idea of r/sys/nodes, but the issue is that it's "globally", and not per nodes and it simplify attackers works to get the lists of all peers of the networks.

@zivkovicmilos zivkovicmilos moved this from TODO to In Progress in 🚀 main.gno.land launch 🚀 Oct 18, 2024
@Kouteki Kouteki moved this from In Progress to Backlog in 🧙‍♂️gno.land core team Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 Watching
Status: Backlog
Development

Successfully merging a pull request may close this issue.

5 participants