Add metrics to interactive-tx funding #2997
Conversation
On funding amount/change, and on actual vs expected mining fee when selling liquidity.
| /** | ||
| * 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)) | ||
| } |
There was a problem hiding this comment.
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.
| // 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) |
There was a problem hiding this comment.
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?
| Metrics.recordInteractiveTxFunding( | ||
| targetAmount = fundingParams.localContribution, | ||
| inputsCount = result.tx.txIn.size - txNotFunded.txIn.size, | ||
| change = result.changePosition.map(i => result.tx.txOut(i).amount).sum) |
There was a problem hiding this comment.
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))
On funding amount/change, and on actual vs expected mining fee when selling liquidity.