Lower minimum remote dust limit

docs/release-notes/eclair-vnext.md

@@ -106,6 +106,16 @@ This is fixed in this release: when using Tor, the watchdogs will now also be qu
 You can also now choose to disable some watchdogs by removing them from the `eclair.blockchain-watchdog.sources` list in `eclair.conf`.
 Head over to [reference.conf](https://github.com/ACINQ/eclair/blob/master/eclair-core/src/main/resources/reference.conf) for more details.
 
+### Dust limit thresholds
+
+Eclair can now use dust limits as low as 354 satoshis.
+This value covers all current and future segwit versions, while ensuring that transactions can relay according to default bitcoin network policies.
+
+With this change, we also disallow non-segwit scripts when closing a channel.
+We still support receiving non-segwit remote scripts, but will force-close if the resulting mutual close transaction would be invalid according to default network policies.
+
+See the [spec discussions](https://github.com/lightningnetwork/lightning-rfc/pull/894) for more details.
+
 ### Sample GUI removed
 
 We previously included code for a sample GUI: `eclair-node-gui`.
@@ -122,6 +132,7 @@ This release contains many API updates:
 - `open` doesn't support the `--feeBaseMsat` and `--feeProportionalMillionths` parameters anymore: you should instead set these with the `updaterelayfee` API, which can now be called before opening a channel (#1890)
 - `updaterelayfee` must now be called with nodeIds instead of channelIds and will update the fees for all channels with the given node(s) at once (#1890)
 - `close` lets you specify a fee range when using quick close through the `--preferredFeerateSatByte`, `--minFeerateSatByte` and `--maxFeerateSatByte` (#1768)
+- `close` now rejects non-segwit `scriptPubKey`
 - `createinvoice` now lets you provide a `--descriptionHash` instead of a `--description` (#1919)
 - `sendtonode` doesn't support providing a `paymentHash` anymore since it uses `keysend` to send the payment (#1840)
 - `payinvoice`, `sendtonode`, `findroute`, `findroutetonode` and `findroutebetweennodes` let you specify `--pathFindingExperimentName` when using path-finding A/B testing (#1930)

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

@@ -233,7 +233,7 @@ object NodeParams extends Logging {
 
     val dustLimitSatoshis = Satoshi(config.getLong("dust-limit-satoshis"))
     if (chainHash == Block.LivenetGenesisBlock.hash) {
-      require(dustLimitSatoshis >= Channel.MIN_DUSTLIMIT, s"dust limit must be greater than ${Channel.MIN_DUSTLIMIT}")
+      require(dustLimitSatoshis >= Channel.MIN_DUST_LIMIT, s"dust limit must be greater than ${Channel.MIN_DUST_LIMIT}")
     }
 
     val htlcMinimum = MilliSatoshi(config.getInt("htlc-minimum-msat"))

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

@@ -79,8 +79,12 @@ object Channel {
   val MAX_FUNDING: Satoshi = 16777216 sat // = 2^24
   val MAX_ACCEPTED_HTLCS = 483
 
-  // we don't want the counterparty to use a dust limit lower than that, because they wouldn't only hurt themselves we may need them to publish their commit tx in certain cases (backup/restore)
-  val MIN_DUSTLIMIT: Satoshi = 546 sat
+  // We may need to rely on our peer's commit tx in certain cases (backup/restore) so we must ensure their transactions
+  // can propagate through the bitcoin network (assuming bitcoin core nodes with default policies).
+  // The various dust limits enforced by the bitcoin network are summarized here:
+  // https://github.com/lightningnetwork/lightning-rfc/blob/master/03-transactions.md#dust-limits
+  // A dust limit of 354 sat ensures all segwit outputs will relay with default relay policies.
+  val MIN_DUST_LIMIT: Satoshi = 354 sat
 
   // we won't exchange more than this many signatures when negotiating the closing fee
   val MAX_NEGOTIATION_ITERATIONS = 20

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

@@ -69,6 +69,7 @@ case class InvalidCommitmentSignature              (override val channelId: Byte
 case class InvalidHtlcSignature                    (override val channelId: ByteVector32, tx: Transaction) extends ChannelException(channelId, s"invalid htlc signature: tx=$tx")
 case class InvalidCloseSignature                   (override val channelId: ByteVector32, tx: Transaction) extends ChannelException(channelId, s"invalid close signature: tx=$tx")
 case class InvalidCloseFee                         (override val channelId: ByteVector32, fee: Satoshi) extends ChannelException(channelId, s"invalid close fee: fee_satoshis=$fee")
+case class InvalidCloseAmountBelowDust             (override val channelId: ByteVector32, tx: Transaction) extends ChannelException(channelId, s"invalid closing tx: some outputs are below dust: tx=$tx")
 case class HtlcSigCountMismatch                    (override val channelId: ByteVector32, expected: Int, actual: Int) extends ChannelException(channelId, s"htlc sig count mismatch: expected=$expected actual: $actual")
 case class ForcedLocalCommit                       (override val channelId: ByteVector32) extends ChannelException(channelId, s"forced local commit")
 case class UnexpectedHtlcId                        (override val channelId: ByteVector32, expected: Long, actual: Long) extends ChannelException(channelId, s"unexpected htlc id: expected=$expected actual=$actual")

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

@@ -138,7 +138,7 @@ object Helpers {
     if (nodeParams.onChainFeeConf.feerateToleranceFor(remoteNodeId).isFeeDiffTooHigh(channelType, localFeeratePerKw, open.feeratePerKw)) return Left(FeerateTooDifferent(open.temporaryChannelId, localFeeratePerKw, open.feeratePerKw))
     // only enforce dust limit check on mainnet
     if (nodeParams.chainHash == Block.LivenetGenesisBlock.hash) {
-      if (open.dustLimitSatoshis < Channel.MIN_DUSTLIMIT) return Left(DustLimitTooSmall(open.temporaryChannelId, open.dustLimitSatoshis, Channel.MIN_DUSTLIMIT))
+      if (open.dustLimitSatoshis < Channel.MIN_DUST_LIMIT) return Left(DustLimitTooSmall(open.temporaryChannelId, open.dustLimitSatoshis, Channel.MIN_DUST_LIMIT))
     }
 
     // we don't check that the funder's amount for the initial commitment transaction is sufficient for full fee payment
@@ -169,7 +169,7 @@ object Helpers {
     if (accept.maxAcceptedHtlcs > Channel.MAX_ACCEPTED_HTLCS) return Left(InvalidMaxAcceptedHtlcs(accept.temporaryChannelId, accept.maxAcceptedHtlcs, Channel.MAX_ACCEPTED_HTLCS))
     // only enforce dust limit check on mainnet
     if (nodeParams.chainHash == Block.LivenetGenesisBlock.hash) {
-      if (accept.dustLimitSatoshis < Channel.MIN_DUSTLIMIT) return Left(DustLimitTooSmall(accept.temporaryChannelId, accept.dustLimitSatoshis, Channel.MIN_DUSTLIMIT))
+      if (accept.dustLimitSatoshis < Channel.MIN_DUST_LIMIT) return Left(DustLimitTooSmall(accept.temporaryChannelId, accept.dustLimitSatoshis, Channel.MIN_DUST_LIMIT))
     }
 
     if (accept.dustLimitSatoshis > nodeParams.maxRemoteDustLimit) return Left(DustLimitTooLarge(open.temporaryChannelId, accept.dustLimitSatoshis, nodeParams.maxRemoteDustLimit))
@@ -518,14 +518,36 @@ object Helpers {
         Left(InvalidCloseFee(commitments.channelId, remoteClosingFee))
       } else {
         val (closingTx, closingSigned) = makeClosingTx(keyManager, commitments, localScriptPubkey, remoteScriptPubkey, ClosingFees(remoteClosingFee, remoteClosingFee, remoteClosingFee))
-        val signedClosingTx = Transactions.addSigs(closingTx, keyManager.fundingPublicKey(commitments.localParams.fundingKeyPath).publicKey, remoteParams.fundingPubKey, closingSigned.signature, remoteClosingSig)
-        Transactions.checkSpendable(signedClosingTx) match {
-          case Success(_) => Right(signedClosingTx, closingSigned)
-          case _ => Left(InvalidCloseSignature(commitments.channelId, signedClosingTx.tx))
+        if (checkClosingDustAmounts(closingTx)) {
+          val signedClosingTx = Transactions.addSigs(closingTx, keyManager.fundingPublicKey(commitments.localParams.fundingKeyPath).publicKey, remoteParams.fundingPubKey, closingSigned.signature, remoteClosingSig)
+          Transactions.checkSpendable(signedClosingTx) match {
+            case Success(_) => Right(signedClosingTx, closingSigned)
+            case _ => Left(InvalidCloseSignature(commitments.channelId, signedClosingTx.tx))
+          }
+        } else {
+          Left(InvalidCloseAmountBelowDust(commitments.channelId, closingTx.tx))
         }
       }
     }
 
+    /**
+     * Check that all closing outputs are above bitcoin's dust limit for their script type, otherwise there is a risk
+     * that the closing transaction will not be relayed to miners' mempool and will not confirm.
+     * The various dust limits are detailed in https://github.com/lightningnetwork/lightning-rfc/blob/master/03-transactions.md#dust-limits
+     */
+    def checkClosingDustAmounts(closingTx: ClosingTx): Boolean = {
+      closingTx.tx.txOut.forall(txOut => {
+        Try(Script.parse(txOut.publicKeyScript)) match {
+          case Success(OP_DUP :: OP_HASH160 :: OP_PUSHDATA(pubkeyHash, _) :: OP_EQUALVERIFY :: OP_CHECKSIG :: Nil) if pubkeyHash.size == 20 => txOut.amount >= 546.sat
+          case Success(OP_HASH160 :: OP_PUSHDATA(scriptHash, _) :: OP_EQUAL :: Nil) if scriptHash.size == 20 => txOut.amount >= 540.sat
+          case Success(OP_0 :: OP_PUSHDATA(pubkeyHash, _) :: Nil) if pubkeyHash.size == 20 => txOut.amount >= 294.sat
+          case Success(OP_0 :: OP_PUSHDATA(scriptHash, _) :: Nil) if scriptHash.size == 32 => txOut.amount >= 330.sat
+          case Success((OP_1 | OP_2 | OP_3 | OP_4 | OP_5 | OP_6 | OP_7 | OP_8 | OP_9 | OP_10 | OP_11 | OP_12 | OP_13 | OP_14 | OP_15 | OP_16) :: OP_PUSHDATA(program, _) :: Nil) if 2 <= program.length && program.length <= 40 => txOut.amount >= 354.sat
+          case _ => txOut.amount >= 546.sat
+        }
+      })
+    }
+
     /** Wraps transaction generation in a Try and filters failures to avoid one transaction negatively impacting a whole commitment. */
     private def generateTx[T <: TransactionWithInputInfo](desc: String)(attempt: => Either[TxGenerationSkipped, T])(implicit log: LoggingAdapter): Option[T] = {
       Try {

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

@@ -17,7 +17,7 @@
 package fr.acinq.eclair.channel
 
 import akka.testkit.{TestFSMRef, TestProbe}
-import fr.acinq.bitcoin.{Btc, OutPoint, SatoshiLong, Transaction, TxOut}
+import fr.acinq.bitcoin._
 import fr.acinq.eclair.TestConstants.Alice.nodeParams
 import fr.acinq.eclair.TestUtils.NoLoggingDiagnostics
 import fr.acinq.eclair.blockchain.bitcoind.ZmqWatcher.WatchFundingSpentTriggered
@@ -28,6 +28,7 @@ import fr.acinq.eclair.wire.protocol.UpdateAddHtlc
 import fr.acinq.eclair.{MilliSatoshiLong, TestKitBaseClass}
 import org.scalatest.Tag
 import org.scalatest.funsuite.AnyFunSuiteLike
+import scodec.bits.HexStringSyntax
 
 import java.util.UUID
 import scala.concurrent.duration._
@@ -226,6 +227,32 @@ class HelpersSpec extends TestKitBaseClass with AnyFunSuiteLike with ChannelStat
     findTimedOutHtlcs(setupHtlcs(Set(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs)), withoutHtlcId = false)
   }
 
+  test("check closing tx amounts above dust") {
+    val p2pkhBelowDust = Seq(TxOut(545 sat, OP_DUP :: OP_HASH160 :: OP_PUSHDATA(hex"0000000000000000000000000000000000000000") :: OP_EQUALVERIFY :: OP_CHECKSIG :: Nil))
+    val p2shBelowDust = Seq(TxOut(539 sat, OP_HASH160 :: OP_PUSHDATA(hex"0000000000000000000000000000000000000000") :: OP_EQUAL :: Nil))
+    val p2wpkhBelowDust = Seq(TxOut(293 sat, OP_0 :: OP_PUSHDATA(hex"0000000000000000000000000000000000000000") :: Nil))
+    val p2wshBelowDust = Seq(TxOut(329 sat, OP_0 :: OP_PUSHDATA(hex"0000000000000000000000000000000000000000000000000000000000000000") :: Nil))
+    val futureSegwitBelowDust = Seq(TxOut(353 sat, OP_3 :: OP_PUSHDATA(hex"0000000000") :: Nil))
+    val allOutputsAboveDust = Seq(
+      TxOut(546 sat, OP_DUP :: OP_HASH160 :: OP_PUSHDATA(hex"0000000000000000000000000000000000000000") :: OP_EQUALVERIFY :: OP_CHECKSIG :: Nil),
+      TxOut(540 sat, OP_HASH160 :: OP_PUSHDATA(hex"0000000000000000000000000000000000000000") :: OP_EQUAL :: Nil),
+      TxOut(294 sat, OP_0 :: OP_PUSHDATA(hex"0000000000000000000000000000000000000000") :: Nil),
+      TxOut(330 sat, OP_0 :: OP_PUSHDATA(hex"0000000000000000000000000000000000000000000000000000000000000000") :: Nil),
+      TxOut(354 sat, OP_3 :: OP_PUSHDATA(hex"0000000000") :: Nil),
+    )
+
+    def toClosingTx(txOut: Seq[TxOut]): ClosingTx = {
+      ClosingTx(InputInfo(OutPoint(ByteVector32.Zeroes, 0), TxOut(1000 sat, Nil), Nil), Transaction(2, Nil, txOut, 0), None)
+    }
+
+    assert(Closing.checkClosingDustAmounts(toClosingTx(allOutputsAboveDust)))
+    assert(!Closing.checkClosingDustAmounts(toClosingTx(p2pkhBelowDust)))
+    assert(!Closing.checkClosingDustAmounts(toClosingTx(p2shBelowDust)))
+    assert(!Closing.checkClosingDustAmounts(toClosingTx(p2wpkhBelowDust)))
+    assert(!Closing.checkClosingDustAmounts(toClosingTx(p2wshBelowDust)))
+    assert(!Closing.checkClosingDustAmounts(toClosingTx(futureSegwitBelowDust)))
+  }
+
   test("tell closing type") {
     val commitments = CommitmentsSpec.makeCommitments(10000 msat, 15000 msat)
     val tx1 :: tx2 :: tx3 :: tx4 :: tx5 :: tx6 :: Nil = List(

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

@@ -177,11 +177,11 @@ class WaitForAcceptChannelStateSpec extends TestKitBaseClass with FixtureAnyFunS
   test("recv AcceptChannel (dust limit too low)", Tag("mainnet")) { f =>
     import f._
     val accept = bob2alice.expectMsgType[AcceptChannel]
-    // we don't want their dust limit to be below 546
-    val lowDustLimitSatoshis = 545.sat
+    // we don't want their dust limit to be below 354
+    val lowDustLimitSatoshis = 353.sat
     alice ! accept.copy(dustLimitSatoshis = lowDustLimitSatoshis)
     val error = alice2bob.expectMsgType[Error]
-    assert(error === Error(accept.temporaryChannelId, DustLimitTooSmall(accept.temporaryChannelId, lowDustLimitSatoshis, Channel.MIN_DUSTLIMIT).getMessage))
+    assert(error === Error(accept.temporaryChannelId, DustLimitTooSmall(accept.temporaryChannelId, lowDustLimitSatoshis, Channel.MIN_DUST_LIMIT).getMessage))
     awaitCond(alice.stateName == CLOSED)
   }
 

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

@@ -18,7 +18,7 @@ package fr.acinq.eclair.api.handlers
 
 import akka.http.scaladsl.server.{MalformedFormFieldRejection, Route}
 import akka.util.Timeout
-import fr.acinq.bitcoin.Satoshi
+import fr.acinq.bitcoin.{Satoshi, Script}
 import fr.acinq.eclair.MilliSatoshi
 import fr.acinq.eclair.api.Service
 import fr.acinq.eclair.api.directives.EclairDirectives
@@ -63,7 +63,11 @@ trait Channel {
             val maxFeerate = maxFeerate_opt.map(feerate => FeeratePerKw(feerate)).getOrElse(preferredFeerate * 2)
             ClosingFeerates(preferredFeerate, minFeerate, maxFeerate)
           })
-          complete(eclairApi.close(channels, scriptPubKey_opt, closingFeerates))
+          if (scriptPubKey_opt.forall(Script.isNativeWitnessScript)) {
+            complete(eclairApi.close(channels, scriptPubKey_opt, closingFeerates))
+          } else {
+            reject(MalformedFormFieldRejection("scriptPubKey", "Non-segwit scripts are not allowed"))
+          }
       }
     }
   }

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

@@ -524,6 +524,21 @@ class ApiServiceSpec extends AnyFunSuite with ScalatestRouteTest with IdiomaticM
       }
   }
 
+  test("'close' rejects non-segwit scripts") {
+    val shortChannelId = "1701x42x3"
+    val eclair = mock[Eclair]
+    val mockService = new MockService(eclair)
+
+    Post("/close", FormData("shortChannelId" -> shortChannelId, "scriptPubKey" -> "a914748284390f9e263a4b766a75d0633c50426eb87587").toEntity) ~>
+      addCredentials(BasicHttpCredentials("", mockApi().password)) ~>
+      addHeader("Content-Type", "application/json") ~>
+      Route.seal(mockService.close) ~>
+      check {
+        assert(handled)
+        assert(status == BadRequest)
+      }
+  }
+
   test("'connect' method should accept a nodeId") {
     val remoteNodeId = PublicKey(hex"030bb6a5e0c6b203c7e2180fb78c7ba4bdce46126761d8201b91ddac089cdecc87")
 
@@ -560,7 +575,6 @@ class ApiServiceSpec extends AnyFunSuite with ScalatestRouteTest with IdiomaticM
       }
   }
 
-
   test("'send' method should handle payment failures") {
     val eclair = mock[Eclair]
     eclair.send(any, any, any, any, any, any)(any[Timeout]) returns Future.failed(new IllegalArgumentException("invoice has expired"))
@@ -982,7 +996,7 @@ class ApiServiceSpec extends AnyFunSuite with ScalatestRouteTest with IdiomaticM
     eclair.findRoute(any, any, any, any)(any[Timeout]) returns Future.successful(Router.RouteResponse(Seq(Router.Route(456.msat, mockHops))))
 
     // invalid format
-    Post("/findroute", FormData("format"-> "invalid-output-format", "invoice" -> invoice, "amountMsat" -> "456")) ~>
+    Post("/findroute", FormData("format" -> "invalid-output-format", "invoice" -> invoice, "amountMsat" -> "456")) ~>
       addCredentials(BasicHttpCredentials("", mockApi().password)) ~>
       addHeader("Content-Type", "application/json") ~>
       Route.seal(mockService.findRoute) ~>