chain: misc BitcoindClient fixes #617
Conversation
…tion 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.
| @@ -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) | |||
There was a problem hiding this comment.
It's already logged by the wallet itself, so opted to remove this to avoid the duplicate info.
| // Determine if this was an RPC error thrown due to the transaction | ||
| // already confirming. | ||
| var rpcTxConfirmed bool | ||
| if rpcErr, ok := err.(*btcjson.RPCError); ok { |
There was a problem hiding this comment.
Possible to add a unit test for this behavior?
There was a problem hiding this comment.
It's somewhat already covered here.
There was a problem hiding this comment.
I was thinking a btcwallet unit test with a mocked chainClient. We could make the mock mimic the behavior of various backends/versions, and ensure we handle the diff errors correctly.
There was a problem hiding this comment.
Since we're doing string matching I'm not sure a unit test with a mock would work here. We'd just be including the same strings in there, which would seem to me like we're testing string.Contains.
wallet/wallet.go
Outdated
| // 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(err.Error(), "transaction already exists"): |
There was a problem hiding this comment.
would prefer if both of these cases were made case insensitive for robustness
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.
Depends on btcsuite/btcd#1428.