Generate local alias before spawning channel #2316

pm47 · 2022-06-15T13:51:03Z

As a first step before addressing #2224 (comment), we need to make the alias generation asynchronous.

Peer generates the alias along with the shutdown script, and before spawning the channel.

This approach has several drawbacks:

changes all data classes in the early channel states
forces us to have a local alias in state WAIT_FOR_FUNDING_CONFIRMED, which is persisted and requires a migration. We can't set it to the real scid because we don't have it yet
not consistent with a future (hypothetical) multi-alias support

eclair-core/src/main/scala/fr/acinq/eclair/wire/internal/channel/version3/ChannelCodecs3.scala

eclair-core/src/main/scala/fr/acinq/eclair/wire/internal/channel/MigrationMethods.scala

t-bast · 2022-06-16T11:49:59Z

I have an alternative design for alias generation: what about inserting a new temporary channel state to generate it? Whenever we currently call acceptFundingTx, we would go to a WAIT_FOR_LOCAL_ALIAS state where we store the next state and next state data, generate the local alias (probably by sending a message to the Router), and once we receive the alias we go to the next state with the next state data. In that state we store incoming lightning messages and replay them once we have our local alias and transition to the next state. It's very simple for transient states, the only case where it may be a bit painful is the unrequested 0-conf where we are in WAIT_FOR_FUNDING_CONFIRMED (which isn't transient), this would need some prototyping...

t-bast · 2022-06-16T13:07:02Z

Something else that comes to mind: for public channels that aren't using 0-conf (which is going to be the most common flow for most node operators), we probably shouldn't really generate a localAlias, we can probably use 0? I'm not sure if we can use that to simplify the flow or not...

pm47 · 2022-06-16T16:07:31Z

I have an alternative design for alias generation: what about inserting a new temporary channel state to generate it?

I thought about that, but I'd don't like adding new states because we then need to handle all the other events (remote errors, funding spent, disconnection, etc.) and it quickly becomes bulky.

Something else that comes to mind: for public channels that aren't using 0-conf (which is going to be the most common flow for most node operators), we probably shouldn't really generate a localAlias, we can probably use 0?

That's a good point, but I think we don't want to have duplicate aliases, it should be an Option in that case. It could complicate things though, let me think about it...

t-bast · 2022-06-17T07:02:13Z

Yes, it's true, after thinking about it more I think we should keep the localAlias non-optional, it really makes the code much cleaner. In that case it makes sense to generate it right at the beginning of the channel opening flow. It can be in the Peer for now, I think that once #2247 is integrated which refactor the channel init state, I may move that in the channel's init state instead, we'll see.

eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala

eclair-core/src/main/scala/fr/acinq/eclair/wire/internal/channel/Mutators.scala

t-bast · 2022-06-17T08:44:00Z

eclair-core/src/main/scala/fr/acinq/eclair/wire/internal/channel/Mutators.scala

