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

Add API functions to start and stop local Monero node #272

Merged
merged 2 commits into from
Apr 4, 2022

Conversation

duriancrepe
Copy link
Contributor

Implemented a gRPC service GrpcMoneroNodeService which calls into CoreMoneroNodeService to start and stop a local monero node. The last successfully started local settings are are persisted and loaded on startup. The CoreMoneroNodeService attempts to connect to a local but externally started daemon process first, then looks for the last MoneroNodeSettings to start the local Monero node process with.

CoreMoneroConnectionsService is modified to accept the local Moner daemon which is connected to or started by the CoreMoneroNodeService. I've also included a small change to KeyStorage which fixes a Java String equality issue when investigating encryption requirements.

Resolves Issue #137

@@ -18,6 +18,9 @@
import javafx.beans.property.SimpleObjectProperty;
import javax.inject.Inject;
import javax.inject.Singleton;

import java.io.IOException;
Copy link
Contributor

@woodser woodser Mar 28, 2022

Choose a reason for hiding this comment

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

I don't see a need for CoreMoneroConnectionsService to be modified as part of this change to start and stop a local Monero node. CoreMoneroConnectionsService is an independent service to track and manage daemon connections. The local node is just another connection which may go online or offline.

If we want to automatically switch to the local node when the user starts it (could make sense), this can be supported by CoreMoneroNodeService starting the node and then manually setting the connection on CoreMoneroConnectionsService.

Update per my other comment: maybe the only warranted change to CoreMoneroConnectionsService is to automatically start the local node during initialization iff it was used last and is offline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was able to decouple the two services by replacing the daemon's external interface to pick the local monero daemon's instance if it had started, see getPreferredDaemon.

I removed an unnecessary call to updateDaemonInfo during initialization since it is done on a timer and could be incorrect since the local node has not started yet. Finally, I added the startMoneroNode of CoreMoneroNodeService at the end of CoreMoneroConnectionsService's initialization in case the user had last started a local node (indicating preference to continue using the local node).

The local node setting and daemon is cleared when stopMoneroNode is called, and CoreMoneroConnectionService will provided the daemon connection as before. This allows the front end test to be simplified since we no longer have to track the additional local node "connection".

I think these changes to CoreMoneroConnectionService are unavoidable, since it provides the interface to access the daemon for the rest of the application via getDaemon.

Copy link
Contributor

@woodser woodser Mar 31, 2022

Choose a reason for hiding this comment

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

See my other comments, but overall we want something like this:

The connection service and the node service are completely decoupled. The only interaction between them is:

  1. the connection service, as a last step in initializing, starts the local node if it was last used and is now offline.
  2. the connection service is registered as a listener on the node service. onNodeStarted(daemon), the connection service calls setConnection(daemon.getRpcConnection()), and onNodeStopped(), the connection service calls checkConnection() to update itself.

MoneroNodeSettings should always be present, defaulting to blanks.

Stopping the node should not clear the previously used node settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the detailed explanation, I think I finally get it.

One issue was that I had to add the hard coded password in the local monero node order to match the connection otherwise once you switch to a local monero node, it becomes persisted and you cannot restore the original hard coded connection with username and password. Efforts to restore the original hard coded value were difficult because the connection manager doesn't allow setting connection unless the node actually responded (the external monero process would be offline during the test).

@woodser
Copy link
Contributor

woodser commented Apr 2, 2022

Great. Please squash. I'm going to submit a commit on top to tweak some things.

@duriancrepe
Copy link
Contributor Author

Great. Please squash. I'm going to submit a commit on top to tweak some things.

Squashed

@woodser
Copy link
Contributor

woodser commented Apr 3, 2022

I opened PRs on your branches for review. Thanks.

duriancrepe#4
duriancrepe/haveno-ts#5

wallet initializes when first connected to get correct height
connect to local node if available and last connection offline
use only one internal daemon in monero node service
@duriancrepe
Copy link
Contributor Author

I opened PRs on your branches for review. Thanks.

duriancrepe#4 duriancrepe/haveno-ui-poc#5

Reviewed and merged in. Reran local monero node tests and confirmed working.

@woodser woodser merged commit fdddc87 into haveno-dex:master Apr 4, 2022
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.

None yet

2 participants