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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lightning pool #313

Merged
merged 1 commit into from Mar 1, 2021
Merged

Lightning pool #313

merged 1 commit into from Mar 1, 2021

Conversation

sputn1ck
Copy link
Contributor

@sputn1ck sputn1ck commented Feb 1, 2021

This PR integrates lightning-pool to nix-bitcoin. Lightning Pool is a non-custodial batched uniform clearing-price auction for Lightning Channel Leases (LCL). An LCL packages up inbound channel liquidity (ability to receive funds) as a fixed income asset with a maturity date expressed in blocks.

I'm pretty novice at nixos, so it's better if you take a closer look 馃槃

Details

  • The tls certificate and key can't be in the /secrets/ folder as the --basedir arg overwrites --tlscertpath
  • The conf file needs to be written in the basedir folder, I don't know if the chmod and chown I did was right
  • It requires the uptime bitcoind rpc call
  • It requires a publicly reachable uri, I tried setting nix-bitcoin.onionServices.lnd.public=true; in the module itself, however it led to error: infinite recursion encountered, which is why I added the assertion
  • Tested with a succesful account and order creation

@erikarvstedt
Copy link
Collaborator

which is why I added the assertion

That's the preferred solution anyways, because users shouldn't be forced to use tor for public addresses. They can also manually define getPublicAddressCmd instead.

Can you add a test? (See tests.nix, tests.py)

@Kixunil
Copy link

Kixunil commented Feb 2, 2021

FYI a change to pool just got merged that allows you to specify a single macaroon instead of the whole directory. So the next release should have it. Using it may be a better design if you decide to move macaroons in the future (e.g. to put them in differently-protected directories).

echo "Fetching latest release"
git clone https://github.com/lightninglabs/pool 2> /dev/null
cd pool
latest=v0.4.3-alpha
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why you're not using the "latest release" logic from the other get-sha256 scripts?

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 think lightning labs did something wrong with their tagging. If i use the other logic i get the auctioneerrpc/v1.0.1 tag

Copy link
Member

Choose a reason for hiding this comment

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

The failure results from multiple tags being on the same commit. Let's use the default logic on the next tag, which will hopefully be the only one on that commit.

@sputn1ck
Copy link
Contributor Author

sputn1ck commented Feb 2, 2021

Can you add a test? (See tests.nix, tests.py)

Just added the tests similiar to those of lightning-loop

@nixbitcoin
Copy link
Member

nixbitcoin commented Feb 2, 2021

Fixups

Great work @sputn1ck! The only thing that annoys me is that there is no proxy option to connect to auction servers over Tor. Should we open an issue upstream?

EDIT: Issue opened lightninglabs/pool#215

@sputn1ck
Copy link
Contributor Author

sputn1ck commented Feb 3, 2021

Fixups

I rebased to the current master and included your fixups

@nixbitcoin
Copy link
Member

Only two very minor fixups I missed during my review, then I'm ready to ACK

nixbitcoin
nixbitcoin previously approved these changes Feb 3, 2021
Copy link
Member

@nixbitcoin nixbitcoin left a comment

Choose a reason for hiding this comment

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK d605c319868f3b5ea34b59cd663860b0190ed3bc
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEV3o0Un8+KoXoD+Fk3RH5rVMIs7oFAmAamcwACgkQ3RH5rVMI
s7p79w//WXkSB2nfWIaXsxXYXXXKC91bQ3RZD/690xTHk/xAocAFMQvg2elWzSEF
MJciKO3O0aNjAIxzQDWyoeknbVlul/iQHU208OqP3tHiCQ0OWC5QATnjHbzE3pTG
AoNz0uUkKujvzbkqAr1rsr/zR9uP77f2TnqaKQpH2Zb9RWWAgrKOlAp0/3i1iOs/
VUJkbngVVMA8oPXiNurnQBaAD+jLgPWsdqXCm0oj4TYPiW602+Oe5kriwlsf2ZEb
3jPFRsllPsqsCRe4/ZgrUlbFM/56rvaDytthO06qD0ZzYUdPukPxXmtOzgX+ypbM
RvauSyFJmu2nld0zQ+R50msjqTZQ7eIRr33CrfDnRc3EdoMW4ki1yvoPMWWNLN9X
Cqkjl5WOPQH4TIigjIhLMxLBQ49uIfLjpW4FJG5YatYWSkr1CIeN+uBR/agzfSA+
qxtLyXTOCoKoi1ub4kp9v+JROVIm/I0x69sWL5+ey2nxCHpySQzo4w3rEOGqpa/Q
tAz+RH9xU2hvXKUXjbdjdmfspcu8RX70NzVI5z0F+pNB0ZcKBshJhSI8/Z49kWcY
nIDFgBXNWlTWhbAWplfQLv2CZBY+2cvoa3fjWslWSJJ750x0AZbWYWDBK09ya9h0
EeEvwyxL3RRCGRNqYZIl1BtAETTmpBLQ2orfPbRKuwAojF6Flhk=
=r9E7
-----END PGP SIGNATURE-----

@sputn1ck sputn1ck mentioned this pull request Feb 3, 2021
Copy link
Member

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

I really don't like that this module leaks your ip address to the pool server. We either need to document this clearly, use iptables to route through tor ("transparent proxy") or wait until pool has a proxy option.

