-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 litecoin docker image and container. #209
Conversation
@AndrewSamokhvalov, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Roasbeef, @murtyjones and @f-a-a to be potential reviewers. |
Coverage decreased (-0.01%) to 73.532% when pulling 7b99e6a937440c1de498a9b313170e1d1f5349c1 on AndrewSamokhvalov:litecoin_docker into 1124b45 on lightningnetwork:master. |
1027636
to
9454bc6
Compare
Coverage remained the same at 73.542% when pulling 9454bc69991751c28c0d5352e1e23138965e9348 on AndrewSamokhvalov:litecoin_docker into 1124b45 on lightningnetwork:master. |
9454bc6
to
dd91d94
Compare
Coverage remained the same at 73.542% when pulling dd91d9423416d9af69c567acc7150a12c40a14c5 on AndrewSamokhvalov:litecoin_docker into 1124b45 on lightningnetwork:master. |
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 tested this out locally and the addition of lnd
into the container configuration works just fine! However, I think we might need to modify the way we currently launch lnd
as in this PR, it's still linked to btcd
. This means that when docker-compose
goes to build the alice
or bob
container, then btcd
will always be built+run regardless of the environment variables we set.
Here's a proposal I think we resolve eveything:
- Create distinct
alice_CHAIN
andbob_CHAIN
containers in thedocker-compose
file. TheCHAIN
portion will either be btc, or ltcd, so there'll bealice_btc
,bob_btc
, `alice_ltc, etc. - In the
btc
orltc
container types for lnd, the link will be either tobtcd
orltcd. * We may even want to scrap the bob+alice distinction all together.
docker-compose` allows one to replicate a container template with a different name at launch time. So users will be able to name the containers w/e they want.
"--$CHAIN.rpccert"="/rpc/rpc.cert" \ | ||
"--$CHAIN.active" \ | ||
"--$CHAIN.$NETWORK" \ | ||
"--$CHAIN.rpchost"="blockchain" \ |
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 reference to blockchain
here doesn't need to be scoped in an environment variable?
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.
Nope :)
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.
Tested this locally, and looks like it does indeed to to be scope somehow.
Run from inside the container:
root@aec17b768d6e:/go/src/github.com/lightningnetwork/lnd# ps xa
PID TTY STAT TIME COMMAND
1 ? Ss 0:00 bash ./start-lnd.sh
11 ? Sl 0:04 lnd --datadir=/data --logdir=/data --bitcoin.rpccert=/rpc/rpc.cert --bitcoin.active --bitcoin.simnet --bitcoin.rpchost=blockchain --bitcoin.rpcuser=devuser --bitcoin.rpcpass=devpass --d
27 pts/0 Ss 0:00 bash
33 pts/0 R+ 0:00 ps xa
The point of interest is: --bitcoin.rpchost=blockchain
, which means the string was passed through without any sort of 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.
Actually scratch that. Not sure what happened before, but I'm able to properly connect now! Once the minor doc comments are addressed, I'll get this merged.
docker/ltcd/Dockerfile
Outdated
# Expose segnet ports (server, rpc) | ||
EXPOSE 28901 28902 | ||
|
||
# Grab and install the latest version of roasbeef's fork of btcd and all |
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.
Minor nit: comment needs to be updated.
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.
Similar comment about the references to btcd
as comment in the file.
Coverage increased (+0.08%) to 73.621% when pulling a0f9ad6a2263ae546dd05b65b01b3cffc400a64c on AndrewSamokhvalov:litecoin_docker into 1124b45 on lightningnetwork:master. |
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.
Took another pass of everything, the PR is nearly complete!
Two things remain
- The documentation needs to be modified as is pointed out in-line.
- Currently a container started using this PR can't connect to the running btcd node as the
rpchost
parameter isn't being passed through properly.
docker/README.md
Outdated
@@ -157,7 +157,7 @@ bob$ lncli listpeers | |||
Create the `Alice<->Bob` channel. | |||
```bash | |||
# Open the channel with "Bob": | |||
alice$ lncli openchannel --node_key=<bob_identity_pubkey> --num_confs=1 --local_amt=1000000 | |||
alice$ lncli openchannel --peer_id=1 --local_amt=1000000 |
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 peer_id
arguments are largely being phased out, why the change to identitfy by the peer ID? As is now, users won't be notified of the peer ID of a peer upon connection.
docker/README.md
Outdated
@@ -90,7 +90,7 @@ Connect `Bob` node to `Alice` node. | |||
|
|||
```bash | |||
# Run "Bob" node and log into it: | |||
$ docker-compose up --no-recreate -d "bob" | |||
$ docker-compose up --no-recreate -d --name "bob" "ltc_btc" |
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 last argument should instead be "lnd_btc"
.
docker/README.md
Outdated
@@ -64,7 +64,7 @@ bitcoin into. | |||
$ export NETWORK="simnet" | |||
|
|||
# Run the "Alice" container and log into it: | |||
$ docker-compose up -d "alice" | |||
$ docker-compose run -d --name alice "lnd_btc" |
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.
Quotes aren't needed around the "lnd_btc"
argument, here or below.
"--$CHAIN.rpccert"="/rpc/rpc.cert" \ | ||
"--$CHAIN.active" \ | ||
"--$CHAIN.$NETWORK" \ | ||
"--$CHAIN.rpchost"="blockchain" \ |
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.
Tested this locally, and looks like it does indeed to to be scope somehow.
Run from inside the container:
root@aec17b768d6e:/go/src/github.com/lightningnetwork/lnd# ps xa
PID TTY STAT TIME COMMAND
1 ? Ss 0:00 bash ./start-lnd.sh
11 ? Sl 0:04 lnd --datadir=/data --logdir=/data --bitcoin.rpccert=/rpc/rpc.cert --bitcoin.active --bitcoin.simnet --bitcoin.rpchost=blockchain --bitcoin.rpcuser=devuser --bitcoin.rpcpass=devpass --d
27 pts/0 Ss 0:00 bash
33 pts/0 R+ 0:00 ps xa
The point of interest is: --bitcoin.rpchost=blockchain
, which means the string was passed through without any sort of substitution.
In order to make the installation process of the btcd robust we should use "glide" instead of "go get".
This parameter now might not belong to the Bitcoin network, because we introduce the Litecoin image soon.
Intoduce the Litecoin network daemon, which will be needed in order to interact with Litecoin blockchain.
After addition of the Litecoin client image (ltcd), we should add additional parameter to notify the start script which network we should use.
Create disctinct containers for Litecoin and Bitcoin, and also change the readme accordingly.
a0f9ad6
to
76cedac
Compare
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.
LGTM 🛰
No description provided.