From af4afdb6f0a80f1aca8db21878d4294218400a3d Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Sun, 25 Mar 2018 19:15:05 -0700 Subject: [PATCH 1/2] lnwallet: use btcutil.NewAmount for BTC -> SAT conversions In this commit, we fix an existing rounding related bug in the codebase. The RPC interface for btcd and bitcoind return values in BTC rather than in satoshis. So in several places, we're forced to convert ourselves manually. The existing logic attempted to do this, but didn't properly account for rounding. As a result, our values can be off due to not rounding incorrectly. The fix for this is easy: simply properly use btcutil.NewAmount everywhere which does rounding properly. Fixes #939. --- lnwallet/btcwallet/blockchain.go | 23 +++++++++++---- lnwallet/btcwallet/btcwallet.go | 9 +++++- lnwallet/channel_test.go | 6 +++- lnwallet/interface_test.go | 49 +++++++++++++++++++++++--------- 4 files changed, 66 insertions(+), 21 deletions(-) diff --git a/lnwallet/btcwallet/blockchain.go b/lnwallet/btcwallet/blockchain.go index 39f73b01..4bb6856e 100644 --- a/lnwallet/btcwallet/blockchain.go +++ b/lnwallet/btcwallet/blockchain.go @@ -7,6 +7,7 @@ import ( "github.com/roasbeef/btcd/chaincfg/chainhash" "github.com/roasbeef/btcd/wire" + "github.com/roasbeef/btcutil" "github.com/lightninglabs/neutrino" "github.com/lightningnetwork/lnd/lnwallet" @@ -77,10 +78,15 @@ func (b *BtcWallet) GetUtxo(op *wire.OutPoint, heightHint uint32) (*wire.TxOut, return nil, err } + // We'll ensure we properly convert the amount given in BTC to + // satoshis. + amt, err := btcutil.NewAmount(txout.Value) + if err != nil { + return nil, err + } + return &wire.TxOut{ - // Sadly, gettxout returns the output value in BTC - // instead of satoshis. - Value: int64(txout.Value * 1e8), + Value: int64(amt), PkScript: pkScript, }, nil @@ -97,10 +103,15 @@ func (b *BtcWallet) GetUtxo(op *wire.OutPoint, heightHint uint32) (*wire.TxOut, return nil, err } + // Sadly, gettxout returns the output value in BTC instead of + // satoshis. + amt, err := btcutil.NewAmount(txout.Value) + if err != nil { + return nil, err + } + return &wire.TxOut{ - // Sadly, gettxout returns the output value in BTC - // instead of satoshis. - Value: int64(txout.Value * 1e8), + Value: int64(amt), PkScript: pkScript, }, nil diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go index c8601dfc..6a2247e3 100644 --- a/lnwallet/btcwallet/btcwallet.go +++ b/lnwallet/btcwallet/btcwallet.go @@ -330,9 +330,16 @@ func (b *BtcWallet) ListUnspentWitness(minConfs int32) ([]*lnwallet.Utxo, error) return nil, err } + // We'll ensure we properly convert the amount given in + // BTC to satoshis. + amt, err := btcutil.NewAmount(output.Amount) + if err != nil { + return nil, err + } + utxo := &lnwallet.Utxo{ AddressType: addressType, - Value: btcutil.Amount(output.Amount * 1e8), + Value: amt, PkScript: pkScript, OutPoint: wire.OutPoint{ Hash: *txid, diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 345affb5..ac4ba1e8 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -163,7 +163,11 @@ func forceStateTransition(chanA, chanB *LightningChannel) error { func createTestChannels(revocationWindow int) (*LightningChannel, *LightningChannel, func(), error) { - channelCapacity := btcutil.Amount(10 * 1e8) + channelCapacity, err := btcutil.NewAmount(10) + if err != nil { + return nil, nil, nil, err + } + channelBal := channelCapacity / 2 aliceDustLimit := btcutil.Amount(200) bobDustLimit := btcutil.Amount(1300) diff --git a/lnwallet/interface_test.go b/lnwallet/interface_test.go index 275b5e65..f01fd1f1 100644 --- a/lnwallet/interface_test.go +++ b/lnwallet/interface_test.go @@ -101,12 +101,14 @@ var ( // assertProperBalance asserts than the total value of the unspent outputs // within the wallet are *exactly* amount. If unable to retrieve the current // balance, or the assertion fails, the test will halt with a fatal error. -func assertProperBalance(t *testing.T, lw *lnwallet.LightningWallet, numConfirms int32, amount int64) { +func assertProperBalance(t *testing.T, lw *lnwallet.LightningWallet, + numConfirms int32, amount float64) { + balance, err := lw.ConfirmedBalance(numConfirms) if err != nil { t.Fatalf("unable to query for balance: %v", err) } - if balance != btcutil.Amount(amount*1e8) { + if balance.ToBTC() != amount { t.Fatalf("wallet credits not properly loaded, should have 40BTC, "+ "instead have %v", balance) } @@ -149,7 +151,7 @@ func calcStaticFee(numHTLCs int) btcutil.Amount { } func loadTestCredits(miner *rpctest.Harness, w *lnwallet.LightningWallet, - numOutputs, btcPerOutput int) error { + numOutputs int, btcPerOutput float64) error { // For initial neutrino connection, wait a second. // TODO(aakselrod): Eliminate the need for this. @@ -159,12 +161,15 @@ func loadTestCredits(miner *rpctest.Harness, w *lnwallet.LightningWallet, } // Using the mining node, spend from a coinbase output numOutputs to // give us btcPerOutput with each output. - satoshiPerOutput := int64(btcPerOutput * 1e8) + satoshiPerOutput, err := btcutil.NewAmount(btcPerOutput) + if err != nil { + return fmt.Errorf("unable to create amt: %v", err) + } expectedBalance, err := w.ConfirmedBalance(1) if err != nil { return err } - expectedBalance += btcutil.Amount(satoshiPerOutput * int64(numOutputs)) + expectedBalance += btcutil.Amount(int64(satoshiPerOutput) * int64(numOutputs)) addrs := make([]btcutil.Address, 0, numOutputs) for i := 0; i < numOutputs; i++ { // Grab a fresh address from the wallet to house this output. @@ -181,7 +186,7 @@ func loadTestCredits(miner *rpctest.Harness, w *lnwallet.LightningWallet, addrs = append(addrs, walletAddr) output := &wire.TxOut{ - Value: satoshiPerOutput, + Value: int64(satoshiPerOutput), PkScript: script, } if _, err := miner.SendOutputs([]*wire.TxOut{output}, 10); err != nil { @@ -278,7 +283,10 @@ func createTestWallet(tempTestDir string, miningNode *rpctest.Harness, func testDualFundingReservationWorkflow(miner *rpctest.Harness, alice, bob *lnwallet.LightningWallet, t *testing.T) { - const fundingAmount = btcutil.Amount(5 * 1e8) + fundingAmount, err := btcutil.NewAmount(5) + if err != nil { + t.Fatalf("unable to create amt: %v", err) + } // In this scenario, we'll test a dual funder reservation, with each // side putting in 10 BTC. @@ -450,7 +458,10 @@ func testFundingTransactionLockedOutputs(miner *rpctest.Harness, alice, _ *lnwallet.LightningWallet, t *testing.T) { // Create a single channel asking for 16 BTC total. - fundingAmount := btcutil.Amount(8 * 1e8) + fundingAmount, err := btcutil.NewAmount(8) + if err != nil { + t.Fatalf("unable to create amt: %v", err) + } feeRate, err := alice.Cfg.FeeEstimator.EstimateFeePerVSize(1) if err != nil { t.Fatalf("unable to query fee estimator: %v", err) @@ -467,7 +478,10 @@ func testFundingTransactionLockedOutputs(miner *rpctest.Harness, // Now attempt to reserve funds for another channel, this time // requesting 900 BTC. We only have around 64BTC worth of outpoints // that aren't locked, so this should fail. - amt := btcutil.Amount(900 * 1e8) + amt, err := btcutil.NewAmount(900) + if err != nil { + t.Fatalf("unable to create amt: %v", err) + } failedReservation, err := alice.InitChannelReservation(amt, amt, 0, feePerKw, feeRate, bobPub, bobAddr, chainHash, lnwire.FFAnnounceChannel) @@ -492,7 +506,10 @@ func testFundingCancellationNotEnoughFunds(miner *rpctest.Harness, feePerKw := feeRate.FeePerKWeight() // Create a reservation for 44 BTC. - fundingAmount := btcutil.Amount(44 * 1e8) + fundingAmount, err := btcutil.NewAmount(44) + if err != nil { + t.Fatalf("unable to create amt: %v", err) + } chanReservation, err := alice.InitChannelReservation(fundingAmount, fundingAmount, 0, feePerKw, feeRate, bobPub, bobAddr, chainHash, lnwire.FFAnnounceChannel) @@ -571,10 +588,13 @@ func testReservationInitiatorBalanceBelowDustCancel(miner *rpctest.Harness, // We'll attempt to create a new reservation with an extremely high fee // rate. This should push our balance into the negative and result in a // failure to create the reservation. - fundingAmount := btcutil.Amount(4 * 1e8) + fundingAmount, err := btcutil.NewAmount(4) + if err != nil { + t.Fatalf("unable to create amt: %v", err) + } feePerVSize := lnwallet.SatPerVByte(btcutil.SatoshiPerBitcoin * 4 / 100) feePerKw := feePerVSize.FeePerKWeight() - _, err := alice.InitChannelReservation( + _, err = alice.InitChannelReservation( fundingAmount, fundingAmount, 0, feePerKw, feePerVSize, bobPub, bobAddr, chainHash, lnwire.FFAnnounceChannel, ) @@ -638,7 +658,10 @@ func testSingleFunderReservationWorkflow(miner *rpctest.Harness, // First, Alice will Initialize a reservation for a channel with 4 BTC // funded solely by us. We'll also initially push 1 BTC of the channel // towards Bob's side. - fundingAmt := btcutil.Amount(4 * 1e8) + fundingAmt, err := btcutil.NewAmount(4) + if err != nil { + t.Fatalf("unable to create amt: %v", err) + } pushAmt := lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin) feeRate, err := alice.Cfg.FeeEstimator.EstimateFeePerVSize(1) if err != nil { From 4253b853097d9c00da835ad7a222d91168aad9a2 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Sun, 25 Mar 2018 19:16:39 -0700 Subject: [PATCH 2/2] test: ensure tests convert from BTC to SAT properly This commit is a follow up to the prior commit which fixed a rounding error bug in lnwallet. For uniformity, we also fix other occurrences in the breach arbiter, as well as the integration tests. --- breacharbiter_test.go | 6 +++++- lnd_test.go | 8 ++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/breacharbiter_test.go b/breacharbiter_test.go index 31d8002a..5ac4efa6 100644 --- a/breacharbiter_test.go +++ b/breacharbiter_test.go @@ -1255,7 +1255,11 @@ func createInitChannels(revocationWindow int) (*lnwallet.LightningChannel, *lnwa bobKeyPriv, bobKeyPub := btcec.PrivKeyFromBytes(btcec.S256(), bobsPrivKey) - channelCapacity := btcutil.Amount(10 * 1e8) + channelCapacity, err := btcutil.NewAmount(10) + if err != nil { + return nil, nil, nil, err + } + channelBal := channelCapacity / 2 aliceDustLimit := btcutil.Amount(200) bobDustLimit := btcutil.Amount(1300) diff --git a/lnd_test.go b/lnd_test.go index 27b34628..3fd20e6b 100644 --- a/lnd_test.go +++ b/lnd_test.go @@ -1418,7 +1418,7 @@ func testChannelForceClosure(net *lntest.NetworkHarness, t *harnessTest) { t.Fatalf("unable to get carol's balance: %v", err) } - carolStartingBalance := btcutil.Amount(carolBalResp.ConfirmedBalance * 1e8) + carolStartingBalance := carolBalResp.ConfirmedBalance ctxt, _ := context.WithTimeout(ctxb, timeout) chanPoint := openChannelAndAssert(ctxt, t, net, net.Alice, carol, @@ -1965,11 +1965,11 @@ func testChannelForceClosure(net *lntest.NetworkHarness, t *harnessTest) { if err != nil { t.Fatalf("unable to get carol's balance: %v", err) } - carolExpectedBalance := carolStartingBalance + pushAmt - if btcutil.Amount(carolBalResp.ConfirmedBalance*1e8) < carolExpectedBalance { + carolExpectedBalance := btcutil.Amount(carolStartingBalance) + pushAmt + if btcutil.Amount(carolBalResp.ConfirmedBalance) < carolExpectedBalance { t.Fatalf("carol's balance is incorrect: expected %v got %v", carolExpectedBalance, - btcutil.Amount(carolBalResp.ConfirmedBalance*1e8)) + carolBalResp.ConfirmedBalance) } }