Separate internal channel config from features by t-bast · Pull Request #1852 · ACINQ/eclair

t-bast · 2021-06-29T17:23:13Z

Our current ChannelVersion field mixes two unrelated concepts: channel features (as defined in Bolt 9) and channel internals (such as custom key derivation). It is more future-proof to separate these two unrelated concepts and will make it easier to implement channel types (see lightning/bolts#880).

The second commit adds an optional shutdown script to the channel's remote params, which will let us support upfront_shutdown_script later.

NB: this is PR to the only-store-remote-sigs branch, to ensure we squash all channel codec changes into a single commit on master.

sstone · 2021-06-30T14:04:36Z

Did you consider modifying ChannelVersion to include both BOLT-specific and eclair-specific fields (i.e ChannelType and ChannelConfigOption) ? It seems that they are related (as they have to be consistent and will be created together) and we often need both fields at the same time ?

t-bast · 2021-06-30T14:46:26Z

Did you consider modifying ChannelVersion to include both BOLT-specific and eclair-specific fields (i.e ChannelType and ChannelConfigOption) ? It seems that they are related (as they have to be consistent and will be created together) and we often need both fields at the same time ?

Yes I did, but I then decided to separate them. In most cases they are really unrelated and you only need one of them.

The only benefit to keep them encapsulated in a ChannelVersion would be to make less changes, but I believe we have to do these changes at some point, so I'd rather do them now (these changes are quite trivial). And it's really confusing that this was named ChannelVersion, and will be even more confusing once channel types are an official term in the spec.

sstone · 2021-06-30T16:03:39Z

Ok. One of my concerns was that to implement custom extensions we would need both fields, and that it may even be true for parameter validation, but overall this PR LGTM.

t-bast · 2021-07-01T07:20:48Z

IMO most custom extensions behave like channel features, so they should use only the channel type part, not the channel internal config.

sstone

I've rebased #1846 on top of this (see https://github.com/ACINQ/eclair/tree/implement-upfront-shutdown-script-channel-version-migration) and everything LGTM.

pm47

The clash between ChannelType and ChannelTypes is unfortunate. I would rename (maybe in a separate PR):

State -> ChannelState
Data -> ChannelData
ChannelTypes.scala -> ChannelData.scala

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

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

eclair-node/src/main/scala/fr/acinq/eclair/api/serde/JsonSerializers.scala

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

t-bast · 2021-07-02T12:23:28Z

The clash between ChannelType and ChannelTypes is unfortunate. I would rename (maybe in a separate PR)

I agree, it's probably better to do it in a separate PR that only does that and no logic change.

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

Our current ChannelVersion field mixes two unrelated concepts: channel features (as defined in Bolt 9) and channel internals (such as custom key derivation). It is more future-proof to separate these two unrelated concepts and will make it easier to implement channel types (see lightning/bolts#880).

This will be necessary when we implement `upfront_shutdown_script`. This field can be set to `None` for all current channels as we don't support the feature yet.

pm47 · 2021-07-06T08:43:13Z

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

+ * Internal configuration option impacting the channel's structure or behavior.
+ * This must be set when creating the channel and cannot be changed afterwards.
+ */
+trait ChannelConfigOption {


Nit:

Suggested change

trait ChannelConfigOption {

trait ChannelConfig {

or

Suggested change

trait ChannelConfigOption {

trait ChannelOptions {

Why? It's really a configuration option...it's not a complete configuration so ChannelConfig feels weird IMO, and ChannelOption is on the contrary a bit too vague, isn't it?

It's just that everywhere we are using: channelConfig: ChannelConfigOptions

But it's different, it's because it's a ChannelConfigOptions, which is a collection of ChannelConfigOption.
I agree this one could be renamed ChannelConfigOptions -> ChannelConfig, but not the trait on individual options.

Done in 4e0c814

pm47 · 2021-07-06T08:43:58Z

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

+
+object ChannelConfigOptions {
+
+  def standard: ChannelConfigOptions = ChannelConfigOptions(activated = Set(FundingPubKeyBasedChannelKeyPath))


Why not a val here?

Suggested change

def standard: ChannelConfigOptions = ChannelConfigOptions(activated = Set(FundingPubKeyBasedChannelKeyPath))

val standard: ChannelConfigOptions = ChannelConfigOptions(activated = Set(FundingPubKeyBasedChannelKeyPath))

No reason, this should be a val indeed.

Done in 4e0c814

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

pm47 · 2021-07-06T09:07:05Z

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

+          (("remoteParams" | remoteParamsCodec) ::
            ("channelFlags" | byte) ::
-              ("localCommit" | localCommitCodec(remoteParams.fundingPubKey)) ::
-              ("remoteCommit" | remoteCommitCodec) ::
-              ("localChanges" | localChangesCodec) ::
-              ("remoteChanges" | remoteChangesCodec) ::
-              ("localNextHtlcId" | uint64overflow) ::
-              ("remoteNextHtlcId" | uint64overflow) ::
-              ("originChannels" | originsMapCodec) ::
-              ("remoteNextCommitInfo" | either(bool, waitingForRevocationCodec, publicKey)) ::
-              ("commitInput" | inputInfoCodec) ::
-              ("remotePerCommitmentSecrets" | ShaChain.shaChainCodec) ::
-              ("channelId" | bytes32)
-          })
-      }).as[Commitments].decodeOnly
+            ("localCommit" | localCommitCodec) ::
+            ("remoteCommit" | remoteCommitCodec) ::
+            ("localChanges" | localChangesCodec) ::
+            ("remoteChanges" | remoteChangesCodec) ::
+            ("localNextHtlcId" | uint64overflow) ::
+            ("remoteNextHtlcId" | uint64overflow) ::
+            ("originChannels" | originsMapCodec) ::
+            ("remoteNextCommitInfo" | either(bool, waitingForRevocationCodec, publicKey)) ::
+            ("commitInput" | inputInfoCodec) ::
+            ("remotePerCommitmentSecrets" | ShaChain.shaChainCodec) ::
+            ("channelId" | bytes32))
+      }).as[ChannelTypes0.Commitments].decodeOnly.map[Commitments](_.migrate()).decodeOnly


The outermost parentheses can be removed (same in other file).

I noticed that when comparing both PRs to master

Done in 4e0c814

pm47 · 2021-07-06T09:30:06Z

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

+    val channelConfigCodec: Codec[ChannelConfigOptions] = lengthDelimited(bytes).xmap(b => {
+      val activated: Set[ChannelConfigOption] = b.bits.toIndexedSeq.reverse.zipWithIndex.collect {
+        case (true, 0) => ChannelConfigOptions.FundingPubKeyBasedChannelKeyPath
+      }.toSet
+      ChannelConfigOptions(activated)
+    }, cfg => {
+      val indices = cfg.activated.map(_.supportBit)
+      if (indices.isEmpty) {
+        ByteVector.empty
+      } else {
+        // NB: when converting from BitVector to ByteVector, scodec pads right instead of left, so we make sure we pad to bytes *before* setting bits.
+        var buffer = BitVector.fill(indices.max + 1)(high = false).bytes.bits
+        indices.foreach(i => buffer = buffer.set(i))
+        buffer.reverse.bytes
+      }
+    })


This codec is a bit 🤢, but unfortunately it seems scodec doesn't work well with bitfields.

Since we use bitfields at several places, it's probably worth it to he define a generic byte-aligned, right-to-left, bitfield codec? I'll give it a try.

Agreed, it's useful so that we don't unintentionally go back to storing something that isn't byte-aligned!

pm47 · 2021-07-06T12:27:09Z

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

+      val indices = cfg.options.map(_.supportBit)
+      if (indices.isEmpty) {
+        ByteVector.empty
+      } else {
+        // NB: when converting from BitVector to ByteVector, scodec pads right instead of left, so we make sure we pad to bytes *before* setting bits.
+        var buffer = BitVector.fill(indices.max + 1)(high = false).bytes.bits
+        indices.foreach(i => buffer = buffer.set(i))
+        buffer.reverse.bytes
+      }


Suggested change

val indices = cfg.options.map(_.supportBit)

if (indices.isEmpty) {

ByteVector.empty

} else {

// NB: when converting from BitVector to ByteVector, scodec pads right instead of left, so we make sure we pad to bytes *before* setting bits.

var buffer = BitVector.fill(indices.max + 1)(high = false).bytes.bits

indices.foreach(i => buffer = buffer.set(i))

buffer.reverse.bytes

}

val indices = cfg.activated.map(_.supportBit)

val maxLength = indices.maxOption.getOrElse(-1) + 1

val buffer = indices.foldLeft(BitVector.low(maxLength)) {

case (bitfield, i) => bitfield.set(i)

}

buffer.bytes.bits.reverse.bytes

}

pm47 · 2021-07-06T14:07:17Z

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

                              htlcBasepoint: PublicKey,
-                              features: Features)
+                              features: Features,
+                              shutdownScript: Option[ByteVector])


