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

TOR patch #1521

Merged
merged 15 commits into from
Nov 5, 2024
Merged

TOR patch #1521

merged 15 commits into from
Nov 5, 2024

Conversation

riccardobl
Copy link
Member

@riccardobl riccardobl commented Oct 25, 2024

Description

This pr adds tor to the dev environment , generates an onion address and fixes some issues that could cause tor
to not work as expected in production.

Closes #1493
Closes #1531

LND and gRPC-js ✓

To connect to an onion address using grpc-js in ln-service, the only available solution appears to be setting the environment variable GRPC_PROXY or HTTPS_PROXY. However, this forces all gRPC traffic through Tor, which is not ideal.

To address this, I set up Privoxy in front of Tor and configured it to route only .onion traffic through the network. This HTTP (gRPC) only proxy runs alongside the Tor HTTP proxy on port 7051. this is now handled by a patched authenticatedLndGrpc that set grpc.enable_http_proxy

CLN ✓✓

cln worked out of the box once the hidden service was connected.

LNBITS ✓✓

lnbits did not work out of the box. I had to replace the use of fetch with cross-fetch to make it functional, as the native fetch lacks support for agents. With this change, the connection works, but I'm still unable to connect successfully since the lnbits instance is set up to use VoidWallet, which is unrelated to this PR.
worked out of the box once the lnd paths were restored

New commands

I've added some new commands to sndev

  • sndev stacker_lnd get_onion : returns the onion host:port for the stacker_lnd instance
  • sndev stacker_lnd get_cert : returns the tls cert for stacker_lnd (it now needs to include the onion address so it cannot be static)
  • sndev tor get_onion: returns the onion address (same as sndev stacker_lnd get_onion but without the lnd port)
  • sndev stacker_clncli get_onion: returns the onion address (host:port)
  • sndev lnbits get_onion: returns the onion address (host:port)

Checklist

Are your changes backwards compatible? Please answer below:

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:

all the wallets connect and the testInvoice succeed (except lnbits, see LNBITS )

Did you introduce any new environment variables? If so, call them out explicitly here:

GRPC_PROXY

@riccardobl riccardobl marked this pull request as ready for review October 26, 2024 09:45
@riccardobl riccardobl changed the title TOR for the dev environment TOR patch Oct 26, 2024
@huumn
Copy link
Member

huumn commented Nov 2, 2024

This looks solid! I'll pull down tomorrow to QA and we'll get it merged.

@huumn
Copy link
Member

huumn commented Nov 3, 2024

When I attempt to use attach lnd with onion I get:

failed to create test invoice: No connection established. Last error: undefined

Did you see that in your testing? Do you know what might be causing that?

@riccardobl
Copy link
Member Author

When I attempt to use attach lnd with onion I get:

failed to create test invoice: No connection established. Last error: undefined

Did you see that in your testing? Do you know what might be causing that?

I've seen this happening if you try to connect it too soon and the hidden service is not ready yet.
But can you please double check the address with sndev stacker_lnd get_onion ?
And if you have tor browser or brave, you could also try to connect to the service and see if resolves.

@huumn
Copy link
Member

huumn commented Nov 3, 2024

sndev stacker_lnd get_onion works and the container has been up for awhile.

@huumn
Copy link
Member

huumn commented Nov 3, 2024

I'll try deleting the containers and starting again. I might not have started the env from scratch.

@riccardobl
Copy link
Member Author

I've just tested it again, i can't reproduce, this is the commands i am using, just so that we are on the same page

riccardo@riccardo-ws:/PROJ/DEV/stacker.news$ bash sndev stop
riccardo@riccardo-ws:/PROJ/DEV/stacker.news$ bash sndev delete
riccardo@riccardo-ws:/PROJ/DEV/stacker.news$ bash sndev start
riccardo@riccardo-ws:/PROJ/DEV/stacker.news$ bash sndev login rblb
riccardo@riccardo-ws:/PROJ/DEV/stacker.news$ bash sndev stacker_lnd get_onion
riccardo@riccardo-ws:/PROJ/DEV/stacker.news$ bash sndev stacker_lndcli -n regtest bakemacaroon invoices:write 
riccardo@riccardo-ws:/PROJ/DEV/stacker.news$ bash sndev stacker_lnd get_cert

@huumn
Copy link
Member

huumn commented Nov 3, 2024

After deleting my containers attaching lnd works.

Copy link
Member

@huumn huumn left a comment

Choose a reason for hiding this comment

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

This breaks nwc_send for some reason (in addition to lnbits auto-configuration). nwc_recv still works however.

