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

Save last seen peers and use them as bootstrap upon restart #1877

Closed
wants to merge 18 commits into from
Closed

Save last seen peers and use them as bootstrap upon restart #1877

wants to merge 18 commits into from

Conversation

experiencedft
Copy link

Follow up to issue #1856.

Sorry if the commits are not clean, it's my first time contributing to a project with Git and there was a lot of weird stuff going on.

These changes allow the user to specify a file where to store the last seen peers, and read from that file to use them as bootstrap peers upon restart.

The run command now requires a new flag --peers which specifies the path to the relevant file. For example, using the docker image for v1.1.3, one could use --peers /mnt/keep-client/config/peers.dat.

The last seen peers are wrote to the specified file by the monitorConnectedPeers function in keep-core/libp2p.go.

Thanks to Discord users ssh, pantsme and postables for the help with either Go, Git, Docker or LibP2P stuff.

Sloan Thompson and others added 18 commits May 5, 2020 14:17
Keep Random Beacon initial realease.
Pinning the token-dashboard keep-core version to the mainnet release
that's getting ready to go out.
First full token-dashboard release on mainnet!
This reverts commit 045c7c0.
This reverts commit 806d357.
Revert "Release 1.1.2"

This reverts commit 045c7c0.

Revert "Explicit depends"

This reverts commit 806d357.

Revert "Release version v1.0.0 of token-dashboard"

This reverts commit d00bd2c.

Revert "Result of running npm upgrade @keep-network/keep-core"

This reverts commit be10d7a.

Revert "Increment token-dashboard patch version"

This reverts commit bf4ba63.

Revert

Actually revert
@mhluongo
Copy link
Member

Excited about this @jeremyid! We're heads down on this next release, but we'll get it triaged soon.

We'll likely need to rebase and squash most of this commit history. On the actual changes, I'm pretty sure the team will want to unify how state is treated and take a different approach on the re-connect, but hoping we can work together on those changes in this PR.

@experiencedft
Copy link
Author

We'll likely need to rebase and squash most of this commit history.

I wish I would know how to deal with all the version control things, sorry about that @mhluongo, I settled for a PR without conflicts and didn't try to go further. It was particularly hard figuring out how to be able to work on this live on testnet which is what led me to revert to 1.0.0 and do some revert commit things.

I'm happy to keep working on these PRs, eager to know what the devs think of this and if they have any suggestion to improve the approach I took.

@mhluongo
Copy link
Member

I wish I would know how to deal with all the version control things, sorry about that

Don't be!

eager to know what the devs think

👌

@pdyraga
Copy link
Member

pdyraga commented Jul 10, 2020

Thanks for the contribution @jeremyid 🙌 and I am sorry for being late here.

I like very much your idea about storing the peer list locally but I would like to propose two changes.

First, I don't think we should treat those peers as bootstraps. Although there is no difference on the network level and bootstraps do not expose any additional services, bootstrap peers are treated differently than other discovered peers. The most important difference is that the client always tries to connect to all bootstrap peers and not being able to establish a connection with bootstrap is treated as a failure. Please see https://github.com/keep-network/go-libp2p-bootstrap for details on that behavior. Also, there are separate metrics for bootstrap peers (see #1850) and they allow evaluating the network health for the given node.

Not being able to connect to a non-bootstrap peer is not great but also not something we should consider a failure. More importantly, for bigger networks, peers may decide to which other peers they want to connect to based on the topics they are subscribed to.

Instead of using last seen peers as bootstrap peers we should find a way to provide them to the routing table and let the node decide in which order to connect and whether to connect or not to connect. I hope that @lukasz-zimnoch can shed some light on whether this is easily achievable or if we need to contribute something to libp2p to make it happen.

My second comment is about moving the last seen peers file location to the config file. All options but the log level and config file location are configured there. Honestly, I think we could even reuse the existing storage option for that. Just add another directory next to current and archive, perhaps network?

@experiencedft
Copy link
Author

Hey @pdyraga -- I agree with most of your remarks, my PR is functional but was meant to be a proof of concept rather than a fully fleshed out addition.

I am aware of the fact that failure to connect to some of the specified bootstrap peers triggers a warning, though I am not sure why the code is such that the node keeps calling the bootstrap function once it is in fact connected to the network. I looked at #1850 and saw the discussion about connected_bootstrap_count as a health metrics -- I don't think this is necessarily a great metric personally but I will comment there more specifically.

On the topic of routing tables, there is this related LibP2P issue that might be of interest to you and the team.

@nkuba nkuba closed this Jul 2, 2021
@nkuba nkuba deleted the branch keep-network:master July 2, 2021 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants