-
Notifications
You must be signed in to change notification settings - Fork 28
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
Remove ipv4 v3 #303
base: main
Are you sure you want to change the base?
Remove ipv4 v3 #303
Conversation
1796d65
to
0c39b18
Compare
0c39b18
to
325ff00
Compare
b26da95
to
7667344
Compare
@@ -468,7 +486,6 @@ def create_lnd_container( | |||
f"--bitcoind.zmqpubrawblock={bitcoind_rpc_host}:{tank.zmqblockport}", | |||
f"--bitcoind.zmqpubrawtx={bitcoind_rpc_host}:{tank.zmqtxport}", | |||
f"--rpclisten=0.0.0.0:{tank.lnnode.rpc_port}", | |||
f"--externalhosts={lightning_dns}", |
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.
might reconsider keeping this. from the doc:
--externalhosts=
Add a hostname:port that should be periodically resolved to announce IPs for. If a port is not specified, the default (9735) will be used.
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.
Hmm, not sure what to do here, as we won't know the ip address until after startup in our classic, re-occurring chicken-and-egg problem.
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.
but lightning_dns
is a domain name isn't it? Sounds to me like lnd will resolve the IP sometime after startup
src/warnet/lnnode.py
Outdated
res = self.lncli("getinfo") | ||
return res["uris"][0] | ||
def get_pubkey(self): | ||
return self.lncli("getinfo")["identity_pubkey"] |
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.
could cache this with a @property
?
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.
Now cached with property
src/warnet/lnnode.py
Outdated
@@ -78,17 +81,21 @@ def generate_cli_command(self, command: list[str]): | |||
return cmd | |||
|
|||
def export(self, config, subdir): | |||
container_name = self.backend.get_container_name(self.tank.index, ServiceType.LIGHTNING) | |||
container_name = self.backend_interface.get_container_name( |
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 this already cached as self.hostname
?
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.
Switched to using self.hostname
here and later in the function.
# TODO: What does this break? | ||
# "address": f"https://{self.ipv4}:{self.rpc_port}", |
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 point actually. So, running warnet+simln locally, simln would be running on the host and not in the docker network at all. It would use the IPs of the lnd nodes inside docker to send RPC commands (remember we made them fix it, bitcoin-dev-project/sim-ln#151).
This may be the only time we explicitly need the actual IP of the lnd node.
Or we could work out how to incorporate sim-ln inside a container in the network, which we will probably need for K8s deployment anyway
Re: #134 Yes this does create each new network with a unique subnet. Test: warcli network start --network=wn1
warcli network start --network=wn2
docker inspect wn1 | grep Subnet
docker inspect wn2 | grep Subnet However I think we could make one additional change to the compose file to make it even more clear in the docker desktop GUI and who knows wherever else: diff --git a/src/backends/compose/compose_backend.py b/src/backends/compose/compose_backend.py
index ecc067c..7eab535 100644
--- a/src/backends/compose/compose_backend.py
+++ b/src/backends/compose/compose_backend.py
@@ -289,7 +289,7 @@ class ComposeBackend(BackendInterface):
def _write_docker_compose(self, warnet):
compose = {
"version": "3.8",
- "name": "warnet",
+ "name": warnet.network_name,
"networks": {
warnet.network_name: {
"name": warnet.network_name,
|
7667344
to
ab7e683
Compare
I just added the fixed subnet back in. It would seem to me to make things much easier for you on the Tor side, whilst not having much downside for us? It's compose-only now, and not part of |
Remove the ipv4 stuff (and fixed subnet from compose).
Closes #253