Add high-level helpers for taproot channels #3028

sstone · 2025-03-05T11:18:08Z

We add a new specific commitment format for taproot channels, and high-level methods for creating and spending taproot channel transactions.
There are no functional changes: protocol messages and codecs have not been updated to support taproot channels, these new methods are only used in unit tests.

We add a new specific commitment format for taproot channels, and high-level methods for creating and spending taproot channel transactions.

t-bast

To be honest I think the changes to Transactions.scala show that our existing architecture doesn't work at all for both segwit and taproot...it's frankly a big red flag that it may be worth rethinking the architecture of those types more in-depth now.

I don't have the solution, but I think we should spend more time prototyping bigger re-works of this architecture to be more future-proof. I'll spend some time next week thinking about it on my side, and it would be good to involve @pm47 as well to make sure we get it right this time, and it will be easy to add the zero-fee commitment variants we want to introduce in the future as well.

t-bast · 2025-03-07T09:44:37Z

eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala

@@ -92,16 +98,25 @@ object Transactions {
   */
  case object ZeroFeeHtlcTxAnchorOutputsCommitmentFormat extends AnchorOutputsCommitmentFormat

+  case object SimpleTaprootChannelCommitmentFormat extends AnchorOutputsCommitmentFormat {


This shouldn't inherit AnchorOutputsCommitmentFormat, this should just inherit CommitmentFormat so that we have the opportunity to treat it differently than anchor outputs everywhere we pattern match on the commitmentFormat. It's dangerous to inherit AnchorOutputsCommitmentFormat because we may miss some code paths and inherit behavior that we don't want.

We shouldn't add useTaproot: Boolean in CommitmentFormat either: when we have multiple commitment formats that use taproot in the future, we will introduce a parent trait for it.

t-bast · 2025-03-07T09:52:11Z

eclair-core/src/main/scala/fr/acinq/eclair/blockchain/fee/OnChainFeeConf.scala

@@ -77,15 +77,15 @@ case class FeerateTolerance(ratioLow: Double, ratioHigh: Double, anchorOutputMax
  def isProposedFeerateTooHigh(commitmentFormat: CommitmentFormat, networkFeerate: FeeratePerKw, proposedFeerate: FeeratePerKw): Boolean = {
    commitmentFormat match {
      case Transactions.DefaultCommitmentFormat => networkFeerate * ratioHigh < proposedFeerate
-      case ZeroFeeHtlcTxAnchorOutputsCommitmentFormat | UnsafeLegacyAnchorOutputsCommitmentFormat => networkFeerate * ratioHigh < proposedFeerate
+      case ZeroFeeHtlcTxAnchorOutputsCommitmentFormat | UnsafeLegacyAnchorOutputsCommitmentFormat | SimpleTaprootChannelCommitmentFormat => networkFeerate * ratioHigh < proposedFeerate


According to https://coin.dance/nodes#nodeVersions, 1/3 of the bitcoin network already uses bitcoind 28.x, which means they support 1-parent-1-child package relay even when the parent transaction is below the mempool feerate. By the time we ship taproot, wouldn't it be a good opportunity to always set the feerate at 1 sat/byte for those channels and disable update_fee entirely?

t-bast · 2025-03-07T09:55:19Z

eclair-core/src/main/scala/fr/acinq/eclair/transactions/Scripts.scala

@@ -223,7 +223,8 @@ object Scripts {

  /** Extract the payment preimage from a 2nd-stage HTLC Success transaction's witness script */
  def extractPreimageFromHtlcSuccess: PartialFunction[ScriptWitness, ByteVector32] = {
-    case ScriptWitness(Seq(ByteVector.empty, _, _, paymentPreimage, _)) if paymentPreimage.size == 32 => ByteVector32(paymentPreimage)
+    case ScriptWitness(Seq(ByteVector.empty, _, _, paymentPreimage, _)) if paymentPreimage.size == 32 => ByteVector32(paymentPreimage) // standard channels
+    case ScriptWitness(Seq(remoteSig, localSig, paymentPreimage, _, _)) if remoteSig.size == 65 && localSig.size == 65 && paymentPreimage.size == 32 => ByteVector32(paymentPreimage) // simple taproot channels


Shouldn't we also update:

extractPreimageFromClaimHtlcSuccess

extractPaymentHashFromHtlcTimeout

extractPaymentHashFromClaimHtlcTimeout

We should also enrich HelpersSpec.scala to verify that this works (maybe we cannot do it in this PR since we don't yet support creating taproot channels, but it should be on the TODO-list for the next PRs).

t-bast · 2025-03-07T09:56:56Z

eclair-core/src/main/scala/fr/acinq/eclair/transactions/Scripts.scala

@@ -307,6 +308,8 @@ object Scripts {

    implicit def scala2kmpscript(input: Seq[fr.acinq.bitcoin.scalacompat.ScriptElt]): java.util.List[fr.acinq.bitcoin.ScriptElt] = input.map(e => scala2kmp(e)).asJava

+    def musig2FundingScript(pubkey1: PublicKey, pubkey2: PublicKey): Seq[ScriptElt] = Script.pay2tr(musig2Aggregate(pubkey1, pubkey2), None)


What about adding a mandatory useTaproot: Boolean parameter to the existing multiSig2of2 function instead? That guarantees that we correctly update all the code paths that use it?

t-bast · 2025-03-07T10:06:55Z

eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala

  // @formatter:off
  case class OutputInfo(index: Long, amount: Satoshi, publicKeyScript: ByteVector)

  sealed trait InputInfo {
    val outPoint: OutPoint
    val txOut: TxOut
+    val isP2tr: Boolean = Try(Script.isPay2tr(Script.parse(txOut.publicKeyScript))).getOrElse(false)


We shouldn't need this, we can rely on whether this is a TaprootInput instead?

t-bast · 2025-03-07T10:39:41Z

eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala

@@ -129,7 +144,7 @@ object Transactions {
      Satoshi(FeeratePerKw.MinimumRelayFeeRate * vsize / 1000)
    }
    /** Sighash flags to use when signing the transaction. */
-    def sighash(txOwner: TxOwner, commitmentFormat: CommitmentFormat): Int = SIGHASH_ALL
+    def sighash(txOwner: TxOwner, commitmentFormat: CommitmentFormat): Int = if (input.isP2tr) SIGHASH_DEFAULT else SIGHASH_ALL


Instead of relying on input.isP2tr (which we should remove) we should pattern match in InputInfo and use SIGHASH_DEFAULT in the TaprootInput case?

t-bast · 2025-03-07T10:42:35Z

eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala

+        ).getRight
+      session.verify(psig.partialSig, psig.nonce, remotePubKey)


This isn't safe since getRight returns a nullable, which breaks our usual Scala assumptions that we never have risks of NullPointerException, can you map or cast to an Option to make this safe?

t-bast · 2025-03-07T10:54:43Z

eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala

+    override def sign(privateKey:  PrivateKey, txOwner:  TxOwner, commitmentFormat:  CommitmentFormat): ByteVector64 = input match {
+      case t: InputInfo.TaprootInput =>
+        val Some(scriptTree: ScriptTree.Branch) = t.scriptTree_opt
+        Transaction.signInputTaprootScriptPath(privateKey, tx, 0, Seq(input.txOut), SigHash.SIGHASH_SINGLE | SigHash.SIGHASH_ANYONECANPAY, KotlinUtils.kmp2scala(scriptTree.getRight.hash()))


You shouldn't use SIGHASH_SINGLE | SIGHASH_ANYONECANPAY regardless of the txOwner: you must use sighash(txOwner, commitmentFormat), if we don't provide a SIGHASH_ALL for our local sig we're creating a big vulnerability! Same everywhere else.

t-bast · 2025-03-07T10:59:25Z

eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala

+    override def desc: String = "htlc-success"
+
+    override def sign(privateKey:  PrivateKey, txOwner:  TxOwner, commitmentFormat:  CommitmentFormat): ByteVector64 = input match {
+      case t: InputInfo.TaprootInput =>


We must provide an implementation of sign whenever the input is using taproot. This isn't correctly reflected in the function hierarchy: in the default sign function from TransactionWithInputInfo we have a case _: InputInfo.TaprootInput => ByteVector64.Zeroes. We shouldn't have this case, it's a red flag that the architecture is wrong.

Maybe a solution is to remove the default implementation of def sign(key: PrivateKey, sighashType: Int): ByteVector64 from TransactionWithInputInfo to force implementation. And the current SegwitInput can be provided as a helper function (to ensure we don't duplicate the same code everywhere).

t-bast · 2025-03-07T11:00:45Z

eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala

+    // these nonces are generated on the fly at during a "simple" closing session and can be forgotten once the session ends
+    // @volatile var localNonce_opt: Option[(SecretNonce, IndividualNonce)] = None


What's that? That definitely shouldn't be how we handle this...the nonces will be in the NEGOTIATING_SIMPLE state or in a var in Channel.scala, but I don't think it should be here with a volatile...

Add high-level helpers for taproot channels

4113a98

We add a new specific commitment format for taproot channels, and high-level methods for creating and spending taproot channel transactions.

sstone requested a review from t-bast March 5, 2025 11:18

t-bast reviewed Mar 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add high-level helpers for taproot channels #3028

Add high-level helpers for taproot channels #3028

sstone commented Mar 5, 2025

t-bast left a comment

t-bast Mar 7, 2025

t-bast Mar 7, 2025

t-bast Mar 7, 2025

t-bast Mar 7, 2025

t-bast Mar 7, 2025

t-bast Mar 7, 2025

t-bast Mar 7, 2025

t-bast Mar 7, 2025

t-bast Mar 7, 2025

t-bast Mar 7, 2025

		@@ -307,6 +308,8 @@ object Scripts {

		implicit def scala2kmpscript(input: Seq[fr.acinq.bitcoin.scalacompat.ScriptElt]): java.util.List[fr.acinq.bitcoin.ScriptElt] = input.map(e => scala2kmp(e)).asJava

		def musig2FundingScript(pubkey1: PublicKey, pubkey2: PublicKey): Seq[ScriptElt] = Script.pay2tr(musig2Aggregate(pubkey1, pubkey2), None)

		).getRight
		session.verify(psig.partialSig, psig.nonce, remotePubKey)

		// these nonces are generated on the fly at during a "simple" closing session and can be forgotten once the session ends
		// @volatile var localNonce_opt: Option[(SecretNonce, IndividualNonce)] = None

Add high-level helpers for taproot channels #3028

Are you sure you want to change the base?

Add high-level helpers for taproot channels #3028

Conversation

sstone commented Mar 5, 2025

t-bast left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment