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

added bootnode on startup #78

Closed

Conversation

JesseAbram
Copy link

As talked about in #77 , fix for issue, feel free to reject if not necessary

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Why not add all nodes as bootnodes into the chainspec?

@JesseAbram
Copy link
Author

Once they connect they connect to alice they seem to find each other fine, also I think I can only get the bootnodes after they launch. I could add alice's to bob then bob's and alice's to charlie etc, but since they find each other after connecting fine, Im not sure it is worthwhile?

@bkchr
Copy link
Member

bkchr commented Mar 29, 2021

Yeah the only require one node to find each other. However, I think we should just add all, because I don't see any downside.

@JesseAbram
Copy link
Author

makes sense, will do.

@bkchr
Copy link
Member

bkchr commented Mar 29, 2021

(It will also make the code cleaner)

@JesseAbram
Copy link
Author

ya, I see that, added it, one unintended consequence is that It has to connect to each node which adds idk like 4 seconds per node, is it still worth it?
image

@bkchr
Copy link
Member

bkchr commented Mar 29, 2021

We configure the ports in the config file. So, I don't see why you need to call the nodes to get this information.

For the node key I would propose we use something deterministic so that we can calculate it before starting the node.

But yeah, maybe we should just take your pr as it is

@bkchr
Copy link
Member

bkchr commented Mar 29, 2021

Maybe you give this a short try, what I mentioned above. If that is too complicated, just lets merge your pr

@JesseAbram
Copy link
Author

There may be a race condition in here that causes nodes to fail, will look into it when have more time, for now closing

@JesseAbram JesseAbram closed this Apr 26, 2021
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.

2 participants