# clearing-price auction for Lightning Channel Leases. pool (lightning-pool
# daemon) will be started automatically. Users can interact with it using
# `pool`.
# Privacy Warning: Lightning-pool currently connects 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.

Would a warning here be enough?

@sputn1ck
Copy link
Contributor Author

sputn1ck commented Feb 3, 2021

I really don't like that this module leaks your ip address to the pool server. We either need to document this clearly, use iptables to route through tor ("transparent proxy") or wait until pool has a proxy option.

I opened a PR upstream lightninglabs/pool#216 which works on my nix-bitcoin branch

@nixbitcoin
Copy link
Member

Don't forget to add lightning-pool to the README.md

@erikarvstedt
Copy link
Collaborator

Fixups.

Does loop require that lnd announces its address via externalip? Or is public reachability sufficient?

Please squash the test commit into the other commit that contains the corresponding functional changes (rationale).

@sputn1ck
Copy link
Contributor Author

sputn1ck commented Feb 3, 2021

Does loop require that lnd announces its address via externalip? Or is public reachability sufficient?

https://github.com/lightninglabs/pool/blob/41d958e284f6c9c25239e06263454d010a6633b7/order/manager.go#L165

// If the order is a ask, then this means they should be an effective
// routing node, so we require them to have at least a single
// advertised address.
if len(nodeAddrs) == 0 && order.Type() == TypeAsk {
    return nil, fmt.Errorf("the lnd node must " +
	    "be reachable on clearnet to negotiate channel " +
	    "ask order")
} 

afaiu it requires it via externalip

@erikarvstedt
Copy link
Collaborator

lnd has other ways like this for setting the external ip, so I'd recommend to remove the getPublicAddressCmd assertion.

@sputn1ck
Copy link
Contributor Author

sputn1ck commented Feb 5, 2021

lightninglabs/pool#216 has been merged, should we wait for a new release?

lnd has other ways like this for setting the external ip, so I'd recommend to remove the getPublicAddressCmd assertion.

Should the information of public addresses be added to the example/configuration.nix ?

@erikarvstedt
Copy link
Collaborator

I'm fine with using the unreleased version.

Should the information...

Yes, great idea!

@jonasnick
Copy link
Member

I'd try to avoid running unreleased versions wherever possible. We can wait for a new pool release or document the privacy implications of the current version in configuration.nix and remove them as soon as we update to the new release.

@Kixunil
Copy link

Kixunil commented Feb 5, 2021

Versions seem to be released roughly monthly and the last one was 21 days ago. So it shouldn't be too long I think.

@nixbitcoin
Copy link
Member

Fixups

Apply and we should be ready for merge

@erikarvstedt
Copy link
Collaborator

Fixups of the fixups. (Guys, we've finally reached peak fixups.)

@sputn1ck
Copy link
Contributor Author

sputn1ck commented Mar 1, 2021

Fixups of the fixups. (Guys, we've finally reached peak fixups.)

Pushed the fixups. Is it ready now? 馃槃

Copy link
Collaborator

@erikarvstedt erikarvstedt left a comment

Choose a reason for hiding this comment

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

ACK eb21012

Copy link
Member

@nixbitcoin nixbitcoin left a comment

Choose a reason for hiding this comment

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK eb210127455ae7de45169237ee8b8eb53585c20b
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEiDOlIkszv4ojhh+OtgROy6La5dAFAmA9XvgACgkQtgROy6La
5dAelA//Ybmj1rtTZFHNK7195PTQPFED77MXWWLPfMo7HFwmGakQ7hNR5AiwYsir
7tmz0LH6MqcanGWk4SyFBqeLrdCsO5zVbpLACPzON3JWO9BrSad58BdI8cgj8PYf
+INJuFxd2LnYk1p9wHNBGGR+Gp0s7F0lf83hqr93COg4e+DMU4Keo98nRSXSkGfN
12tAVy0/n3o/MFfBoMYM/Qx0zEGUx4h0gqTWzBevog68CVf2FhfkRLvtNY6tdlon
5zrTJ7zzLA6KPD1mcVfzf4BL+A3Nmo15Jb/dio9dfs9eh4VU9ZLbXuKBd3Z2z6IF
u4FxZvDQlHPn0W1kSBsoB3q2t480IBhOjl3RU58SyC565BmObjeI4N9QwcYR1dxk
RpjccBqL9F6fXyelcxc5on3Il6IzWXu1qlnuxgKTZlQNjrdwrX2V/Xik8517wgH6
Oo0uzrog7VrTTGmSFeuS3d9urPIsWPnU70oVhdIvm1gGEiNn4Ckj/MdYL5yJ8Z6C
misDDb+Ic6PDy2KUujLvQditckzXWsuelLzPfmd2+Nr8PQJ9nD/85eC+8jsFdti0
YMrRwpgSF79NXcEt5UuFyM41AGapy94f2z6C6+QRhTBm429wM5c2FqIOgzoRlpMt
hab64LCj+bHhyYdqLZbxiSlyO7rpqe25Xsf+RT7nZO4Ir31ahZM=
=SxML
-----END PGP SIGNATURE-----

@jonasnick jonasnick merged commit 820de2e into fort-nix:master Mar 1, 2021
@jonasnick
Copy link
Member

Excellent, thanks everyone!

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.

None yet

5 participants