-
Notifications
You must be signed in to change notification settings - Fork 268
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.
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
Endorse htlc and local reputation #2716
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,7 @@ import fr.acinq.eclair.payment.offer.OfferManager | |
import fr.acinq.eclair.payment.receive.PaymentHandler | ||
import fr.acinq.eclair.payment.relay.{AsyncPaymentTriggerer, PostRestartHtlcCleaner, Relayer} | ||
import fr.acinq.eclair.payment.send.{Autoprobe, PaymentInitiator} | ||
import fr.acinq.eclair.reputation.ReputationRecorder | ||
import fr.acinq.eclair.router._ | ||
import fr.acinq.eclair.tor.{Controller, TorProtocolHandler} | ||
import fr.acinq.eclair.wire.protocol.NodeAddress | ||
|
@@ -360,7 +361,8 @@ class Setup(val datadir: File, | |
offerManager = system.spawn(Behaviors.supervise(OfferManager(nodeParams, router, paymentTimeout = 1 minute)).onFailure(typed.SupervisorStrategy.resume), name = "offer-manager") | ||
paymentHandler = system.actorOf(SimpleSupervisor.props(PaymentHandler.props(nodeParams, register, offerManager), "payment-handler", SupervisorStrategy.Resume)) | ||
triggerer = system.spawn(Behaviors.supervise(AsyncPaymentTriggerer()).onFailure(typed.SupervisorStrategy.resume), name = "async-payment-triggerer") | ||
relayer = system.actorOf(SimpleSupervisor.props(Relayer.props(nodeParams, router, register, paymentHandler, triggerer, Some(postRestartCleanUpInitialized)), "relayer", SupervisorStrategy.Resume)) | ||
reputationRecorder = system.spawn(Behaviors.supervise(ReputationRecorder(nodeParams.relayParams.peerReputationConfig, Map.empty)).onFailure(typed.SupervisorStrategy.resume), name = "reputation-recorder") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We must make this optional and let node operator disable it entirely if they'd like to run the standard blip-0004 relay. I've done this in #2893. |
||
relayer = system.actorOf(SimpleSupervisor.props(Relayer.props(nodeParams, router, register, paymentHandler, triggerer, reputationRecorder, Some(postRestartCleanUpInitialized)), "relayer", SupervisorStrategy.Resume)) | ||
_ = relayer ! PostRestartHtlcCleaner.Init(channels) | ||
// Before initializing the switchboard (which re-connects us to the network) and the user-facing parts of the system, | ||
// we want to make sure the handler for post-restart broken HTLCs has finished initializing. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -429,7 +429,7 @@ case class Commitment(fundingTxIndex: Long, | |
localCommit.spec.htlcs.collect(DirectedHtlc.incoming).filter(nearlyExpired) | ||
} | ||
|
||
def canSendAdd(amount: MilliSatoshi, params: ChannelParams, changes: CommitmentChanges, feerates: FeeratesPerKw, feeConf: OnChainFeeConf): Either[ChannelException, Unit] = { | ||
def canSendAdd(amount: MilliSatoshi, params: ChannelParams, changes: CommitmentChanges, feerates: FeeratesPerKw, feeConf: OnChainFeeConf, confidence: Double): Either[ChannelException, Unit] = { | ||
// we allowed mismatches between our feerates and our remote's as long as commitments didn't contain any HTLC at risk | ||
// we need to verify that we're not disagreeing on feerates anymore before offering new HTLCs | ||
// NB: there may be a pending update_fee that hasn't been applied yet that needs to be taken into account | ||
|
@@ -488,7 +488,8 @@ case class Commitment(fundingTxIndex: Long, | |
if (allowedHtlcValueInFlight < htlcValueInFlight) { | ||
return Left(HtlcValueTooHighInFlight(params.channelId, maximum = allowedHtlcValueInFlight, actual = htlcValueInFlight)) | ||
} | ||
if (Seq(params.localParams.maxAcceptedHtlcs, params.remoteParams.maxAcceptedHtlcs).min < outgoingHtlcs.size) { | ||
val maxAcceptedHtlcs = params.localParams.maxAcceptedHtlcs.min(params.remoteParams.maxAcceptedHtlcs) | ||
if (maxAcceptedHtlcs < outgoingHtlcs.size) { | ||
return Left(TooManyAcceptedHtlcs(params.channelId, maximum = Seq(params.localParams.maxAcceptedHtlcs, params.remoteParams.maxAcceptedHtlcs).min)) | ||
} | ||
|
||
|
@@ -505,6 +506,18 @@ case class Commitment(fundingTxIndex: Long, | |
return Left(RemoteDustHtlcExposureTooHigh(params.channelId, maxDustExposure, remoteDustExposureAfterAdd)) | ||
} | ||
|
||
// Jamming protection | ||
// Must be the last checks so that they can be ignored for shadow deployment. | ||
for ((amountMsat, i) <- outgoingHtlcs.toSeq.map(_.amountMsat).sorted.zipWithIndex) { | ||
if ((amountMsat.toLong < 1) || (math.log(amountMsat.toLong.toDouble) * maxAcceptedHtlcs / math.log(params.localParams.maxHtlcValueInFlightMsat.toLong.toDouble / maxAcceptedHtlcs) < i)) { | ||
return Left(TooManySmallHtlcs(params.channelId, number = i + 1, below = amountMsat)) | ||
} | ||
} | ||
val occupancy = (outgoingHtlcs.size.toDouble / maxAcceptedHtlcs).max(htlcValueInFlight.toLong.toDouble / allowedHtlcValueInFlight.toLong.toDouble) | ||
if (confidence + 0.05 < occupancy) { | ||
return Left(ConfidenceTooLow(params.channelId, confidence, occupancy)) | ||
} | ||
|
||
Right(()) | ||
} | ||
|
||
|
@@ -552,6 +565,14 @@ case class Commitment(fundingTxIndex: Long, | |
return Left(TooManyAcceptedHtlcs(params.channelId, maximum = params.localParams.maxAcceptedHtlcs)) | ||
} | ||
|
||
// Jamming protection | ||
// Must be the last checks so that they can be ignored for shadow deployment. | ||
for ((amountMsat, i) <- incomingHtlcs.toSeq.map(_.amountMsat).sorted.zipWithIndex) { | ||
if ((amountMsat.toLong < 1) || (math.log(amountMsat.toLong.toDouble) * params.localParams.maxAcceptedHtlcs / math.log(params.localParams.maxHtlcValueInFlightMsat.toLong.toDouble / params.localParams.maxAcceptedHtlcs) < i)) { | ||
return Left(TooManySmallHtlcs(params.channelId, number = i + 1, below = amountMsat)) | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't have jamming protection in |
||
|
||
Right(()) | ||
} | ||
|
||
|
@@ -835,7 +856,7 @@ case class Commitments(params: ChannelParams, | |
* @param cmd add HTLC command | ||
* @return either Left(failure, error message) where failure is a failure message (see BOLT #4 and the Failure Message class) or Right(new commitments, updateAddHtlc) | ||
*/ | ||
def sendAdd(cmd: CMD_ADD_HTLC, currentHeight: BlockHeight, channelConf: ChannelConf, feerates: FeeratesPerKw, feeConf: OnChainFeeConf): Either[ChannelException, (Commitments, UpdateAddHtlc)] = { | ||
def sendAdd(cmd: CMD_ADD_HTLC, currentHeight: BlockHeight, channelConf: ChannelConf, feerates: FeeratesPerKw, feeConf: OnChainFeeConf)(implicit log: LoggingAdapter): Either[ChannelException, (Commitments, UpdateAddHtlc)] = { | ||
// we must ensure we're not relaying htlcs that are already expired, otherwise the downstream channel will instantly close | ||
// NB: we add a 3 blocks safety to reduce the probability of running into this when our bitcoin node is slightly outdated | ||
val minExpiry = CltvExpiry(currentHeight + 3) | ||
|
@@ -859,12 +880,28 @@ case class Commitments(params: ChannelParams, | |
val changes1 = changes.addLocalProposal(add).copy(localNextHtlcId = changes.localNextHtlcId + 1) | ||
val originChannels1 = originChannels + (add.id -> cmd.origin) | ||
// we verify that this htlc is allowed in every active commitment | ||
active.map(_.canSendAdd(add.amountMsat, params, changes1, feerates, feeConf)) | ||
.collectFirst { case Left(f) => Left(f) } | ||
val canSendAdds = active.map(_.canSendAdd(add.amountMsat, params, changes1, feerates, feeConf, cmd.confidence)) | ||
// Log only for jamming protection. | ||
canSendAdds.collectFirst { | ||
case Left(f: TooManySmallHtlcs) => | ||
log.info("TooManySmallHtlcs: {} outgoing HTLCs are below {}}", f.number, f.below) | ||
Metrics.dropHtlc(f, Tags.Directions.Outgoing) | ||
case Left(f: ConfidenceTooLow) => | ||
log.info("ConfidenceTooLow: confidence is {}% while channel is {}% full", (100 * f.confidence).toInt, (100 * f.occupancy).toInt) | ||
Metrics.dropHtlc(f, Tags.Directions.Outgoing) | ||
} | ||
canSendAdds.flatMap { // TODO: We ignore jamming protection, delete this flatMap to activate jamming protection. | ||
case Left(_: TooManySmallHtlcs) | Left(_: ConfidenceTooLow) => None | ||
case x => Some(x) | ||
} | ||
.collectFirst { case Left(f) => | ||
Metrics.dropHtlc(f, Tags.Directions.Outgoing) | ||
Left(f) | ||
} | ||
Comment on lines
+875
to
+892
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems overly complex: can't you keep This will duplicate the log/metric when we have pending splices, but pending splices is something that won't happen frequently enough to skew the data? Or even better, since HTLCs are the same in all active commitments, you can remove the changes to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've done this in #2898 however I don't like because it just duplicates a lot of |
||
.getOrElse(Right(copy(changes = changes1, originChannels = originChannels1), add)) | ||
} | ||
|
||
def receiveAdd(add: UpdateAddHtlc, feerates: FeeratesPerKw, feeConf: OnChainFeeConf): Either[ChannelException, Commitments] = { | ||
def receiveAdd(add: UpdateAddHtlc, feerates: FeeratesPerKw, feeConf: OnChainFeeConf)(implicit log: LoggingAdapter): Either[ChannelException, Commitments] = { | ||
if (add.id != changes.remoteNextHtlcId) { | ||
return Left(UnexpectedHtlcId(channelId, expected = changes.remoteNextHtlcId, actual = add.id)) | ||
} | ||
|
@@ -877,8 +914,21 @@ case class Commitments(params: ChannelParams, | |
|
||
val changes1 = changes.addRemoteProposal(add).copy(remoteNextHtlcId = changes.remoteNextHtlcId + 1) | ||
// we verify that this htlc is allowed in every active commitment | ||
active.map(_.canReceiveAdd(add.amountMsat, params, changes1, feerates, feeConf)) | ||
.collectFirst { case Left(f) => Left(f) } | ||
val canReceiveAdds = active.map(_.canReceiveAdd(add.amountMsat, params, changes1, feerates, feeConf)) | ||
// Log only for jamming protection. | ||
canReceiveAdds.collectFirst { | ||
case Left(f: TooManySmallHtlcs) => | ||
log.info("TooManySmallHtlcs: {} incoming HTLCs are below {}}", f.number, f.below) | ||
Metrics.dropHtlc(f, Tags.Directions.Incoming) | ||
} | ||
canReceiveAdds.flatMap { // TODO: We ignore jamming protection, delete this flatMap to activate jamming protection. | ||
case Left(_: TooManySmallHtlcs) | Left(_: ConfidenceTooLow) => None | ||
case x => Some(x) | ||
} | ||
.collectFirst { case Left(f) => | ||
Metrics.dropHtlc(f, Tags.Directions.Incoming) | ||
Left(f) | ||
} | ||
.getOrElse(Right(copy(changes = changes1))) | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is much clearer. I'm wondering how you came up with this default value though? Can you explain it and provide some guidance on what node operators should take into account when they change that default value?