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

Node manager #1146

Merged
merged 20 commits into from
May 11, 2021
Merged

Node manager #1146

merged 20 commits into from
May 11, 2021

Conversation

ben-kaufman
Copy link
Contributor

@ben-kaufman ben-kaufman commented May 4, 2021

  • Add node and node manager classes
  • Allow adding, removing, and switching between nodes
  • Fix internal node functionality
  • Remove old Bitcoin Core settings screen
  • Migrate existing node configurations to the new format
  • Fix the tests
  • (Optional, maybe in another PR) Make wallets belong to a specific node (this could be a problem currently if user has multiple nodes of the same network)
  • (Optional, maybe in another PR), allow multiple "built in" nodes for multiple networks.

@ben-kaufman ben-kaufman marked this pull request as ready for review May 6, 2021 11:16
@k9ert
Copy link
Collaborator

k9ert commented May 7, 2021

Looks good to me and works pretty much as expected.
The Logging button "see bitcoind logs" for the internal node doesn't point to the new directory, though.

@k9ert
Copy link
Collaborator

k9ert commented May 7, 2021

There is imho one more question: How do we migrate the data on the user-side already using internal nodes? There are two possibilities:

  • Do it automatically somehow. In that case we should try to get a more generic mechanism in place which executes specific funtions based on specific configurations (like an array of executed migration-functions).
  • Or we simply don't do that and let the people recreate their internal node and this needs to be documented doemhow.

What do you think? I tend to the second solution. The first one is pretty huge and should be a PR of itself. And maybe we want to still defer that mechanism until we might change persistence-technology.

@ben-kaufman
Copy link
Contributor Author

This should already work with migrating an existing internal node, I also tested and it worked well. The code responsible for this is at https://github.com/cryptoadvance/specter-desktop/pull/1146/files#diff-88c6a6cabbeff77aafe908b914ca9b08c2c0f5942b54b10b782fbc9dded8c6a6R569

@k9ert
Copy link
Collaborator

k9ert commented May 11, 2021

There are known issues with the tor-test in cirrus, that's why this test is failing. Locally, all tests are green. That's why i'll merge this now nevertheless.

@k9ert k9ert merged commit 7ac1f2b into cryptoadvance:master May 11, 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