From 255f38e72d16756038db6b86512d95f8b606405b Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Fri, 9 Nov 2018 17:38:01 -0800 Subject: [PATCH 1/5] lnwallet/btcwallet: check output is under our control in FetchInputInfo In this commit, we add an additional check to btcwallet's FetchInputInfo method to ensure the output is actually under control of the wallet. Previously, the wallet would assume the output was under its control if the txid of the output was found within the wallet. This is not a safe assumption to make however, because if we happened to be the sender of this transaction, it would be found within the wallet but it's not actually under our control. To fix this, we explicitly check that there exists an address in our wallet for this output. --- lnwallet/btcwallet/signer.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lnwallet/btcwallet/signer.go b/lnwallet/btcwallet/signer.go index 98b9d909..21f09592 100644 --- a/lnwallet/btcwallet/signer.go +++ b/lnwallet/btcwallet/signer.go @@ -44,7 +44,14 @@ func (b *BtcWallet) FetchInputInfo(prevOut *wire.OutPoint) (*wire.TxOut, error) return nil, lnwallet.ErrNotMine } + // With the output retrieved, we'll make an additional check to ensure + // we actually have control of this output. We do this because the check + // above only guarantees that the transaction is somehow relevant to us, + // like in the event of us being the sender of the transaction. output = txDetail.TxRecord.MsgTx.TxOut[prevOut.Index] + if _, err := b.fetchOutputAddr(output.PkScript); err != nil { + return nil, err + } b.cacheMtx.Lock() b.utxoCache[*prevOut] = output @@ -72,7 +79,7 @@ func (b *BtcWallet) fetchOutputAddr(script []byte) (waddrmgr.ManagedAddress, err } } - return nil, errors.Errorf("address not found") + return nil, lnwallet.ErrNotMine } // fetchPrivKey attempts to retrieve the raw private key corresponding to the @@ -196,7 +203,7 @@ func (b *BtcWallet) ComputeInputScript(tx *wire.MsgTx, outputScript := signDesc.Output.PkScript walletAddr, err := b.fetchOutputAddr(outputScript) if err != nil { - return nil, nil + return nil, err } pka := walletAddr.(waddrmgr.ManagedPubKeyAddress) From 5b42a38c0f37a5968aac94aa5345027dd6f4d3d9 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 13 Nov 2018 20:01:54 -0800 Subject: [PATCH 2/5] lnwallet/interface_test: add test to detect confirmation of change output spend tx In this commit, we add a new test to the existing set of wallet tests to ensure we can properly detect the confirmation of transactions that spend our change outputs. We do this as a measure to prevent future regressions from happening where the wallet doesn't request its backend to be notified of when an on-chain transaction pays to a change address, like with the recently discovered SendOutputs bug. As is, this test will not pass until we update the btcwallet dependency in the next commit. --- lnwallet/interface_test.go | 209 +++++++++++++++++++++++++++++++++++++ 1 file changed, 209 insertions(+) diff --git a/lnwallet/interface_test.go b/lnwallet/interface_test.go index da3240ee..3adcc475 100644 --- a/lnwallet/interface_test.go +++ b/lnwallet/interface_test.go @@ -134,6 +134,124 @@ func assertReservationDeleted(res *lnwallet.ChannelReservation, t *testing.T) { } } +// mineAndAssertTxInBlock asserts that a transaction is included within the next +// block mined. +func mineAndAssertTxInBlock(t *testing.T, miner *rpctest.Harness, + txid chainhash.Hash) { + + t.Helper() + + // First, we'll wait for the transaction to arrive in the mempool. + if err := waitForMempoolTx(miner, &txid); err != nil { + t.Fatalf("unable to find %v in the mempool: %v", txid, err) + } + + // We'll mined a block to confirm it. + blockHashes, err := miner.Node.Generate(1) + if err != nil { + t.Fatalf("unable to generate new block: %v", err) + } + + // Finally, we'll check it was actually mined in this block. + block, err := miner.Node.GetBlock(blockHashes[0]) + if err != nil { + t.Fatalf("unable to get block %v: %v", blockHashes[0], err) + } + if len(block.Transactions) != 2 { + t.Fatalf("expected 2 transactions in block, found %d", + len(block.Transactions)) + } + txHash := block.Transactions[1].TxHash() + if txHash != txid { + t.Fatalf("expected transaction %v to be mined, found %v", txid, + txHash) + } +} + +// newPkScript generates a new public key script of the given address type. +func newPkScript(t *testing.T, w *lnwallet.LightningWallet, + addrType lnwallet.AddressType) []byte { + + t.Helper() + + addr, err := w.NewAddress(addrType, false) + if err != nil { + t.Fatalf("unable to create new address: %v", err) + } + pkScript, err := txscript.PayToAddrScript(addr) + if err != nil { + t.Fatalf("unable to create output script: %v", err) + } + + return pkScript +} + +// sendCoins is a helper function that encompasses all the things needed for two +// parties to send on-chain funds to each other. +func sendCoins(t *testing.T, miner *rpctest.Harness, + sender, receiver *lnwallet.LightningWallet, output *wire.TxOut, + feeRate lnwallet.SatPerKWeight) *wire.MsgTx { + + t.Helper() + + tx, err := sender.SendOutputs([]*wire.TxOut{output}, 2500) + if err != nil { + t.Fatalf("unable to send transaction: %v", err) + } + + mineAndAssertTxInBlock(t, miner, tx.TxHash()) + + if err := waitForWalletSync(miner, sender); err != nil { + t.Fatalf("unable to sync alice: %v", err) + } + if err := waitForWalletSync(miner, receiver); err != nil { + t.Fatalf("unable to sync bob: %v", err) + } + + return tx +} + +// assertTxInWallet asserts that a transaction exists in the wallet with the +// expected confirmation status. +func assertTxInWallet(t *testing.T, w *lnwallet.LightningWallet, + txHash chainhash.Hash, confirmed bool) { + + t.Helper() + + // If the backend is Neutrino, then we can't determine unconfirmed + // transactions since it's not aware of the mempool. + if !confirmed && w.BackEnd() == "neutrino" { + return + } + + // We'll fetch all of our transaction and go through each one until + // finding the expected transaction with its expected confirmation + // status. + txs, err := w.ListTransactionDetails() + if err != nil { + t.Fatalf("unable to retrieve transactions: %v", err) + } + for _, tx := range txs { + if tx.Hash != txHash { + continue + } + if tx.NumConfirmations <= 0 && confirmed { + t.Fatalf("expected transaction %v to be confirmed", + txHash) + } + if tx.NumConfirmations > 0 && !confirmed { + t.Fatalf("expected transaction %v to be unconfirmed", + txHash) + } + + // We've found the transaction and it matches the desired + // confirmation status, so we can exit. + return + } + + t.Fatalf("transaction %v not found", txHash) +} + // calcStaticFee calculates appropriate fees for commitment transactions. This // function provides a simple way to allow test balance assertions to take fee // calculations into account. @@ -1962,6 +2080,90 @@ func testReorgWalletBalance(r *rpctest.Harness, w *lnwallet.LightningWallet, } } +// testChangeOutputSpendConfirmation ensures that when we attempt to spend a +// change output created by the wallet, the wallet receives its confirmation +// once included in the chain. +func testChangeOutputSpendConfirmation(r *rpctest.Harness, + alice, bob *lnwallet.LightningWallet, t *testing.T) { + + // In order to test that we see the confirmation of a transaction that + // spends an output created by SendOutputs, we'll start by emptying + // Alice's wallet so that no other UTXOs can be picked. To do so, we'll + // generate an address for Bob, who will receive all the coins. + // Assuming a balance of 80 BTC and a transaction fee of 2500 sat/kw, + // we'll craft the following transaction so that Alice doesn't have any + // UTXOs left. + aliceBalance, err := alice.ConfirmedBalance(0) + if err != nil { + t.Fatalf("unable to retrieve alice's balance: %v", err) + } + bobPkScript := newPkScript(t, bob, lnwallet.WitnessPubKey) + + // We'll use a transaction fee of 13020 satoshis, which will allow us to + // sweep all of Alice's balance in one transaction containing 1 input + // and 1 output. + // + // TODO(wilmer): replace this once SendOutputs easily supports sending + // all funds in one transaction. + txFeeRate := lnwallet.SatPerKWeight(2500) + txFee := btcutil.Amount(14380) + output := &wire.TxOut{ + Value: int64(aliceBalance - txFee), + PkScript: bobPkScript, + } + tx := sendCoins(t, r, alice, bob, output, txFeeRate) + txHash := tx.TxHash() + assertTxInWallet(t, alice, txHash, true) + assertTxInWallet(t, bob, txHash, true) + + // With the transaction sent and confirmed, Alice's balance should now + // be 0. + aliceBalance, err = alice.ConfirmedBalance(0) + if err != nil { + t.Fatalf("unable to retrieve alice's balance: %v", err) + } + if aliceBalance != 0 { + t.Fatalf("expected alice's balance to be 0 BTC, found %v", + aliceBalance) + } + + // Now, we'll send an output back to Alice from Bob of 1 BTC. + alicePkScript := newPkScript(t, alice, lnwallet.WitnessPubKey) + output = &wire.TxOut{ + Value: btcutil.SatoshiPerBitcoin, + PkScript: alicePkScript, + } + tx = sendCoins(t, r, bob, alice, output, txFeeRate) + txHash = tx.TxHash() + assertTxInWallet(t, alice, txHash, true) + assertTxInWallet(t, bob, txHash, true) + + // Alice now has an available output to spend, but it was not a change + // output, which is what the test expects. Therefore, we'll generate one + // by sending Bob back some coins. + output = &wire.TxOut{ + Value: btcutil.SatoshiPerBitcent, + PkScript: bobPkScript, + } + tx = sendCoins(t, r, alice, bob, output, txFeeRate) + txHash = tx.TxHash() + assertTxInWallet(t, alice, txHash, true) + assertTxInWallet(t, bob, txHash, true) + + // Then, we'll spend the change output and ensure we see its + // confirmation come in. + tx = sendCoins(t, r, alice, bob, output, txFeeRate) + txHash = tx.TxHash() + assertTxInWallet(t, alice, txHash, true) + assertTxInWallet(t, bob, txHash, true) + + // Finally, we'll replenish Alice's wallet with some more coins to + // ensure she has enough for any following test cases. + if err := loadTestCredits(r, alice, 20, 4); err != nil { + t.Fatalf("unable to replenish alice's wallet: %v", err) + } +} + type walletTestCase struct { name string test func(miner *rpctest.Harness, alice, bob *lnwallet.LightningWallet, @@ -1969,6 +2171,13 @@ type walletTestCase struct { } var walletTests = []walletTestCase{ + { + // TODO(wilmer): this test should remain first until the wallet + // can properly craft a transaction that spends all of its + // on-chain funds. + name: "change output spend confirmation", + test: testChangeOutputSpendConfirmation, + }, { name: "insane fee reject", test: testReservationInitiatorBalanceBelowDustCancel, From f9b15e97f39b6cd478d05e4cfc231ad453dfd9ab Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 13 Nov 2018 20:06:18 -0800 Subject: [PATCH 3/5] build: update btcwallet dependency to address SendOutputs bug In this commit, we update our btcwallet dependency to include the latest changes that aim to address the recently discovered SendOutputs bug. This commit will also allow the lnwallet test case added in the previous commit to succeed. --- Gopkg.lock | 4 ++-- Gopkg.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 4b7c2f39..90bdd4ca 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -106,7 +106,7 @@ revision = "ab6388e0c60ae4834a1f57511e20c17b5f78be4b" [[projects]] - digest = "1:2995aa2bcb95d13a8df309e1dcb6ac20786acb90df5a090bf5e07c2086219ce8" + digest = "1:014bf3112e2bc78db2409f1d7b328c642fe27f2e0b5983595b240bf12578f335" name = "github.com/btcsuite/btcwallet" packages = [ "chain", @@ -127,7 +127,7 @@ "wtxmgr", ] pruneopts = "UT" - revision = "6d43b2e29b5eef0f000a301ee6fbd146db75d118" + revision = "4c01c0878c4ea6ff80711dbfe49e49199ca07607" [[projects]] branch = "master" diff --git a/Gopkg.toml b/Gopkg.toml index ec90ab07..e04905a9 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -72,7 +72,7 @@ [[constraint]] name = "github.com/btcsuite/btcwallet" - revision = "6d43b2e29b5eef0f000a301ee6fbd146db75d118" + revision = "4c01c0878c4ea6ff80711dbfe49e49199ca07607" [[constraint]] name = "github.com/tv42/zbase32" From b1860a95e0075a30c0d427833db50a0559e500dc Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 13 Nov 2018 20:07:54 -0800 Subject: [PATCH 4/5] lnwallet/btcwallet: add lightning addr scope before wallet start In this commit, we add the lightning address scope before the wallet starts to prevent a race condition between the wallet syncing and adding the scope itself. This became more apparent with the recent btcwallet fixes, as several database transactions now occur between the wallet being started and it syncing. --- lnwallet/btcwallet/btcwallet.go | 34 ++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go index 74364ec0..b60ccb54 100644 --- a/lnwallet/btcwallet/btcwallet.go +++ b/lnwallet/btcwallet/btcwallet.go @@ -153,26 +153,13 @@ func (b *BtcWallet) InternalWallet() *base.Wallet { // // This is a part of the WalletController interface. func (b *BtcWallet) Start() error { - // Establish an RPC connection in addition to starting the goroutines - // in the underlying wallet. - if err := b.chain.Start(); err != nil { - return err - } - - // Start the underlying btcwallet core. - b.wallet.Start() - - // Pass the rpc client into the wallet so it can sync up to the - // current main chain. - b.wallet.SynchronizeRPC(b.chain) - + // We'll start by unlocking the wallet and ensuring that the KeyScope: + // (1017, 1) exists within the internal waddrmgr. We'll need this in + // order to properly generate the keys required for signing various + // contracts. if err := b.wallet.Unlock(b.cfg.PrivatePass, nil); err != nil { return err } - - // We'll now ensure that the KeyScope: (1017, 1) exists within the - // internal waddrmgr. We'll need this in order to properly generate the - // keys required for signing various contracts. _, err := b.wallet.Manager.FetchScopedKeyManager(b.chainKeyScope) if err != nil { // If the scope hasn't yet been created (it wouldn't been @@ -191,6 +178,19 @@ func (b *BtcWallet) Start() error { } } + // Establish an RPC connection in addition to starting the goroutines + // in the underlying wallet. + if err := b.chain.Start(); err != nil { + return err + } + + // Start the underlying btcwallet core. + b.wallet.Start() + + // Pass the rpc client into the wallet so it can sync up to the + // current main chain. + b.wallet.SynchronizeRPC(b.chain) + return nil } From 9ca9802d9c669ff7591ccb7cb08c135a51b89ad8 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 13 Nov 2018 20:08:54 -0800 Subject: [PATCH 5/5] lnwallet/btcwallet: check wallet rescan is complete within IsSynced In this commit, we add an additional check to btcwallet's IsSynced method to ensure that it is not currently undergoing a rescan. We do this to block upon starting the server and all other dependent subsystems until the rescan is complete. --- lnwallet/btcwallet/btcwallet.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go index b60ccb54..2341ba5d 100644 --- a/lnwallet/btcwallet/btcwallet.go +++ b/lnwallet/btcwallet/btcwallet.go @@ -714,6 +714,11 @@ func (b *BtcWallet) SubscribeTransactions() (lnwallet.TransactionSubscription, e // // This is a part of the WalletController interface. func (b *BtcWallet) IsSynced() (bool, int64, error) { + // First, we'll ensure the wallet is not currently undergoing a rescan. + if !b.wallet.ChainSynced() { + return false, 0, nil + } + // Grab the best chain state the wallet is currently aware of. syncState := b.wallet.Manager.SyncedTo()