nwc_send  | 2024-11-03T22:19:37.091621419Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
nwc_send  | 2024-11-03T22:20:37.217989419Z thread 'main' panicked at src/main.rs:57:14:
nwc_send  | 2024-11-03T22:20:37.218000669Z Failed to get lnd info: Status { code: Unknown, message: "channel closed", source: Some(hyper::Error(ChannelClosed)) }
nwc_send  | 2024-11-03T22:20:37.218002419Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
nwc_send  | 2024-11-03T22:21:37.359979336Z thread 'main' panicked at src/main.rs:57:14:
nwc_send  | 2024-11-03T22:21:37.359994419Z Failed to get lnd info: Status { code: Unknown, message: "operation was canceled: connection closed", source: Some(hyper::Error(Canceled, "connection closed")) }
nwc_send  | 2024-11-03T22:21:37.359997086Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
nwc_send  | 2024-11-03T22:22:37.473181336Z thread 'main' panicked at src/main.rs:57:14:
nwc_send  | 2024-11-03T22:22:37.473201003Z Failed to get lnd info: Status { code: Unknown, message: "channel closed", source: Some(hyper::Error(ChannelClosed)) }
nwc_send  | 2024-11-03T22:22:37.473203378Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

We can live with lnbits requiring manual config but nwc_send is a must have. Ideally, our changes don't cause known regressions in general though.

Please see if you can:

  1. fix nwc_send
  2. document what's required to connect to lnbits now in wallets/lnbits/ATTACH.md
  3. fix sndev stacker_cln get_onion - it does not work currently
  4. document, if not fix, that cln's cert does not work with the onion

@riccardobl
Copy link
Member Author

riccardobl commented Nov 4, 2024

Oh, I didn't realize the configuration was held together by the certificate under ./docker/lnd, thanks for testing it out.
I've fixed it by making the docker containers use the stacker_lnd volume from where they can get the generated certificate instead

  • fix nwc_send
  • document what's required to connect to lnbits now in wallets/lnbits/ATTACH.md
    fixed, it just works now
  • fix sndev stacker_cln get_onion - it does not work currently
    was a typo in the doc, the command is actually sndev stacker_clncli get_onion
  • document, if not fix, that cln's cert does not work with the onion
    where does it fail? it attaches correctly for me

@riccardobl riccardobl requested a review from huumn November 4, 2024 09:36
Copy link
Member

@huumn huumn left a comment

Choose a reason for hiding this comment

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

This fixed the issues, thank you. Also, I was wrong about cln - it works but I expected it not to given we added another hostname to it.

The worker won't start as a result of the original changes (I just noticed while testing Tor autowithdraw):

worker  | 2024-11-04T18:10:44.486365170Z file:///app/lib/lnd.js:16
worker  | 2024-11-04T18:10:44.486389879Z   const lightningModulePath = require.resolve('lightning')
worker  | 2024-11-04T18:10:44.486391504Z                               ^
worker  | 2024-11-04T18:10:44.486392795Z 
worker  | 2024-11-04T18:10:44.486394170Z ReferenceError: require is not defined in ES module scope, you can use import instead
worker  | 2024-11-04T18:10:44.486395254Z This file is being treated as an ES module because it has a '.js' file extension and '/app/worker/package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.
worker  | 2024-11-04T18:10:44.486396670Z     at authenticatedLndGrpc (file:///app/lib/lnd.js:16:31)
worker  | 2024-11-04T18:10:44.486409629Z     at file:///app/api/lnd/index.js:6:27
worker  | 2024-11-04T18:10:44.486411545Z     at ModuleJob.run (node:internal/modules/esm/module_job:195:25)
worker  | 2024-11-04T18:10:44.486412337Z     at async ModuleLoader.import (node:internal/modules/esm/loader:337:24)
worker  | 2024-11-04T18:10:44.486413129Z     at async loadESM (node:internal/process/esm_loader:34:7)
worker  | 2024-11-04T18:10:44.486413920Z     at async handleMainPromise (node:internal/modules/run_main:106:12)
worker  | 2024-11-04T18:10:44.486414670Z 
worker  | 2024-11-04T18:10:44.486415379Z Node.js v18.20.4
worker  | 2024-11-04T18:10:44.499232295Z Failed running 'worker/index.js'

This is the result of us running worker out of the context of nextjs/webpack and using tsx to allow es6 imports. tbh we don't fully understand why things sometimes work and sometimes don't, but it can sometimes require getting creative like in lib/validate.js.

@riccardobl
Copy link
Member Author

should be fixed by the last commit

@riccardobl riccardobl requested a review from huumn November 4, 2024 19:10
@huumn
Copy link
Member

huumn commented Nov 5, 2024

That looked tricky, nice work!

@huumn huumn merged commit 3112fc3 into stackernews:master Nov 5, 2024
6 checks passed
huumn added a commit that referenced this pull request Nov 6, 2024
This reverts commit 3112fc3, reversing
changes made to 803daed.
@huumn
Copy link
Member

huumn commented Nov 6, 2024

I ended up having to revert this. See #1547

riccardobl added a commit to riccardobl/stacker.news that referenced this pull request Nov 6, 2024
riccardobl added a commit to riccardobl/stacker.news that referenced this pull request Nov 6, 2024
huumn added a commit that referenced this pull request Nov 7, 2024
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.

Fix LNBits auto-config funding source regression in #1521 add a tor container to sndev env
2 participants