From 4ac7cc719f0be3191de2a9ea54fca6a9b19def99 Mon Sep 17 00:00:00 2001 From: bryanvu Date: Mon, 1 May 2017 11:45:02 -0700 Subject: [PATCH] lnwallet: replace hard-coded fees and adjust tests accordingly This commit replaces the hard-coded 5000 satoshi fees with calls to the FeeEstimator interface. This should provide a way to cleanly plug in additional fee calculation algorithms in the future. This change affected quite a few tests. When possible, the tests were changed to assert amounts sent rather than balances so that fees wouldn't need to be taken into account. There were several tests for which this wasn't possible, so calls to the static fee calculator were made. --- lnd_test.go | 29 +++- lnwallet/channel.go | 79 +++++++-- lnwallet/channel_test.go | 298 +++++++++++----------------------- lnwallet/interface_test.go | 31 +++- lnwallet/reservation.go | 10 +- lnwallet/wallet.go | 11 +- routing/notifications_test.go | 4 +- routing/router.go | 2 +- 8 files changed, 222 insertions(+), 242 deletions(-) diff --git a/lnd_test.go b/lnd_test.go index 27da0729..8c0fc59c 100644 --- a/lnd_test.go +++ b/lnd_test.go @@ -283,6 +283,20 @@ func assertNumConnections(ctxt context.Context, t *harnessTest, } } +// calcStaticFee calculates appropriate fees for commitment transactions. This +// function provides a simple way to allow test balance assertions to take fee +// calculations into account. +// TODO(bvu): Refactor when dynamic fee estimation is added. +func calcStaticFee(numHTLCs int) btcutil.Amount { + const ( + commitWeight = btcutil.Amount(724) + htlcWeight = 172 + feePerByte = btcutil.Amount(250 * 4) + ) + return feePerByte * (commitWeight + + btcutil.Amount(htlcWeight*numHTLCs)) / 1000 +} + // testBasicChannelFunding performs a test exercising expected behavior from a // basic funding workflow. The test creates a new channel between Alice and // Bob, then immediately closes the channel after asserting some expected post @@ -322,9 +336,9 @@ func testBasicChannelFunding(net *networkHarness, t *harnessTest) { if err != nil { t.Fatalf("unable to get bobs's balance: %v", err) } - if aliceBal.Balance != int64(chanAmt-pushAmt) { + if aliceBal.Balance != int64(chanAmt-pushAmt-calcStaticFee(0)) { t.Fatalf("alice's balance is incorrect: expected %v got %v", - chanAmt-pushAmt, aliceBal) + chanAmt-pushAmt-calcStaticFee(0), aliceBal) } if bobBal.Balance != int64(pushAmt) { t.Fatalf("bob's balance is incorrect: expected %v got %v", @@ -631,7 +645,7 @@ func testChannelBalance(net *networkHarness, t *harnessTest) { // As this is a single funder channel, Alice's balance should be // exactly 0.5 BTC since now state transitions have taken place yet. - checkChannelBalance(net.Alice, amount) + checkChannelBalance(net.Alice, amount-calcStaticFee(0)) // Ensure Bob currently has no available balance within the channel. checkChannelBalance(net.Bob, 0) @@ -1864,6 +1878,7 @@ func testHtlcErrorPropagation(net *networkHarness, t *harnessTest) { t.Fatalf("channel not seen by alice before timeout: %v", err) } + commitFee := calcStaticFee(0) assertBaseBalance := func() { balReq := &lnrpc.ChannelBalanceRequest{} aliceBal, err := net.Alice.ChannelBalance(ctxb, balReq) @@ -1874,13 +1889,13 @@ func testHtlcErrorPropagation(net *networkHarness, t *harnessTest) { if err != nil { t.Fatalf("unable to get channel balance: %v", err) } - if aliceBal.Balance != int64(chanAmt) { + if aliceBal.Balance != int64(chanAmt-commitFee) { t.Fatalf("alice has an incorrect balance: expected %v got %v", - int64(chanAmt), aliceBal) + int64(chanAmt-commitFee), aliceBal) } - if bobBal.Balance != int64(chanAmt) { + if bobBal.Balance != int64(chanAmt-commitFee) { t.Fatalf("bob has an incorrect balance: expected %v got %v", - int64(chanAmt), bobBal) + int64(chanAmt-commitFee), bobBal) } } diff --git a/lnwallet/channel.go b/lnwallet/channel.go index ad433a24..22fe00b7 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -683,8 +683,7 @@ type LightningChannel struct { // automatically persist pertinent state to the database in an efficient // manner. func NewLightningChannel(signer Signer, events chainntnfs.ChainNotifier, - fe FeeEstimator, state *channeldb.OpenChannel) (*LightningChannel, - error) { + fe FeeEstimator, state *channeldb.OpenChannel) (*LightningChannel, error) { lc := &LightningChannel{ signer: signer, @@ -719,6 +718,7 @@ func NewLightningChannel(signer Signer, events chainntnfs.ChainNotifier, ourMessageIndex: 0, theirBalance: state.TheirBalance, theirMessageIndex: 0, + fee: state.CommitFee, }) walletLog.Debugf("ChannelPoint(%v), starting local commitment: %v", state.ChanID, newLogClosure(func() string { @@ -739,6 +739,7 @@ func NewLightningChannel(signer Signer, events chainntnfs.ChainNotifier, ourMessageIndex: 0, theirBalance: state.TheirBalance, theirMessageIndex: 0, + fee: state.CommitFee, } if logTail == nil { remoteCommitment.height = 0 @@ -1193,8 +1194,6 @@ func (lc *LightningChannel) restoreStateLogs() error { lc.localCommitChain.tail().theirMessageIndex = theirCounter lc.remoteCommitChain.tail().ourMessageIndex = ourCounter lc.remoteCommitChain.tail().theirMessageIndex = theirCounter - lc.localCommitChain.tail().fee = lc.channelState.CommitFee - lc.remoteCommitChain.tail().fee = lc.channelState.CommitFee return nil } @@ -1262,6 +1261,10 @@ func (lc *LightningChannel) fetchCommitmentView(remoteChain bool, ourBalance = commitChain.tip().ourBalance theirBalance = commitChain.tip().theirBalance + // Add the fee from the previous commitment state back to the + // initiator's balance, so that the fee can be recalculated and + // re-applied in case fee estimation parameters have changed or the + // number of outstanding HTLCs has changed. if lc.channelState.IsInitiator { ourBalance = ourBalance + commitChain.tip().fee } else if !lc.channelState.IsInitiator { @@ -1278,24 +1281,54 @@ func (lc *LightningChannel) fetchCommitmentView(remoteChain bool, filteredHTLCView := lc.evaluateHTLCView(htlcView, &ourBalance, &theirBalance, nextHeight, remoteChain) + // Determine how many current HTLCs are over the dust limit, and should + // be counted for the purpose of fee calculation. + var dustLimit btcutil.Amount + if remoteChain { + dustLimit = lc.channelState.TheirDustLimit + } else { + dustLimit = lc.channelState.OurDustLimit + } + numHTLCs := 0 + for _, htlc := range filteredHTLCView.ourUpdates { + if htlc.Amount < dustLimit { + continue + } + numHTLCs++ + } + for _, htlc := range filteredHTLCView.theirUpdates { + if htlc.Amount < dustLimit { + continue + } + numHTLCs++ + } + + // Calculate the fee for the commitment transaction based on its size. + commitFee := btcutil.Amount(lc.feeEstimator.EstimateFeePerWeight(1)) * + (commitWeight + btcutil.Amount(htlcWeight*numHTLCs)) / 1000 + + if lc.channelState.IsInitiator { + ourBalance = ourBalance - commitFee + } else if !lc.channelState.IsInitiator { + theirBalance = theirBalance - commitFee + } + var selfKey *btcec.PublicKey var remoteKey *btcec.PublicKey var delay uint32 - var delayBalance, p2wkhBalance, dustLimit btcutil.Amount + var delayBalance, p2wkhBalance btcutil.Amount if remoteChain { selfKey = lc.channelState.TheirCommitKey remoteKey = lc.channelState.OurCommitKey delay = lc.channelState.RemoteCsvDelay delayBalance = theirBalance p2wkhBalance = ourBalance - dustLimit = lc.channelState.TheirDustLimit } else { selfKey = lc.channelState.OurCommitKey remoteKey = lc.channelState.TheirCommitKey delay = lc.channelState.LocalCsvDelay delayBalance = ourBalance p2wkhBalance = theirBalance - dustLimit = lc.channelState.OurDustLimit } // Generate a new commitment transaction with all the latest @@ -1352,6 +1385,7 @@ func (lc *LightningChannel) fetchCommitmentView(remoteChain bool, ourMessageIndex: ourLogIndex, theirMessageIndex: theirLogIndex, theirBalance: theirBalance, + fee: commitFee, } // In order to ensure _none_ of the HTLC's associated with this new @@ -2502,6 +2536,17 @@ func (lc *LightningChannel) InitCooperativeClose() ([]byte, *chainhash.Hash, err return nil, nil, ErrChanClosing } + // Calculate the fee for the commitment transaction based on its size. + // For a cooperative close, there should be no HTLCs. + commitFee := btcutil.Amount(lc.feeEstimator.EstimateFeePerWeight(1)) * + commitWeight / 1000 + + if lc.channelState.IsInitiator { + lc.channelState.OurBalance = lc.channelState.OurBalance - commitFee + } else { + lc.channelState.TheirBalance = lc.channelState.TheirBalance - commitFee + } + closeTx := CreateCooperativeCloseTx(lc.fundingTxIn, lc.channelState.OurDustLimit, lc.channelState.TheirDustLimit, lc.channelState.OurBalance, lc.channelState.TheirBalance, @@ -2552,6 +2597,17 @@ func (lc *LightningChannel) CompleteCooperativeClose(remoteSig []byte) (*wire.Ms return nil, ErrChanClosing } + // Calculate the fee for the commitment transaction based on its size. + // For a cooperative close, there should be no HTLCs. + commitFee := btcutil.Amount(lc.feeEstimator.EstimateFeePerWeight(1)) * + commitWeight / 1000 + + if lc.channelState.IsInitiator { + lc.channelState.OurBalance = lc.channelState.OurBalance - commitFee + } else { + lc.channelState.TheirBalance = lc.channelState.TheirBalance - commitFee + } + // Create the transaction used to return the current settled balance // on this active channel back to both parties. In this current model, // the initiator pays full fees for the cooperative close transaction. @@ -2695,15 +2751,6 @@ func CreateCooperativeCloseTx(fundingTxIn *wire.TxIn, closeTx := wire.NewMsgTx(2) closeTx.AddTxIn(fundingTxIn) - // The initiator of a cooperative closure pays the fee in entirety. - // Determine if we're the initiator so we can compute fees properly. - if initiator { - // TODO(roasbeef): take sat/byte here instead of properly calc - ourBalance -= 5000 - } else { - theirBalance -= 5000 - } - // Create both cooperative closure outputs, properly respecting the // dust limits of both parties. if ourBalance >= localDust { diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 1a5d0c6e..fa4dc075 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -329,6 +329,20 @@ func createTestChannels(revocationWindow int) (*LightningChannel, *LightningChan return channelAlice, channelBob, cleanUpFunc, nil } +// calcStaticFee calculates appropriate fees for commitment transactions. This +// function provides a simple way to allow test balance assertions to take fee +// calculations into account. +// TODO(bvu): Refactor when dynamic fee estimation is added. +func calcStaticFee(numHTLCs int) btcutil.Amount { + const ( + commitWeight = btcutil.Amount(724) + htlcWeight = 172 + feePerByte = btcutil.Amount(50 * 4) + ) + return feePerByte * (commitWeight + + btcutil.Amount(htlcWeight*numHTLCs)) / 1000 +} + // TestSimpleAddSettleWorkflow tests a simple channel scenario wherein the // local node (Alice in this case) creates a new outgoing HTLC to bob, commits // this change, then bob immediately commits a settlement of the HTLC after the @@ -429,25 +443,27 @@ func TestSimpleAddSettleWorkflow(t *testing.T) { "forward %v", len(htlcs)) } - // At this point, both sides should have the proper balance, and - // commitment height updated within their local channel state. - aliceBalance := btcutil.Amount(4 * 1e8) - bobBalance := btcutil.Amount(5 * 1e8) - if aliceChannel.channelState.OurBalance != aliceBalance { - t.Fatalf("alice has incorrect local balance %v vs %v", - aliceChannel.channelState.OurBalance, aliceBalance) + // At this point, both sides should have the proper number of satoshis + // sent, and commitment height updated within their local channel + // state. + aliceSent := uint64(0) + bobSent := uint64(0) + + if aliceChannel.channelState.TotalSatoshisSent != aliceSent { + t.Fatalf("alice has incorrect satoshis sent: %v vs %v", + aliceChannel.channelState.TotalSatoshisSent, aliceSent) } - if aliceChannel.channelState.TheirBalance != bobBalance { - t.Fatalf("alice has incorrect remote balance %v vs %v", - aliceChannel.channelState.TheirBalance, bobBalance) + if aliceChannel.channelState.TotalSatoshisReceived != bobSent { + t.Fatalf("alice has incorrect satoshis received %v vs %v", + aliceChannel.channelState.TotalSatoshisReceived, bobSent) } - if bobChannel.channelState.OurBalance != bobBalance { - t.Fatalf("bob has incorrect local balance %v vs %v", - bobChannel.channelState.OurBalance, bobBalance) + if bobChannel.channelState.TotalSatoshisSent != bobSent { + t.Fatalf("bob has incorrect satoshis sent %v vs %v", + bobChannel.channelState.TotalSatoshisSent, bobSent) } - if bobChannel.channelState.TheirBalance != aliceBalance { - t.Fatalf("bob has incorrect remote balance %v vs %v", - bobChannel.channelState.TheirBalance, aliceBalance) + if bobChannel.channelState.TotalSatoshisReceived != aliceSent { + t.Fatalf("bob has incorrect satoshis received %v vs %v", + bobChannel.channelState.TotalSatoshisReceived, aliceSent) } if bobChannel.currentHeight != 1 { t.Fatalf("bob has incorrect commitment height, %v vs %v", @@ -526,25 +542,7 @@ func TestSimpleAddSettleWorkflow(t *testing.T) { // 4 BTC. Alice's channel should show 1 BTC sent and Bob's channel should // show 1 BTC received. They should also be at commitment height two, // with the revocation window extended by by 1 (5). - aliceSettleBalance := btcutil.Amount(4 * 1e8) - bobSettleBalance := btcutil.Amount(6 * 1e8) satoshisTransferred := uint64(100000000) - if aliceChannel.channelState.OurBalance != aliceSettleBalance { - t.Fatalf("alice has incorrect local balance %v vs %v", - aliceChannel.channelState.OurBalance, aliceSettleBalance) - } - if aliceChannel.channelState.TheirBalance != bobSettleBalance { - t.Fatalf("alice has incorrect remote balance %v vs %v", - aliceChannel.channelState.TheirBalance, bobSettleBalance) - } - if bobChannel.channelState.OurBalance != bobSettleBalance { - t.Fatalf("bob has incorrect local balance %v vs %v", - bobChannel.channelState.OurBalance, bobSettleBalance) - } - if bobChannel.channelState.TheirBalance != aliceSettleBalance { - t.Fatalf("bob has incorrect remote balance %v vs %v", - bobChannel.channelState.TheirBalance, aliceSettleBalance) - } if aliceChannel.channelState.TotalSatoshisSent != satoshisTransferred { t.Fatalf("alice satoshis sent incorrect %v vs %v expected", aliceChannel.channelState.TotalSatoshisSent, @@ -1060,6 +1058,42 @@ func TestCheckDustLimit(t *testing.T) { } defer cleanUp() + assertChannelState := func(chanA, chanB *LightningChannel, numOutsA, + numOutsB int, amountSentA, amountSentB uint64) { + + commitment := chanA.localCommitChain.tip() + if len(commitment.txn.TxOut) != numOutsA { + t.Fatalf("incorrect # of outputs: expected %v, got %v", + numOutsA, len(commitment.txn.TxOut)) + } + commitment = chanB.localCommitChain.tip() + if len(commitment.txn.TxOut) != numOutsB { + t.Fatalf("Incorrect # of outputs: expected %v, got %v", + numOutsB, len(commitment.txn.TxOut)) + } + + if chanA.channelState.TotalSatoshisSent != amountSentA { + t.Fatalf("alice satoshis sent incorrect: expected %v, "+ + "got %v", amountSentA, + chanA.channelState.TotalSatoshisSent) + } + if chanA.channelState.TotalSatoshisReceived != amountSentB { + t.Fatalf("alice satoshis received incorrect: expected"+ + "%v, got %v", amountSentB, + chanA.channelState.TotalSatoshisReceived) + } + if chanB.channelState.TotalSatoshisSent != amountSentB { + t.Fatalf("bob satoshis sent incorrect: expected %v, "+ + " got %v", amountSentB, + chanB.channelState.TotalSatoshisSent) + } + if chanB.channelState.TotalSatoshisReceived != amountSentA { + t.Fatalf("bob satoshis received incorrect: expected "+ + "%v, got %v", amountSentA, + chanB.channelState.TotalSatoshisReceived) + } + } + aliceDustLimit := aliceChannel.channelState.OurDustLimit bobDustLimit := bobChannel.channelState.OurDustLimit htlcAmount := btcutil.Amount(500) @@ -1070,9 +1104,6 @@ func TestCheckDustLimit(t *testing.T) { bobDustLimit) } - aliceAmount := aliceChannel.channelState.OurBalance - bobAmount := bobChannel.channelState.OurBalance - htlc, preimage := createHTLC(0, htlcAmount) if _, err := aliceChannel.AddHTLC(htlc); err != nil { t.Fatalf("alice unable to add htlc: %v", err) @@ -1090,34 +1121,8 @@ func TestCheckDustLimit(t *testing.T) { // will be settled. // From Alice point of view HTLC's amount is bigger than dust limit. - commitment := aliceChannel.localCommitChain.tip() - if len(commitment.txn.TxOut) != 3 { - t.Fatalf("incorrect number of outputs: expected %v, got %v", - 3, len(commitment.txn.TxOut)) - } - if commitment.ourBalance != aliceAmount-htlcAmount { - t.Fatalf("Alice has wrong balance: expected %v, got %v", - aliceAmount-htlcAmount, commitment.ourBalance) - } - if commitment.theirBalance != bobAmount { - t.Fatalf("Alice has wrong balance for Bob: expected %v, got %v", - bobAmount, commitment.theirBalance) - } - // From Bob point of view HTLC's amount is lower then dust limit. - commitment = bobChannel.localCommitChain.tip() - if len(commitment.txn.TxOut) != 2 { - t.Fatalf("Incorrect number of outputs: expected %v, got %v", - 2, len(commitment.txn.TxOut)) - } - if commitment.theirBalance != aliceAmount-htlcAmount { - t.Fatalf("Bob has wrong balance for Alice: expected %v, got %v", - aliceAmount-htlcAmount, commitment.theirBalance) - } - if commitment.ourBalance != bobAmount { - t.Fatalf("Bob has wrong balance: expected %v, got %v", - bobAmount, commitment.ourBalance) - } + assertChannelState(aliceChannel, bobChannel, 3, 2, 0, 0) // Settle HTLC and sign new commitment. settleIndex, err := bobChannel.SettleHTLC(preimage) @@ -1132,41 +1137,15 @@ func TestCheckDustLimit(t *testing.T) { t.Fatalf("state transition error: %v", err) } - commitment = aliceChannel.localCommitChain.tip() - if len(commitment.txn.TxOut) != 2 { - t.Fatalf("wrong number of outputs: expected %v, got %v", - 2, len(commitment.txn.TxOut)) - } - if commitment.ourBalance != aliceAmount-htlcAmount { - t.Fatalf("Alice has wrong balance: expected %v, got %v", - aliceAmount-htlcAmount, commitment.ourBalance) - } - if commitment.theirBalance != bobAmount+htlcAmount { - t.Fatalf("Alice has wrong balance for Bob: expected %v, got %v", - bobAmount, commitment.theirBalance) - } - - commitment = bobChannel.localCommitChain.tip() - if len(commitment.txn.TxOut) != 2 { - t.Fatalf("wrong number of outputs: expected %v, got %v", - 2, len(commitment.txn.TxOut)) - } - if commitment.ourBalance != bobAmount+htlcAmount { - t.Fatalf("Bob has wrong balance: expected %v, got %v", - bobAmount+htlcAmount, commitment.ourBalance) - } - if commitment.theirBalance != aliceAmount-htlcAmount { - t.Fatalf("Bob has wrong balance for Alice: expected %v, got %v", - aliceAmount-htlcAmount, commitment.theirBalance) - } + assertChannelState(aliceChannel, bobChannel, 2, 2, uint64(htlcAmount), 0) // Next we will test when a non-HTLC output in the commitment // transaction is below the dust limit. We create an HTLC that will // only leave a small enough amount to Alice such that Bob will // consider it a dust output. - aliceAmount = aliceChannel.channelState.OurBalance - bobAmount = bobChannel.channelState.OurBalance - htlcAmount2 := aliceAmount - htlcAmount + + aliceAmount := btcutil.Amount(5e8) + htlcAmount2 := aliceAmount - btcutil.Amount(1000) htlc, preimage = createHTLC(0, htlcAmount2) if _, err := aliceChannel.AddHTLC(htlc); err != nil { t.Fatalf("alice unable to add htlc: %v", err) @@ -1179,34 +1158,8 @@ func TestCheckDustLimit(t *testing.T) { } // From Alice's point of view, her output is bigger than the dust limit - commitment = aliceChannel.localCommitChain.tip() - if len(commitment.txn.TxOut) != 3 { - t.Fatalf("incorrect number of outputs in commitment transaction "+ - "expected %v, got %v", 3, len(commitment.txn.TxOut)) - } - if commitment.ourBalance != aliceAmount-htlcAmount2 { - t.Fatalf("Alice has wrong balance: expected %v, got %v", - aliceAmount-htlcAmount, commitment.ourBalance) - } - if commitment.theirBalance != bobAmount { - t.Fatalf("Alice has wrong balance for Bob: expected %v, got %v", - bobAmount, commitment.theirBalance) - } - // From Bob's point of view, Alice's output is lower than the dust limit - commitment = bobChannel.localCommitChain.tip() - if len(commitment.txn.TxOut) != 2 { - t.Fatalf("incorrect number of outputs in commitment transaction "+ - "expected %v, got %v", 2, len(commitment.txn.TxOut)) - } - if commitment.theirBalance != aliceAmount-htlcAmount2 { - t.Fatalf("Bob has wrong balance for Alice: expected %v, got %v", - aliceAmount-htlcAmount, commitment.theirBalance) - } - if commitment.ourBalance != bobAmount { - t.Fatalf("Bob has wrong balance: expected %v, got %v", - bobAmount+htlcAmount, commitment.ourBalance) - } + assertChannelState(aliceChannel, bobChannel, 3, 2, uint64(htlcAmount), 0) // Settle HTLC and sign new commitment. settleIndex, err = bobChannel.SettleHTLC(preimage) @@ -1221,33 +1174,8 @@ func TestCheckDustLimit(t *testing.T) { t.Fatalf("state transition error: %v", err) } - commitment = aliceChannel.localCommitChain.tip() - if len(commitment.txn.TxOut) != 2 { - t.Fatalf("incorrect number of outputs in commitment transaction, "+ - "expected %v got %v", 2, len(commitment.txn.TxOut)) - } - if commitment.ourBalance != aliceAmount-htlcAmount2 { - t.Fatalf("Alice has wrong balance: expected %v, got %v", - aliceAmount-htlcAmount, commitment.ourBalance) - } - if commitment.theirBalance != bobAmount+htlcAmount2 { - t.Fatalf("Alice has wrong balance for Bob: expected %v, got %v", - bobAmount, commitment.theirBalance) - } - - commitment = bobChannel.localCommitChain.tip() - if len(commitment.txn.TxOut) != 1 { - t.Fatalf("incorrect number of outputs in commitment transaction, "+ - "expected %v got %v", 1, len(commitment.txn.TxOut)) - } - if commitment.ourBalance != bobAmount+htlcAmount2 { - t.Fatalf("Bob has wrong balance: expected %v, got %v", - bobAmount+htlcAmount, commitment.ourBalance) - } - if commitment.theirBalance != aliceAmount-htlcAmount2 { - t.Fatalf("Bob has wrong balance for Alice: expected %v, got %v", - aliceAmount-htlcAmount, commitment.theirBalance) - } + assertChannelState(aliceChannel, bobChannel, 2, 1, + uint64(htlcAmount+htlcAmount2), 0) } func TestStateUpdatePersistence(t *testing.T) { @@ -1267,9 +1195,6 @@ func TestStateUpdatePersistence(t *testing.T) { t.Fatalf("unable to sync bob's channel: %v", err) } - aliceStartingBalance := aliceChannel.channelState.OurBalance - bobStartingBalance := bobChannel.channelState.OurBalance - const numHtlcs = 4 // Alice adds 3 HTLCs to the update log, while Bob adds a single HTLC. @@ -1281,7 +1206,7 @@ func TestStateUpdatePersistence(t *testing.T) { rHash := sha256.Sum256(alicePreimage[:]) h := &lnwire.UpdateAddHTLC{ PaymentHash: rHash, - Amount: btcutil.Amount(500), + Amount: btcutil.Amount(5000), Expiry: uint32(10), } @@ -1295,7 +1220,7 @@ func TestStateUpdatePersistence(t *testing.T) { rHash := sha256.Sum256(bobPreimage[:]) bobh := &lnwire.UpdateAddHTLC{ PaymentHash: rHash, - Amount: btcutil.Amount(500), + Amount: btcutil.Amount(5000), Expiry: uint32(10), } if _, err := bobChannel.AddHTLC(bobh); err != nil { @@ -1319,20 +1244,6 @@ func TestStateUpdatePersistence(t *testing.T) { t.Fatalf("unable to complete bob's state transition: %v", err) } - // The balances of both channels should be updated accordingly. - aliceBalance := aliceChannel.channelState.OurBalance - expectedAliceBalance := aliceStartingBalance - btcutil.Amount(1500) - bobBalance := bobChannel.channelState.OurBalance - expectedBobBalance := bobStartingBalance - btcutil.Amount(500) - if aliceBalance != expectedAliceBalance { - t.Fatalf("expected %v alice balance, got %v", int64(expectedAliceBalance), - int64(aliceBalance)) - } - if bobBalance != expectedBobBalance { - t.Fatalf("expected %v bob balance, got %v", int64(expectedBobBalance), - int64(bobBalance)) - } - // The latest commitment from both sides should have all the HTLCs. numAliceOutgoing := aliceChannel.localCommitChain.tail().outgoingHTLCs numAliceIncoming := aliceChannel.localCommitChain.tail().incomingHTLCs @@ -1419,7 +1330,7 @@ func TestStateUpdatePersistence(t *testing.T) { if bobChannel.localUpdateLog.Len() != bobChannelNew.localUpdateLog.Len() { t.Fatalf("bob log len: expected %v, got %v", - bobChannelNew.localUpdateLog.Len(), + bobChannel.localUpdateLog.Len(), bobChannelNew.localUpdateLog.Len()) } if bobChannel.remoteUpdateLog.Len() != @@ -1511,37 +1422,23 @@ func TestStateUpdatePersistence(t *testing.T) { t.Fatalf("unable to update commitments: %v", err) } - // The balances of both sides should have been updated accordingly. - aliceBalance = aliceChannelNew.channelState.OurBalance - expectedAliceBalance = aliceStartingBalance - btcutil.Amount(1000) - bobBalance = bobChannelNew.channelState.OurBalance - expectedBobBalance = bobStartingBalance + btcutil.Amount(1000) - if aliceBalance != expectedAliceBalance { - t.Fatalf("expected %v alice balance, got %v", expectedAliceBalance, - aliceBalance) - } - if bobBalance != expectedBobBalance { - t.Fatalf("expected %v bob balance, got %v", expectedBobBalance, - bobBalance) - } - // The amounts transferred should been updated as per the amounts in // the HTLCs - if aliceChannelNew.channelState.TotalSatoshisSent != 1500 { + if aliceChannelNew.channelState.TotalSatoshisSent != 15000 { t.Fatalf("expected %v alice satoshis sent, got %v", - 3000, aliceChannelNew.channelState.TotalSatoshisSent) + 15000, aliceChannelNew.channelState.TotalSatoshisSent) } - if aliceChannelNew.channelState.TotalSatoshisReceived != 500 { + if aliceChannelNew.channelState.TotalSatoshisReceived != 5000 { t.Fatalf("expected %v alice satoshis received, got %v", - 1000, aliceChannelNew.channelState.TotalSatoshisReceived) + 5000, aliceChannelNew.channelState.TotalSatoshisReceived) } - if bobChannelNew.channelState.TotalSatoshisSent != 500 { + if bobChannelNew.channelState.TotalSatoshisSent != 5000 { t.Fatalf("expected %v bob satoshis sent, got %v", - 1000, bobChannel.channelState.TotalSatoshisSent) + 5000, bobChannel.channelState.TotalSatoshisSent) } - if bobChannelNew.channelState.TotalSatoshisReceived != 1500 { + if bobChannelNew.channelState.TotalSatoshisReceived != 15000 { t.Fatalf("expected %v bob satoshis sent, got %v", - 3000, bobChannel.channelState.TotalSatoshisSent) + 15000, bobChannel.channelState.TotalSatoshisSent) } } @@ -1580,7 +1477,8 @@ func TestCancelHTLC(t *testing.T) { // With the HTLC committed, Alice's balance should reflect the clearing // of the new HTLC. - aliceExpectedBalance := btcutil.Amount(btcutil.SatoshiPerBitcoin * 4) + aliceExpectedBalance := btcutil.Amount(btcutil.SatoshiPerBitcoin*4) - + calcStaticFee(1) if aliceChannel.channelState.OurBalance != aliceExpectedBalance { t.Fatalf("Alice's balance is wrong: expected %v, got %v", aliceExpectedBalance, aliceChannel.channelState.OurBalance) @@ -1622,9 +1520,10 @@ func TestCancelHTLC(t *testing.T) { } expectedBalance := btcutil.Amount(btcutil.SatoshiPerBitcoin * 5) - if aliceChannel.channelState.OurBalance != expectedBalance { + if aliceChannel.channelState.OurBalance != expectedBalance-calcStaticFee(0) { t.Fatalf("balance is wrong: expected %v, got %v", - aliceChannel.channelState.OurBalance, expectedBalance) + aliceChannel.channelState.OurBalance, expectedBalance- + calcStaticFee(0)) } if aliceChannel.channelState.TheirBalance != expectedBalance { t.Fatalf("balance is wrong: expected %v, got %v", @@ -1634,9 +1533,10 @@ func TestCancelHTLC(t *testing.T) { t.Fatalf("balance is wrong: expected %v, got %v", bobChannel.channelState.OurBalance, expectedBalance) } - if bobChannel.channelState.TheirBalance != expectedBalance { + if bobChannel.channelState.TheirBalance != expectedBalance-calcStaticFee(0) { t.Fatalf("balance is wrong: expected %v, got %v", - bobChannel.channelState.TheirBalance, expectedBalance) + bobChannel.channelState.TheirBalance, + expectedBalance-calcStaticFee(0)) } } @@ -1699,7 +1599,7 @@ func TestCooperativeCloseDustAdherence(t *testing.T) { // Next we'll modify the current balances and dust limits such that // Bob's current balance is above _below_ his dust limit. aliceBal := btcutil.Amount(btcutil.SatoshiPerBitcoin) - bobBal := btcutil.Amount(500) + bobBal := btcutil.Amount(250) setBalances(aliceBal, bobBal) // Attempt another cooperative channel closure. It should succeed @@ -1716,14 +1616,13 @@ func TestCooperativeCloseDustAdherence(t *testing.T) { // The closure transaction should only have a single output, and that // output should be Alice's balance. - // TODO(roasbeef): remove -5000 after fees are no longer hard coded if len(closeTx.TxOut) != 1 { t.Fatalf("close tx has wrong number of outputs: expected %v "+ "got %v", 1, len(closeTx.TxOut)) } - if closeTx.TxOut[0].Value != int64(aliceBal-5000) { + if closeTx.TxOut[0].Value != int64(aliceBal-calcStaticFee(0)) { t.Fatalf("alice's balance is incorrect: expected %v, got %v", - aliceBal-5000, closeTx.TxOut[0].Value) + aliceBal-calcStaticFee(0), closeTx.TxOut[0].Value) } // Finally, we'll modify the current balances and dust limits such that @@ -1745,7 +1644,6 @@ func TestCooperativeCloseDustAdherence(t *testing.T) { // The closure transaction should only have a single output, and that // output should be Bob's balance. - // TODO(roasbeef): remove -5000 after fees are no longer hard coded if len(closeTx.TxOut) != 1 { t.Fatalf("close tx has wrong number of outputs: expected %v "+ "got %v", 1, len(closeTx.TxOut)) diff --git a/lnwallet/interface_test.go b/lnwallet/interface_test.go index f6bfe7eb..e6ff1a3f 100644 --- a/lnwallet/interface_test.go +++ b/lnwallet/interface_test.go @@ -101,6 +101,20 @@ func assertReservationDeleted(res *lnwallet.ChannelReservation, t *testing.T) { } } +// calcStaticFee calculates appropriate fees for commitment transactions. This +// function provides a simple way to allow test balance assertions to take fee +// calculations into account. +// TODO(bvu): Refactor when dynamic fee estimation is added. +func calcStaticFee(numHTLCs int) btcutil.Amount { + const ( + commitWeight = btcutil.Amount(724) + htlcWeight = 172 + feePerByte = btcutil.Amount(250 * 4) + ) + return feePerByte * (commitWeight + + btcutil.Amount(htlcWeight*numHTLCs)) / 1000 +} + // bobNode represents the other party involved as a node within LN. Bob is our // only "default-route", we have a direct connection with him. type bobNode struct { @@ -454,7 +468,7 @@ func testDualFundingReservationWorkflow(miner *rpctest.Harness, wallet *lnwallet // TODO(roasbeef): account for current hard-coded commit fee, // need to remove bob all together - chanCapacity := int64(10e8 + 5000) + chanCapacity := int64(10e8) // Alice responds with her output, change addr, multi-sig key and signatures. // Bob then responds with his signatures. bobsSigs, err := bobNode.signFundingTx(fundingTx) @@ -572,9 +586,10 @@ func testCancelNonExistantReservation(miner *rpctest.Harness, t.Log("Running cancel reservation tests") + feeRate := btcutil.Amount(wallet.FeeEstimator.EstimateFeePerWeight(1)) // Create our own reservation, give it some ID. - res := lnwallet.NewChannelReservation(1000, 1000, 5000, wallet, 22, - numReqConfs, 10) + res := lnwallet.NewChannelReservation(1000, 1000, feeRate, wallet, + 22, numReqConfs, 10) // Attempt to cancel this reservation. This should fail, we know // nothing of it. @@ -691,7 +706,7 @@ func testSingleFunderReservationWorkflowInitiator(miner *rpctest.Harness, chanReservation.FundingRedeemScript(), // TODO(roasbeef): account for current hard-coded fee, need to // remove bobNode entirely - int64(fundingAmt)+5000) + int64(fundingAmt)) if err != nil { t.Fatalf("bob is unable to sign alice's commit tx: %v", err) } @@ -781,6 +796,7 @@ func testSingleFunderReservationWorkflowResponder(miner *rpctest.Harness, // include any inputs or change addresses, as only Bob will construct // the funding transaction. bobContribution := bobNode.Contribution(ourContribution.CommitKey) + bobContribution.DustLimit = lnwallet.DefaultDustLimit() if err := chanReservation.ProcessSingleContribution(bobContribution); err != nil { t.Fatalf("unable to process bob's contribution: %v", err) } @@ -815,7 +831,7 @@ func testSingleFunderReservationWorkflowResponder(miner *rpctest.Harness, ourContribution.MultiSigKey.SerializeCompressed(), bobContribution.MultiSigKey.SerializeCompressed(), // TODO(roasbeef): account for hard-coded fee, remove bob node - int64(capacity)+5000) + int64(capacity)) if err != nil { t.Fatalf("unable to generate multi-sig output: %v", err) } @@ -843,10 +859,11 @@ func testSingleFunderReservationWorkflowResponder(miner *rpctest.Harness, // Next, manually create Alice's commitment transaction, signing the // fully sorted and state hinted transaction. fundingTxIn := wire.NewTxIn(fundingOutpoint, nil, nil) + aliceCommitTx, err := lnwallet.CreateCommitTx(fundingTxIn, ourContribution.CommitKey, bobContribution.CommitKey, ourContribution.RevocationKey, ourContribution.CsvDelay, 0, - capacity, lnwallet.DefaultDustLimit()) + capacity-calcStaticFee(0), lnwallet.DefaultDustLimit()) if err != nil { t.Fatalf("unable to create alice's commit tx: %v", err) } @@ -857,7 +874,7 @@ func testSingleFunderReservationWorkflowResponder(miner *rpctest.Harness, } bobCommitSig, err := bobNode.signCommitTx(aliceCommitTx, // TODO(roasbeef): account for hard-coded fee, remove bob node - fundingRedeemScript, int64(capacity)+5000) + fundingRedeemScript, int64(capacity)) if err != nil { t.Fatalf("unable to sign alice's commit tx: %v", err) } diff --git a/lnwallet/reservation.go b/lnwallet/reservation.go index 74c13c8e..9968c3e0 100644 --- a/lnwallet/reservation.go +++ b/lnwallet/reservation.go @@ -155,6 +155,8 @@ func NewChannelReservation(capacity, fundingAmt btcutil.Amount, minFeeRate btcut initiator bool ) + commitFee := minFeeRate * commitWeight / 1000 + // If we're the responder to a single-funder reservation, then we have // no initial balance in the channel unless the remote party is pushing // some funds to us within the first commitment state. @@ -166,20 +168,22 @@ func NewChannelReservation(capacity, fundingAmt btcutil.Amount, minFeeRate btcut // TODO(roasbeef): need to rework fee structure in general and // also when we "unlock" dual funder within the daemon - if capacity == fundingAmt+commitFee { + if capacity == fundingAmt { // If we're initiating a single funder workflow, then // we pay all the initial fees within the commitment // transaction. We also deduct our balance by the // amount pushed as part of the initial state. ourBalance = capacity - commitFee - pushSat + theirBalance = capacity - fundingAmt + pushSat } else { // Otherwise, this is a dual funder workflow where both // slides split the amount funded and the commitment // fee. - ourBalance = fundingAmt - commitFee + ourBalance = fundingAmt - (commitFee / 2) + theirBalance = capacity - fundingAmt - + (commitFee / 2) + pushSat } - theirBalance = capacity - fundingAmt - commitFee + pushSat initiator = true } diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index e415e6de..455b9d28 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -37,7 +37,8 @@ const ( // rotations, etc. identityKeyIndex = hdkeychain.HardenedKeyStart + 2 - commitFee = 5000 + commitWeight = btcutil.Amount(724) + htlcWeight = 172 ) var ( @@ -526,8 +527,7 @@ func (l *LightningWallet) handleFundingReserveRequest(req *initFundingReserveMsg } id := atomic.AddUint64(&l.nextFundingID, 1) - totalCapacity := req.capacity + commitFee - reservation := NewChannelReservation(totalCapacity, req.fundingAmount, + reservation := NewChannelReservation(req.capacity, req.fundingAmount, req.minFeeRate, l, id, req.numConfs, req.pushSat) // Grab the mutex on the ChannelReservation to ensure thread-safety @@ -551,9 +551,8 @@ func (l *LightningWallet) handleFundingReserveRequest(req *initFundingReserveMsg if req.fundingAmount != 0 { // TODO(roasbeef): consult model for proper fee rate on funding // tx - feeRate := uint64(10) - amt := req.fundingAmount + commitFee - err := l.selectCoinsAndChange(feeRate, amt, ourContribution) + err := l.selectCoinsAndChange(uint64(req.minFeeRate), + req.fundingAmount, ourContribution) if err != nil { req.err <- err req.resp <- nil diff --git a/routing/notifications_test.go b/routing/notifications_test.go index 21ac129a..cbbf238a 100644 --- a/routing/notifications_test.go +++ b/routing/notifications_test.go @@ -367,7 +367,7 @@ func TestEdgeUpdateNotification(t *testing.T) { } // TODO(roasbeef): this is a hack, needs to be removed // after commitment fees are dynamic. - if edgeUpdate.Capacity != chanValue-5000 { + if edgeUpdate.Capacity != chanValue { t.Fatalf("capacity of edge doesn't match: "+ "expected %v, got %v", chanValue, edgeUpdate.Capacity) } @@ -682,7 +682,7 @@ func TestChannelCloseNotification(t *testing.T) { } // TODO(roasbeef): this is a hack, needs to be removed // after commitment fees are dynamic. - if closedChan.Capacity != chanValue-5000 { + if closedChan.Capacity != chanValue { t.Fatalf("capacity of closed channel doesn't match: "+ "expected %v, got %v", chanValue, closedChan.Capacity) } diff --git a/routing/router.go b/routing/router.go index 8b88d3fc..d24e4882 100644 --- a/routing/router.go +++ b/routing/router.go @@ -622,7 +622,7 @@ func (r *ChannelRouter) processUpdate(msg interface{}) error { // TODO(roasbeef): this is a hack, needs to be removed // after commitment fees are dynamic. - msg.Capacity = btcutil.Amount(chanUtxo.Value) - btcutil.Amount(5000) + msg.Capacity = btcutil.Amount(chanUtxo.Value) msg.ChannelPoint = *fundingPoint if err := r.cfg.Graph.AddChannelEdge(msg); err != nil { return errors.Errorf("unable to add edge: %v", err)