The naming is inconsistent LocalParams.defaultFinalScriptPubKey and RemoteParams.shutdownScript

Either:

LocalParams.defaultFinalScriptPubKey and RemoteParams.upfrontFinalScriptPubKey
or

LocalParams.defaultShutdownScript and RemoteParams.upfrontShutdownScript

I would keep the upfront in any case.

Let's revisit the naming afterwards.

* Separate internal channel config from features Our current ChannelVersion field mixes two unrelated concepts: channel features (as defined in Bolt 9) and channel internals (such as custom key derivation). It is more future-proof to separate these two unrelated concepts and will make it easier to implement channel types (see lightning/bolts#880). * Add shutdown script to channel remote params This will be necessary when we implement `upfront_shutdown_script`. This field can be set to `None` for all current channels as we don't support the feature yet.

t-bast requested review from pm47 and sstone June 29, 2021 17:23

sstone approved these changes Jul 1, 2021

View reviewed changes

pm47 reviewed Jul 2, 2021

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelConfigOptions.scala Outdated Show resolved Hide resolved

t-bast force-pushed the channel-version-migration branch 2 times, most recently from 0adaeaa to 25a983e Compare July 5, 2021 16:26

t-bast added 2 commits July 6, 2021 08:52

Add shutdown script to channel remote params

0b520dd

This will be necessary when we implement `upfront_shutdown_script`. This field can be set to `None` for all current channels as we don't support the feature yet.

t-bast force-pushed the channel-version-migration branch from 25a983e to 0b520dd Compare July 6, 2021 06:52

pm47 reviewed Jul 6, 2021

View reviewed changes

Fix PR comments

4e0c814

pm47 reviewed Jul 6, 2021

View reviewed changes

pm47 approved these changes Jul 6, 2021

View reviewed changes

t-bast merged commit e88e6c5 into only-store-remote-sigs Jul 6, 2021

t-bast deleted the channel-version-migration branch December 15, 2021 09:46


		object ChannelConfigOptions {

		def standard: ChannelConfigOptions = ChannelConfigOptions(activated = Set(FundingPubKeyBasedChannelKeyPath))

	def standard: ChannelConfigOptions = ChannelConfigOptions(activated = Set(FundingPubKeyBasedChannelKeyPath))
	val standard: ChannelConfigOptions = ChannelConfigOptions(activated = Set(FundingPubKeyBasedChannelKeyPath))

Conversation

t-bast commented Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sstone commented Jun 30, 2021

Uh oh!

t-bast commented Jun 30, 2021

Uh oh!

sstone commented Jun 30, 2021

Uh oh!

t-bast commented Jul 1, 2021

Uh oh!

sstone left a comment

Choose a reason for hiding this comment

Uh oh!

pm47 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t-bast commented Jul 2, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pm47 Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

t-bast commented Jun 29, 2021 •

edited

Loading

pm47 Jul 6, 2021 •

edited

Loading