LNP-BP / rust-lnpbp

Library implementing LNPBP standards related to client-side-validation paradigm
https://lnp-bp.org
MIT License
79 stars 38 forks source link

Adopt TORv2 addresses #29

Closed dr-orlovsky closed 4 years ago

dr-orlovsky commented 4 years ago

@afilini was right https://github.com/LNP-BP/rust-lnpbp/pull/24#discussion_r403920267 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: https://github.com/LNP-BP/rust-lnpbp/pull/24#discussion_r403921325

Kixunil commented 4 years ago

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

dr-orlovsky commented 4 years ago

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 commented 4 years ago

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

rajarshimaitra commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

Kixunil commented 4 years ago

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

dr-orlovsky commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

Kixunil commented 4 years ago

such that Copy is unreasonable?

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