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

Clarify temporary_channel_id's acceptable usages #496

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@TheBlueMatt
Contributor

TheBlueMatt commented Oct 29, 2018

Because apparently we're dead-set on making sure that the obvious way to implement the spec is incorrect (and will lead to bugs downstream of implementors, see #475), we need to be extra-doubly-sure that we spell this out. Note that based on their wiki at least @ACINQ (@pm47 @sstone) have a bug here, and the lnd/c-lightning docs need to clarify this as they appear to indicate to users they can do this.

@sstone

This comment has been minimized.

Collaborator

sstone commented Oct 29, 2018

We use random temporary channel ids and route incoming messages first by remote node id and then by (temporary) channel id) but yes our API just takes a channel id id so there may be a problem identifying incoming channels before the funding tx has been created, I guess this is what you're referring to ?

@TheBlueMatt

This comment has been minimized.

Contributor

TheBlueMatt commented Oct 29, 2018

Indeed, the text here points out that this is likely to be a bug not just in differently-structure lightning implementations but in higher-level client users based on likely APIs.

@rustyrussell

This comment has been minimized.

Collaborator

rustyrussell commented Oct 30, 2018

@TheBlueMatt be nice please.

While I never liked the temporary ids at all (they are only required because we mux channels, and people wanted more than one simultaneous open), they're dangerous if exposed outside the three messages they're used in. It's definitely useful to document this!

But your previous proposal was inadequate to make them safe. It required (without explicitly saying so) that it had to be not only unique, but secret. You implied that you didn't key them by peer internally, either: this is terrible code hygiene. Worse, I can send you a channel_open with a temporary_channel_id equal to a channel I know you already have established. If you isolate peers properly you don't even have to consider these cases.

If we want the temporary_channel_id to have these properties it must be a mutual construction between peers, AFAICT.

@TheBlueMatt

This comment has been minimized.

Contributor

TheBlueMatt commented Oct 31, 2018

You implied that you didn't key them by peer internally, either: this is terrible code hygiene.

Keying by peer is different from ensuring peer_ids match. As you point out the current spec allows multiple channels per peer, so "peers" don't make much sense as the "root object" in lightning anymore either. Having channels as the "root object" is also kinda required if you are in a world where you may have different types of message delivery mechanisms - ie you have to have a root that can generate messages for different peer reference mechanisms instead of having the peer objects own their own channels. Different goals lead to different designs, not necessarily in a bad way.

It required (without explicitly saying so) that it had to be not only unique, but secret. Worse, I can send you a channel_open with a temporary_channel_id equal to a channel I know you already have established.

Not necessarily - it only indicated that such duplicates can fail. If they are random (or at least not duplicative) you are protected from your own opens failing, but someone else could try (and fail) to open something that has the same temporary_channel_id if they learn about some other channels' temporary_channel_id later.

In any case, I updated the text to point out further API concerns with usage of channel_id as a public API-exposed bit - it also requires careful work to make sure pre-funding_locked channel_id exposure is safe or you may have users referencing channel_ids in their own local state and getting duplicate opens confusing them.

@TheBlueMatt TheBlueMatt changed the title from Clarify temporary_channel_id's acceptable usages (bugs in the wild) to Clarify temporary_channel_id's acceptable usages Nov 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment