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

Bolt8 transport #198

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Bolt8 transport #198

wants to merge 3 commits into from

Conversation

ecdsa
Copy link
Member

@ecdsa ecdsa commented Nov 3, 2022

companion PR of spesmilo/electrum#8049

@@ -43,6 +43,9 @@ def __init__(self, coin=None):

# Core items

self.is_public = self.boolean('PUBLIC', False)
Copy link
Member

@SomberNight SomberNight Nov 3, 2022

Choose a reason for hiding this comment

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

IMO True would be a better default. I expect most people will want True, and then it is one fewer thing to configure.

@@ -43,6 +43,9 @@ def __init__(self, coin=None):

# Core items

self.is_public = self.boolean('PUBLIC', False)
self.is_watchtower = self.boolean('WATCHTOWER', False)
assert not (self.is_public and self.is_watchtower)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert not (self.is_public and self.is_watchtower)
if self.is_public and self.is_watchtower:
raise cls.Error("WATCHTOWERs are not safe to use atm for PUBLIC servers")

note: operators of public servers might want to use the watchtower functionality for themselves though. So we could allow PUBLIC and WATCHTOWER, but restrict the watchtower-related RPCs to the whitelist.

@@ -99,6 +101,8 @@ def __init__(self, coin=None):
if {service.protocol for service in self.services}.intersection(self.SSL_PROTOCOLS):
self.ssl_certfile = self.required('SSL_CERTFILE')
self.ssl_keyfile = self.required('SSL_KEYFILE')
if any([service.protocol == 'bolt8' for service in self.services]):
self.bolt8_keyfile = self.required('BOLT8_KEYFILE')
Copy link
Member

@SomberNight SomberNight Nov 3, 2022

Choose a reason for hiding this comment

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

Maybe make this config key optional (and put it inside DB_DIRECTORY) by default.
Regardless, the envvar should be documented.

Comment on lines +183 to +185
from electrum import ecc
from electrum import logging
from electrum.lntransport import create_bolt8_server
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to avoid depending on electrum. Or at least, let's try not to make it a required dependency.
The watchtower functionality could be optional, and if enabled, we could then require electrum.

Re the bolt8 transport bits, we should consider splitting that functionality out of electrum into a separate package, and only depend on that here.

Regardless, electrum is currently not distributed on PyPI. Whatever we end up depending on here, we should either distribute that on PyPI, or maybe make it a git submodule.

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.

2 participants