New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chain: share the same bitcoind connection between multiple rescan clients #511

Merged
merged 11 commits into from Aug 1, 2018

Conversation

Projects
None yet
3 participants
@wpaulino
Contributor

wpaulino commented Jun 22, 2018

In this PR, we introduce a nice optimization to btcwallet's interaction with a bitcoind node. This was mostly inspired by lightningnetwork/lnd#1174. Within lnd, we currently have three different subsystems responsible for watching the chain: chainntnfs, lnwallet, and routing/chainview. Each of these subsystems has an active RPC and ZMQ connection to the underlying bitcoind node. This would incur a toll on the underlying bitcoind node and would cause us to miss ZMQ events, which are crucial to lnd. This PR attempts to remedy this by reworking how each BitcoindClient retrieves these events.

We introduce a BitcoindConn struct which is responsible for maintaining the RPC and ZMQ connection to a bitcoind node. This struct will be shared among all of the BitcoindClients created, and it will send each event to the currently active clients. This allows us to reduce the toll on the underlying bitcoind node and increases the reliability of the delivery of ZMQ events.

Some other notable changes:

  • Used a concurrent unbounded queue for notification handling and delivery.
  • Improved overall documentation of methods and structs.

@wpaulino wpaulino force-pushed the wpaulino:bitcoind-rescan-client branch 6 times, most recently from 187b08e to 0a0d2cc Jun 22, 2018

@wpaulino wpaulino changed the title from chain: refactor bitcoind chain client to allow multiple rescan clients to chain: share the same bitcoind connection between multiple rescan clients Jun 26, 2018

@wpaulino wpaulino force-pushed the wpaulino:bitcoind-rescan-client branch from 0a0d2cc to ba461cd Jun 26, 2018

