Add support for zero-conf and scid-alias #2224

pm47 · 2022-04-04T16:23:39Z

Builds on #2221 and #2269.

Commit-by-commit review strongly advised. 7e28c1a is a bit of a mouthful, but the other commits are mostly small.

Commit e7f7ef0 is optional, it skips blockchain validation for local channels, which would save a lot of bitcoind calls at startup for large nodes.

pm47 · 2022-05-17T16:21:14Z

eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala

+            case w: WatchFundingConfirmed if confirmations == 0 && w.minDepth == 0 =>
+              // if the tx doesn't have confirmations but we don't require any, we reply with a fake block index
+              // otherwise, we get the real short id
+              context.self ! TriggerEvent(w.replyTo, w, WatchFundingConfirmedTriggered(BlockHeight(0), 0, tx))


Using a blockheight 0 for unconfirmed transactions is a bit hacky.

pm47 · 2022-05-17T16:22:55Z

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

@@ -101,33 +101,46 @@ object ChannelTypes {
    override def commitmentFormat: CommitmentFormat = UnsafeLegacyAnchorOutputsCommitmentFormat
    override def toString: String = "anchor_outputs"
  }
-  case object AnchorOutputsZeroFeeHtlcTx extends SupportedChannelType {
-    override def features: Set[InitFeature] = Set(Features.StaticRemoteKey, Features.AnchorOutputsZeroFeeHtlcTx)
+  case class AnchorOutputsZeroFeeHtlcTx(scidAlias: Boolean, zeroConf: Boolean) extends SupportedChannelType {


To reduce the cardinality of channel types, only AnchorOutputsZeroFeeHtlcTx channels can support scidAlias or zeroConf.

I agree with that choice, it's a sensible one, but it means we won't be compatible with LDK (I believe they haven't shipped anchor outputs yet). But it's fine, we can wait for them to ship anchor outputs which should high be on their roadmap.

We do need to add tests to verify that we correctly reject channel types that use zero_conf or scid_alias with previous commitment types.

pm47 · 2022-05-17T16:26:10Z

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

+     * As funder we trust ourselves to not double spend funding txs: we could always use a zero-confirmation watch,
+     * but we need a scid to send the initial channel_update and remote may not provide an alias (and we don't want to
+     * trust the real scid sent by remote in their channel_ready). So we always wait for one conf, except if the channel
+     * has the zero-conf feature (because presumably the peer will sends an alias in that case).


Note: a consequence of this is that the channel will only be zero-conf if the feature is set. It's not enough for the fundee to just trust us and send an early channel_ready because we will already have decided to wait for one conf (we would receive their alias in the channel_ready, but we put the watch before that in the channel fsm). I guess if we receive an early channel_ready with an alias, we can put a 2nd zero-conf watch, the first one will be ignored.

pm47 · 2022-05-17T16:27:43Z

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

+     * @return number of confirmations needed
+     */
+    def minDepthFundee(channelConf: ChannelConf, channelFeatures: ChannelFeatures, fundingSatoshis: Satoshi): Long = fundingSatoshis match {
+      case _ if channelFeatures.hasFeature(Features.ZeroConf) => 0 // zero-conf stay zero-conf, whatever the funding amount is


A bit reckless, even if option_zeroconf should only be enabled for a set of trusted peers.

pm47 · 2022-05-17T16:55:53Z

eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala

@@ -59,6 +60,7 @@ class Router(val nodeParams: NodeParams, watcher: typed.ActorRef[ZmqWatcher.Comm
  // we pass these to helpers classes so that they have the logging context
  implicit def implicitLog: DiagnosticLoggingAdapter = diagLog

+  context.system.eventStream.subscribe(self, classOf[ShortChannelIdAssigned])


Not really happy with the router listening to yet another event, but I couldn't find a better way to fix the race condition (see bf39b07).

At least I was able to have ChannelRelayer stop listening anymore to that same event 🤷

pm47 · 2022-05-17T17:03:14Z

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

+        ("shortChannelId" | realshortchannelid) ::
+        ("lastSent" | channelReadyCodec)).map {
+      case commitments :: shortChannelId :: lastSent :: HNil =>
+        DATA_WAIT_FOR_CHANNEL_READY(commitments, realShortChannelId_opt = Some(shortChannelId), localAlias = shortChannelId.toAlias, lastSent = lastSent)


Note how for backward compatibility we set both the realShortChannelId_opt and localAlias to the same value. It may not actually be the real scid on feature branches.

pm47 · 2022-05-17T17:11:07Z

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

@@ -56,6 +65,8 @@ object ShortChannelId {
  def outputIndex(shortChannelId: ShortChannelId): Int = (shortChannelId.id & 0xFFFF).toInt

  def coordinates(shortChannelId: ShortChannelId): TxCoordinates = TxCoordinates(blockHeight(shortChannelId), txIndex(shortChannelId), outputIndex(shortChannelId))
+
+  def generateLocalAlias(): LocalAlias = new ShortChannelId(System.nanoTime()) with LocalAlias // TODO: fixme (duplicate, etc.)


⚠️ needs fixing, this is a dummy impl that leaks information on the creation date of the channel.

Since generating an alias only happens at channel creation, we can afford doing something "costly". For example this could be an AtomicLong that we initialize at startup by checking all currently assigned localAlias, and then we do an addAndGet with a random increment when we need to generate a new alias.

I don't think we really need a counter, wouldn't a randomLong() in the range that cannot be used by normal channels be enough?

We cannot afford a duplicate here though and the space isn't that big. We should expect a collision for 5 billion attempts on 64 bits and only 80k attempts for 32 bits right? https://en.wikipedia.org/wiki/Birthday_attack

It's true, the space isn't that big, but I'm afraid there may be privacy issues with generating those incrementally...I'll check what other implementations do

I did some maths to confirm this, let me know if my calculations look correct:

scids are 64 bits long

we want to reserve the range [0; 2^22] for real scids

that leaves us with 2^42 available values for aliases

we'll consider that we want to have 250k channels using aliases (~2^18)

the probability of a collision is then p(n) ~= 2^36 / 2^43 ~= 0.8%

This is indeed not great! But using a counter that we increment leaks information about the approximate creation time of the channel, which could let an attacker find the channel outpoint on-chain...so I believe that what we should do instead is:

generate an alias randomly in the [2^22:2^64] range

check our alias map to see if it's already assigned and re-roll if it is

It means we'll probably need to send a message to the router to obtain a new alias, the router is where we can guarantee uniqueness.

pm47 · 2022-05-20T08:29:51Z

Rebased

t-bast

This is a first pass on some details and high-level architecture, I'll dive into the actual usage of scid_alias in a second pass later, my brain wasn't able to handle it today :)

eclair-core/src/main/resources/reference.conf

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

t-bast · 2022-05-20T16:52:56Z

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

@@ -56,6 +65,8 @@ object ShortChannelId {
  def outputIndex(shortChannelId: ShortChannelId): Int = (shortChannelId.id & 0xFFFF).toInt

  def coordinates(shortChannelId: ShortChannelId): TxCoordinates = TxCoordinates(blockHeight(shortChannelId), txIndex(shortChannelId), outputIndex(shortChannelId))
+
+  def generateLocalAlias(): LocalAlias = new ShortChannelId(System.nanoTime()) with LocalAlias // TODO: fixme (duplicate, etc.)


I don't think we really need a counter, wouldn't a randomLong() in the range that cannot be used by normal channels be enough?

eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForAcceptChannelStateSpec.scala

eclair-core/src/test/scala/fr/acinq/eclair/wire/protocol/LightningMessageCodecsSpec.scala

eclair-node/src/main/scala/fr/acinq/eclair/api/handlers/Channel.scala

eclair-node/src/test/scala/fr/acinq/eclair/api/ApiServiceSpec.scala

codecov-commenter · 2022-06-05T18:25:25Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 95.79439% with 18 lines in your changes missing coverage. Please review.

Project coverage is 84.76%. Comparing base (4722755) to head (04fc0e6).
Report is 382 commits behind head on master.

Files with missing lines	Patch %	Lines
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala	91.89%	6 Missing ⚠️
...main/scala/fr/acinq/eclair/router/Validation.scala	96.93%	3 Missing ⚠️
...rc/main/scala/fr/acinq/eclair/ShortChannelId.scala	94.73%	1 Missing ⚠️
...n/scala/fr/acinq/eclair/balance/BalanceActor.scala	0.00%	1 Missing ⚠️
...n/scala/fr/acinq/eclair/balance/CheckBalance.scala	50.00%	1 Missing ⚠️
...ain/scala/fr/acinq/eclair/balance/Monitoring.scala	0.00%	1 Missing ⚠️
.../scala/fr/acinq/eclair/channel/ChannelEvents.scala	80.00%	1 Missing ⚠️
...q/eclair/channel/fsm/ChannelOpenSingleFunder.scala	97.72%	1 Missing ⚠️
...ire/internal/channel/version0/ChannelCodecs0.scala	87.50%	1 Missing ⚠️
...ire/internal/channel/version1/ChannelCodecs1.scala	80.00%	1 Missing ⚠️
... and 1 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2224      +/-   ##
==========================================
+ Coverage   84.56%   84.76%   +0.20%     
==========================================
  Files         193      194       +1     
  Lines       14476    14624     +148     
  Branches      649      621      -28     
==========================================
+ Hits        12241    12396     +155     
+ Misses       2235     2228       -7

Files with missing lines	Coverage Δ
...e/src/main/scala/fr/acinq/eclair/BlockHeight.scala	`71.42% <100.00%> (+2.19%)`	⬆️
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala	`51.36% <100.00%> (+0.53%)`	⬆️
...core/src/main/scala/fr/acinq/eclair/Features.scala	`99.21% <100.00%> (+0.03%)`	⬆️
.../acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala	`88.13% <ø> (ø)`
.../fr/acinq/eclair/blockchain/fee/FeeEstimator.scala	`100.00% <ø> (ø)`
...lair/blockchain/watchdogs/BlockchainWatchdog.scala	`58.53% <100.00%> (ø)`
...in/scala/fr/acinq/eclair/channel/ChannelData.scala	`100.00% <100.00%> (ø)`
...cala/fr/acinq/eclair/channel/ChannelFeatures.scala	`100.00% <100.00%> (ø)`
...in/scala/fr/acinq/eclair/channel/Commitments.scala	`94.76% <ø> (ø)`
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala	`95.06% <100.00%> (+0.08%)`	⬆️
... and 38 more

... and 6 files with indirect coverage changes

pm47 · 2022-06-06T13:03:41Z

Rebased.

pm47 · 2022-06-06T14:48:46Z

Follow up on #2269 (comment): I think it's better to defer to the dual-funding PR, as the problem still doesn't exist, and the present PR is already heavy enough.

…e subclass registered

t-bast

I've finished reviewing the code up to 957f014, but I haven't reviewed the tests yet.

Submitting this PR round and waiting for the changes to the scid types and #2310 and I'll move on to review the tests.

eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/ChannelRelayer.scala

eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala

eclair-core/src/main/scala/fr/acinq/eclair/router/Validation.scala

Instead of using a watch with minDepth=0, we can directly skip the wait_for_funding_confirmed state when using 0-conf, which is less hacky. Co-authored-by: pm47 <pm.padiou@gmail.com>

t-bast

LGTM, I have some test improvements / fixes in #2314 and we should be good to go. I'll do some end-to-end tests on regtest with other implementations.

EDIT: we need to add release notes, can you do that @pm47?

t-bast

LGTM, we're almost ready to go!

t-bast · 2022-06-15T08:44:40Z

eclair-core/src/test/scala/fr/acinq/eclair/integration/basic/ZeroConfActivationSpec.scala

+    assert(!getChannelData(bob, channelId).asInstanceOf[PersistentChannelData].commitments.channelFeatures.hasFeature(ZeroConf))
+  }
+
+  test("open a channel alice-bob (zero-conf disabled on both sides, requested via channel type by alice)") { f =>


In a follow-up PR, we should introduce a whitelist of nodes from which we accept 0-conf channels. If nodes are in that whitelist, we should accept the channel_type even though they didn't set the feature in their init.

We should maybe even go further than that and use 0-conf with those peers even if the channel_type doesn't include it. Either they'll accept it and send their channel_ready, or they won't and it's ok, we'll just wait for their channel_ready automatically.

eclair-core/src/test/scala/fr/acinq/eclair/integration/basic/ZeroConfActivationSpec.scala

docs/release-notes/eclair-vnext.md

t-bast

LGTM 🚀 ⚡

pm47 · 2022-06-15T11:08:39Z

🤟

pm47 · 2022-06-15T11:13:20Z

Follow-ups to this PR include:

better way of generating aliases (see Add support for zero-conf and scid-alias #2224 (comment))
whitelist of zeroconf peers:
- prevent users from accidentally enabling zeroconf for all peers
- set min_depth=0 and send early channel_ready even if option_zeroconf bit isn't set in the channel type

pm47 mentioned this pull request Apr 7, 2022

Refactor channel transitions #2228

Merged

t-bast added the Optech Make Me Famous! label Apr 7, 2022

pm47 force-pushed the zeroconf-scid-alias branch 2 times, most recently from 682d9cb to d091440 Compare April 11, 2022 15:57

pm47 force-pushed the zeroconf-scid-alias branch from d091440 to 32543a3 Compare May 3, 2022 09:10

pm47 force-pushed the zeroconf-scid-alias branch 3 times, most recently from f21861b to aa2956f Compare May 17, 2022 16:59

pm47 commented May 17, 2022

View reviewed changes

pm47 force-pushed the zeroconf-scid-alias branch from aa2956f to 8aee27a Compare May 18, 2022 11:53

pm47 requested a review from t-bast May 18, 2022 12:11

pm47 marked this pull request as ready for review May 18, 2022 12:11

pm47 marked this pull request as draft May 18, 2022 12:11

This was referenced May 18, 2022

Add simple integration test between Channel and Router #2270

Merged

Use long id instead of short id in ChannelRelay #2221

Merged

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

Merged

pm47 force-pushed the zeroconf-scid-alias branch from 8aee27a to 5cbecac Compare May 20, 2022 08:29

t-bast reviewed May 20, 2022

View reviewed changes

pm47 force-pushed the zeroconf-scid-alias branch 2 times, most recently from 802ac3a to 9e1a675 Compare June 5, 2022 18:08

pm47 force-pushed the zeroconf-scid-alias branch 3 times, most recently from fc7d4ee to ea388c5 Compare June 6, 2022 13:03

pm47 marked this pull request as ready for review June 6, 2022 14:44

pm47 added 2 commits June 8, 2022 15:28

rename funding_locked -> channel_ready

bbd0b34

add new options, reassign trampoline bit

244d6dd

fix error: knownDirectSubclasses of RealShortChannelId observed befor…

957f014

…e subclass registered

t-bast mentioned this pull request Jun 10, 2022

Remove watch-confirmed with depth 0 in zero-conf #2310

Merged

convert scid traits to case classes

db232e1

t-bast reviewed Jun 10, 2022

View reviewed changes

leverage 31799b7 to simplify generation of routing hints

c20f672

pm47 force-pushed the zeroconf-scid-alias branch from da46456 to c20f672 Compare June 13, 2022 11:51

pm47 and others added 2 commits June 13, 2022 15:10

simplify scid.equals()

305124f

Remove watch-confirmed with depth 0 in zero-conf (#2310)

79ef2c3

Instead of using a watch with minDepth=0, we can directly skip the wait_for_funding_confirmed state when using 0-conf, which is less hacky. Co-authored-by: pm47 <pm.padiou@gmail.com>

t-bast mentioned this pull request Jun 14, 2022

Improve tests in #2224 #2314

Merged

t-bast reviewed Jun 14, 2022

View reviewed changes

t-bast and others added 4 commits June 14, 2022 16:06

Improve tests in #2224 (#2314)

189d101

implement waitFor with Thread.sleep

b5f137f

first draft of release notes

dcc5236

test activation of zeroconf feature

02bfd0f

pm47 force-pushed the zeroconf-scid-alias branch from e9739d1 to 02bfd0f Compare June 14, 2022 16:17

t-bast reviewed Jun 15, 2022

View reviewed changes

pm47 added 3 commits June 15, 2022 10:53

also remove real scid from the map on channel down

a1ebd76

address review comments

ac60f87

fixup! test activation of zeroconf feature

4f6385b

t-bast previously approved these changes Jun 15, 2022

View reviewed changes

simplify and fix tests

04fc0e6

pm47 dismissed t-bast’s stale review via 04fc0e6 June 15, 2022 09:55

t-bast approved these changes Jun 15, 2022

View reviewed changes

pm47 merged commit e5f5cd1 into master Jun 15, 2022

pm47 deleted the zeroconf-scid-alias branch June 15, 2022 11:09

pm47 mentioned this pull request Jun 15, 2022

Generate local alias before spawning channel #2316

Closed

t-bast mentioned this pull request Jun 15, 2022

Implement first steps of the dual funding flow #2247

Merged

pm47 mentioned this pull request Aug 10, 2022

Implement the interactive-tx protocol #2273

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for zero-conf and scid-alias #2224

Add support for zero-conf and scid-alias #2224

pm47 commented Apr 4, 2022 •

edited

Loading

pm47 May 17, 2022

pm47 May 17, 2022

t-bast May 20, 2022

pm47 May 17, 2022 •

edited

Loading

pm47 May 17, 2022

pm47 May 17, 2022

pm47 May 17, 2022

pm47 May 17, 2022

t-bast May 20, 2022

pm47 May 25, 2022

t-bast May 25, 2022

t-bast May 28, 2022

pm47 commented May 20, 2022

t-bast left a comment

t-bast May 20, 2022

codecov-commenter commented Jun 5, 2022 •

edited

Loading

pm47 commented Jun 6, 2022

pm47 commented Jun 6, 2022

t-bast left a comment

t-bast left a comment •

edited

Loading

t-bast left a comment

t-bast Jun 15, 2022

t-bast left a comment

pm47 commented Jun 15, 2022

pm47 commented Jun 15, 2022

Add support for zero-conf and scid-alias #2224

Add support for zero-conf and scid-alias #2224

Conversation

pm47 commented Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pm47 May 17, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pm47 commented May 20, 2022

t-bast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 5, 2022 • edited Loading

Codecov Report

pm47 commented Jun 6, 2022

pm47 commented Jun 6, 2022

t-bast left a comment

Choose a reason for hiding this comment

t-bast left a comment • edited Loading

Choose a reason for hiding this comment

t-bast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-bast left a comment

Choose a reason for hiding this comment

pm47 commented Jun 15, 2022

pm47 commented Jun 15, 2022

pm47 commented Apr 4, 2022 •

edited

Loading

pm47 May 17, 2022 •

edited

Loading

codecov-commenter commented Jun 5, 2022 •

edited

Loading

t-bast left a comment •

edited

Loading