From a1a9834a534cb75df3b91f34a02006426b2cf397 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Mon, 15 Jan 2018 23:26:35 +0100 Subject: [PATCH 1/8] lnwallet: add PublishTransaction error types --- lnwallet/interface.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lnwallet/interface.go b/lnwallet/interface.go index c80d4479..87d1ea81 100644 --- a/lnwallet/interface.go +++ b/lnwallet/interface.go @@ -35,6 +35,11 @@ const ( PubKeyHash ) +// ErrDoubleSpend is returned from PublishTransaction in case the +// tx being published is spending an output spent by a conflicting +// transaction. +var ErrDoubleSpend = errors.New("Transaction rejected: output already spent") + // Utxo is an unspent output denoted by its outpoint, and output value of the // original output. type Utxo struct { @@ -183,6 +188,11 @@ type WalletController interface { // PublishTransaction performs cursory validation (dust checks, etc), // then finally broadcasts the passed transaction to the Bitcoin network. + // If the transaction is rejected because it is conflicting with an + // already known transaction, ErrDoubleSpend is returned. If the + // transaction is already known (published already), no error will be + // returned. Other error returned depends on the currently active chain + // backend. PublishTransaction(tx *wire.MsgTx) error // SubscribeTransactions returns a TransactionSubscription client which From 1dcc89cca9d3c45479b319444735a4881b5180db Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Mon, 15 Jan 2018 23:27:27 +0100 Subject: [PATCH 2/8] lnwallet/btcwallet: return concrete error type from PublishTransaction --- lnwallet/btcwallet/btcwallet.go | 85 ++++++++++++++++++++++++++++++++- 1 file changed, 83 insertions(+), 2 deletions(-) diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go index c6229edc..4c4f50f9 100644 --- a/lnwallet/btcwallet/btcwallet.go +++ b/lnwallet/btcwallet/btcwallet.go @@ -5,6 +5,7 @@ import ( "encoding/hex" "fmt" "math" + "strings" "sync" "time" @@ -399,9 +400,89 @@ func (b *BtcWallet) ListUnspentWitness(minConfs int32) ([]*lnwallet.Utxo, error) } // PublishTransaction performs cursory validation (dust checks, etc), then -// finally broadcasts the passed transaction to the Bitcoin network. +// finally broadcasts the passed transaction to the Bitcoin network. If +// publishing the transaction fails, an error describing the reason is +// returned (currently ErrDoubleSpend). If the transaction is already +// published to the network (either in the mempool or chain) no error +// will be returned. func (b *BtcWallet) PublishTransaction(tx *wire.MsgTx) error { - return b.wallet.PublishTransaction(tx) + if err := b.wallet.PublishTransaction(tx); err != nil { + switch b.chain.(type) { + case *chain.RPCClient: + if strings.Contains(err.Error(), "already have") { + // Transaction was already in the mempool, do + // not treat as an error. We do this to mimic + // the behaviour of bitcoind, which will not + // return an error if a transaction in the + // mempool is sent again using the + // sendrawtransaction RPC call. + return nil + } + if strings.Contains(err.Error(), "already exists") { + // Transaction was already mined, we don't + // consider this an error. + return nil + } + if strings.Contains(err.Error(), "already spent") { + // Output was already spent. + return lnwallet.ErrDoubleSpend + } + if strings.Contains(err.Error(), "orphan transaction") { + // Transaction is spending either output that + // is missing or already spent. + return lnwallet.ErrDoubleSpend + } + + case *chain.BitcoindClient: + if strings.Contains(err.Error(), "txn-already-in-mempool") { + // Transaction in mempool, treat as non-error. + return nil + } + if strings.Contains(err.Error(), "txn-already-known") { + // Transaction in mempool, treat as non-error. + return nil + } + if strings.Contains(err.Error(), "already in block") { + // Transaction was already mined, we don't + // consider this an error. + return nil + } + if strings.Contains(err.Error(), "txn-mempool-conflict") { + // Output was spent by other transaction + // already in the mempool. + return lnwallet.ErrDoubleSpend + } + if strings.Contains(err.Error(), "insufficient fee") { + // RBF enabled transaction did not have enough fee. + return lnwallet.ErrDoubleSpend + } + if strings.Contains(err.Error(), "Missing inputs") { + // Transaction is spending either output that + // is missing or already spent. + return lnwallet.ErrDoubleSpend + } + + case *chain.NeutrinoClient: + if strings.Contains(err.Error(), "already have") { + // Transaction was already in the mempool, do + // not treat as an error. + return nil + } + if strings.Contains(err.Error(), "already exists") { + // Transaction was already mined, we don't + // consider this an error. + return nil + } + if strings.Contains(err.Error(), "already spent") { + // Output was already spent. + return lnwallet.ErrDoubleSpend + } + + default: + } + return err + } + return nil } // extractBalanceDelta extracts the net balance delta from the PoV of the From d96b5b62eb843901a2b2d238aa6e34e9bd66eb5e Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 19 Jan 2018 14:24:30 +0100 Subject: [PATCH 3/8] lnwallet test: add test for PublishTransaction return errors --- lnwallet/interface_test.go | 324 +++++++++++++++++++++++++++++++++++++ 1 file changed, 324 insertions(+) diff --git a/lnwallet/interface_test.go b/lnwallet/interface_test.go index 612be9c7..5e002eb6 100644 --- a/lnwallet/interface_test.go +++ b/lnwallet/interface_test.go @@ -1114,6 +1114,326 @@ func testTransactionSubscriptions(miner *rpctest.Harness, } } +// testPublishTransaction checks that PublishTransaction returns the +// expected error types in case the transaction being published +// conflicts with the current mempool or chain. +func testPublishTransaction(r *rpctest.Harness, + alice, _ *lnwallet.LightningWallet, t *testing.T) { + + // mineAndAssert mines a block and ensures the passed TX + // is part of that block. + mineAndAssert := func(tx *wire.MsgTx) error { + blockHashes, err := r.Node.Generate(1) + if err != nil { + return fmt.Errorf("unable to generate block: %v", err) + } + + block, err := r.Node.GetBlock(blockHashes[0]) + if err != nil { + return fmt.Errorf("unable to find block: %v", err) + } + + if len(block.Transactions) != 2 { + return fmt.Errorf("expected 2 txs in block, got %d", + len(block.Transactions)) + } + + blockTx := block.Transactions[1] + if blockTx.TxHash() != tx.TxHash() { + return fmt.Errorf("incorrect transaction was mined") + } + + // Sleep for a second before returning, to make sure the + // block has propagated. + time.Sleep(1 * time.Second) + return nil + } + + // Generate a pubkey, and pay-to-addr script. + pubKey, err := alice.NewRawKey() + if err != nil { + t.Fatalf("unable to obtain public key: %v", err) + } + pubkeyHash := btcutil.Hash160(pubKey.SerializeCompressed()) + keyAddr, err := btcutil.NewAddressWitnessPubKeyHash(pubkeyHash, + &chaincfg.RegressionNetParams) + if err != nil { + t.Fatalf("unable to create addr: %v", err) + } + keyScript, err := txscript.PayToAddrScript(keyAddr) + if err != nil { + t.Fatalf("unable to generate script: %v", err) + } + + // txFromOutput takes a tx, and creates a new tx that spends + // the output from this tx, to an address derived from payToPubKey. + // NB: assumes that the output from tx is paid to pubKey. + txFromOutput := func(tx *wire.MsgTx, payToPubKey *btcec.PublicKey, + txFee btcutil.Amount) *wire.MsgTx { + // Create a script to pay to. + payToPubkeyHash := btcutil.Hash160(payToPubKey.SerializeCompressed()) + payToKeyAddr, err := btcutil.NewAddressWitnessPubKeyHash(payToPubkeyHash, + &chaincfg.RegressionNetParams) + if err != nil { + t.Fatalf("unable to create addr: %v", err) + } + payToScript, err := txscript.PayToAddrScript(payToKeyAddr) + if err != nil { + t.Fatalf("unable to generate script: %v", err) + } + + // We assume the output was paid to the keyScript made earlier. + var outputIndex uint32 + if len(tx.TxOut) == 1 || bytes.Equal(tx.TxOut[0].PkScript, keyScript) { + outputIndex = 0 + } else { + outputIndex = 1 + } + outputValue := tx.TxOut[outputIndex].Value + + // With the index located, we can create a transaction spending + // the referenced output. + tx1 := wire.NewMsgTx(2) + tx1.AddTxIn(&wire.TxIn{ + PreviousOutPoint: wire.OutPoint{ + Hash: tx.TxHash(), + Index: outputIndex, + }, + // We don't support RBF, so set sequence to max. + Sequence: wire.MaxTxInSequenceNum, + }) + tx1.AddTxOut(&wire.TxOut{ + Value: outputValue - int64(txFee), + PkScript: payToScript, + }) + + // Now we can populate the sign descriptor which we'll use to + // generate the signature. + signDesc := &lnwallet.SignDescriptor{ + PubKey: pubKey, + WitnessScript: keyScript, + Output: tx.TxOut[outputIndex], + HashType: txscript.SigHashAll, + SigHashes: txscript.NewTxSigHashes(tx1), + InputIndex: 0, // Has only one input. + } + + // With the descriptor created, we use it to generate a + // signature, then manually create a valid witness stack we'll + // use for signing. + spendSig, err := alice.Cfg.Signer.SignOutputRaw(tx1, signDesc) + if err != nil { + t.Fatalf("unable to generate signature: %v", err) + } + witness := make([][]byte, 2) + witness[0] = append(spendSig, byte(txscript.SigHashAll)) + witness[1] = pubKey.SerializeCompressed() + tx1.TxIn[0].Witness = witness + + // Finally, attempt to validate the completed transaction. This + // should succeed if the wallet was able to properly generate + // the proper private key. + vm, err := txscript.NewEngine(keyScript, + tx1, 0, txscript.StandardVerifyFlags, nil, + nil, outputValue) + if err != nil { + t.Fatalf("unable to create engine: %v", err) + } + if err := vm.Execute(); err != nil { + t.Fatalf("spend is invalid: %v", err) + } + return tx1 + } + + // newTx sends coins from Alice's wallet, mines this transaction, + // and creates a new, unconfirmed tx that spends this output to + // pubKey. + newTx := func() *wire.MsgTx { + + // With the script fully assembled, instruct the wallet to fund + // the output with a newly created transaction. + newOutput := &wire.TxOut{ + Value: btcutil.SatoshiPerBitcoin, + PkScript: keyScript, + } + txid, err := alice.SendOutputs([]*wire.TxOut{newOutput}, 10) + if err != nil { + t.Fatalf("unable to create output: %v", err) + } + + // Query for the transaction generated above so we can located + // the index of our output. + err = waitForMempoolTx(r, txid) + if err != nil { + t.Fatalf("tx not relayed to miner: %v", err) + } + tx, err := r.Node.GetRawTransaction(txid) + if err != nil { + t.Fatalf("unable to query for tx: %v", err) + } + + if err := mineAndAssert(tx.MsgTx()); err != nil { + t.Fatalf("unable to mine tx: %v", err) + } + txFee := btcutil.Amount(0.1 * btcutil.SatoshiPerBitcoin) + tx1 := txFromOutput(tx.MsgTx(), pubKey, txFee) + + return tx1 + } + + // We will first check that publishing a transaction already + // in the mempool does NOT return an error. Create the tx. + tx1 := newTx() + + // Publish the transaction. + if err := alice.PublishTransaction(tx1); err != nil { + t.Fatalf("unable to publish: %v", err) + } + + txid1 := tx1.TxHash() + err = waitForMempoolTx(r, &txid1) + if err != nil { + t.Fatalf("tx not relayed to miner: %v", err) + } + + // Publish the exact same transaction again. This should + // not return an error, even though the transaction is + // already in the mempool. + if err := alice.PublishTransaction(tx1); err != nil { + t.Fatalf("unable to publish: %v", err) + } + + // Mine the transaction. + if _, err := r.Node.Generate(1); err != nil { + t.Fatalf("unable to generate block: %v", err) + } + + // We'll now test that we don't get an error if we try + // to publish a transaction that is already mined. + // + // Create a new transaction. We must do this to properly + // test the reject messages from our peers. They might + // only send us a reject message for a given tx once, + // so we create a new to make sure it is not just + // immediately rejected. + tx2 := newTx() + + // Publish this tx. + if err := alice.PublishTransaction(tx2); err != nil { + t.Fatalf("unable to publish: %v", err) + } + + txid2 := tx2.TxHash() + err = waitForMempoolTx(r, &txid2) + if err != nil { + t.Fatalf("tx not relayed to miner: %v", err) + } + + // Mine the transaction. + if err := mineAndAssert(tx2); err != nil { + t.Fatalf("unable to mine tx: %v", err) + } + + // Publish the transaction again. It is already mined, + // and we don't expect this to return an error. + if err := alice.PublishTransaction(tx2); err != nil { + t.Fatalf("unable to publish: %v", err) + } + + // Now we'll try to double spend an output with a different + // transaction. Create a new tx and publish it. This is + // the output we'll try to double spend. + tx3 := newTx() + if err := alice.PublishTransaction(tx3); err != nil { + t.Fatalf("unable to publish: %v", err) + } + + txid3 := tx3.TxHash() + err = waitForMempoolTx(r, &txid3) + if err != nil { + t.Fatalf("tx not relayed to miner: %v", err) + } + + // Mine the transaction. + if err := mineAndAssert(tx3); err != nil { + t.Fatalf("unable to mine tx: %v", err) + } + + // Now we create a transaction that spends the output + // from the tx just mined. This should be accepted + // into the mempool. + txFee := btcutil.Amount(0.05 * btcutil.SatoshiPerBitcoin) + tx4 := txFromOutput(tx3, pubKey, txFee) + if err := alice.PublishTransaction(tx4); err != nil { + t.Fatalf("unable to publish: %v", err) + } + + txid4 := tx4.TxHash() + err = waitForMempoolTx(r, &txid4) + if err != nil { + t.Fatalf("tx not relayed to miner: %v", err) + } + + // Create a new key we'll pay to, to ensure we create + // a unique transaction. + pubKey2, err := alice.NewRawKey() + if err != nil { + t.Fatalf("unable to obtain public key: %v", err) + } + + // Create a new transaction that spends the output from + // tx3, and that pays to a different address. We expect + // this to be rejected because it is a double spend. + tx5 := txFromOutput(tx3, pubKey2, txFee) + if err := alice.PublishTransaction(tx5); err != lnwallet.ErrDoubleSpend { + t.Fatalf("expected ErrDoubleSpend, got: %v", err) + } + + // Create another transaction that spends the same output, + // but has a higher fee. We expect also this tx to be + // rejected, since the sequence number of tx3 is set to Max, + // indicating it is not replacable. + pubKey3, err := alice.NewRawKey() + if err != nil { + t.Fatalf("unable to obtain public key: %v", err) + } + tx6 := txFromOutput(tx3, pubKey3, 3*txFee) + + // Expect rejection. + if err := alice.PublishTransaction(tx6); err != lnwallet.ErrDoubleSpend { + t.Fatalf("expected ErrDoubleSpend, got: %v", err) + } + + // At last we try to spend an output already spent by a + // confirmed transaction. + // TODO(halseth): we currently skip this test for neutrino, + // as the backing btcd node will consider the tx being an + // orphan, and will accept it. Should look into if this is + // the behavior also for bitcoind, and update test + // accordingly. + if alice.BackEnd() != "neutrino" { + // Mine the tx spending tx3. + if err := mineAndAssert(tx4); err != nil { + t.Fatalf("unable to mine tx: %v", err) + } + + // Create another tx spending tx3. + pubKey4, err := alice.NewRawKey() + if err != nil { + t.Fatalf("unable to obtain public key: %v", err) + } + tx7 := txFromOutput(tx3, pubKey4, txFee) + + // Expect rejection. + if err := alice.PublishTransaction(tx7); err != lnwallet.ErrDoubleSpend { + t.Fatalf("expected ErrDoubleSpend, got: %v", err) + } + } + + // TODO(halseth): test replaceable transactions when btcd + // gets RBF support. +} + func testSignOutputUsingTweaks(r *rpctest.Harness, alice, _ *lnwallet.LightningWallet, t *testing.T) { @@ -1469,6 +1789,10 @@ var walletTests = []walletTestCase{ name: "transaction details", test: testListTransactionDetails, }, + { + name: "publish transaction", + test: testPublishTransaction, + }, { name: "signed with tweaked pubkeys", test: testSignOutputUsingTweaks, From db0928fa6f3a3e309cc7860ae525e13d0502107e Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 12 Jan 2018 15:29:46 +0100 Subject: [PATCH 4/8] chancloser: don't check error returned from broadcastTx This commit removes the inspection of the return error from broadcastTx. This is done since the new error checking added to PublishTransaction will return a nil error in case the transaction already exists in the mempool. --- chancloser.go | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/chancloser.go b/chancloser.go index 46cafe91..24dc18e5 100644 --- a/chancloser.go +++ b/chancloser.go @@ -2,7 +2,6 @@ package main import ( "fmt" - "strings" "github.com/davecgh/go-spew/spew" "github.com/lightningnetwork/lnd/channeldb" @@ -439,19 +438,7 @@ func (c *channelCloser) ProcessCloseMsg(msg lnwire.Message) ([]lnwire.Message, b return spew.Sdump(closeTx) })) if err := c.cfg.broadcastTx(closeTx); err != nil { - // TODO(halseth): add relevant error types to the - // WalletController interface as this is quite fragile. - switch { - case strings.Contains(err.Error(), "already exists"): - fallthrough - case strings.Contains(err.Error(), "already have"): - peerLog.Debugf("channel close tx from "+ - "ChannelPoint(%v) already exist, "+ - "probably broadcast by peer: %v", - c.chanPoint, err) - default: - return nil, false, err - } + return nil, false, err } // Clear out the current channel state, marking the channel as From 7aaa15b8b553f4dfbab72fad1c1ab6fc3116fa69 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 12 Jan 2018 15:31:02 +0100 Subject: [PATCH 5/8] utxonursery: don't ignore any returned error from PublishTransaction --- utxonursery.go | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/utxonursery.go b/utxonursery.go index 3c01967b..1f0045d7 100644 --- a/utxonursery.go +++ b/utxonursery.go @@ -5,7 +5,6 @@ import ( "encoding/binary" "fmt" "io" - "strings" "sync" "sync/atomic" @@ -1125,9 +1124,7 @@ func (u *utxoNursery) sweepMatureOutputs(classHeight uint32, finalTx *wire.MsgTx // With the sweep transaction fully signed, broadcast the transaction // to the network. Additionally, we can stop tracking these outputs as // they've just been swept. - // TODO(conner): handle concrete error types returned from publication - err := u.cfg.PublishTransaction(finalTx) - if err != nil && !strings.Contains(err.Error(), "TX rejected:") { + if err := u.cfg.PublishTransaction(finalTx); err != nil { utxnLog.Errorf("unable to broadcast sweep tx: %v, %v", err, spew.Sdump(finalTx)) return err @@ -1233,14 +1230,9 @@ func (u *utxoNursery) sweepCribOutput(classHeight uint32, baby *babyOutput) erro // We'll now broadcast the HTLC transaction, then wait for it to be // confirmed before transitioning it to kindergarten. - // - // TODO(conner): handle concrete error types returned from publication - err := u.cfg.PublishTransaction(baby.timeoutTx) - if err != nil && - !strings.Contains(err.Error(), "TX rejected:") { + if err := u.cfg.PublishTransaction(baby.timeoutTx); err != nil { utxnLog.Errorf("Unable to broadcast baby tx: "+ - "%v, %v", err, - spew.Sdump(baby.timeoutTx)) + "%v, %v", err, spew.Sdump(baby.timeoutTx)) return err } From 3fd7f28b39c2e7bd92cc5afb284c71d9d827fda6 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 12 Jan 2018 15:31:43 +0100 Subject: [PATCH 6/8] lnwallet: don't ignore any returned error from PublishTransaction --- lnwallet/wallet.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index 8adce2c3..8a1924ca 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -5,7 +5,6 @@ import ( "crypto/sha256" "fmt" "net" - "strings" "sync" "sync/atomic" @@ -1114,12 +1113,9 @@ func (l *LightningWallet) handleFundingCounterPartySigs(msg *addCounterPartySigs // Broadcast the finalized funding transaction to the network. if err := l.PublishTransaction(fundingTx); err != nil { - // TODO(roasbeef): need to make this into a concrete error - if !strings.Contains(err.Error(), "already have") { - msg.err <- err - msg.completeChan <- nil - return - } + msg.err <- err + msg.completeChan <- nil + return } msg.completeChan <- res.partialState From 2ae1b7dbbe5750acc46ed43e354d96cbe7977ccb Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Mon, 5 Feb 2018 17:26:35 -0500 Subject: [PATCH 7/8] contractcourt: remove TODO for checking double spends from PublishTx --- contractcourt/channel_arbitrator.go | 1 - contractcourt/contract_resolvers.go | 1 - 2 files changed, 2 deletions(-) diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index eb786d49..668309c1 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -449,7 +449,6 @@ func (c *ChannelArbitrator) stateStep(bestHeight uint32, bestHash *chainhash.Has // At this point, we'll now broadcast the commitment // transaction itself. if err := c.cfg.PublishTx(closeTx); err != nil { - // TODO(roasbeef): need to check for errors (duplicate) log.Errorf("ChannelArbitrator(%v): unable to broadcast "+ "close tx: %v", c.cfg.ChanPoint, err) return StateError, closeTx, err diff --git a/contractcourt/contract_resolvers.go b/contractcourt/contract_resolvers.go index 2fa4e2c7..5cc9ffa6 100644 --- a/contractcourt/contract_resolvers.go +++ b/contractcourt/contract_resolvers.go @@ -582,7 +582,6 @@ func (h *htlcSuccessResolver) Resolve() (ContractResolver, error) { // // TODO(roasbeef): after changing sighashes send to tx bundler if err := h.PublishTx(h.htlcResolution.SignedSuccessTx); err != nil { - // TODO(roasbeef): detect double spends return nil, err } From cabc07ea7d8e49e05edb9baa619ed767a2257c45 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 14 Feb 2018 12:28:19 +0100 Subject: [PATCH 8/8] breacharbiter: check ErrDoubleSpend from PublishTransaction --- breacharbiter.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/breacharbiter.go b/breacharbiter.go index 0093fb41..1771b81f 100644 --- a/breacharbiter.go +++ b/breacharbiter.go @@ -5,7 +5,6 @@ import ( "encoding/binary" "errors" "io" - "strings" "sync" "sync/atomic" @@ -580,7 +579,7 @@ secondLevelCheck: if err != nil { brarLog.Errorf("unable to broadcast "+ "justice tx: %v", err) - if strings.Contains(err.Error(), "already been spent") { + if err == lnwallet.ErrDoubleSpend { brarLog.Infof("Attempting to transfer HTLC revocations " + "to the second level") finalTx = nil