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

Multiple internal nodes #1165

Merged

Conversation

ben-kaufman
Copy link
Contributor

Allows setting up multiple internal nodes with different networks.
Currently, this will work only for one node per network, since the port will be already occupied, but the architecture already supports running multiple nodes of the same network on different ports, which is one of the 2 missing pieces we need in order to allow Quicksync option to have a background full sync (the second piece needed is way to migrate wallets between nodes).

@k9ert
Copy link
Collaborator

k9ert commented May 19, 2021

LGTM but as long as we can't practically test that with e.g. testnet, signet or elements, this is imho not practical to merge. Maybe convert to draft?

@ben-kaufman
Copy link
Contributor Author

Is it possible to mark as draft after creating the PR? Can't find out how...
I'll add a Cypress test for it.

@ben-kaufman
Copy link
Contributor Author

@k9ert added a test for this now

@@ -137,6 +137,8 @@ def add_node(
def add_internal_node(
self,
name,
network="mainnet",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not "main"? Looks like we have now main and mainnet that are basically the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@stepansnigirev
Copy link
Collaborator

If I try to "forget the node" with built-in node I face certain issues:

  • Specter doesn't stop it, so I have to kill it manually
  • Next time I create a node they appear again but I can't edit them anymore

failed-to-delete

@stepansnigirev stepansnigirev merged commit 90d9331 into cryptoadvance:master May 24, 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.

3 participants