From 8d6dd92706b8c4dd5bbaa68f295b820fb24c2e92 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Sat, 11 May 2019 13:05:17 -0700 Subject: [PATCH 1/5] chain: fix Fatalf format verb error --- chain/block_filterer_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chain/block_filterer_test.go b/chain/block_filterer_test.go index 7ae8bb9..7b9e222 100644 --- a/chain/block_filterer_test.go +++ b/chain/block_filterer_test.go @@ -319,6 +319,6 @@ func assertRelevantTxnsContains(t *testing.T, bf *chain.BlockFilterer, wantTx *w } } - t.Fatalf("unable to find tx: %x in %d relevant txns", wantTx, + t.Fatalf("unable to find tx: %x in %d relevant txns", wantTx.TxHash(), len(bf.RelevantTxns)) } From ecfebcea030ec5c23fdf06304f180d0c6edac163 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Sat, 11 May 2019 13:05:23 -0700 Subject: [PATCH 2/5] chain: remove unnecessary returned error from BitcoindClient.filterBlock --- chain/bitcoind_client.go | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/chain/bitcoind_client.go b/chain/bitcoind_client.go index e193800..148f8cc 100644 --- a/chain/bitcoind_client.go +++ b/chain/bitcoind_client.go @@ -336,7 +336,7 @@ func (c *BitcoindClient) RescanBlocks( continue } - relevantTxs, err := c.filterBlock(block, header.Height, false) + relevantTxs := c.filterBlock(block, header.Height, false) if len(relevantTxs) > 0 { rescannedBlock := btcjson.RescannedBlock{ Hash: hash.String(), @@ -567,14 +567,7 @@ func (c *BitcoindClient) ntfnHandler() { c.bestBlockMtx.Unlock() if newBlock.Header.PrevBlock == bestBlock.Hash { newBlockHeight := bestBlock.Height + 1 - _, err := c.filterBlock( - newBlock, newBlockHeight, true, - ) - if err != nil { - log.Errorf("Unable to filter block %v: %v", - newBlock.BlockHash(), err) - continue - } + _ = c.filterBlock(newBlock, newBlockHeight, true) // With the block succesfully filtered, we'll // make it our new best block. @@ -843,10 +836,7 @@ func (c *BitcoindClient) reorg(currentBlock waddrmgr.BlockStamp, return err } - _, err = c.filterBlock(nextBlock, nextHeight, true) - if err != nil { - return err - } + _ = c.filterBlock(nextBlock, nextHeight, true) currentBlock.Height = nextHeight currentBlock.Hash = nextHash @@ -1065,9 +1055,7 @@ func (c *BitcoindClient) rescan(start chainhash.Hash) error { headers.PushBack(previousHeader) // Notify the block and any of its relevant transacations. - if _, err = c.filterBlock(block, i, true); err != nil { - return err - } + _ = c.filterBlock(block, i, true) if i%10000 == 0 { c.onRescanProgress( @@ -1101,12 +1089,12 @@ func (c *BitcoindClient) rescan(start chainhash.Hash) error { // filterBlock filters a block for watched outpoints and addresses, and returns // any matching transactions, sending notifications along the way. func (c *BitcoindClient) filterBlock(block *wire.MsgBlock, height int32, - notify bool) ([]*wtxmgr.TxRecord, error) { + notify bool) []*wtxmgr.TxRecord { // If this block happened before the client's birthday, then we'll skip // it entirely. if block.Header.Timestamp.Before(c.birthday) { - return nil, nil + return nil } if c.shouldNotifyBlocks() { @@ -1162,7 +1150,7 @@ func (c *BitcoindClient) filterBlock(block *wire.MsgBlock, height int32, c.onBlockConnected(&blockHash, height, block.Header.Timestamp) } - return relevantTxs, nil + return relevantTxs } // filterTx determines whether a transaction is relevant to the client by From 28161bee422143606d947aa06f211e94fbe9deee Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Sat, 11 May 2019 13:05:27 -0700 Subject: [PATCH 3/5] chain: improve error logging within BitcoindClient.reorg --- chain/bitcoind_client.go | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/chain/bitcoind_client.go b/chain/bitcoind_client.go index 148f8cc..ff27ee9 100644 --- a/chain/bitcoind_client.go +++ b/chain/bitcoind_client.go @@ -738,19 +738,22 @@ func (c *BitcoindClient) onRescanFinished(hash *chainhash.Hash, height int32, func (c *BitcoindClient) reorg(currentBlock waddrmgr.BlockStamp, reorgBlock *wire.MsgBlock) error { - log.Debugf("Possible reorg at block %s", reorgBlock.BlockHash()) - // Retrieve the best known height based on the block which caused the // reorg. This way, we can preserve the chain of blocks we need to // retrieve. bestHash := reorgBlock.BlockHash() bestHeight, err := c.GetBlockHeight(&bestHash) if err != nil { - return err + return fmt.Errorf("unable to get block height for %v: %v", + bestHash, err) } + log.Debugf("Possible reorg at block: height=%v, hash=%s", bestHash, + bestHeight) + if bestHeight < currentBlock.Height { - log.Debug("Detected multiple reorgs") + log.Debugf("Detected multiple reorgs: best_height=%v below "+ + "current_height=%v", bestHeight, currentBlock.Height) return nil } @@ -763,7 +766,8 @@ func (c *BitcoindClient) reorg(currentBlock waddrmgr.BlockStamp, for i := bestHeight - 1; i >= currentBlock.Height; i-- { block, err := c.GetBlock(&previousBlock) if err != nil { - return err + return fmt.Errorf("unable to get block %v: %v", + previousBlock, err) } blocksToNotify.PushFront(block) previousBlock = block.Header.PrevBlock @@ -776,7 +780,8 @@ func (c *BitcoindClient) reorg(currentBlock waddrmgr.BlockStamp, // We'll start by retrieving the header to the best block known to us. currentHeader, err := c.GetBlockHeader(¤tBlock.Hash) if err != nil { - return err + return fmt.Errorf("unable to get block header for %v: %v", + currentBlock.Hash, err) } // Then, we'll walk backwards in the chain until we find our common @@ -785,8 +790,8 @@ func (c *BitcoindClient) reorg(currentBlock waddrmgr.BlockStamp, // Since the previous hashes don't match, the current block has // been reorged out of the chain, so we should send a // BlockDisconnected notification for it. - log.Debugf("Disconnecting block %d (%v)", currentBlock.Height, - currentBlock.Hash) + log.Debugf("Disconnecting block: height=%v, hash=%v", + currentBlock.Height, currentBlock.Hash) c.onBlockDisconnected( ¤tBlock.Hash, currentBlock.Height, @@ -797,7 +802,8 @@ func (c *BitcoindClient) reorg(currentBlock waddrmgr.BlockStamp, // continue the common ancestor search. currentHeader, err = c.GetBlockHeader(¤tHeader.PrevBlock) if err != nil { - return err + return fmt.Errorf("unable to get block header for %v: %v", + currentHeader.PrevBlock, err) } currentBlock.Height-- @@ -808,7 +814,8 @@ func (c *BitcoindClient) reorg(currentBlock waddrmgr.BlockStamp, // once we've found our common ancestor. block, err := c.GetBlock(&previousBlock) if err != nil { - return err + return fmt.Errorf("unable to get block %v: %v", + previousBlock, err) } blocksToNotify.PushFront(block) previousBlock = block.Header.PrevBlock @@ -817,8 +824,8 @@ func (c *BitcoindClient) reorg(currentBlock waddrmgr.BlockStamp, // Disconnect the last block from the old chain. Since the previous // block remains the same between the old and new chains, the tip will // now be the last common ancestor. - log.Debugf("Disconnecting block %d (%v)", currentBlock.Height, - currentBlock.Hash) + log.Debugf("Disconnecting block: height=%v, hash=%v", + currentBlock.Height, currentBlock.Hash) c.onBlockDisconnected( ¤tBlock.Hash, currentBlock.Height, currentHeader.Timestamp, @@ -833,7 +840,8 @@ func (c *BitcoindClient) reorg(currentBlock waddrmgr.BlockStamp, nextHash := nextBlock.BlockHash() nextHeader, err := c.GetBlockHeader(&nextHash) if err != nil { - return err + return fmt.Errorf("unable to get block header for %v: %v", + nextHash, err) } _ = c.filterBlock(nextBlock, nextHeight, true) @@ -906,8 +914,6 @@ func (c *BitcoindClient) FilterBlocks( // the client in the watch list. This is called only within a queue processing // loop. func (c *BitcoindClient) rescan(start chainhash.Hash) error { - log.Infof("Starting rescan from block %s", start) - // We start by getting the best already processed block. We only use // the height, as the hash can change during a reorganization, which we // catch by testing connectivity from known blocks to the previous From 0efe836773ec5ab078a508be1998ca9560a9d1b3 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Sat, 11 May 2019 13:05:32 -0700 Subject: [PATCH 4/5] chain: avoid using defer for BitcoindClient onRescanFinished notification Since defer will copy the function with the parameters evaluated at its invocation, the RescanFinished notification would be dispatched with the incorrect block. To fix this, we'll just send the notification at the end ourselves manually. --- chain/bitcoind_client.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/chain/bitcoind_client.go b/chain/bitcoind_client.go index ff27ee9..2e42e7d 100644 --- a/chain/bitcoind_client.go +++ b/chain/bitcoind_client.go @@ -945,13 +945,6 @@ func (c *BitcoindClient) rescan(start chainhash.Hash) error { } headers.PushBack(previousHeader) - // Queue a RescanFinished notification to the caller with the last block - // processed throughout the rescan once done. - defer c.onRescanFinished( - previousHash, previousHeader.Height, - time.Unix(previousHeader.Time, 0), - ) - // Cycle through all of the blocks known to bitcoind, being mindful of // reorgs. for i := previousHeader.Height + 1; i <= bestBlock.Height; i++ { @@ -1089,6 +1082,8 @@ func (c *BitcoindClient) rescan(start chainhash.Hash) error { } } + c.onRescanFinished(bestHash, bestHeight, time.Unix(bestHeader.Time, 0)) + return nil } From bf9cc2004560704bef177f98baec6a2b989eefc7 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Sat, 11 May 2019 13:05:36 -0700 Subject: [PATCH 5/5] wallet: check RPC error code for rejected confirmed transactions This unifies the logic of receiving an error when broadcasting a confirmed transaction through btcd's/bitcoind's RPC interface. The btcd dependency update is required in order for it to match bitcoind's behavior. For older nodes that have yet to update, the confirmed transaction will still be caught by the "transaction already exists" case. This is not needed for bitcoind however, because its been sending the same RPC error code for several major releases now. --- go.mod | 4 ++-- go.sum | 5 ++++- wallet/wallet.go | 34 ++++++++++++++++++++++++---------- 3 files changed, 30 insertions(+), 13 deletions(-) diff --git a/go.mod b/go.mod index 1f240d7..6f83b2c 100644 --- a/go.mod +++ b/go.mod @@ -1,9 +1,9 @@ module github.com/btcsuite/btcwallet require ( - github.com/btcsuite/btcd v0.0.0-20190213025234-306aecffea32 + github.com/btcsuite/btcd v0.0.0-20190523000118-16327141da8c github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f - github.com/btcsuite/btcutil v0.0.0-20190207003914-4c204d697803 + github.com/btcsuite/btcutil v0.0.0-20190425235716-9e5f4b9a998d github.com/btcsuite/golangcrypto v0.0.0-20150304025918-53f62d9b43e8 github.com/btcsuite/websocket v0.0.0-20150119174127-31079b680792 github.com/coreos/bbolt v1.3.2 diff --git a/go.sum b/go.sum index 7a9c5c7..664ee2e 100644 --- a/go.sum +++ b/go.sum @@ -4,14 +4,17 @@ github.com/aead/siphash v1.0.1/go.mod h1:Nywa3cDsYNNK3gaciGTWPwHt0wlpNV15vwmswBA github.com/boltdb/bolt v1.3.1 h1:JQmyP4ZBrce+ZQu0dY660FMfatumYDLun9hBCUVIkF4= github.com/boltdb/bolt v1.3.1/go.mod h1:clJnj/oiGkjum5o1McbSZDSLxVThjynRyGBgiAx27Ps= github.com/btcsuite/btcd v0.0.0-20180823030728-d81d8877b8f3/go.mod h1:Dmm/EzmjnCiweXmzRIAiUWCInVmPgjkzgv5k4tVyXiQ= -github.com/btcsuite/btcd v0.0.0-20190213025234-306aecffea32 h1:qkOC5Gd33k54tobS36cXdAzJbeHaduLtnLQQwNoIi78= github.com/btcsuite/btcd v0.0.0-20190213025234-306aecffea32/go.mod h1:DrZx5ec/dmnfpw9KyYoQyYo7d0KEvTkk/5M/vbZjAr8= +github.com/btcsuite/btcd v0.0.0-20190523000118-16327141da8c h1:aEbSeNALREWXk0G7UdNhR3ayBV7tZ4M2PNmnrCAph6Q= +github.com/btcsuite/btcd v0.0.0-20190523000118-16327141da8c/go.mod h1:3J08xEfcugPacsc34/LKRU2yO7YmuT8yt28J8k2+rrI= github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f h1:bAs4lUbRJpnnkd9VhRV3jjAVU7DJVjMaK+IsvSeZvFo= github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f/go.mod h1:TdznJufoqS23FtqVCzL0ZqgP5MqXbb4fg/WgDys70nA= github.com/btcsuite/btcutil v0.0.0-20180706230648-ab6388e0c60a h1:RQMUrEILyYJEoAT34XS/kLu40vC0+po/UfxrBBA4qZE= github.com/btcsuite/btcutil v0.0.0-20180706230648-ab6388e0c60a/go.mod h1:+5NJ2+qvTyV9exUAL/rxXi3DcLg2Ts+ymUAY5y4NvMg= github.com/btcsuite/btcutil v0.0.0-20190207003914-4c204d697803 h1:j3AgPKKZtZStM2nyhrDSLSYgT7YHrZKdSkq1OYeLjvM= github.com/btcsuite/btcutil v0.0.0-20190207003914-4c204d697803/go.mod h1:+5NJ2+qvTyV9exUAL/rxXi3DcLg2Ts+ymUAY5y4NvMg= +github.com/btcsuite/btcutil v0.0.0-20190425235716-9e5f4b9a998d h1:yJzD/yFppdVCf6ApMkVy8cUxV0XrxdP9rVf6D87/Mng= +github.com/btcsuite/btcutil v0.0.0-20190425235716-9e5f4b9a998d/go.mod h1:+5NJ2+qvTyV9exUAL/rxXi3DcLg2Ts+ymUAY5y4NvMg= github.com/btcsuite/btcwallet v0.0.0-20180904010540-284e2e0e696e33d5be388f7f3d9a26db703e0c06/go.mod h1:/d7QHZsfUAruXuBhyPITqoYOmJ+nq35qPsJjz/aSpCg= github.com/btcsuite/btcwallet v0.0.0-20190313032608-acf3b04b0273/go.mod h1:mkOYY8/psBiL5E+Wb0V7M0o+N7NXi2SZJz6+RKkncIc= github.com/btcsuite/go-socks v0.0.0-20170105172521-4720035b7bfd h1:R/opQEbFEy9JGkIguV40SvRY1uliPX8ifOvi6ICsFCw= diff --git a/wallet/wallet.go b/wallet/wallet.go index f9cce35..1c5755f 100644 --- a/wallet/wallet.go +++ b/wallet/wallet.go @@ -3414,6 +3414,14 @@ func (w *Wallet) publishTransaction(tx *wire.MsgTx) (*chainhash.Hash, error) { } txid, err := chainClient.SendRawTransaction(tx, false) + + // Determine if this was an RPC error thrown due to the transaction + // already confirming. + var rpcTxConfirmed bool + if rpcErr, ok := err.(*btcjson.RPCError); ok { + rpcTxConfirmed = rpcErr.Code == btcjson.ErrRPCTxAlreadyInChain + } + switch { case err == nil: return txid, nil @@ -3425,12 +3433,16 @@ func (w *Wallet) publishTransaction(tx *wire.MsgTx) (*chainhash.Hash, error) { // // This error is returned when broadcasting/sending a transaction to a // btcd node that already has it in their mempool. - case strings.Contains(err.Error(), "already have transaction"): + case strings.Contains( + strings.ToLower(err.Error()), "already have transaction", + ): fallthrough // This error is returned when broadcasting a transaction to a bitcoind // node that already has it in their mempool. - case strings.Contains(err.Error(), "txn-already-in-mempool"): + case strings.Contains( + strings.ToLower(err.Error()), "txn-already-in-mempool", + ): return txid, nil // If the transaction has already confirmed, we can safely remove it @@ -3438,19 +3450,21 @@ func (w *Wallet) publishTransaction(tx *wire.MsgTx) (*chainhash.Hash, error) { // confirmed store. We'll avoid returning an error as the broadcast was // in a sense successful. // - // This error is returned when broadcasting/sending a transaction that - // has already confirmed to a btcd node. - case strings.Contains(err.Error(), "transaction already exists"): + // This error is returned when sending a transaction that has already + // confirmed to a btcd/bitcoind node over RPC. + case rpcTxConfirmed: fallthrough // This error is returned when broadcasting a transaction that has - // already confirmed to a bitcoind node. - case strings.Contains(err.Error(), "txn-already-known"): + // already confirmed to a btcd node over the P2P network. + case strings.Contains( + strings.ToLower(err.Error()), "transaction already exists", + ): fallthrough - // This error is returned when sending a transaction that has already - // confirmed to a bitcoind node over RPC. - case strings.Contains(err.Error(), "transaction already in block chain"): + // This error is returned when broadcasting a transaction that has + // already confirmed to a bitcoind node over the P2P network. + case strings.Contains(strings.ToLower(err.Error()), "txn-already-known"): dbErr := walletdb.Update(w.db, func(dbTx walletdb.ReadWriteTx) error { txmgrNs := dbTx.ReadWriteBucket(wtxmgrNamespaceKey) txRec, err := wtxmgr.NewTxRecordFromMsgTx(tx, time.Now())