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

Index unannounced channels on channelId instead of shortChannelId in the router #2269

Merged
merged 2 commits into from
May 19, 2022

Conversation

pm47
Copy link
Member

@pm47 pm47 commented May 13, 2022

Builds on #2270.

This is the final preparatory step before we can introduce channel aliases, which are arbitrary values of type ShortChannelId. We had to move to a stable identifier for private channels. Public channels can keep using the shortChannelId, it is stable because it is a "real scid" (and only used after 6 confs) and is useful as an index for channel queries.

@pm47 pm47 changed the title Index unannounced channels on channelId instead of shortChannelId in Router Index unannounced channels on channelId instead of shortChannelId in the router May 13, 2022
@pm47 pm47 force-pushed the router-local-channel-index branch from b016368 to 1b431c3 Compare May 13, 2022 16:54
@pm47 pm47 marked this pull request as draft May 13, 2022 17:06
@pm47 pm47 force-pushed the router-local-channel-index branch from 1b431c3 to e02752a Compare May 18, 2022 09:50
@pm47 pm47 requested a review from t-bast May 18, 2022 10:52
@pm47 pm47 marked this pull request as ready for review May 18, 2022 10:52
Comment on lines 637 to 631
resolveScid.get(scid).flatMap(privateChannels.get) match {
case Some(privateChannel) => Some(privateChannel)
case None => None
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: you don't need the extra patter-matching:

resolveScid.get(scid).flatMap(privateChannels.get)

But if you want to keep it for to make the code clearer, that works for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was for symmetry with the line above : Some(publicChannel)/Some(privateChannel). I'm not really happy with the resolve method.

@@ -614,11 +621,26 @@ object Router {
stash: Stash,
rebroadcast: Rebroadcast,
awaiting: Map[ChannelAnnouncement, Seq[GossipOrigin]], // note: this is a seq because we want to preserve order: first actor is the one who we need to send a tcp-ack when validation is done
privateChannels: Map[ShortChannelId, PrivateChannel],
privateChannels: Map[ByteVector32, PrivateChannel], // indexed by channel id
resolveScid: Map[ShortChannelId, ByteVector32], // scid to channel_id
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we make it more explicit that this map is only used for private channels (currently) and unconfirmed channels (in the future)? There's no reason to store the mapping for public, announced channels, it's already in channels, right?

Copy link
Member Author

@pm47 pm47 May 19, 2022

Choose a reason for hiding this comment

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

Done with 1c0f453, also renamed the field. Added the comment, but it's weak, I wish we could enforce that somehow. Well we could by making fields private, but then updating the case class would be horrible. 🤷‍♂️

@pm47 pm47 force-pushed the router-local-channel-index branch from e02752a to 1c0f453 Compare May 19, 2022 09:23
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@pm47 pm47 merged commit d98da6f into master May 19, 2022
@pm47 pm47 deleted the router-local-channel-index branch May 19, 2022 11:31
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.

2 participants