-
Notifications
You must be signed in to change notification settings - Fork 409
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
App submission: monero #690
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check my comments about versioning for the app itself and also the Monero node image used.
Updates complete. Thanks! |
Apologies for the delay in reviewing this @deverickapollo! I have had my head down working on other parts of the code-base. I will try and review soon. Thanks @m1m3-50 and @IMPranshu for stepping up and helping. Much appreciated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work on this @deverickapollo! This is a fantastic addition for the Monero community. See below for some suggested changes.
monero/docker-compose.yml
Outdated
healthcheck: | ||
disable: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this to override a healthcheck designated in the Dockerfile that occurs super often?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into this now - I believe this is related to some work I'm doing to prepare monerod for an integration with btcpay. I believe we can remove this for now.
I installed the app and am syncing the blockchain now. Initial sync is working great right off the bat. The primary purpose of my review is just to make sure that the app is packaged correctly for Umbrel; however, since I have some familiarity with the UI and server code of your app from working on our Bitcoin Node app, I can offer some additional help. Check out this video below (no sound) where I highlight a few bugs I found while checking out the app. This is my first time playing around with monero, so apologies if I have misunderstood anything. monero-bugs.mp4Notes for the video:
This was just from a quick play, so there are probably other sneaky bugs that have understandably resulted from translating the bitcoin app to a monero app. |
@deverickapollo I noticed that monerod uses quite a lot of RAM. This is during initial block download, with DB Sync Mode set to the
I have also noticed that if I try and restart my Umbrel while Monero is installed and running, it can take a very long time to shut down. Then when everything comes back online, the monero app UI can become stuck in this state even though monerod is plugging away in the background: Monero-restart.mp4It may be that during late-stage initial block download, monerod can take a while before it accepts RPC calls |
@nmfretz - This is great! Thanks for taking the time to review the app! A few of these issues I am aware of. I should be able to address most in the next week. I'll reach out once that work is complete. |
Excellent, looking forward to it @deverickapollo! I have a monero node happily plugging away on one of my Umbrels, so I can help with any testing you need. |
I'm running this. Fully synced it's sitting at <1gb ram and during syncing I never saw it go above 4gb |
@deverickapollo let me know whenever you are ready for me to continue reviewing so I don't accidentally hold you up! |
Hey @deverickapollo , just following up on the PR. Is it ready to be reviewed? |
@nmfretz - Bumped the app to 1.0.4 which should handle the startup issues noted. Some of the peering nodes defined in the config file were offline and caused the latency during boot. Let me know if you're still experiencing any issues there. If twe're good there, I am ok with shipping the current version. |
Thanks @deverickapollo, I just tested now and I still have the same issue noted here where monerod isn't accepting rpc calls from the UI during the first X% of IBD. When I view the monerod logs I see that sync starts right away and progresses normally. It's just that the frontend UI can't get any info back from monerod for quite a while into sync. I tested rpc calls from inside the monerod container and they work fine during initial sync. What is your preference for launching now with this in the app description:
versus continuing to troubleshoot? |
@nmfretz - That should be fine. I'll investigate those issue and update in a later release. |
Any update on when to expect this in the official App Store? I got this running via community app store, but would love to recommend it to friends without signing up to be their IT Support on how to use community app stores. :) |
Hey @deverickapollo I thought I had posted this here, but it appears I never did. Sorry about that. When I was testing last month, I fully synced Monero and noticed that the UI shows 0% synced when monerod is on the latest block. This results in the loading animation permanently running in the Blockchain card and could be confusing to most users. Is this the case for you? |
@nmfretz Funny you mention it - I was just playing around with this last night. It turns out once the node is synced, target_height goes to 0 Issue 7029. I was using target_height to track our sync status and some of my recent changes didn't consider this. I should have a fix ready to go soon. |
Please note @deverickapollo that I'll be shutting down my Docker Hub account at some point in the near future, so GHCR should be used for my image here instead, i.e. for v0.18.3.3:
|
Appreciate the heads up! I'll get that change in asap |
Thanks a bunch for noting that @sethforprivacy 🤝 |
Monerod version change complete. Fix for sync % displayed on startup and full sync also added |
@deverickapollo Any way you will have the option to enable ZMQ in your app for us P2Pool users? |
Thanks @deverickapollo. I still get the same original issue where sync percent doesn't show in the UI: Can you confirm whether you have this same issue? |
+1 for this. Monero is the best crypto and a dedicated server is the best way to sync it |
I've tested the app myself and can confirm that the issue is present. While the node is syncing, no progress gets displayed |
@highghlow - thanks for confirming. I'll review this asap. I just completed a refactor on the backend to replace monero-javascript with monero-ts so I'll include those changes and retest. |
Thanks @deverickapollo, ping me when you're ready! |
Update: it updates the sync progress when Umbrel is rebooted |
Hey @deverickapollo, out of curiosity I did a deep dive into this issue this morning. Hopefully this will help you with a fix! TLDR:
After that this is good to go from a packaging standpoint! 1) First, the general RPC issue where the UI cannot make any calls to monerod:In version 1.0.1 you removed these cli arguments that used to be passed to the monerod command on container start, and placed their equivalents in the logic that creates the default bitmonero.conf: From exports.sh: deverickapollo@aa48aaa
From umbrel-monero logic/disk.js: deverickapollo/umbrel-monero@2c612fb
This change causes issues because there isn't actually any bitmonero.conf when the app is first installed. It's created by the server container after initial boot, so without a custom bitmonero.conf or There is logic in the server container to restart monerod when the bitmonero.conf file is created (see next point below for a second issue), but without rpc-bind-ip=0.0.0.0 on initial boot of monerod, the server can't actually send an RPC call to tell monerod to restart. @highghlow is right that when you restart umbrel, the UI will then start showing sync progress. And actually it's just that when you restart the monerod container, progress will be shown, because monerod will then source the bitmonero.conf that the server container created. You can test this by installing monero and seeing that no RPC calls can be made externally. Then run: 2) Even when starting monerod with rpc-bind-ip=0.0.0.0, the server container doesn't seem to be able to restart monerod when it is first booting up.Once monerod has initialized and the UI shows data, you can restart monerod from Advanced Settings. However, when the app is first installed you will see this error:
The server will keep trying to restart monerod for 1 minute in order to source the newly created bitmonero.conf: bin/www:
I haven't explored this one further, but it is possible that monerod just isn't ready to accept RPC requests this early on in the initial boot. Without this initial restart, users will be running monerod with default config values and not inheriting anything you have set as your initial bitmonero.conf. They would only source the bitmonero.conf if they restart the app, restart umbrel, or change settings and restart through Advanced Settings within the monero app. |
Hey @deverickapollo ping me when you are ready for me to test! |
@nmfretz Appreciate that additional input! I was able to reproduce this issue and identify the problem. I've included a fix in the latest release that should address the startup issue. |
Well done @deverickapollo! This is good to go from a packaging perspective. Welcome to the official app store 🎉 |
App Submission
monero
256x256 SVG icon
Gallery images
I have tested my app on: