Skip to content

Add metrics to interactive-tx funding #2997

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
pm47 wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add metrics to interactive-tx funding #2997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion eclair-core/src/main/scala/fr/acinq/eclair/channel/Monitoring.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

package fr.acinq.eclair.channel

import fr.acinq.bitcoin.scalacompat.SatoshiLong
import fr.acinq.bitcoin.scalacompat.{Satoshi, SatoshiLong}
import fr.acinq.eclair.channel.fund.InteractiveTxBuilder.{InteractiveTxParams, SharedTransaction}
import fr.acinq.eclair.transactions.{CommitmentSpec, DirectedHtlc}
import kamon.Kamon
Expand All @@ -33,6 +33,10 @@ object Monitoring {
val HtlcValueInFlightGlobal = Kamon.gauge("channels.htlc-value-in-flight-global", "Global HTLC value in flight across all channels")
val LocalFeeratePerByte = Kamon.histogram("channels.local-feerate-per-byte")
val RemoteFeeratePerByte = Kamon.histogram("channels.remote-feerate-per-byte")
val InteractiveTxFundingTargetAmount = Kamon.histogram("channels.interactive-tx-funding.target-amount", "Interactive tx funding target amount")
val InteractiveTxFundingInputsCount = Kamon.histogram("channels.interactive-tx-funding.inputs-count", "Interactive tx funding inputs count")
val InteractiveTxFundingHasChange = Kamon.histogram("channels.interactive-tx-funding.change", "Interactive tx funding change")
val InteractiveTxMiningFeeDiff = Kamon.histogram("channels.interactive-tx-funding.mining-fee", "Interactive tx mining fee diff")
val Splices = Kamon.histogram("channels.splices", "Splices")
val ProcessMessage = Kamon.timer("channels.messages-processed")

Expand All @@ -51,6 +55,24 @@ object Monitoring {
}
}

/**
* Record parameters and results for calls to bitcoind's `fundrawtransaction`.
*/
def recordInteractiveTxFunding(targetAmount: Satoshi, inputsCount: Int, change: Satoshi): Unit = {
Metrics.InteractiveTxFundingTargetAmount.withoutTags().record(targetAmount.toLong)
Metrics.InteractiveTxFundingInputsCount.withoutTags().record(inputsCount)
Metrics.InteractiveTxFundingHasChange.withoutTags().record(change.toLong)
}

/**
* Record actual vs estimated mining fee
*/
def recordInteractiveTxMiningFeeDiff(userMiningFee: Satoshi, actualMiningFee: Satoshi): Unit = {
// If actual mining fee is greater than what the user paid, we lose money
val diff = userMiningFee - actualMiningFee
Metrics.InteractiveTxMiningFeeDiff.withTag(Tags.DiffSign, if (diff > 0.sat) Tags.DiffSigns.plus else Tags.DiffSigns.minus).record(Math.abs(diff.toLong))
}
Comment on lines +67 to +74
Copy link
Member

@t-bast t-bast Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is confusing: it's very specific to liquidity ads but without knowing this beforehand, it's really hard to figure out from the code. More generally, I think we should also add liquidity ads metrics in this PR:

  • amount purchased
  • service fee paid/received
  • mining fee refunded

That's where it would make sense to have this metric, which should be renamed LiquidityAdsMiningFeeRefund. The userMiningFee parameter from this function should be renamed refundedMiningFee IMO.


/**
* This is best effort! It is not possible to attribute a type to a splice in all cases. For example, if remote provides
* both inputs and outputs, it could be a splice-in (with change), or a combined splice-in + splice-out.
Expand Down Expand Up @@ -84,7 +106,9 @@ object Monitoring {
val Origin = "origin"
val State = "state"
val CommitmentFormat = "commitment-format"
val Amount = "amount"
val SpliceType = "splice-type"
val DiffSign = "sign"

object Events {
val Created = "created"
Expand All @@ -107,6 +131,12 @@ object Monitoring {
val SpliceOut = "splice-out"
val SpliceCpfp = "splice-cpfp"
}

/** we can't chart negative amounts in Kamon */
object DiffSigns {
val plus = "plus"
val minus = "minus"
}
}

}
5 changes: 5 additions & 0 deletions eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import fr.acinq.eclair.blockchain.OnChainChannelFunder
import fr.acinq.eclair.blockchain.fee.FeeratePerKw
import fr.acinq.eclair.channel.Helpers.Closing.MutualClose
import fr.acinq.eclair.channel.Helpers.Funding
import fr.acinq.eclair.channel.Monitoring.Metrics
import fr.acinq.eclair.channel._
import fr.acinq.eclair.channel.fund.InteractiveTxBuilder.Output.Local
import fr.acinq.eclair.channel.fund.InteractiveTxBuilder.Purpose
Expand Down Expand Up @@ -884,6 +885,10 @@ private class InteractiveTxBuilder(replyTo: ActorRef[InteractiveTxBuilder.Respon
incomingHtlcCount = purpose.remoteNextHtlcId,
)
context.system.eventStream ! EventStream.Publish(ChannelLiquidityPurchased(replyTo.toClassic, channelParams.channelId, remoteNodeId, purchase))
// Liquidity requestor will pay the mining fee for the raw splice tx with a negative funding contribution, on top of that we add the mining fee for our inputs bring liquidity
val userMiningFee = -fundingParams.remoteContribution + p.fees.miningFee
log.info("funding fee check: serviceFee={} userMiningFee={} actualMiningFee={}", p.fees.serviceFee, userMiningFee, signedTx.tx.fees)
Metrics.recordInteractiveTxMiningFeeDiff(userMiningFee, signedTx.tx.fees)
Comment on lines +888 to +891
Copy link
Member

@t-bast t-bast Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's incorrect? We should compute this by doing signedTx.tx.localFees - p.fees.miningFee? Or maybe the end result is the same, but it's less clear what we're computing, since we don't really care about the mining fee they're paying from their channel balance, the only thing we care about is comparing how much mining fees we paid compared to how much they're refunding as part of the liquidity purchase?

}
val signingSession = InteractiveTxSigningSession.WaitingForSigs(
fundingParams,
Expand Down
8 changes: 7 additions & 1 deletion eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxFunder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import fr.acinq.bitcoin.scalacompat.Crypto.PublicKey
import fr.acinq.bitcoin.scalacompat.{KotlinUtils, OutPoint, Satoshi, SatoshiLong, Script, ScriptWitness, Transaction, TxIn, TxOut}
import fr.acinq.eclair.blockchain.OnChainChannelFunder
import fr.acinq.eclair.blockchain.fee.FeeratePerKw
import fr.acinq.eclair.channel.Monitoring.Metrics
import fr.acinq.eclair.channel.fund.InteractiveTxBuilder._
import fr.acinq.eclair.transactions.Transactions
import fr.acinq.eclair.wire.protocol.TxAddInput
Expand Down Expand Up @@ -239,7 +240,12 @@ private class InteractiveTxFunder(replyTo: ActorRef[InteractiveTxFunder.Response
}
context.pipeToSelf(wallet.fundTransaction(txNotFunded, fundingParams.targetFeerate, replaceable = true, externalInputsWeight = sharedInputWeight, feeBudget_opt = feeBudget_opt)) {
case Failure(t) => WalletFailure(t)
case Success(result) => FundTransactionResult(result.tx, result.changePosition)
case Success(result) =>
Metrics.recordInteractiveTxFunding(
targetAmount = fundingParams.localContribution,
inputsCount = result.tx.txIn.size - txNotFunded.txIn.size,
change = result.changePosition.map(i => result.tx.txOut(i).amount).sum)
Copy link
Member

@t-bast t-bast Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right place for this metric: since we have a mechanism to filter inputs below and retry calling bitcoind if it inserted inputs that cannot be used for dual funding, we may incorrectly increment the metric multiple times for the same funding attempt.

I think this should be done in filterInputs before returning the result to the caller, around those lines:

// We unlock the unusable inputs (if any) as they can be used outside of interactive-tx sessions.
sendResultAndStop(fundingContributions, unusableInputs.map(_.outpoint))

FundTransactionResult(result.tx, result.changePosition)
}
Behaviors.receiveMessagePartial {
case FundTransactionResult(fundedTx, changePosition) =>
Expand Down