-
Notifications
You must be signed in to change notification settings - Fork 653
Add mempool app #470
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 mempool app #470
Conversation
|
Where can we find your source code (Dockerfiles, etc)? |
|
@louneskmt I've forked the main repo, but I'm actually doing a "clean" way to do it so we can work with all the new releases I'll send you the link when I've finished ;) |
apps/mempool/docker-compose.yml
Outdated
| tag: "umbrel-app {{.Name}}" | ||
|
|
||
| services: | ||
| app: |
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.
Should be named web to keep consistency with other apps.
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.
Fixed 9d61783
| db: | ||
| image: bguillaumat/mempool-db:v2.0.1 | ||
| logging: *default-logging | ||
| restart: on-failure | ||
| stop_grace_period: 1m | ||
| volumes: | ||
| - ${APP_DATA_DIR}/mysql:/var/lib/mysql | ||
| environment: | ||
| MYSQL_USER: "mempool" | ||
| MYSQL_PASSWORD: "mempool" | ||
| expose: | ||
| - "3306" | ||
| networks: | ||
| default: | ||
| ipv4_address: $APP_MEMPOOL_DB_IP |
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.
It may be better to directly use official MariaDB/mysql images here (see Luke's review and b444272 as I did exactly this today).
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.
Hum I guess it can be better but since my other repo split every files and format them so we can easily track mempool updates it's will be a "lost of time" to copy the files here every time we update the app.
If you really think we needs to put it here I will 😃
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.
Yeah, thinking about this more, it's fine if you guys wanna do it like this.
I'm thinking ahead about how app developers will want to package apps when the Umbrel app configuration lives in the app's own repo. It makes sense in that case because they will probably already have an existing Docker image, so they don't want to create and maintain yet another child Dockerfile specifically for Umbrel, it's handy if they can use oll their existing Docker images (or official images) and then just pass in some Umbrel specific init scripts.
But that's not how things work right now, and you guys are already having to create your own Docker images for many projects, so I guess there's no issue with you including whatever you want directly in the image. It also means you can edit and maintain them all in one place.
So it's fine to keep doing it like this. (sorry Lounes! feel free to undo your changes if you want.)
It won't make sense in the future but it's fine to do it like this now.
apps/mempool/docker-compose.yml
Outdated
| expose: | ||
| - "8999" |
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.
See Luke's comment: #461 (comment)
(I know we both did the same mistakes the same day haha)
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.
0c33e79 to
9d61783
Compare
|
Also, if you want this PR to automatically close the linked issue, you need to change |
|
I'm seeing these logs on first boot: Anyone else? |
Yeah got the same the first time I dont know if its a error from mempool or due to me It seems I can add option in the sql file to resolve that I'll tell you when I've tried 😉 |
lukechilds
left a comment
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.
Awesome stuff @bguillaumat!
Running locally for me and it's working great.
Looks like the Docker containers are running as root which we should resolve.
Also I noticed in your Dockerfiles you're running npm install. If you instead run npm ci --production it should result in much smaller Docker images.
apps/mempool/docker-compose.yml
Outdated
|
|
||
| services: | ||
| web: | ||
| image: bguillaumat/mempool-frontend:v2.0.1 |
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.
Run container as non-root:
| image: bguillaumat/mempool-frontend:v2.0.1 | |
| image: bguillaumat/mempool-frontend:v2.0.1 | |
| user: "1000:1000" |
apps/mempool/docker-compose.yml
Outdated
| default: | ||
| ipv4_address: $APP_MEMPOOL_IP | ||
| api: | ||
| image: bguillaumat/mempool-backend:v2.0.1 |
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.
Run container as non-root:
| image: bguillaumat/mempool-backend:v2.0.1 | |
| image: bguillaumat/mempool-backend:v2.0.1 | |
| user: "1000:1000" |
apps/mempool/docker-compose.yml
Outdated
| default: | ||
| ipv4_address: $APP_MEMPOOL_API_IP | ||
| db: | ||
| image: bguillaumat/mempool-db:v2.0.1 |
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.
Run container as non-root:
| image: bguillaumat/mempool-db:v2.0.1 | |
| image: bguillaumat/mempool-db:v2.0.1 | |
| user: "1000:1000" |
apps/mempool/docker-compose.yml
Outdated
| logging: *default-logging | ||
| restart: on-failure | ||
| stop_grace_period: 1m | ||
| command: --sql-mode="STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION" |
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.
Maybe this belongs in the Dockerfile not the Docker Compose configuration to keep the concerns separated?
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.
I will try to make it works on the Dockerfile 😃
|
Also just a side note but I noticed logs about writing a cache file which seems to be from here: https://github.com/mempool/mempool/blob/5f1f06fecf978b3d05e7da10fb0023e1ba9443ad/backend/src/api/disk-cache.ts#L19 It's writing the cache to a file called Since that's happening inside the container and not in a volume:
Ideally this should be on a volume that's persisted and exists on the SSD for faster app start and less SD card wear. It's just dumped in the CWD so we'd need to add an env var to mempool like |
|
After leaving mempool running over night on my node I'm getting this error in a loop: Seems related to the cache file. |
I had to restart the app and it was fixed when I've implemented the volume for the cach we'll see if its fixed |
|
@lukechilds since I've put the cache files in a volume as you've said no more errors 😉 |
|
Awesome! I see you're mapping a volume at |
Yes, in my repo I rewrite their ./cache.json & ./cache2.json to the |
|
Oh I see, nice. Maybe it would be better if we can submit a PR and fix it properly in the mempool repo. E.g allow passing in |
|
@lukechilds Done mempool/mempool#317 I will update my repo when this will be accepted and a new build is done because my repo use a tag version not a commit |
This bug is related to this fix mempool/mempool#304 I expect mempool v2.1 to be the release for Umbrel, it's not tagged yet. |
|
Awesome, thanks @softsimon! Either way, would be good to get mempool/mempool#317 merged too for v2.1 so we can write the cache file to a volume. |
Yes, that's the plan. :) |
wiz
left a comment
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.
Add SHA256 hashes of the multi-architecture manifests for mempool/frontend and mempool/backend
Co-authored-by: wiz <j@wiz.biz>
wiz
left a comment
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.
Updating SHA256 hashes due to having to to move the mempool v2.1.0 tag for a last minute fix
Co-authored-by: wiz <j@wiz.biz>
Done 🙂 |
| MYSQL_DATABASE: "mempool" | ||
| MYSQL_USER: "mempool" | ||
| MYSQL_PASSWORD: "mempool" | ||
| MYSQL_ROOT_PASSWORD: "moneyprintergobrrr" |
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.
How is this password passed into the API container?
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.
If you're just implicitly relying on the value being the same in the mempool app, it would be better to instead explicitly pass it in.
Otherwise if the default password value is ever changed in the mempool app things will suddenly break when we update and it won't be obvious why to the person implementing the update if they aren't familiar with the currently implicit password behaviour.
With the app configurations, it's always better to favour being more explicit at the cost of extra lines of code.
apps/mempool/docker-compose.yml
Outdated
| stop_grace_period: 1m | ||
| command: "./wait-for db:3306 --timeout=720 -- nginx -g 'daemon off;'" | ||
| ports: | ||
| - ${APP_MEMPOOL_PORT}:8080 |
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.
This is running on port 8080 on the container but $APP_MEMPOOL_PORT (3006) on the host.
This breaks the hidden service which is proxying to <app-mempool-ip>:<app-mempool-port> when the app is actually running at <app-mempool-ip>:8080.
It should be running on APP_MEMPOOL_PORT on the container too:
| - ${APP_MEMPOOL_PORT}:8080 | |
| - ${APP_MEMPOOL_PORT}:${APP_MEMPOOL_PORT} |
You'll also need to pass this value to the NGINX to tell it to run on that port instead of 8080.
louneskmt
left a comment
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.
I think you shouldn't have removed the api container IP, as all containers need a fixed IP address in order to prevent IP collisions.
|
Yes, @louneskmt is correct, it's ok to refer to containers via the hostname, you don't need to refer via IP, but you do still need to assign a static IP to the container, otherwise we'll get collisions. |
apps/mempool/docker-compose.yml
Outdated
| restart: on-failure | ||
| stop_grace_period: 1m | ||
| command: "./wait-for db:3306 --timeout=720 -- nginx -g 'daemon off;'" | ||
| command: /bin/sh -c "cp /etc/nginx/nginx.conf /patch/nginx.conf && sed -i 's/8080/${APP_MEMPOOL_PORT}/g' /patch/nginx.conf && ./wait-for db:3306 --timeout=720 -- nginx -g 'daemon off;' -c /patch/nginx.conf" |
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.
This should be done in the mempool Docker image, not from our side.
This still implicitly relies on the port being hardcoded to 8080 in the mempool source.
We should be able to pass in a PORT env var to the mempool image that tells it to run the server on that port.
Also I'd recommend using envsubst not hand rolling it with sed. If the NGINX config is ever changed to include the string 8080 elsehwere, (a hash?) this sed script will break the configuration file.
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.
Okay sure but if this is the only issue I would prefer to fix it in a future release
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.
I've made a PR to handle this case and the MySQL vars
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.
@lukechilds This is fixed in the last version of Mempool (v2.1.1)
Thanks to @wiz 🚀
wiz
left a comment
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.
ACK @ 9373bc0
mempool/frontend:v2.1.1@sha256:17e7fedcd27b6f99de2f159f0f6372b76dd5825f1a98c1b0114ea9564cab1c0e
mempool/backend:v2.1.1@sha256:c15c3d1af0f9df4672f3760b0ff9b79d1dd60235cde7316a7860b1d650b31360
|
@bguillaumat can you enable edits from maintainers so I can push some tweaks to your branch? Then this is good to be merged! 🎉 |
lukechilds
left a comment
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.
Great job everyone!
|
Seriously awesome work on this, @bguillaumat and everyone! Gallery images here: getumbrel/umbrel-apps-gallery#1 Meanwhile, @wiz and @softsimon, feel free to suggest/update the tagline (max 50 chars) and description (max 200 words) that you'd like to see in the app store. |
App Submission
App name
Mempool Space
Version
2.1.1
One line description of the app
(max 50 characters)
A mempool visualizer, explorer and fee estimator
Summary of the app
(50 to 200 words)
Mempool is the official self-hosted version of the fully featured explorer, visualizer, fee estimator, and API service running on mempool.space, an open source project developed and operated for the benefit of the Bitcoin community, with a focus on the emerging transaction fee market to help our transition into a multi-layer ecosystem.
Developed by
mempool.space
Developer website
https://mempool.space/about
Source code repository
https://github.com/mempool/mempool
Support link
(Link to your Telegram support channel, GitHub issues/discussions, support portal, or any other place where users could contact you for support.)
https://t.me/mempoolspace
Uses
256x256 SVG icon
(GitHub doesn't allow uploadig SVGs directly. Upload your file to an alternate service, like https://svgur.com, and paste the link below.)
App screenshots
(Upload 3 to 5 high-quality screenshots (at least 1280x800px) of your app in PNG format.)
I have tested my app on:
resolve #452