Skip to content

Commit 76eb6cb

Browse files
authored
Move addSigs into each dedicated class (#3069)
Instead of having multiple overloads for `addSigs`, we move each one of them inside the corresponding `TransactionWithInputInfo`. This is part of an effort to group the logic belonging to each type of transaction in the same place to make it easier to review and use. This is a purely mechanical refactoring, where code has simply been cut and pasted. The only functional change is for `HtlcPenaltyTx`, where we incorrectly used the witness for a `MainPenaltyTx`, which was actually smaller. We were thus under-estimating the fees we had to pay to reach the desired feerate, which isn't much of an issue, but it's better to use the correct witness for consistency. We also remove the unused `minRelayFee` field.
1 parent a626281 commit 76eb6cb

File tree

12 files changed

+295
-254
lines changed

12 files changed

+295
-254
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -666,7 +666,7 @@ case class Commitment(fundingTxIndex: Long,
666666
val fundingKey = channelKeys.fundingKey(fundingTxIndex)
667667
val localSig = unsignedCommitTx.sign(fundingKey, TxOwner.Local, params.commitmentFormat, Map.empty)
668668
val RemoteSignature.FullSignature(remoteSig) = localCommit.commitTxAndRemoteSig.remoteSig
669-
val commitTx = addSigs(unsignedCommitTx, fundingKey.publicKey, remoteFundingPubKey, localSig, remoteSig)
669+
val commitTx = unsignedCommitTx.addSigs(fundingKey.publicKey, remoteFundingPubKey, localSig, remoteSig)
670670
// We verify the remote signature when receiving their commit_sig, so this check should always pass.
671671
require(checkSpendable(commitTx).isSuccess, "commit signatures are invalid")
672672
commitTx

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

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -660,7 +660,7 @@ object Helpers {
660660
def firstClosingFee(commitment: FullCommitment, localScriptPubkey: ByteVector, remoteScriptPubkey: ByteVector, feerates: ClosingFeerates)(implicit log: LoggingAdapter): ClosingFees = {
661661
// this is just to estimate the weight, it depends on size of the pubkey scripts
662662
val dummyClosingTx = Transactions.makeClosingTx(commitment.commitInput, localScriptPubkey, remoteScriptPubkey, commitment.localParams.paysClosingFees, Satoshi(0), Satoshi(0), commitment.localCommit.spec)
663-
val closingWeight = Transaction.weight(Transactions.addSigs(dummyClosingTx, Transactions.PlaceHolderPubKey, commitment.remoteFundingPubKey, Transactions.PlaceHolderSig, Transactions.PlaceHolderSig).tx)
663+
val closingWeight = Transaction.weight(dummyClosingTx.addSigs(Transactions.PlaceHolderPubKey, commitment.remoteFundingPubKey, Transactions.PlaceHolderSig, Transactions.PlaceHolderSig).tx)
664664
log.info(s"using feerates=$feerates for initial closing tx")
665665
feerates.computeFees(closingWeight)
666666
}
@@ -703,7 +703,7 @@ object Helpers {
703703
def checkClosingSignature(channelKeys: ChannelKeys, commitment: FullCommitment, localScriptPubkey: ByteVector, remoteScriptPubkey: ByteVector, remoteClosingFee: Satoshi, remoteClosingSig: ByteVector64)(implicit log: LoggingAdapter): Either[ChannelException, (ClosingTx, ClosingSigned)] = {
704704
val (closingTx, closingSigned) = makeClosingTx(channelKeys, commitment, localScriptPubkey, remoteScriptPubkey, ClosingFees(remoteClosingFee, remoteClosingFee, remoteClosingFee))
705705
if (checkClosingDustAmounts(closingTx)) {
706-
val signedClosingTx = Transactions.addSigs(closingTx, channelKeys.fundingKey(commitment.fundingTxIndex).publicKey, commitment.remoteFundingPubKey, closingSigned.signature, remoteClosingSig)
706+
val signedClosingTx = closingTx.addSigs(channelKeys.fundingKey(commitment.fundingTxIndex).publicKey, commitment.remoteFundingPubKey, closingSigned.signature, remoteClosingSig)
707707
Transactions.checkSpendable(signedClosingTx) match {
708708
case Success(_) => Right(signedClosingTx, closingSigned)
709709
case _ => Left(InvalidCloseSignature(commitment.channelId, signedClosingTx.tx.txid))
@@ -720,7 +720,7 @@ object Helpers {
720720
val dummyClosingTxs = Transactions.makeSimpleClosingTxs(commitment.commitInput, commitment.localCommit.spec, SimpleClosingTxFee.PaidByUs(0 sat), currentBlockHeight.toLong, localScriptPubkey, remoteScriptPubkey)
721721
dummyClosingTxs.preferred_opt match {
722722
case Some(dummyTx) =>
723-
val dummySignedTx = Transactions.addSigs(dummyTx, Transactions.PlaceHolderPubKey, Transactions.PlaceHolderPubKey, Transactions.PlaceHolderSig, Transactions.PlaceHolderSig)
723+
val dummySignedTx = dummyTx.addSigs(Transactions.PlaceHolderPubKey, Transactions.PlaceHolderPubKey, Transactions.PlaceHolderSig, Transactions.PlaceHolderSig)
724724
SimpleClosingTxFee.PaidByUs(Transactions.weight2fee(feerate, dummySignedTx.tx.weight()))
725725
case None => return Left(CannotGenerateClosingTx(commitment.channelId))
726726
}
@@ -767,7 +767,7 @@ object Helpers {
767767
case Some((closingTx, remoteSig, sigToTlv)) =>
768768
val localFundingKey = channelKeys.fundingKey(commitment.fundingTxIndex)
769769
val localSig = closingTx.sign(localFundingKey, TxOwner.Local, commitment.params.commitmentFormat, Map.empty)
770-
val signedClosingTx = Transactions.addSigs(closingTx, localFundingKey.publicKey, commitment.remoteFundingPubKey, localSig, remoteSig)
770+
val signedClosingTx = closingTx.addSigs(localFundingKey.publicKey, commitment.remoteFundingPubKey, localSig, remoteSig)
771771
Transactions.checkSpendable(signedClosingTx) match {
772772
case Failure(_) => Left(InvalidCloseSignature(commitment.channelId, signedClosingTx.tx.txid))
773773
case Success(_) => Right(signedClosingTx, ClosingSig(commitment.channelId, remoteScriptPubkey, localScriptPubkey, closingComplete.fees, closingComplete.lockTime, TlvStream(sigToTlv(localSig))))
@@ -793,7 +793,7 @@ object Helpers {
793793
case Some((closingTx, remoteSig)) =>
794794
val localFundingKey = channelKeys.fundingKey(commitment.fundingTxIndex)
795795
val localSig = closingTx.sign(localFundingKey, TxOwner.Local, commitment.params.commitmentFormat, Map.empty)
796-
val signedClosingTx = Transactions.addSigs(closingTx, localFundingKey.publicKey, commitment.remoteFundingPubKey, localSig, remoteSig)
796+
val signedClosingTx = closingTx.addSigs(localFundingKey.publicKey, commitment.remoteFundingPubKey, localSig, remoteSig)
797797
Transactions.checkSpendable(signedClosingTx) match {
798798
case Failure(_) => Left(InvalidCloseSignature(commitment.channelId, signedClosingTx.tx.txid))
799799
case Success(_) => Right(signedClosingTx)
@@ -874,7 +874,7 @@ object Helpers {
874874
val mainDelayedTx = withTxGenerationLog("local-main-delayed") {
875875
Transactions.makeClaimLocalDelayedOutputTx(commitmentKeys, commitTx, commitment.localParams.dustLimit, commitment.remoteParams.toSelfDelay, finalScriptPubKey, feeratePerKwDelayed).map(claimDelayed => {
876876
val sig = claimDelayed.sign(commitmentKeys.ourDelayedPaymentKey, TxOwner.Local, commitment.params.commitmentFormat, Map.empty)
877-
Transactions.addSigs(claimDelayed, sig)
877+
claimDelayed.addSigs(sig)
878878
})
879879
}
880880

@@ -932,7 +932,7 @@ object Helpers {
932932
// We immediately spend incoming htlcs for which we have the preimage.
933933
Some(txInfo.input.outPoint -> withTxGenerationLog("htlc-success") {
934934
val localSig = txInfo.sign(commitKeys.ourHtlcKey, TxOwner.Local, commitment.params.commitmentFormat, Map.empty)
935-
Right(Transactions.addSigs(txInfo, localSig, remoteSig, hash2Preimage(paymentHash), commitment.params.commitmentFormat))
935+
Right(txInfo.addSigs(localSig, remoteSig, hash2Preimage(paymentHash), commitment.params.commitmentFormat))
936936
})
937937
} else if (failedIncomingHtlcs.contains(txInfo.htlcId)) {
938938
// We can ignore incoming htlcs that we started failing: our peer will claim them after the timeout.
@@ -953,7 +953,7 @@ object Helpers {
953953
// back after the timeout.
954954
Some(txInfo.input.outPoint -> withTxGenerationLog("htlc-timeout") {
955955
val localSig = txInfo.sign(commitKeys.ourHtlcKey, TxOwner.Local, commitment.params.commitmentFormat, Map.empty)
956-
Right(Transactions.addSigs(txInfo, localSig, remoteSig, commitment.params.commitmentFormat))
956+
Right(txInfo.addSigs(localSig, remoteSig, commitment.params.commitmentFormat))
957957
})
958958
}.flatten.toMap
959959
}
@@ -973,7 +973,7 @@ object Helpers {
973973
// if our peer was able to claim the HTLC output before us (race condition between success and timeout).
974974
Transactions.makeHtlcDelayedTx(commitKeys, tx, commitment.localParams.dustLimit, commitment.remoteParams.toSelfDelay, finalScriptPubKey, feeratePerKwDelayed).map(claimDelayed => {
975975
val sig = claimDelayed.sign(commitKeys.ourDelayedPaymentKey, TxOwner.Local, commitment.params.commitmentFormat, Map.empty)
976-
Transactions.addSigs(claimDelayed, sig)
976+
claimDelayed.addSigs(sig)
977977
})
978978
}
979979
val localCommitPublished1 = localCommitPublished.copy(claimHtlcDelayedTxs = localCommitPublished.claimHtlcDelayedTxs ++ htlcDelayedTx_opt.toSeq)
@@ -1048,13 +1048,13 @@ object Helpers {
10481048
case DefaultCommitmentFormat => withTxGenerationLog("remote-main") {
10491049
Transactions.makeClaimP2WPKHOutputTx(commitKeys, commitTx, params.localParams.dustLimit, finalScriptPubKey, feeratePerKwMain).map(claimMain => {
10501050
val sig = claimMain.sign(ourPaymentKey, TxOwner.Local, params.commitmentFormat, Map.empty)
1051-
Transactions.addSigs(claimMain, commitKeys, sig)
1051+
claimMain.addSigs(commitKeys, sig)
10521052
})
10531053
}
10541054
case _: AnchorOutputsCommitmentFormat => withTxGenerationLog("remote-main-delayed") {
10551055
Transactions.makeClaimRemoteDelayedOutputTx(commitKeys, commitTx, params.localParams.dustLimit, finalScriptPubKey, feeratePerKwMain).map(claimMain => {
10561056
val sig = claimMain.sign(ourPaymentKey, TxOwner.Local, params.commitmentFormat, Map.empty)
1057-
Transactions.addSigs(claimMain, sig)
1057+
claimMain.addSigs(sig)
10581058
})
10591059
}
10601060
}
@@ -1094,7 +1094,7 @@ object Helpers {
10941094
// We immediately spend incoming htlcs for which we have the preimage.
10951095
Some(claimHtlcTx.input.outPoint -> withTxGenerationLog("claim-htlc-success") {
10961096
val sig = claimHtlcTx.sign(commitKeys.ourHtlcKey, TxOwner.Local, commitment.params.commitmentFormat, Map.empty)
1097-
Right(Transactions.addSigs(claimHtlcTx, sig, hash2Preimage(add.paymentHash)))
1097+
Right(claimHtlcTx.addSigs(sig, hash2Preimage(add.paymentHash)))
10981098
})
10991099
} else if (failedIncomingHtlcs.contains(add.id)) {
11001100
// We can ignore incoming htlcs that we started failing: our peer will claim them after the timeout.
@@ -1120,7 +1120,7 @@ object Helpers {
11201120
}.map(claimHtlcTx => {
11211121
Some(claimHtlcTx.input.outPoint -> withTxGenerationLog("claim-htlc-timeout") {
11221122
val sig = claimHtlcTx.sign(commitKeys.ourHtlcKey, TxOwner.Local, commitment.params.commitmentFormat, Map.empty)
1123-
Right(Transactions.addSigs(claimHtlcTx, sig))
1123+
Right(claimHtlcTx.addSigs(sig))
11241124
})
11251125
})
11261126
}.toSeq.flatten.flatten.toMap
@@ -1182,13 +1182,13 @@ object Helpers {
11821182
case DefaultCommitmentFormat => withTxGenerationLog("remote-main") {
11831183
Transactions.makeClaimP2WPKHOutputTx(commitKeys, commitTx, localParams.dustLimit, finalScriptPubKey, feerateMain).map(claimMain => {
11841184
val sig = claimMain.sign(ourPaymentKey, TxOwner.Local, commitmentFormat, Map.empty)
1185-
Transactions.addSigs(claimMain, commitKeys, sig)
1185+
claimMain.addSigs(commitKeys, sig)
11861186
})
11871187
}
11881188
case _: AnchorOutputsCommitmentFormat => withTxGenerationLog("remote-main-delayed") {
11891189
Transactions.makeClaimRemoteDelayedOutputTx(commitKeys, commitTx, localParams.dustLimit, finalScriptPubKey, feerateMain).map(claimMain => {
11901190
val sig = claimMain.sign(ourPaymentKey, TxOwner.Local, commitmentFormat, Map.empty)
1191-
Transactions.addSigs(claimMain, sig)
1191+
claimMain.addSigs(sig)
11921192
})
11931193
}
11941194
}
@@ -1198,7 +1198,7 @@ object Helpers {
11981198
val mainPenaltyTx = withTxGenerationLog("main-penalty") {
11991199
Transactions.makeMainPenaltyTx(commitKeys, commitTx, localParams.dustLimit, finalScriptPubKey, localParams.toSelfDelay, feeratePenalty).map(txinfo => {
12001200
val sig = txinfo.sign(revocationKey, TxOwner.Local, commitmentFormat, Map.empty)
1201-
Transactions.addSigs(txinfo, sig)
1201+
txinfo.addSigs(sig)
12021202
})
12031203
}
12041204

@@ -1216,9 +1216,9 @@ object Helpers {
12161216
val htlcPenaltyTxs = commitTx.txOut.zipWithIndex.collect { case (txOut, outputIndex) if htlcsRedeemScripts.contains(txOut.publicKeyScript) =>
12171217
val htlcRedeemScript = htlcsRedeemScripts(txOut.publicKeyScript)
12181218
withTxGenerationLog("htlc-penalty") {
1219-
Transactions.makeHtlcPenaltyTx(commitTx, outputIndex, htlcRedeemScript, localParams.dustLimit, finalScriptPubKey, feeratePenalty).map(htlcPenalty => {
1219+
Transactions.makeHtlcPenaltyTx(commitKeys, commitTx, outputIndex, htlcRedeemScript, localParams.dustLimit, finalScriptPubKey, feeratePenalty).map(htlcPenalty => {
12201220
val sig = htlcPenalty.sign(revocationKey, TxOwner.Local, commitmentFormat, Map.empty)
1221-
Transactions.addSigs(htlcPenalty, commitKeys, sig)
1221+
htlcPenalty.addSigs(commitKeys, sig)
12221222
})
12231223
}
12241224
}.toList.flatten
@@ -1264,7 +1264,7 @@ object Helpers {
12641264
withTxGenerationLog("htlc-delayed-penalty") {
12651265
claimHtlcDelayedOutputPenaltyTx.map(htlcDelayedPenalty => {
12661266
val sig = htlcDelayedPenalty.sign(revocationKey, TxOwner.Local, params.commitmentFormat, Map.empty)
1267-
val signedTx = Transactions.addSigs(htlcDelayedPenalty, sig)
1267+
val signedTx = htlcDelayedPenalty.addSigs(sig)
12681268
// We need to make sure that the tx is indeed valid.
12691269
Transaction.correctlySpends(signedTx.tx, Seq(htlcTx), ScriptFlags.STANDARD_SCRIPT_VERIFY_FLAGS)
12701270
log.warning("txId={} is a 2nd level htlc tx spending revoked commit txId={}: publishing htlc-penalty txId={}", htlcTx.txid, revokedCommitPublished.commitTx.txid, signedTx.tx.txid)

eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenSingleFunded.scala

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@ trait ChannelOpenSingleFunded extends SingleFundingHandlers with ErrorHandlers {
269269
case Right((localSpec, localCommitTx, remoteSpec, remoteCommitTx)) =>
270270
// check remote signature validity
271271
val localSigOfLocalTx = localCommitTx.sign(fundingKey, TxOwner.Local, params.commitmentFormat, Map.empty)
272-
val signedLocalCommitTx = Transactions.addSigs(localCommitTx, fundingKey.publicKey, remoteFundingPubKey, localSigOfLocalTx, remoteSig)
272+
val signedLocalCommitTx = localCommitTx.addSigs(fundingKey.publicKey, remoteFundingPubKey, localSigOfLocalTx, remoteSig)
273273
Transactions.checkSpendable(signedLocalCommitTx) match {
274274
case Failure(_) => handleLocalError(InvalidCommitmentSignature(temporaryChannelId, fundingTxId, commitmentNumber = 0, localCommitTx.tx), d, None)
275275
case Success(_) =>
@@ -318,7 +318,7 @@ trait ChannelOpenSingleFunded extends SingleFundingHandlers with ErrorHandlers {
318318
// we make sure that their sig checks out and that our first commit tx is spendable
319319
val fundingKey = channelKeys.fundingKey(fundingTxIndex = 0)
320320
val localSigOfLocalTx = localCommitTx.sign(fundingKey, TxOwner.Local, params.commitmentFormat, Map.empty)
321-
val signedLocalCommitTx = Transactions.addSigs(localCommitTx, fundingKey.publicKey, remoteFundingPubKey, localSigOfLocalTx, remoteSig)
321+
val signedLocalCommitTx = localCommitTx.addSigs(fundingKey.publicKey, remoteFundingPubKey, localSigOfLocalTx, remoteSig)
322322
Transactions.checkSpendable(signedLocalCommitTx) match {
323323
case Failure(cause) =>
324324
// we rollback the funding tx, it will never be published

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxFunder.scala

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -124,8 +124,8 @@ object ReplaceableTxFunder {
124124
require(claimHtlcTx.txInfo.tx.txIn.size == 1, "claim-htlc transaction should have a single input")
125125
require(claimHtlcTx.txInfo.tx.txOut.size == 1, "claim-htlc transaction should have a single output")
126126
val dummySignedTx = claimHtlcTx.txInfo match {
127-
case tx: ClaimHtlcSuccessTx => addSigs(tx, PlaceHolderSig, ByteVector32.Zeroes)
128-
case tx: ClaimHtlcTimeoutTx => addSigs(tx, PlaceHolderSig)
127+
case tx: ClaimHtlcSuccessTx => tx.addSigs(PlaceHolderSig, ByteVector32.Zeroes)
128+
case tx: ClaimHtlcTimeoutTx => tx.addSigs(PlaceHolderSig)
129129
case tx: LegacyClaimHtlcSuccessTx => tx
130130
}
131131
val targetFee = weight2fee(targetFeerate, dummySignedTx.tx.weight())
@@ -312,14 +312,14 @@ private class ReplaceableTxFunder(replyTo: ActorRef[ReplaceableTxFunder.FundingR
312312
case claimAnchorTx: ClaimAnchorWithWitnessData =>
313313
val fundingKey = cmd.channelKeys.fundingKey(cmd.commitment.fundingTxIndex)
314314
val localSig = claimAnchorTx.txInfo.sign(fundingKey, TxOwner.Local, cmd.commitment.params.commitmentFormat, walletUtxos)
315-
val signedTx = claimAnchorTx.copy(txInfo = addSigs(claimAnchorTx.txInfo, localSig))
315+
val signedTx = claimAnchorTx.copy(txInfo = claimAnchorTx.txInfo.addSigs(localSig))
316316
signWalletInputs(signedTx, txFeerate, amountIn, walletUtxos)
317317
case htlcTx: HtlcWithWitnessData =>
318318
val commitmentKeys = cmd.commitment.localKeys(cmd.channelKeys)
319319
val localSig = htlcTx.txInfo.sign(commitmentKeys.ourHtlcKey, TxOwner.Local, cmd.commitment.params.commitmentFormat, walletUtxos)
320320
val signedTx = htlcTx match {
321-
case htlcSuccess: HtlcSuccessWithWitnessData => htlcSuccess.copy(txInfo = addSigs(htlcSuccess.txInfo, localSig, htlcSuccess.remoteSig, htlcSuccess.preimage, cmd.commitment.params.commitmentFormat))
322-
case htlcTimeout: HtlcTimeoutWithWitnessData => htlcTimeout.copy(txInfo = addSigs(htlcTimeout.txInfo, localSig, htlcTimeout.remoteSig, cmd.commitment.params.commitmentFormat))
321+
case htlcSuccess: HtlcSuccessWithWitnessData => htlcSuccess.copy(txInfo = htlcSuccess.txInfo.addSigs(localSig, htlcSuccess.remoteSig, htlcSuccess.preimage, cmd.commitment.params.commitmentFormat))
322+
case htlcTimeout: HtlcTimeoutWithWitnessData => htlcTimeout.copy(txInfo = htlcTimeout.txInfo.addSigs(localSig, htlcTimeout.remoteSig, cmd.commitment.params.commitmentFormat))
323323
}
324324
val hasWalletInputs = htlcTx.txInfo.tx.txIn.size > 1
325325
if (hasWalletInputs) {
@@ -336,9 +336,9 @@ private class ReplaceableTxFunder(replyTo: ActorRef[ReplaceableTxFunder.FundingR
336336
val commitmentKeys = cmd.commitment.remoteKeys(cmd.channelKeys, remotePerCommitmentPoint)
337337
val sig = claimHtlcTx.txInfo.sign(commitmentKeys.ourHtlcKey, TxOwner.Local, cmd.commitment.params.commitmentFormat, walletUtxos)
338338
val signedTx = claimHtlcTx match {
339-
case claimSuccess: ClaimHtlcSuccessWithWitnessData => claimSuccess.copy(txInfo = addSigs(claimSuccess.txInfo, sig, claimSuccess.preimage))
339+
case claimSuccess: ClaimHtlcSuccessWithWitnessData => claimSuccess.copy(txInfo = claimSuccess.txInfo.addSigs(sig, claimSuccess.preimage))
340340
case legacyClaimHtlcSuccess: LegacyClaimHtlcSuccessWithWitnessData => legacyClaimHtlcSuccess
341-
case claimTimeout: ClaimHtlcTimeoutWithWitnessData => claimTimeout.copy(txInfo = addSigs(claimTimeout.txInfo, sig))
341+
case claimTimeout: ClaimHtlcTimeoutWithWitnessData => claimTimeout.copy(txInfo = claimTimeout.txInfo.addSigs(sig))
342342
}
343343
replyTo ! TransactionReady(FundedTx(signedTx, amountIn, txFeerate, walletUtxos))
344344
Behaviors.stopped

0 commit comments

Comments
 (0)