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

docker: setup bitcoind docker image in the docker-compose.yml file #8354

Conversation

mohamedawnallah
Copy link
Contributor

@mohamedawnallah mohamedawnallah commented Jan 8, 2024

Change Description

Added bitcoind docker image setup in the docker-compose.yml file.

Closes #2827

Steps to Test

  1. Testing Procedure: I ran the bitcoind docker image locally alongside the lnd image.
  2. Networks Tested: Checked functionality on regtest, testnet, and mainnet.
  3. Regression Testing: Ensured no issues arise for btcd implementation.

Demo

Here's a quick demo showcasing the setup run:

docker-compose-up-bitcoind-lnd.mov

Pull Request Checklist

Testing

  • Your PR passes all CI checks.
  • Tests covering the positive and negative (error paths) are included.
  • Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

📝 Please see our Contribution Guidelines for further guidance.

@guggero guggero self-requested a review January 8, 2024 08:57
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Very nice! Should definitely be useful to have.

@@ -0,0 +1,8 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: empty line at the beginning of the file.

@@ -1,23 +1,22 @@
version: '2'
version: '3'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should keep the docker-compose.yml with btcd to not break users that are already using it. So just add a new docker-compose-bitcoind.yml instead.

docker/bitcoind/start-bitcoind.sh Show resolved Hide resolved
@mohamedawnallah mohamedawnallah force-pushed the add-bitcoind-docker-image-compose-setup branch from b9e6f58 to 75f1171 Compare January 11, 2024 18:04
# container start within starting script.
btcd:
image: btcd
container_name: btcd
Copy link
Collaborator

Choose a reason for hiding this comment

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

And now we have two files for btcd???

Copy link
Contributor Author

@mohamedawnallah mohamedawnallah Jan 12, 2024

Choose a reason for hiding this comment

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

Sorry, It seems I've mistakenly replaced the docker-compose-bitcoind.yml with docker-compose-btcd.yml! Returned it...


PARAMS=$(echo $PARAMS \
"-debug"="$DEBUG" \
"-rpcuser"="$RPCUSER" \

Choose a reason for hiding this comment

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

Any reason this is using rpcuser/rpcpassword rather than rpcauth or cookie based authentication?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To use rpcauth you need to first hash the password for which you need to have the appropriate tooling available in the shell script. And cookie based auth would require files to be shared between docker containers while maintaining the proper permissions, which in my experience isn't worth the hassle.
Maybe we should make it more clear that these scripts/containers/composes are just meant as a quick development setup and shouldn't be used in production.

Copy link

@fanquake fanquake Jan 12, 2024

Choose a reason for hiding this comment

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

Sure, my point is more that at some point user/pass will be removed from Core (they are mostly considered deprecated), and then the alternative methods will need to be used in either case, so it seems beneficial to start using them now.

Copy link
Collaborator

@guggero guggero Jan 12, 2024

Choose a reason for hiding this comment

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

Hmm, yeah, I keep forgetting that it's deprecated. I guess we can just replace $RPCUSER and $RPCPASSWORD with $RPCAUTH within this script and compose file and leave the hashing of the password to the user that wants to change the values (and perhaps link to the python script that can generate it.

Copy link
Contributor Author

@mohamedawnallah mohamedawnallah Jan 12, 2024

Choose a reason for hiding this comment

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

Okay ACK will update the PR accordingly! That said I've noticed that lnd doesn't have parameter for bitcoind.rpcauth ?

bitcoind:
      --bitcoind.dir=                                        The base directory that contains the
                                                             node's data, logs, configuration
                                                             file, etc. (default:
                                                             /Users/mohamedawnallah/Library/Applic-

                                                             ation Support/Bitcoin)
      --bitcoind.config=                                     Configuration filepath. If not set,
                                                             will default to the default filename
                                                             under 'dir'.
      --bitcoind.rpccookie=                                  Authentication cookie file for RPC
                                                             connections. If not set, will default
                                                             to .cookie under 'dir'.
      --bitcoind.rpchost=                                    The daemon's rpc listening address.
                                                             If a port is omitted, then the
                                                             default port for the selected chain
                                                             parameters will be used. (default:
                                                             localhost)
      --bitcoind.rpcuser=                                    Username for RPC connections
      --bitcoind.rpcpass=                                    Password for RPC connections
      --bitcoind.zmqpubrawblock=                             The address listening for ZMQ
                                                             connections to deliver raw block
                                                             notifications
      --bitcoind.zmqpubrawtx=                                The address listening for ZMQ
                                                             connections to deliver raw
                                                             transaction notifications
      --bitcoind.zmqreaddeadline=                            The read deadline for reading ZMQ
                                                             messages from both the block and tx
                                                             subscriptions (default: 5s)
      --bitcoind.estimatemode=                               The fee estimate mode. Must be either
                                                             ECONOMICAL or CONSERVATIVE. (default:
                                                             CONSERVATIVE)
      --bitcoind.pruned-node-max-peers=                      The maximum number of peers lnd will
                                                             choose from the backend node to
                                                             retrieve pruned blocks from. This
                                                             only applies to pruned nodes.
                                                             (default: 4)
      --bitcoind.rpcpolling                                  Poll the bitcoind RPC interface for
                                                             block and transaction notifications
                                                             instead of using the ZMQ interface
      --bitcoind.blockpollinginterval=                       The interval that will be used to
                                                             poll bitcoind for new blocks. Only
                                                             used if rpcpolling is true.
      --bitcoind.txpollinginterval=                          The interval that will be used to
                                                             poll bitcoind for new tx. Only used
                                                             if rpcpolling is true.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's correct. lnd still needs to know the actual password since it needs to send it over the RPC. The rpcauth config option in bitcoind just allows users to not need to specify a clear text password in bitcoin.conf. But any RPC client still needs to know the clear text password (or use the cookie authentication instead).

@mohamedawnallah mohamedawnallah force-pushed the add-bitcoind-docker-image-compose-setup branch 3 times, most recently from 6dee3e5 to fed63bc Compare January 12, 2024 16:15
@mohamedawnallah mohamedawnallah force-pushed the add-bitcoind-docker-image-compose-setup branch from fed63bc to 0223058 Compare January 12, 2024 16:34
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the update!

The code looks good to me, just a couple of comments about the additions to the docs.

docker/README.md Outdated
```

#### Generating RPCAUTH
Since using `rpcuser` and `rpcpassword` is deprecated in bitcoind core we need to use `rpcauth` instead. You could generate `rpcauth` using [rpcauth.py script](https://github.com/bitcoin/bitcoin/blob/master/share/rpcauth/rpcauth.py) from bitcoin core.
Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned, the rpcauth parameter can only be used on the server side. On the client side you still need to know username/password.

docker/README.md Outdated
#### Mining in regtest using bitcoin-cli
For mining in the regtest network, you can use `bitcoin-cli` with the following command:
```shell
$ bitcoin-cli -chain=regtest -rpcauth=<your_rpc_auth> generatetoaddress 10 <your_regtest_address>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this only works within the bitcoind container, we should change this to:

$ docker exec -ti bitcoind bitcoin-cli -chain=regtest -rpcuser=devuser -rpcpassword=devpass generatetoaddress 10 2N1NQzFjCy1NnpAH3cT4h4GoByrAAkiH7zu

And mention that the address is just a random/dummy address and can be replaced with your own regtest address. But to get lnd syncing (so before you can ask it to give you a wallet address you can then send coins to), you need to mine at least 1 block. So there's a bit of a catch-22 situation. That's why it's best to just give the user a command that works for mining the first block and then later mention they can use an address from their lnd wallet to send themselves coins.

docker/README.md Outdated
#### Generating Wallet in regtest using Bitcoin Core
To create a wallet in the regtest network using `bitcoin-cli`, use the following command:
```shell
$ bitcoin-cli -chain=regtest -rpcauth=<your_rpc_auth> createwallet <wallet_name>
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we do the above, then we don't need a wallet in bitcoind and can only use the one in lnd. Probably easier for the users to understand.

# Set default variables if needed.

# This password is for testing and it is equivalent to `devpass`.
# It is generated from (Note: Due to salting, it is not fully deterministic):
Copy link
Collaborator

Choose a reason for hiding this comment

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

micro-nit: it is fully deterministic, but because the salt is generated at random by the script, the actual hash always looks different.

Copy link
Contributor

coderabbitai bot commented Jan 18, 2024

Important

Auto Review Skipped

Auto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository from git and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

@mohamedawnallah mohamedawnallah force-pushed the add-bitcoind-docker-image-compose-setup branch from ea40bfc to a9e7aeb Compare January 18, 2024 12:14
@mohamedawnallah mohamedawnallah force-pushed the add-bitcoind-docker-image-compose-setup branch from a9e7aeb to 452c945 Compare January 18, 2024 12:22
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM 🎉

@@ -13,6 +13,8 @@ docker | 1.13.0
### Table of content
* [Create lightning network cluster](#create-lightning-network-cluster)
* [Connect to faucet lightning node](#connect-to-faucet-lightning-node)
* [Building standalone docker images](#building-standalone-docker-images)
* [Using bitcoind version](#using-bitcoind-version)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: these titles and links no longer match the actual headings below.

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. I added the sub-headings for the Using bitcoind version heading.

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've checked this heading and it matches the title and the link ?

[Building standalone docker images](#building-standalone-docker-images)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, sorry. I guess I missed that this was a heading that was already present in the document, just not in the table of contents. All good.

@guggero guggero requested a review from bhandras January 19, 2024 09:16
Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM (pending Oli's comment) 🎉


# set_default function gives the ability to move the setting of default
# env variable from docker file to the script thereby giving the ability to the
# user override it during container start.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: to override

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. I've just fixed this also in docker/btcd/start-btcd.sh since it is related.

exit 0
}

# return is used within bash function in order to return the value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: within the

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. I've just fixed this also in docker/btcd/start-btcd.sh since it is related.

"-txindex"
)

# Add user parameters to command.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: to the

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. I've just fixed this also in docker/btcd/start-btcd.sh since it is related.

@mohamedawnallah mohamedawnallah force-pushed the add-bitcoind-docker-image-compose-setup branch from 452c945 to cd1e751 Compare January 19, 2024 13:24
@@ -13,6 +13,8 @@ docker | 1.13.0
### Table of content
* [Create lightning network cluster](#create-lightning-network-cluster)
* [Connect to faucet lightning node](#connect-to-faucet-lightning-node)
* [Building standalone docker images](#building-standalone-docker-images)
* [Using bitcoind version](#using-bitcoind-version)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, sorry. I guess I missed that this was a heading that was already present in the document, just not in the table of contents. All good.

@guggero guggero merged commit cc18ec4 into lightningnetwork:master Jan 22, 2024
22 of 25 checks passed
@mohamedawnallah mohamedawnallah deleted the add-bitcoind-docker-image-compose-setup branch March 31, 2024 12:16
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.

Bitcoind docker image
4 participants