-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Update step-by-step send payment guide with docker #10461
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
base: master
Are you sure you want to change the base?
Update step-by-step send payment guide with docker #10461
Conversation
Summary of ChangesHello @Euler-B, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a thoroughly validated and updated step-by-step guide for setting up and sending payments using LND with Docker. It resolves previous workflow issues, ensuring a reliable and out-of-the-box experience for users interacting with the LND payment flow. The changes encompass dependency updates, command syntax modernization, and configuration adjustments across the Docker environment for LND and BTCD. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request provides a welcome update to the Docker-based lnd payment guide. Key improvements include updating to the docker compose v2 syntax, removing an outdated section, and upgrading dependencies. The Dockerfiles are also enhanced with better version management and a health check.
However, I've identified a critical error in docker/lnd/start-lnd.sh. The script configures lnd to use the wrong port for the btcd RPC connection on simnet, which will prevent the setup from working as described. I've provided specific comments with suggestions to resolve this.
docker/lnd/start-lnd.sh
Outdated
| "--$CHAIN.node"="$BACKEND" \ | ||
| "--$BACKEND.rpccert"="$RPCCRTPATH" \ | ||
| "--$BACKEND.rpchost"="$RPCHOST" \ | ||
| "--$BACKEND.rpchost"="$RPCHOST:18555" \ |
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.
The btcd RPC host is being configured with port 18555, which is the P2P port for simnet, not the RPC port. The correct RPC port for btcd on simnet is 18556. This configuration will prevent lnd from connecting to btcd and should be corrected.
| "--$BACKEND.rpchost"="$RPCHOST:18555" \ | |
| "--$BACKEND.rpchost"="$RPCHOST:18556" \ |
docker/lnd/start-lnd.sh
Outdated
| # Also, setting --rpclisten to $HOSTNAME will cause it to listen on an IP | ||
| # address that is reachable on the internal network. If you do this outside of | ||
| # docker, this might be a security concern! | ||
| # [:18555] is the default btcd RPC port in SIMNET network. |
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 comment incorrectly states that 18555 is the default btcd RPC port for simnet. The correct default RPC port for btcd on simnet is 18556. Port 18555 is the P2P port. This is misleading and should be corrected to avoid confusion, especially as the incorrect port is used later in this script.
| # [:18555] is the default btcd RPC port in SIMNET network. | |
| # [:18556] is the default btcd RPC port in SIMNET network. |
8ae435a to
dca2938
Compare
|
Hi! Thank you for that PR! |
Hi! Thanks for the review. Sorry for the confusion; here are the details: The guide is intended to use Simnet, as it's the fastest way to test the payment flows locally. The Regarding your question about the Dockerfile: I believe it's best to keep the Simnet focus to ensure the fastest onboarding for developers. I will refine the documentation to make this distinction clear and remove any ambiguous references to Signet where they aren't needed. I've just updated the PR with these corrections. Let me know if you have any further suggestions! |
dca2938 to
f7af220
Compare
|
@Liongrass I have completed the requested changes and the PR is ready for review. Thank you!" |
|
/gemini review |
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.
Code Review
This pull request provides a great update to the Docker-based step-by-step guide for sending payments. Focusing on simnet and removing the obsolete faucet information makes the guide much more approachable and reliable for newcomers. The updates to use the modern docker compose syntax and improve the Docker configurations with version pinning, health checks, and more secure defaults are excellent enhancements. I have one minor suggestion to improve the clarity of the documentation.
docker/README.md
Outdated
| the Lightning Network Daemon (`lnd`). This folder uses `docker` to | ||
| package `lnd` and `btcd` together to make deploying the two daemons as easy as | ||
| typing a few commands. All configuration between `lnd` and `btcd` are handled | ||
| automatically by their `docker-compose` config 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.
For better clarity and consistency, consider changing docker to docker compose. While docker compose is part of the docker CLI, the next sentence specifically mentions the docker-compose config file. Using docker compose explicitly would make it clearer to the user which tool is being referred to, especially since the rest of the document has been updated to use the docker compose syntax.
| the Lightning Network Daemon (`lnd`). This folder uses `docker` to | |
| package `lnd` and `btcd` together to make deploying the two daemons as easy as | |
| typing a few commands. All configuration between `lnd` and `btcd` are handled | |
| automatically by their `docker-compose` config file. | |
| the Lightning Network Daemon (`lnd`). This folder uses `docker compose` to | |
| package `lnd` and `btcd` together to make deploying the two daemons as easy as | |
| typing a few commands. All configuration between `lnd` and `btcd` are handled | |
| automatically by their `docker-compose` config file. |
31d206f to
380e41b
Compare
|
/gemini review |
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.
Code Review
This pull request provides a great update to the Docker-based step-by-step guide for sending payments. The focus on simnet, removal of obsolete information about the faucet, and updates to use modern Docker commands and practices significantly improve the guide's usability and reliability for new developers. The Dockerfile enhancements, such as version pinning, adding a health check, and improving security defaults, are also valuable contributions. I've found one minor inconsistency in the documentation that I've commented on. Overall, this is a solid improvement.
|
Hi Euler! Looks good so far, thank you for these changes. Next will be for me to try it out by following the guide strictly! |
Perfect, thanks! Looking forward to your feedback. I'll be available to make any necessary adjustments based on your testing." |
380e41b to
5be4bc5
Compare
|
@Euler-B, remember to re-request review from reviewers when ready |
|
Thank you for this pull request and all the changes!
|
5be4bc5 to
6d82b86
Compare
|
@Liongrass based on your feedback, I have implemented the following changes:
|
|
/gemini review |
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.
Code Review
This pull request provides a significant update to the Docker-based guide for sending payments with lnd. The changes focus on providing a seamless simnet experience, removing obsolete information like the non-functional faucet, and updating Docker configurations to follow modern best practices. The introduction of a HEALTHCHECK in the btcd Dockerfile and the update to docker compose syntax are great improvements. The guide is now much clearer and more reliable for new users. I have one suggestion in docker/README.md to improve the security recommendation regarding running Docker commands.
docker/README.md
Outdated
| > All commands should be executed from the `lnd/docker/` directory. To avoid adding `sudo` to every command, switch to the root user first: | ||
| > ```bash | ||
| > sudo -i | ||
| > ``` | ||
| > **Security Note:** Running as root can be dangerous. Once you have finished executing the necessary commands, remember to exit the root shell by typing `exit` or pressing `Ctrl+D` to return to your normal user and avoid accidental system changes. |
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.
Suggesting users switch to a root shell with sudo -i can be risky, especially for newcomers who might not be fully aware of the implications. It's generally better to promote safer practices.
A good alternative is to instruct users to run Docker commands with sudo if needed, or to follow Docker's official post-installation steps to manage Docker as a non-root user, which usually involves adding the user to the docker group.
Consider rephrasing this section to either show sudo in the commands or to link to Docker's documentation on non-root user setup.
For example:
> All commands should be executed from the `lnd/docker/` directory. Depending on your system's configuration, you may need to prefix `docker` commands with `sudo`. Alternatively, you can follow the [official Docker documentation](https://docs.docker.com/engine/install/linux-postinstall/) to allow running Docker commands without `sudo`.6d82b86 to
693707a
Compare
|
/gemini review |
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.
Code Review
This pull request provides an excellent update to the Docker-based getting-started guide. It modernizes the workflow by focusing on simnet, removing obsolete information, and updating dependencies and commands. The changes make the guide much more accurate and user-friendly for new developers. I've added a few minor suggestions to improve consistency in the documentation's formatting.
| This document is intended for those looking to get started with | ||
| the Lightning Network Daemon (lnd). This folder uses docker to package lnd and | ||
| btcd together, making the deployment of both daemons as simple as running a | ||
| few commands. All configuration between lnd and btcd is handled automatically | ||
| by the docker-compose.yml 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.
The updated introduction removes backticks from terms like lnd, btcd, and docker-compose.yml. For consistency with the rest of the document, where such terms are enclosed in backticks for code formatting, it would be better to add them.
| This document is intended for those looking to get started with | |
| the Lightning Network Daemon (lnd). This folder uses docker to package lnd and | |
| btcd together, making the deployment of both daemons as simple as running a | |
| few commands. All configuration between lnd and btcd is handled automatically | |
| by the docker-compose.yml file | |
| This document is intended for those looking to get started with | |
| the Lightning Network Daemon (`lnd`). This folder uses `docker` to package `lnd` and | |
| `btcd` together, making the deployment of both daemons as simple as running a | |
| few commands. All configuration between `lnd` and `btcd` is handled automatically | |
| by the `docker-compose.yml` file |
docker/README.md
Outdated
| > [!IMPORTANT] | ||
| > **Prerequisites:** This guide assumes you have **Docker** installed. If not, please follow the [official Docker installation guide](https://docs.docker.com/get-docker/). | ||
| > | ||
| >All commands should be executed from the `lnd/docker/` directory. Depending on your system's configuration, you may need to prefix **docker** commands with **sudo**. For `Linux` users, we highly recommend following the official [Docker documentation to manage Docker as a non-root](https://docs.docker.com/engine/install/linux-postinstall/) user, which allows you to run commands without sudo safely |
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.
For consistency, it's better to use backticks for command names like docker and sudo instead of bolding. Also, there's an extra space after Linux that should be removed, and sudo at the end of the sentence could also be in backticks for clarity.
| >All commands should be executed from the `lnd/docker/` directory. Depending on your system's configuration, you may need to prefix **docker** commands with **sudo**. For `Linux` users, we highly recommend following the official [Docker documentation to manage Docker as a non-root](https://docs.docker.com/engine/install/linux-postinstall/) user, which allows you to run commands without sudo safely | |
| >All commands should be executed from the `lnd/docker/` directory. Depending on your system's configuration, you may need to prefix `docker` commands with `sudo`. For `Linux` users, we highly recommend following the official [Docker documentation to manage Docker as a non-root](https://docs.docker.com/engine/install/linux-postinstall/) user, which allows you to run commands without `sudo` safely |
docker/README.md
Outdated
| # Run "Bob" node and log into it: | ||
| $ docker-compose run -d --name bob --volume simnet_lnd_bob:/root/.lnd lnd | ||
| $ docker compose run -d --name bob --volume simnet_lnd_bob:/root/.lnd lnd | ||
| $ docker exec -i -t bob bash |
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.
693707a to
c6be6e4
Compare
|
/gemini review |
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.
Code Review
This pull request provides a significant and valuable update to the Docker-based "Send Payment" guide. The changes modernize the documentation by focusing on simnet, removing obsolete sections like the offline faucet, and updating commands to the current docker compose syntax. The underlying Docker configuration is also improved with best practices such as version pinning for base images, adding health checks, and enhancing security by restricting RPC listening to localhost. The guide is now much clearer and more reliable for new developers. I have a couple of minor suggestions to further improve the polish of the documentation and Dockerfile.
docker/README.md
Outdated
| > [!IMPORTANT] | ||
| > **Prerequisites:** This guide assumes you have **Docker** installed. If not, please follow the [official Docker installation guide](https://docs.docker.com/get-docker/). | ||
| > | ||
| >All commands should be executed from the `lnd/docker/` directory. Depending on your system's configuration, you may need to prefix **docker** commands with **sudo**. For `Linux` users, we highly recommend following the official [Docker documentation to manage Docker as a non-root](https://docs.docker.com/engine/install/linux-postinstall/) user, which allows you to run commands without sudo safely |
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.
There's a minor typo with a double space after `Linux`. Removing it will improve the readability of this important note for users.
| >All commands should be executed from the `lnd/docker/` directory. Depending on your system's configuration, you may need to prefix **docker** commands with **sudo**. For `Linux` users, we highly recommend following the official [Docker documentation to manage Docker as a non-root](https://docs.docker.com/engine/install/linux-postinstall/) user, which allows you to run commands without sudo safely | |
| >All commands should be executed from the `lnd/docker/` directory. Depending on your system's configuration, you may need to prefix **docker** commands with **sudo**. For `Linux` users, we highly recommend following the official [Docker documentation to manage Docker as a non-root](https://docs.docker.com/engine/install/linux-postinstall/) user, which allows you to run commands without sudo safely |
docker/btcd/Dockerfile
Outdated
|
|
||
| # Pin down btcd to a version that we know works with lnd. | ||
| ARG BTCD_VERSION=v0.23.4 | ||
| ARG BTCD_VERSION=v0.25.0 |
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.
| --noseedbackup \ | ||
| "--$CHAIN.active" \ | ||
| "--$CHAIN.$NETWORK" \ | ||
| "--$CHAIN.node"="$BACKEND" \ | ||
| "--$BACKEND.rpccert"="$RPCCRTPATH" \ | ||
| "--$BACKEND.rpchost"="$RPCHOST" \ | ||
| "--$BACKEND.rpcuser"="$RPCUSER" \ | ||
| "--$BACKEND.rpcpass"="$RPCPASS" \ | ||
| "--rpclisten=$HOSTNAME:10009" \ | ||
| "--rpclisten=localhost:10009" \ |
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.
The removal of --$CHAIN.active and --rpclisten=$HOSTNAME:10009 are great improvements. The former is deprecated and the latter improves security by only listening on localhost within the container. However, the lnd command for the btcd backend is missing the --$CHAIN.active flag that is still present for the bitcoind backend. While this flag is deprecated, for consistency it might be better to remove it from the bitcoind block as well.
a4a149b to
61f2a49
Compare
|
/gemini review |
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.
Code Review
This pull request provides a much-needed update to the Docker-based "Send Payment" guide. The changes significantly improve the user experience by focusing on a simnet environment, removing obsolete information about the offline faucet, and modernizing the Docker commands and configurations. The use of version pinning in Dockerfiles and the addition of a healthcheck are excellent improvements for reproducibility and robustness. Overall, this is a great contribution that makes the guide more accessible and reliable for new developers. I have one minor suggestion to improve clarity in one of the scripts.
docker/lnd/start-lnd.sh
Outdated
| # Also, setting --rpclisten to $HOSTNAME will cause it to listen on an IP | ||
| # address that is reachable on the internal network. If you do this outside of | ||
| # docker, this might be a security concern! | ||
| # [:18556] is the default btcd RPC port in SIMNET network. |
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 comment about the btcd RPC port seems slightly misplaced. It's located within a section discussing lnd's RPC listener configuration (--rpclisten), which could be confusing as it refers to a different service and port. For better clarity, consider moving this comment closer to where the btcd backend is configured (e.g., near the --$BACKEND.rpchost flag) or removing it if it's deemed non-essential.
61f2a49 to
ae3fe60
Compare
|
/gemini review |
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.
Code Review
This pull request provides a much-needed update to the Docker-based step-by-step guide for sending payments. The changes are excellent, focusing on simnet for a smoother developer experience, removing obsolete sections, and updating commands to the modern docker compose syntax. The underlying Docker configuration is also improved with updated dependencies, version pinning, a health check for btcd, and a fix for certificate generation. Overall, this makes the guide significantly more reliable and user-friendly. I've found a couple of minor areas for improvement to ensure consistency throughout the documentation and scripts.
| --noseedbackup \ | ||
| "--$CHAIN.active" \ | ||
| "--$CHAIN.$NETWORK" \ |
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.
The --$CHAIN.active flag was removed for the btcd backend, but it's still present for the bitcoind backend. While lnd might default to activating the bitcoin chain, it's better to be explicit for clarity and consistency between the two backend configurations in this script.
| --noseedbackup \ | |
| "--$CHAIN.active" \ | |
| "--$CHAIN.$NETWORK" \ | |
| --noseedbackup \ | |
| "--$CHAIN.active" \ | |
| "--$CHAIN.$NETWORK" \ |
ae3fe60 to
53bec97
Compare
|
/gemini review |
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.
Code Review
This pull request provides a significant improvement to the Docker-based "send payment" guide. The changes, including focusing on simnet, removing obsolete information about the faucet, and modernizing Docker commands, make the guide much clearer and more reliable for new users. The updates to the Dockerfiles, such as version pinning, adding health checks, and improving configuration flexibility, are also excellent enhancements. The code is of high quality and the changes are well-executed. I have one suggestion to improve consistency in the documentation.
|
@Liongrass I've also added |
|
Hi Euler, I assume this line Other than that, looks great now, thank you very much for your work! |
|
Hi @Liongrass, My apologies for the unintentional use of the |
53bec97 to
75c1367
Compare
|
Super cool, thank you for these changes and your contribution to LND! I cannot promise you how fast this will get merged, as reviews can often take time, but it doesn't touch any of the core codebase either, so we can be optimistic! |
Change Description
This
PRis the result of a step-by-step validation of the Send Payment Guide withDocker. After encountering several issues with the original workflow, I conducted a deep dive into the underlying failures, testing all available options described in the guide and researching related documentation.The result is an updated, functional guide that works out-of-the-box. Key changes include:
Simnet Focus: This update specifically focuses on Simnet due to its simplicity in block generation. It provides a seamless environment for new developers to test LND payment commands instantly without external dependencies.
Removal of Obsolete Resources: I have removed the section regarding the
faucet.lightning.community.web, as it is currently offline.Signet Considerations: I've clarified that obtaining funds via Signet can be time-consuming, as it depends on available computing power for mining/proof-of-work requirements, which might frustrate newcomers.
This guide now serves as a reliable and modern reference for anyone’s first encounter with the
LNDpayment flow..Steps to Test
Follow the instructions provided in the updated
docker/README.md.Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.