New more flexible address format code #440
Conversation
delbonis
added
enhancement
delbonis
removed
the
wip
|
Not that lnp2p doesn't use this code at all yet, we should have another PR that actually uses this address parser for figuring out connections. |
| * Ok so there's a lot of stuff going on here. Lit natively supports addresses | ||
| * that are just the pkh of the pubkey encoded in bech32. And that's pretty | ||
| * cool, you look up the IP+port in a tracker (or DHT) and connected with | ||
| * Noise_XX, then verify that the pubkey they provide you matches the pkh you |
There was a problem hiding this comment.
So how lit should work after #437 is that if we haven't connected to the peer before, we still go ahead with NOISE_XX and get the pubkey of the host. But if we are connecting to a host whose pubkey we know already, we use NOISE_XK, since we already know the remote node's pubkey and theres no requirement to ask again. This is similar to how we using ssh asks you if you want to remember and I guess an implementing UI could ask the user for his preference as well
lncore/addr.go
Outdated
| } | ||
|
|
||
| // LnDefaultPort is from BOLT1. | ||
| const LnDefaultPort = 9735 |
There was a problem hiding this comment.
we could rename this to BOLTDefaultPort or just plain DefaultPort since Ln is implied :)
There was a problem hiding this comment.
RPC though? Although that is in a different package so maybe it doesn't matter.
lncore/addr.go
Outdated
| const LnDefaultPort = 9735 | ||
|
|
||
| // PkhBech32Prefix if for addresses lit already uses. | ||
| const PkhBech32Prefix = "ln" |
There was a problem hiding this comment.
Another small issue, but this is just lit's bech32 prefix and it would be better named that way
There was a problem hiding this comment.
Fixed, along with the port const naming.
| const PkhBech32Prefix = "ln" | ||
|
|
||
| // PubkeyBech32Prefix is for encoding the full pubkey in bech32. | ||
| const PubkeyBech32Prefix = "lnpk" |
There was a problem hiding this comment.
Why would we need this though, can't we parse with just the length of the resultant bech32 string?
There was a problem hiding this comment.
Yeah that's true, I suppose I wanted to make the difference more obvious. Especially since technically ln1dfasdfasdf:lnpk1asdfsadfasdf is a valid address, it'd look a little weird to me if it was ln1:ln1, and it'd be less obvious for the average joe to know how to pick out the "short" version of their address, which we want to make the pkh.
There was a problem hiding this comment.
Sounds good thanks! Although I wonder why people would want to use bech32 pks (since lit right now supports only bech32 pkh and BOLT supports hex pks). Would be nice if some implementations decide to change though, so having this does make sense.
There was a problem hiding this comment.
I mean, it doesn't really matter. They get a little bit of error correction (maybe), but since most of the time addresses are going to be copy/pasted I don't think it matters. I added it to quell arguments about "well hex might be bad but bech32 is better" since this is demonstrably not that much shorter and not worth the extra effort.
| const AddrFmtFull = "full" | ||
|
|
||
| // AddrFmtFullBech32Pk is <bech32 pkh>:<bech32 pk>@<ip>:<port> | ||
| const AddrFmtFullBech32Pk = "full_bech32_pk" |
There was a problem hiding this comment.
// AddrFmtFullBech32Pk is <bech32 pkh>:<bech32 pk>@<ip>:<port>
in the above, if we have the bech32 pk, we could easily derive the bech32 kph (LitAddr). Maybe we could just have the bech32 pk in full and then use the relevant methods where we need the pkh?
There was a problem hiding this comment.
Oh yes you can definitely derive the pkh from the pk. I hope nobody uses this format except when making fully-qualified addresses for whatever reason. But in practical use I don't really see why anyone would use it. I left it (as with the variant with the hex pk) just as "one option".
| const AddrFmtLit = "lit" | ||
|
|
||
| // AddrFmtPubkey is <hex pubkey> | ||
| const AddrFmtPubkey = "hex_pk" |
There was a problem hiding this comment.
Again, unless we're using this in some internal parsing, the hex_pk on its own is not that useful (since there's no tracker that lnd uses). How do you envision using this?
There was a problem hiding this comment.
In c-lightning you can connect to nodes if you just have the pubkey, I'm not sure how it looks up the IP address but it is a valid "thing" you can connect with.
| const AddrFmtPubkey = "hex_pk" | ||
|
|
||
| // AddrFmtBech32Pubkey is <bech32 pk> | ||
| const AddrFmtBech32Pubkey = "bech32_pk" |
There was a problem hiding this comment.
Somewhat similar to the comment above in that the bech32 pk can be converted to a bech32 pkh and then used instead of parsing pks and pkh's separately. I assume that requiring the full bech32 pk would not be needed at all (except for internal parsing, where we could use defined methods / functions)
There was a problem hiding this comment.
No it's not really necessary but I thought I'd include it "just because".
| } | ||
| } | ||
|
|
||
| // TODO more |
There was a problem hiding this comment.
I think these formats are enough actually, we just need two formats (maybe three, if you count just the pkh as one format).
- Lit Addr (pkh) with Port
- BOLT Addr (hex pubkey) with Port
(3). Lit Addr (pkh)
Again, dependent on how we would use this since displaying addresses requires only the two formats above unless I'mm missing something here.
There was a problem hiding this comment.
Yeah people would probably only end up using those 3 formats often, but since people do weird things I thought it makes sense to include all possible variations? Especially since it's not hard for a person to manually convert most formats to simpler versions.
There was a problem hiding this comment.
Yeah, I guess with your overall rationale in the other comments, I can see why you would want to include other formats here.
| +8 g f 2 t v d w 0 | ||
| +16 s 3 j n 5 4 k h | ||
| +24 c e 6 m u a 7 l | ||
| */ |
There was a problem hiding this comment.
I guess we could omit this? (Or add this as a comment below the bech32 regex).
There was a problem hiding this comment.
Yeah that was to help explain the regex.
Adds partial support for decoding both Lit-style and BOLT-style LN addresses.
See
lncore/addr.go.