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

Adding Mdns and Kad Toggle #647

Merged
merged 14 commits into from
Aug 31, 2020
Merged

Adding Mdns and Kad Toggle #647

merged 14 commits into from
Aug 31, 2020

Conversation

RajarupanSampanthan
Copy link
Contributor

@RajarupanSampanthan RajarupanSampanthan commented Aug 24, 2020

Summary of changes
Changes introduced in this pull request:

  • Adds Toggle wrapper around mdns and kad fields in ForestBehaviour struct and the necessary minor fixes

Sample Config File :
data_dir = "test_dir"

[network]
listening_multiaddr = "/ip4/0.0.0.0/tcp/40000"
bootstrap_peers = ["/ip4/54.186.82.90/tcp/1347"]
mdns = false
kademlia = false

Reference issue to close (if applicable)

Closes
#502

Other information and links

@ec2
Copy link
Member

ec2 commented Aug 25, 2020

I'm confused by the tags haha. It's currently do not merge? Is there a reason you didnt open a draft PR? Or did you mislabel accidentally?

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good, but can you also add a flag to the forest binary so you can configure bootstrap config without requiring a config file. (Lotus has a flag --bootstrap that defaults to true and can disable kad, but I see the benefit in having both --kademlia/--bootstrap and --mdns flags that default to true)

Open to suggestions on the usage of this though, just sharing thoughts

@RajarupanSampanthan
Copy link
Contributor Author

I'm confused by the tags haha. It's currently do not merge? Is there a reason you didnt open a draft PR? Or did you mislabel accidentally?

I was debating whether to hold off on submitting this because our daemon doesnt start up so actual testing might've been difficult, which is why I added those tags. Wasn't sure what the best tag was for it lol. Logically though its pretty simple and makes sense which is why I ended up submitting it anyway

node/forest_libp2p/src/config.rs Outdated Show resolved Hide resolved
@RajarupanSampanthan RajarupanSampanthan merged commit 2434164 into main Aug 31, 2020
@RajarupanSampanthan RajarupanSampanthan deleted the rupan/MDNS branch August 31, 2020 17:47
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.

None yet

3 participants