// Stop ends the goroutine that moves items from the in channel to the out
// channel.
func (cq *ConcurrentQueue) Stop() {

This comment has been minimized.

@Roasbeef

Roasbeef Jul 13, 2018

Member

If the caller isn't aware, also closing the chan in and out can lead to panics. The version we have within lnd atm only closes the quit channel due to that.

This comment has been minimized.

@wpaulino

wpaulino Jul 14, 2018

Contributor

Yeah, good point. I added it because we currently iterate over the notification channel in wallet.chainntfns.go, but I decided to modify that instead with a follow-up commit.

@@ -1157,83 +1150,3 @@ func (c *BitcoindClient) filterTx(tx *wire.MsgTx,
return notifyTx, rec, nil
}

This comment has been minimized.

@Roasbeef

Roasbeef Jul 13, 2018

Member

👍

Should eventually also be done for the neutrino and btcd backends.

@@ -364,6 +365,21 @@ func (c *BitcoindClient) Start() error {
return errors.New("mismatched networks")
}
// Get initial conditions.

This comment has been minimized.

@Roasbeef

Roasbeef Jul 13, 2018

Member

Why do this outside of establishing a connection? As is now, it's possible that we have stale information by the time the client is actually connected.

This comment has been minimized.

@wpaulino

wpaulino Jul 14, 2018

Contributor

I did it this way rather than within the goroutine (as it was done before) so that we actually return the error to the caller, signifying that something could potentially be wrong. FWIW, it will be updated within ntfnHandler as soon as a new block notification arrives from bitcoind.

@@ -235,7 +235,7 @@ func (c *BitcoindClient) LoadTxFilter(reset bool,
// If we reset, signal that.
if reset {
select {
case c.rescanUpdate <- reset:
case c.rescanUpdate <- struct{}{}:

This comment has been minimized.

@Roasbeef

Roasbeef Jul 13, 2018

Member

What's the diff?

This comment has been minimized.

@wpaulino

wpaulino Jul 14, 2018

Contributor

The event loop was previously not handling receiving bool as you can see in https://github.com/btcsuite/btcwallet/blob/master/chain/bitcoind.go#L570.

This comment has been minimized.

@halseth

halseth Jul 25, 2018

Contributor

So this didn't work before?

This comment has been minimized.

@wpaulino

wpaulino Jul 27, 2018

Contributor

I believe so yeah 😅

// ErrBitcoindClientShuttingDown is an error returned when we attempt
// to receive a notification for a specific item and the bitcoind client
// is in the middle of shutting down.
ErrBitcoindClientShuttingDown = errors.New("client is shutting down")

This comment has been minimized.

@Roasbeef

Roasbeef Jul 13, 2018

Member

Are we able to recover from this error?

This comment has been minimized.

@wpaulino

wpaulino Jul 14, 2018

Contributor

Nope. The only way this error can be returned is if the caller explicitly called Stop on either the BitcoindClient or BitcoindConn.

for {
select {
case update := <-c.rescanUpdate:
switch update := update.(type) {

This comment has been minimized.

@Roasbeef

Roasbeef Jul 13, 2018

Member

We're missing the two chain hash slice instances. Atm, these are used for tx confirmation notifications.

This comment has been minimized.

@wpaulino

wpaulino Jul 14, 2018

Contributor

Fixed.

c.clientMtx.RLock()
if _, ok := c.memPool[tx.TxHash()]; ok {
c.clientMtx.RUnlock()
if _, ok := c.mempool[tx.TxHash()]; ok {

This comment has been minimized.

@Roasbeef

Roasbeef Jul 13, 2018

Member

godoc comment should be updated to reflect that now the caller is responsible for holding the mutex while calling this method.

This comment has been minimized.

@wpaulino

wpaulino Jul 30, 2018

Contributor

Fixed.

// We have an event! We'll now determine which kind of event was
// read, deserialize it, and report it to the different rescan
// clients.
switch string(msgBytes[0]) {

This comment has been minimized.

@Roasbeef

Roasbeef Jul 13, 2018

Member

Should have a default case for logging purposes.

This comment has been minimized.

@wpaulino

wpaulino Jul 14, 2018

Contributor

Fixed.

// LoadTxFilter uses the given outpoints and addresses as filters to what we
// should match transactions against to determine if they are relevant to the
// client. The reset argument is used to reset the current filters.
func (c *BitcoindClient) LoadTxFilter(reset bool, outpoints []wire.OutPoint,

This comment has been minimized.

@Roasbeef

Roasbeef Jul 13, 2018

Member

This is too restrictive. As is, things beyond outpoint and addresses can be sent.

This comment has been minimized.

@wpaulino

wpaulino Jul 14, 2018

Contributor

Fixed.

switch string(msgBytes[0]) {
case "rawtx":
tx := &wire.MsgTx{}
if string(msgBytes[0]) == "rawblock" {

This comment has been minimized.

@Roasbeef

Roasbeef Jul 13, 2018

Member

Should have an else case for logging purposes.

This comment has been minimized.

@wpaulino

wpaulino Jul 14, 2018

Contributor

Fixed.

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Jul 13, 2018

While this is still in flight, we should also start to create the corresponding lnd PR to ensure that the set of integration tests that exercise bitcoind still pass.

@wpaulino wpaulino force-pushed the wpaulino:bitcoind-rescan-client branch from ab34c26 to 56d5944 Jul 14, 2018

@wpaulino

This comment has been minimized.

Contributor

wpaulino commented Jul 14, 2018

I already have a local branch for lnd that incorporates these changes, was just waiting on the switch to btcsuite.

@wpaulino wpaulino force-pushed the wpaulino:bitcoind-rescan-client branch from 56d5944 to a342da8 Jul 16, 2018

@wpaulino wpaulino force-pushed the wpaulino:bitcoind-rescan-client branch from a342da8 to dd6d99b Jul 17, 2018

@halseth

On first review pass, these changes look very solid to me!

// popping items from the out channel. There is a goroutine that manages moving
// items from the in channel to the out channel in the correct order that must
// be started by calling Start().
type ConcurrentQueue struct {

This comment has been minimized.

@halseth

halseth Jul 17, 2018

Contributor

Maybe not in the scope for this PR, but would be nice to extract this into its own package/repo, such that it could be shared between lnd and btcwallet,

// expiredMempool keeps track of a set of confirmed transactions along
// with the height at which they were included in a block. These
// transactions will then be removed from the mempool after a period of
// 288 blocks. This is done to ensure the tranasctions are safe from a

This comment has been minimized.

@halseth

halseth Jul 17, 2018

Contributor

nit: tranasctions

This comment has been minimized.

@wpaulino

wpaulino Jul 21, 2018

Contributor

Fixed.

select {
case update := <-c.rescanUpdate:
switch update := update.(type) {
case struct{}:

This comment has been minimized.

@halseth

halseth Jul 17, 2018

Contributor

The cases should probably have a short comment explaining them.

This comment has been minimized.

@wpaulino

wpaulino Jul 21, 2018

Contributor

Fixed.

@@ -1097,6 +1024,9 @@ func (c *BitcoindClient) filterTx(tx *wire.MsgTx,
blockDetails *btcjson.BlockDetails, notify bool) (bool,
*wtxmgr.TxRecord, error) {
c.mu.Lock()

This comment has been minimized.

@halseth

halseth Jul 17, 2018

Contributor

nit: move the locking down to where it is first needed

This comment has been minimized.

@wpaulino

wpaulino Jul 21, 2018

Contributor

Fixed.

msgBytes, err := conn.Receive()
if err != nil {
err, ok := err.(net.Error)
if !ok || (ok && !err.Timeout()) {

This comment has been minimized.

@halseth

halseth Jul 17, 2018

Contributor

second ok can be removed

This comment has been minimized.

@wpaulino

wpaulino Jul 21, 2018

Contributor

Fixed.

msgBytes, err := conn.Receive()
if err != nil {
err, ok := err.(net.Error)
if !ok || (ok && !err.Timeout()) {

This comment has been minimized.

@halseth

halseth Jul 17, 2018

Contributor

same

This comment has been minimized.

@wpaulino

wpaulino Jul 21, 2018

Contributor

Fixed.

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Jul 21, 2018

Needs a rebase!

@wpaulino wpaulino force-pushed the wpaulino:bitcoind-rescan-client branch 2 times, most recently from 19b12a2 to 56e6daf Jul 21, 2018

bs := &waddrmgr.BlockStamp{Hash: *hash, Height: height}
// TODO: Rather than leaving this as an unbounded queue for all types of

This comment has been minimized.

@halseth

halseth Jul 25, 2018

Contributor

This red so pretty 😭😭

// We have a reorg.
err = c.reorg(bs, block)
err = c.reorg(bestBlock, block)

This comment has been minimized.

@halseth

halseth Jul 25, 2018

Contributor

At this point the mutex is unlocked.

This comment has been minimized.

@wpaulino

wpaulino Jul 27, 2018

Contributor

That's fine as it is a copy.

bs.Timestamp = block.Header.Timestamp
_, err = c.filterBlock(block, bs.Height, true)
bestBlock.Hash = block.BlockHash()
bestBlock.Height++

This comment has been minimized.

@halseth

halseth Jul 25, 2018

Contributor

This seems dangerous.. Are we incrementing the height of c.bestBlock here, or only the copy?

If we are intentionally working on the copy, it seems like we could release the mutex right after making the copy. If not, it seems like we should modify c.bestBlock directly (or making it a pointer).

This comment has been minimized.

@wpaulino

wpaulino Jul 27, 2018

Contributor

Fixed. A copy is made so that we can also use it within c.reorg. In the case that this is the next block, we'll filter the new block, modify the copy to be the new block, and finally assign it to c.bestBlock.

@@ -16,10 +16,11 @@ import (
)
func (w *Wallet) handleChainNotifications() {
defer w.wg.Done()

This comment has been minimized.

@halseth

halseth Jul 25, 2018

Contributor

👍

}
c.quitMtx.Unlock()
close(c.quit)

This comment has been minimized.

@halseth

halseth Jul 25, 2018

Contributor

Much nicer!

// Once the client has started successfully, we'll include it in the set
// of rescan clients of the backing bitcoind connection in order to
// received ZMQ event notifications.
c.chainConn.rescanClientsMtx.Lock()

This comment has been minimized.

@halseth

halseth Jul 25, 2018

Contributor

Feels like this could be done in an AddClient method on the ChainConn.

This comment has been minimized.

@wpaulino

wpaulino Jul 27, 2018

Contributor

Good point, fixed!

// Remove this client's reference from the bitcoind connection to
// prevent sending notifications to it after it's been stopped.
c.chainConn.rescanClientsMtx.Lock()

This comment has been minimized.

@halseth

halseth Jul 25, 2018

Contributor

same, RemoveClient method on ChainConn

This comment has been minimized.

@wpaulino

wpaulino Jul 27, 2018

Contributor

Fixed.

select {
case c.rescanUpdate <- outPoints:
case <-c.quit:
return ErrBitcoindClientShuttingDown
}
// Kick off the rescan with the starting block hash.

This comment has been minimized.

@halseth

halseth Jul 25, 2018

Contributor

I think we should keep these comments.

This comment has been minimized.

@wpaulino

wpaulino Jul 27, 2018

Contributor

Fixed.

@@ -1144,5 +1212,7 @@ func (c *BitcoindClient) filterTx(tx *wire.MsgTx,
c.mempool[tx.TxHash()] = struct{}{}
}
return notifyTx, rec, nil
c.onRelevantTx(rec, blockDetails)

This comment has been minimized.

@halseth

halseth Jul 25, 2018

Contributor

Is this new behaviour?

This comment has been minimized.

@wpaulino

wpaulino Jul 27, 2018

Contributor

Nope, the logic was simply reworked to make it a bit easier to follow.

// GetBestBlock returns the highest block known to bitcoind.
func (c *BitcoindClient) GetBestBlock() (*chainhash.Hash, int32, error) {
bcinfo, err := c.client.GetBlockChainInfo()
bcinfo, err := c.chainConn.client.GetBlockChainInfo()

This comment has been minimized.

@halseth

halseth Jul 25, 2018

Contributor

you have b.bestBlock, any reason it is not used?

This comment has been minimized.

@wpaulino

wpaulino Jul 27, 2018

Contributor

Not sure, just copied the existing behavior.

wpaulino added some commits Jun 26, 2018

chain: keep track of the best block within BitcoindClient
In the previous commit, we modified our BitcoindClient struct to use a
ConcurrentQueue struct to handle its notifications to the caller. Before
this, the BitcoindClient had a goroutine that would handle these
notifications in the background and detect when a OnBlockConnected
notification was received in order to update the best block. Due to this
logic being removed, we now keep track of the best block througout the
struct as a whole.

@wpaulino wpaulino force-pushed the wpaulino:bitcoind-rescan-client branch from 56e6daf to dbb1261 Jul 27, 2018

c.bestBlockMtx.Unlock()
if block.Header.PrevBlock == bestBlock.Hash {
_, err := c.filterBlock(
block, bestBlock.Height, true,

This comment has been minimized.

@halseth

halseth Jul 27, 2018

Contributor

should be block.Height?

This comment has been minimized.

@wpaulino

wpaulino Jul 30, 2018

Contributor

bestBlock.Height+1 actually. Nice catch 🙈

continue
}
// With the block succesfully filtered, we'll

This comment has been minimized.

@halseth

halseth Jul 27, 2018

Contributor

This is easier to follow! 👍

return fmt.Errorf("unable to update filter: unknown "+
"type %T", list)
}
if err != nil {

This comment has been minimized.

@halseth

halseth Jul 27, 2018

Contributor

doesn't look like this can ever be non-nil

This comment has been minimized.

@wpaulino

wpaulino Jul 30, 2018

Contributor

Fixed.

// watchedAddresses, watchedOutPoints, and watchedTxs are the set of
// items we should match transactions against while processing a chain
// rescan to determine if they are relevant to the client.
mu sync.RWMutex

This comment has been minimized.

@halseth

halseth Jul 27, 2018

Contributor

nit: if this mustex is used exclusively for these maps, a better name would be watchMtx or similar

This comment has been minimized.

@wpaulino

wpaulino Jul 30, 2018

Contributor

Fixed.

// GetBestBlock returns the highest block known to bitcoind.
func (c *BitcoindClient) GetBestBlock() (*chainhash.Hash, int32, error) {
bcinfo, err := c.client.GetBlockChainInfo()
bcinfo, err := c.chainConn.client.GetBlockChainInfo()

This comment has been minimized.

@halseth

halseth Jul 27, 2018

Contributor

I think it is actually better to return the bestBlock here, as we might risk that and what gets returned from the backend to be out of sync, and I just imagine that could lead to weird issues.

This comment has been minimized.

@wpaulino

wpaulino Jul 30, 2018

Contributor

GetBestBlock is actually needed for whenever we detect a reorg, as it will fetch the actual tip of the chain. We could however implement a GetClientBestBlock method which returns c.bestBlock.

// If this transaction matched any of our filters but it has yet to be
// confirmed, we'll defer sending its notification until it has been
// mined as part of FilteredBlockConnected. Otherwise, we'll send a

This comment has been minimized.

@halseth

halseth Jul 27, 2018

Contributor

"Otherwise, we'll send a notification immediately" -> I don't see that this is being done

This comment has been minimized.

@wpaulino

wpaulino Jul 30, 2018

Contributor

This can be ignored, an incorrect fixup commit was applied to this.

return false, rec, nil
}
// If this transaction matched any of our filters but it has yet to be

This comment has been minimized.

@halseth

halseth Jul 27, 2018

Contributor

Interesting, so we never forward unconfirmed transactions to the wallet?

This comment has been minimized.

@wpaulino

wpaulino Jul 30, 2018

Contributor

We used to and we still do.

@@ -1121,6 +1121,14 @@ func (c *BitcoindClient) filterTx(tx *wire.MsgTx,
}
// If the transaction didn't pay to any of our watched addresses, we'll
// check if we're currently watching for the hash of this transaction.
if !isRelevant {

This comment has been minimized.

@halseth

halseth Jul 27, 2018

Contributor

nit: the outer if can be removed, really.

headers.PushBack(lastHeader)
// We always send a RescanFinished message when we're done.
defer func() {

This comment has been minimized.

@halseth

halseth Jul 27, 2018

Contributor

why don't we want to defer this?

This comment has been minimized.

@wpaulino

wpaulino Jul 30, 2018

Contributor

Fixed.

GetBlockHeaderVerboseResult)
lastHash, err = chainhash.
NewHashFromStr(lastHeader.Hash)
previousHeader = headers.Back().Value.(*btcjson.GetBlockHeaderVerboseResult)

This comment has been minimized.

@halseth

halseth Jul 27, 2018

Contributor

line a bit long. Possible to keep on multiple lines?

This comment has been minimized.

@wpaulino

wpaulino Jul 30, 2018

Contributor

Fixed.

@wpaulino wpaulino force-pushed the wpaulino:bitcoind-rescan-client branch 4 times, most recently from 51a1a66 to 00c24f9 Jul 30, 2018

wpaulino added some commits Jun 22, 2018

wallet: remove the need to set the birthday for bitcoind chain clients
Due to the previous commit allowing us to specify the birthday of the
wallet at the time of the BitcoindClient's creation, this is now
unnecessary.
wallet: stop handling chain notifications once wallet has stopped
In this commit, we alter the behavior for handling chain notifications
within the wallet. The previous code would assume that the channel would
close, but due to now using a ConcurrentQueue to handle notifications,
this assumption no longer stands. Now, we'll stop handling notifications
either once the wallet has or stopped or once the notifications channel
has been closed.
chain: match transaction against currently watched transactions
In this commit, we extend the client's filtering process to also look at
the set of currently watched transactions. The logic to watch for
transaction hashes was previously there, but it was not used to filter
against incoming transactions.

@wpaulino wpaulino force-pushed the wpaulino:bitcoind-rescan-client branch from 00c24f9 to dec9978 Jul 31, 2018

@Roasbeef

LGTM 🔮

@Roasbeef Roasbeef merged commit 1ede0a1 into btcsuite:master Aug 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wpaulino wpaulino deleted the wpaulino:bitcoind-rescan-client branch Aug 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment