From 8636c11fef7089f98bf2bef93db8d50f24cdf470 Mon Sep 17 00:00:00 2001 From: pm47 Date: Fri, 6 Sep 2019 15:28:10 +0200 Subject: [PATCH 1/2] check funds in millisatoshi Instead of satoshi, which could introduce rounding errors. Also, we check first the balance before the max-inflight amount, because it makes more sense in terms of error management. --- .../fr/acinq/eclair/channel/Commitments.scala | 30 +++++++++---------- .../channel/states/e/NormalStateSpec.scala | 12 ++++++++ 2 files changed, 27 insertions(+), 15 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala index 35eed22370..e9cbef80a9 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala @@ -148,6 +148,14 @@ object Commitments { // the HTLC we are about to create is outgoing, but from their point of view it is incoming val outgoingHtlcs = reduced.htlcs.filter(_.direction == IN) + // a node cannot spend pending incoming htlcs, and need to keep funds above the reserve required by the counterparty, after paying the fee + // we look from remote's point of view, so if local is funder remote doesn't pay the fees + val fees = if (commitments1.localParams.isFunder) commitTxFee(commitments1.remoteParams.dustLimit, reduced) else 0.sat + val missing = reduced.toRemote - commitments1.remoteParams.channelReserve - fees + if (missing < 0.sat) { + return Left(InsufficientFunds(commitments.channelId, amount = cmd.amount, missing = -missing.truncateToSatoshi, reserve = commitments1.remoteParams.channelReserve, fees = fees)) + } + val htlcValueInFlight = outgoingHtlcs.map(_.add.amountMsat).sum if (commitments1.remoteParams.maxHtlcValueInFlightMsat < htlcValueInFlight) { // TODO: this should be a specific UPDATE error @@ -158,14 +166,6 @@ object Commitments { return Left(TooManyAcceptedHtlcs(commitments.channelId, maximum = commitments1.remoteParams.maxAcceptedHtlcs)) } - // a node cannot spend pending incoming htlcs, and need to keep funds above the reserve required by the counterparty, after paying the fee - // we look from remote's point of view, so if local is funder remote doesn't pay the fees - val fees = if (commitments1.localParams.isFunder) commitTxFee(commitments1.remoteParams.dustLimit, reduced) else 0.sat - val missing = reduced.toRemote.truncateToSatoshi - commitments1.remoteParams.channelReserve - fees - if (missing < 0.sat) { - return Left(InsufficientFunds(commitments.channelId, amount = cmd.amount, missing = -missing, reserve = commitments1.remoteParams.channelReserve, fees = fees)) - } - Right(commitments1, add) } @@ -183,6 +183,13 @@ object Commitments { val reduced = CommitmentSpec.reduce(commitments1.localCommit.spec, commitments1.localChanges.acked, commitments1.remoteChanges.proposed) val incomingHtlcs = reduced.htlcs.filter(_.direction == IN) + // a node cannot spend pending incoming htlcs, and need to keep funds above the reserve required by the counterparty, after paying the fee + val fees = if (commitments1.localParams.isFunder) 0.sat else Transactions.commitTxFee(commitments1.localParams.dustLimit, reduced) + val missing = reduced.toRemote - commitments1.localParams.channelReserve - fees + if (missing < 0.sat) { + throw InsufficientFunds(commitments.channelId, amount = add.amountMsat, missing = -missing.truncateToSatoshi, reserve = commitments1.localParams.channelReserve, fees = fees) + } + val htlcValueInFlight = incomingHtlcs.map(_.add.amountMsat).sum if (commitments1.localParams.maxHtlcValueInFlightMsat < htlcValueInFlight) { throw HtlcValueTooHighInFlight(commitments.channelId, maximum = commitments1.localParams.maxHtlcValueInFlightMsat, actual = htlcValueInFlight) @@ -192,13 +199,6 @@ object Commitments { throw TooManyAcceptedHtlcs(commitments.channelId, maximum = commitments1.localParams.maxAcceptedHtlcs) } - // a node cannot spend pending incoming htlcs, and need to keep funds above the reserve required by the counterparty, after paying the fee - val fees = if (commitments1.localParams.isFunder) 0.sat else Transactions.commitTxFee(commitments1.localParams.dustLimit, reduced) - val missing = reduced.toRemote.truncateToSatoshi - commitments1.localParams.channelReserve - fees - if (missing < 0.sat) { - throw InsufficientFunds(commitments.channelId, amount = add.amountMsat, missing = -missing, reserve = commitments1.localParams.channelReserve, fees = fees) - } - commitments1 } diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala index c66daeedb7..5cccec7309 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala @@ -161,6 +161,18 @@ class NormalStateSpec extends TestkitBaseClass with StateTestsHelperMethods { alice2bob.expectNoMsg(200 millis) } + test("recv CMD_ADD_HTLC (insufficient funds, missing 1 msat)") { f => + import f._ + val sender = TestProbe() + val initialState = bob.stateData.asInstanceOf[DATA_NORMAL] + val add = CMD_ADD_HTLC(initialState.commitments.availableBalanceForSend + 1.msat, randomBytes32, CltvExpiryDelta(144).toCltvExpiry, TestConstants.emptyOnionPacket, upstream = Left(UUID.randomUUID())) + sender.send(bob, add) + + val error = InsufficientFunds(channelId(alice), amount = add.amount, missing = 0 sat, reserve = 10000 sat, fees = 0 sat) + sender.expectMsg(Failure(AddHtlcFailed(channelId(alice), add.paymentHash, error, Local(add.upstream.left.get, Some(sender.ref)), Some(initialState.channelUpdate), Some(add)))) + alice2bob.expectNoMsg(200 millis) + } + test("recv CMD_ADD_HTLC (insufficient funds w/ pending htlcs and 0 balance)") { f => import f._ val sender = TestProbe() From bce502182f82c4c62e7b54566f6087475b7dfcc0 Mon Sep 17 00:00:00 2001 From: Pierre-Marie Padiou Date: Fri, 6 Sep 2019 16:13:36 +0200 Subject: [PATCH 2/2] use msat values in comparison Co-Authored-By: Bastien Teinturier <31281497+t-bast@users.noreply.github.com> --- .../src/main/scala/fr/acinq/eclair/channel/Commitments.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala index e9cbef80a9..d2434bedbe 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala @@ -152,7 +152,7 @@ object Commitments { // we look from remote's point of view, so if local is funder remote doesn't pay the fees val fees = if (commitments1.localParams.isFunder) commitTxFee(commitments1.remoteParams.dustLimit, reduced) else 0.sat val missing = reduced.toRemote - commitments1.remoteParams.channelReserve - fees - if (missing < 0.sat) { + if (missing < 0.msat) { return Left(InsufficientFunds(commitments.channelId, amount = cmd.amount, missing = -missing.truncateToSatoshi, reserve = commitments1.remoteParams.channelReserve, fees = fees)) } @@ -186,7 +186,7 @@ object Commitments { // a node cannot spend pending incoming htlcs, and need to keep funds above the reserve required by the counterparty, after paying the fee val fees = if (commitments1.localParams.isFunder) 0.sat else Transactions.commitTxFee(commitments1.localParams.dustLimit, reduced) val missing = reduced.toRemote - commitments1.localParams.channelReserve - fees - if (missing < 0.sat) { + if (missing < 0.msat) { throw InsufficientFunds(commitments.channelId, amount = add.amountMsat, missing = -missing.truncateToSatoshi, reserve = commitments1.localParams.channelReserve, fees = fees) }