+ *   .as[MyClass]
+ * }}}
+ */
+object Mutators {


Should we put that in a versioned namespace (this one would go in the version3 package since we introduced the need for it in version3 and won't need it for future versions)?

Maybe, although we sort of adopted the opposite approach for ChannelTypes0.

On the contrary, we put ChannelTypes0 in the version1 package? But it doesn't matter right now, we can move those mutators to a versioned package when we introduce new ones that are for a different codecs version.

eclair-core/src/test/scala/fr/acinq/eclair/channel/FuzzySpec.scala

Using the first 7B increases the risk of collision at migration due to how `channel_id` is built. Collisions would happen when multiple channels are funded with the same transaction.

pm47 · 2022-06-17T12:05:21Z

eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala

+    val localAlias_p = Promise[Alias]()
+    context.system.eventStream.publish(Register.GenerateLocalAlias(localAlias_p))


This isn't very academic, but:

using the eventStream saves us from having to pass a reference to register all the way to the peer

using a Promise saves us from creating an ad-hoc actor (pipeTo doesn't work with the eventStream approach)

t-bast

Looks good, only a few comments (and a rebase is needed).

t-bast · 2022-06-17T14:59:44Z

eclair-core/src/main/scala/fr/acinq/eclair/ShortChannelId.scala

+    // at block height 1000 LN didn't exist, so all real scids less than that will never be used
+    val upperBound = RealShortChannelId.apply(BlockHeight(1000),1,0).toLong


Why do we use such a small range? We can go up to at least block 350 000, can't we?

The space is already huge : 1_099_511_627_841_536 (*) and we are checking for duplicates. I figure leaving some values available could prove handy later?

(*) So huge in fact that I believe our previous computation was invalid. If we indeed go up to block height 350 000 it gives us a ceiling of 384_829_069_721_665_536 which is about 2^59. The probability of a collision for 250 000 channels would be 0.000011 %, not 0.8 % ?

t-bast · 2022-06-17T15:00:34Z

eclair-core/src/main/scala/fr/acinq/eclair/ShortChannelId.scala

+  def generateLocalAlias(): Alias = {
+    // at block height 1000 LN didn't exist, so all real scids less than that will never be used
+    val upperBound = RealShortChannelId.apply(BlockHeight(1000),1,0).toLong
+    Alias(Random.nextLong(upperBound))


We should use the randomLong from package.scala here.

t-bast · 2022-06-17T15:02:12Z

eclair-core/src/main/scala/fr/acinq/eclair/wire/internal/channel/Mutators.scala

+ *   .as[MyClass]
+ * }}}
+ */
+object Mutators {


On the contrary, we put ChannelTypes0 in the version1 package? But it doesn't matter right now, we can move those mutators to a versioned package when we introduce new ones that are for a different codecs version.

t-bast · 2022-06-17T15:15:54Z

eclair-core/src/main/scala/fr/acinq/eclair/channel/Register.scala

@@ -79,6 +82,13 @@ class Register extends Actor with ActorLogging {
        case Some(channel) => channel.tell(msg, compatReplyTo)
        case None => compatReplyTo ! ForwardShortIdFailure(fwd)
      }
+
+    case GenerateLocalAlias(promise) =>


Since we re-populate the shortIds map asynchronously as channels are restored and send ShortChannelIdAssigned, we may receive that message before we've restored all channels and recovered a complete shortIds map.

I believe it won't be an issue in practice as long as the range we use for ShortChannelId.generateLocalAlias() is big enough to make collisions unlikely.

t-bast · 2022-06-17T15:23:25Z

eclair-core/src/test/scala/fr/acinq/eclair/io/PeerSpec.scala

+    system.eventStream.subscribe(generateLocalAliasListener.ref, classOf[GenerateLocalAlias])
+    waitEventStreamSynced(system.eventStream)
+    generateLocalAliasListener.setAutoPilot {
+      case (_, gen:GenerateLocalAlias)  =>


nit:

Suggested change

case (_, gen:GenerateLocalAlias) =>

case (_, gen: GenerateLocalAlias) =>

pm47 · 2022-06-29T10:00:12Z

Superseded by #2337.

pm47 commented Jun 15, 2022

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/wire/internal/channel/version3/ChannelCodecs3.scala Outdated Show resolved Hide resolved

eclair-core/src/main/scala/fr/acinq/eclair/wire/internal/channel/MigrationMethods.scala Outdated Show resolved Hide resolved

pm47 requested a review from t-bast June 15, 2022 13:54

pm47 force-pushed the local-alias-gen branch 2 times, most recently from 6fc252f to 3becfb8 Compare June 16, 2022 11:16

generate local alias before spawning channel

5f3fa19

pm47 force-pushed the local-alias-gen branch from 3becfb8 to 5f3fa19 Compare June 16, 2022 11:40

pm47 marked this pull request as ready for review June 17, 2022 07:47

t-bast reviewed Jun 17, 2022

View reviewed changes

pm47 added 3 commits June 17, 2022 11:03

use last 7B of channel_id for alias

d900265

Using the first 7B increases the risk of collision at migration due to how `channel_id` is built. Collisions would happen when multiple channels are funded with the same transaction.

cleanup

4ee1025

random local alias with collision protection

60f581c

pm47 commented Jun 17, 2022

View reviewed changes

t-bast reviewed Jun 17, 2022

View reviewed changes

pm47 mentioned this pull request Jun 28, 2022

Random values for generateLocalAlias() #2337

Merged

pm47 closed this Jun 29, 2022

t-bast deleted the local-alias-gen branch August 16, 2022 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate local alias before spawning channel #2316

Generate local alias before spawning channel #2316

pm47 commented Jun 15, 2022

t-bast commented Jun 16, 2022

t-bast commented Jun 16, 2022 •

edited

Loading

pm47 commented Jun 16, 2022

t-bast commented Jun 17, 2022

t-bast Jun 17, 2022

pm47 Jun 17, 2022

t-bast Jun 17, 2022

pm47 Jun 17, 2022

t-bast left a comment •

edited

Loading

t-bast Jun 17, 2022

pm47 Jun 28, 2022

t-bast Jun 17, 2022

t-bast Jun 17, 2022

t-bast Jun 17, 2022

t-bast Jun 17, 2022

pm47 commented Jun 29, 2022

		val localAlias_p = Promise[Alias]()
		context.system.eventStream.publish(Register.GenerateLocalAlias(localAlias_p))

		// at block height 1000 LN didn't exist, so all real scids less than that will never be used
		val upperBound = RealShortChannelId.apply(BlockHeight(1000),1,0).toLong

	case (_, gen:GenerateLocalAlias) =>
	case (_, gen: GenerateLocalAlias) =>

Generate local alias before spawning channel #2316

Generate local alias before spawning channel #2316

Conversation

pm47 commented Jun 15, 2022

t-bast commented Jun 16, 2022

t-bast commented Jun 16, 2022 • edited Loading

pm47 commented Jun 16, 2022

t-bast commented Jun 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-bast left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pm47 commented Jun 29, 2022

t-bast commented Jun 16, 2022 •

edited

Loading

t-bast left a comment •

edited

Loading