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

Adopt TORv2 addresses #29

Closed
dr-orlovsky opened this issue Apr 9, 2020 · 12 comments · Fixed by #54
Closed

Adopt TORv2 addresses #29

dr-orlovsky opened this issue Apr 9, 2020 · 12 comments · Fixed by #54
Assignees
Labels
good first issue Good for newcomers
Milestone

Comments

@dr-orlovsky
Copy link
Member

dr-orlovsky commented Apr 9, 2020

@afilini was right #24 (comment) that we need to add support of TORv2 addresses additionally to TORv3: at least they are a part of Lightning Network gossip protocol https://github.com/lightningnetwork/lightning-rfc/blob/master/07-routing-gossip.md#the-node_announcement-message and we have to support them in LNP part of the library.

Basically, we need to duplicate all TORv3-related code and adjust it to TORv2-specific types in here: https://github.com/LNP-BP/rust-lnpbp/blob/master/src/common/internet.rs. Also there is a need to add feature requirement to Cargo.toml torut crate as pointed out here: #24 (comment)

@dr-orlovsky dr-orlovsky added the good first issue Good for newcomers label Apr 9, 2020
@Kixunil
Copy link
Member

Kixunil commented Apr 11, 2020

Would it be possible to disregard the distinction between addresses and just defer to torut::OnionAddress/torut::TorPublicKey?

@dr-orlovsky
Copy link
Member Author

Hard to say... We do distinguish IPv4 from IPv6 in the enum, so at least at that level it would be nice to distinguish TORv2 from TORv3.

Don't spend time on this issue, I left it for new contributors b/c it's very simple to implement as a first issue

@Kixunil
Copy link
Member

Kixunil commented Apr 12, 2020

Sure, was just randomly looking around and noticed it because I had to look into torut to implement parse_arg properly. :)

@rajarshimaitra
Copy link
Contributor

rajarshimaitra commented Jul 21, 2020

If this issue is still open, I am willing to work on it.

Small note after initial look, torut doesn't seem to support TorPubKeyV2 to OnionAddressV2 conversion. As we are only storing the publickeys in the structure do we plan to implement the conversion too? Or there are other ways of doing it?

@dr-orlovsky
Copy link
Member Author

It is still open, you are welcome to pick it!

As for V2, I think we only need to support OnionAddressV2, w/o support for TorPubKeyV2

@rajarshimaitra
Copy link
Contributor

Ok, I went some distance and so far it seems workable.

Need some technical help. OnionAddressV2 from torut doesn't implement Copy. This is causing error in #[derive(Copy)] for InetAddr which wraps over OnionAddressV2. I cant manually implement Copy as OnionAddressV2 is from an external crate.
One option is to disable Copy derivation on InetAddr but that will cause ripple effects upstream.

So what's the best way to fix this situation?

@dr-orlovsky
Copy link
Member Author

Good news! Lets try to remove Copy on InetAddr and see how much problems we will get after

@Kixunil
Copy link
Member

Kixunil commented Jul 23, 2020

@rajarshimaitra What about a PR against torut for the address to implement Copy?

@dr-orlovsky
Copy link
Member Author

We can do that for sure, but I expect there are some limitations b/c of the key structure such that Copy is unreasonable?

@rajarshimaitra
Copy link
Contributor

rajarshimaitra commented Jul 23, 2020

@rajarshimaitra What about a PR against torut for the address to implement Copy?

@Kixunil I thought of the same at first. But the OnionAddressV3 also doesn't implement copy. So there is probably something with it as @dr-orlovsky suggested.

Good news! Lets try to remove Copy on InetAddr and see how much problems we will get after

I gave a try at removing Copy derivations. The changes ripples through few more modules of the library, but nothing major. Not sure about the rgb node though. There might be more dependency on it.

@rajarshimaitra
Copy link
Contributor

Opened the PR. @Kixunil would appreciate your review. So far it can only parse onion v2 from an address string.

@dr-orlovsky dr-orlovsky linked a pull request Jul 30, 2020 that will close this issue
@Kixunil
Copy link
Member

Kixunil commented Jul 31, 2020

such that Copy is unreasonable?

I wonder why though, keys should be representable as a fixed-length array of bytes, which is always Copy...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants