From af4afdb6f0a80f1aca8db21878d4294218400a3d Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Sun, 25 Mar 2018 19:15:05 -0700 Subject: [PATCH] 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 39f73b01b..4bb6856eb 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 c8601dfcd..6a2247e37 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 345affb58..ac4ba1e8a 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 275b5e656..f01fd1f19 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 {