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

generate a static tor onion address persistent unique to the node id #3155

Conversation

Saibato
Copy link
Contributor

@Saibato Saibato commented Oct 13, 2019

We now can generate a V3 tor service address from a blob

--addr=statictor:127.0.0.1:9051[/torblob=[blob]]

Or generate a unique static tor address bound to our node id /pub key

--addr=statictor:127.0.0.1:9051

TODO:

  • Documentation

  • Write some simple py-tests

EDIT: With this you are able to keep your public known or group known tor address static
and independent of torrc file or the tor instance ( if multiple ), in case you have to
move your node to an other PC or system/phone, as long as there is a tor service
your node will be reachable even if ip, NAT, firewall have changed by that static V3 address.

EDIT: related issues #2476 #3085

EDIT_2:

With this PR we can now also select the extern tor port we assign our local port to

--addr=statictor:127.0.0.1:9051[/torblob=[blob]][/torport=EXTERNPORT]

Or generate a unique static tor address bound to our node id /pub key

--addr=statictor:127.0.0.1:9051[/torport=EXTERNPORT]

@Saibato Saibato force-pushed the generate-a-static-tor-onion-for-node-id branch 2 times, most recently from 10e2779 to 80c6316 Compare October 13, 2019 16:43
@Saibato Saibato marked this pull request as ready for review October 13, 2019 19:13
@Saibato Saibato requested a review from cdecker as a code owner October 14, 2019 16:18
@cdecker cdecker self-assigned this Oct 14, 2019
@cdecker cdecker added this to the 0.7.4 milestone Oct 14, 2019
@Saibato Saibato force-pushed the generate-a-static-tor-onion-for-node-id branch from 20141dc to ea68f2d Compare October 14, 2019 19:50
@Saibato
Copy link
Contributor Author

Saibato commented Oct 14, 2019

@cdecker rebased and ready to rumble ... 👍

@Saibato Saibato force-pushed the generate-a-static-tor-onion-for-node-id branch from 3afbb39 to cf2643c Compare October 14, 2019 22:13
@Saibato Saibato changed the title generate a static tor onion address unique to the node id generate a static tor onion address persistent unique to the node id Oct 14, 2019
@Saibato Saibato force-pushed the generate-a-static-tor-onion-for-node-id branch 15 times, most recently from 9417bcf to 9233e15 Compare October 16, 2019 09:53
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

There is a lot of error-prone string parsing in the PR, I wonder if using
tal_strreg
might simplify the code and make it more flexible for future changes.

As it is now this resembles more a key-value bag than an address, so maybe we
should embrace that and actualy use : as the key-value separator in a stream
of pairs that use a different separator? Similar to the URL encoding commonly
used on the web, and making the relationship beween torport and 45321
clearer (I fell for that myself, not realizing that these are effectively
key-value pairs and not understanding the format 🙁). We are using /
elsewhere in the RPC as separator sometimes.

If we were to introduce backwards incompatible changes I'd advocate for a
format similar to this:

statictor=127.0.0.1:9151/torport=45321/torblob=11234567890123456789012345678901

The use of : as separator is really causing a bit of trouble since it is
commonly used as <address>:<port> separator, and for internal formatting of
IPv6 addresses (who came up with that design...🤬)

As a side note, I had to review this as a complete diff, since the incremental changes made it really hard to analyse the commits individually (I'd also be doing a squash merge on this since we can't guarantee bisectability).

We tend to normally have self-contained commits that are minimal in what they do, and then we fix them up using either fixup! commits (like I usually do) or by amending commits in-place (rusty uses guilt iirc). Both methods result in a clean and easy to follow commit history that is easier to review, and to audit later, whereas just adding incremental changes in cleanup commits added to the end of the PR make things hard. I'd strongly suggest using ones of those processes for future pull requests 😉

common/base64.c Outdated Show resolved Hide resolved
common/wireaddr.c Outdated Show resolved Hide resolved
common/wireaddr.h Outdated Show resolved Hide resolved
common/wireaddr.c Outdated Show resolved Hide resolved
common/wireaddr.c Outdated Show resolved Hide resolved
connectd/tor_autoservice.c Outdated Show resolved Hide resolved
connectd/tor_autoservice.c Outdated Show resolved Hide resolved
connectd/tor_autoservice.c Outdated Show resolved Hide resolved
connectd/tor_autoservice.c Outdated Show resolved Hide resolved
connectd/tor_autoservice.c Outdated Show resolved Hide resolved
@Saibato Saibato force-pushed the generate-a-static-tor-onion-for-node-id branch 4 times, most recently from 0d87016 to 478b541 Compare November 15, 2019 17:03
Copy link
Contributor Author

@Saibato Saibato left a comment

Choose a reason for hiding this comment

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

addressed the most/all points of your review. Thanks. 👍
Please review again

Its now rebased, bisectable and structured in 3 parts.

@Saibato Saibato force-pushed the generate-a-static-tor-onion-for-node-id branch 5 times, most recently from 1cb6281 to 5942998 Compare November 17, 2019 16:08
@Saibato Saibato force-pushed the generate-a-static-tor-onion-for-node-id branch 3 times, most recently from c0c4197 to c11566c Compare November 25, 2019 15:44
@Saibato
Copy link
Contributor Author

Saibato commented Nov 25, 2019

A bit of side show, but almost all PR not only this, fail the Travis lottery on first or second push with random errors unrelated to the changes.
@rustyrussell @cdecker #3286 solved all ? Travis failed https://travis-ci.org/ElementsProject/lightning/builds/616642255?utm_source=github_status&utm_medium=notification
I will now force push the same PR again and again 🤦‍♀️ until I win the Travis check lottery 🎲 🎲 🎲 take two won ...

@cdecker
Copy link
Member

cdecker commented Nov 25, 2019

A bit of side show, but almost all PR not only this, fail the Travis lottery on first or second push with random errors unrelated to the changes.
@rustyrussell @cdecker #3286 solved all ? Travis failed https://travis-ci.org/ElementsProject/lightning/builds/616642255?utm_source=github_status&utm_medium=notification
I will now force push the same PR again and again until I win the Travis check lottery take two won ...

I'm restarting failed tests and keeping track of which tests were flaky, so you don't need to push just to get Travis to like your PR 😉

Pushing the same PR just to retry Travis just makes tracking changes rather hard: http://46.101.246.115:8888/ElementsProject/lightning/pull/3155

@Saibato Saibato force-pushed the generate-a-static-tor-onion-for-node-id branch 3 times, most recently from d4d1563 to 2d23a62 Compare November 30, 2019 17:44
We  want to have a static Tor service created from a blob bound to
our node on cmdline

Changelog-added: persistent Tor address support
Changelog-added: allow the Tor inbound service port differ from 9735

Signed-off-by: Saibato <saibato.naga@pm.me>

Add base64 encode/decode to common

We need this to encode the blob for the tor service

Signed-off-by: Saibato <saibato.naga@pm.me>
Signed-off-by: Saibato <saibato.naga@pm.me>
Signed-off-by: Saibato <saibato.naga@pm.me>
@Saibato Saibato force-pushed the generate-a-static-tor-onion-for-node-id branch from 2d23a62 to 14882ec Compare November 30, 2019 18:05
@cdecker
Copy link
Member

cdecker commented Dec 3, 2019

Excellent changes @Saibato, sorry for the delay in reviewing it 👍

ACK 14882ec

@cdecker cdecker merged commit 2c16b41 into ElementsProject:master Dec 3, 2019
@jb55
Copy link
Collaborator

jb55 commented Dec 11, 2019

very cool, thanks @Saibato !

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.

4 participants