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

multi: share the same bitcoind connection between multiple rescan clients #1566

Merged
merged 4 commits into from Aug 10, 2018

Conversation

Projects
None yet
5 participants
@wpaulino
Collaborator

wpaulino commented Jul 17, 2018

In this PR, we introduce a nice optimization with regards to lnd's interaction with a bitcoind backend. 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. We remedy this issue by sharing the same connection to a bitcoind node between the different clients within lnd.

We now also require the backing bitcoind node to use different hosts to provide its ZMQ raw block and raw transaction notifications (zmqpubrawblock and zmqpubrawtx). This was needed as the notification queue maintained by the bitcoind node would sometimes overflow with transactions and would cause block notifications to be dropped/missed.

Depends on btcsuite/btcwallet#511.
Fixes #1174.

@wpaulino wpaulino force-pushed the wpaulino:bitcoind-rescan-client branch 2 times, most recently from deec23e to 9d7f4f5 Jul 17, 2018

@Roasbeef Roasbeef requested a review from halseth Jul 17, 2018

@Roasbeef Roasbeef added this to the 0.5 milestone Jul 17, 2018

@halseth

utACK, pending the btcwallet dependency 👍

config.go Outdated
RPCUser string `long:"rpcuser" description:"Username for RPC connections"`
RPCPass string `long:"rpcpass" default-mask:"-" description:"Password for RPC connections"`
ZMQBlockHost string `long:"zmqblockhost" description:"The ZMQ host providing raw block notifications"`
ZMQTxHost string `long:"zmqtxhost" description:"The ZMQ host providing raw transaction notifications"`

This comment has been minimized.

@halseth

halseth Jul 17, 2018

Collaborator

Does it make sense to call these zmqpubrawtx and zmqpubrawblock?

This comment has been minimized.

@wpaulino

wpaulino Jul 21, 2018

Collaborator

Fixed.

"github.com/btcsuite/btcwallet/walletdb"
_ "github.com/btcsuite/btcwallet/walletdb/bdb" // Required to register the boltdb walletdb implementation.
"github.com/lightninglabs/neutrino"
"github.com/ltcsuite/ltcd/btcjson"

This comment has been minimized.

@halseth

halseth Jul 17, 2018

Collaborator

guess this should be btcd, really

This comment has been minimized.

@wpaulino

wpaulino Jul 21, 2018

Collaborator

Fixed.

@wpaulino wpaulino force-pushed the wpaulino:bitcoind-rescan-client branch from 9d7f4f5 to 47c3ac2 Jul 21, 2018

@wpaulino

This comment has been minimized.

Collaborator

wpaulino commented Jul 21, 2018

Note that this won't build until we're caught up with btcsuite master.

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Aug 1, 2018

The dependent btcwallet PR has been merged now, so this needs a rebase! You may want to wait until after #1579 is in however.

if err != nil {
t.Fatalf("couldn't start alice client: %v", err)
}
time.Sleep(time.Second)

This comment has been minimized.

@Roasbeef

Roasbeef Aug 1, 2018

Member

Why's this sleep necessary now?

This comment has been minimized.

@wpaulino

wpaulino Aug 1, 2018

Collaborator

To ensure bitcoind has started before we attempt to connect to it.

This comment has been minimized.

@halseth

halseth Aug 9, 2018

Collaborator

Has that changed with the new dependency, or is it something that could fail also earlier?

This comment has been minimized.

@wpaulino

wpaulino Aug 10, 2018

Collaborator

Something that could have happened earlier. It's also done within the ChainNotifier and FilteredChainView tests.

@Roasbeef Roasbeef added ready to merge and removed needs review labels Aug 1, 2018

@wpaulino wpaulino force-pushed the wpaulino:bitcoind-rescan-client branch from 47c3ac2 to 486b6fa Aug 1, 2018

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Aug 9, 2018

Needs rebase.

wpaulino added some commits Jul 16, 2018

config: parse different ZMQ hosts for block and tx notifications
Due to recent changes to the BitcoindClient interface, we now require
the backing bitcoind to use different hosts for its ZMQ raw block and
raw transaction notifications. This was needed as the notification queue
maintained by the bitcoind node would sometimes overflow with
transactions and cause block notifications to be dropped/missed.
In this commit, we expand extractBitcoindRPCParams to account for this.
multi: update to latest BitcoindClient interface
In this commit, we introduce a nice optimization with regards to lnd's
interaction with a bitcoind backend. 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. We remedy this issue by sharing the
same connection to a bitcoind node between the different clients within
lnd.

@wpaulino wpaulino force-pushed the wpaulino:bitcoind-rescan-client branch from 486b6fa to e58486b Aug 9, 2018

@halseth

halseth approved these changes Aug 9, 2018

}
// If only one or two of the parameters are set, we assume the

This comment has been minimized.

@halseth

halseth Aug 9, 2018

Collaborator

nit: ...or three 😛

if err != nil {
t.Fatalf("couldn't start alice client: %v", err)
}
time.Sleep(time.Second)

This comment has been minimized.

@halseth

halseth Aug 9, 2018

Collaborator

Has that changed with the new dependency, or is it something that could fail also earlier?

@Roasbeef

LGTM 💧

@Roasbeef Roasbeef merged commit 30b156c into lightningnetwork:master Aug 10, 2018

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.02%) to 54.579%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

@githorray

This comment has been minimized.

githorray commented Aug 10, 2018

Typo in the docs?

lnd --bitcoin.active --bitcoin.testnet --debuglevel=debug --bitcoin.node=bitcoind --bitcoind.rpcuser=REPLACEME --bitcoind.rpcpass=REPLACEME --bitcoind.zmqpubrawblock=tcp://127.0.0.1:28332 --bitcoind.zmqpubrawblock=tcp://127.0.0.1:28333 --externalip=X.X.X.X

Results in:

unable to load RPC credentials for bitcoind: please set all or none of bitcoind.rpcuser, bitcoind.rpcpass, bitcoind.zmqpubrawblock, bitcoind.zmqpubrawtx

@wpaulino

This comment has been minimized.

Collaborator

wpaulino commented Aug 10, 2018

Whoops, yeah the second should be --bitcoind.zmqpubrawtx=tcp://127.0.0.1:28333 .

@githorray

This comment has been minimized.

githorray commented Aug 10, 2018

Also, [INF] LNWL: Started listening for blocks via ZMQ on never appears in the log after the update and LND keeps losing chain sync. I am still trying to track that one down.

@bretton

This comment has been minimized.

Contributor

bretton commented Aug 10, 2018

So now bitcoin.conf must set different ports for zmq as follows

zmqpubrawblock=tcp://127.0.0.1:28332
zmqpubrawtx=tcp://127.0.0.1:28333

lnd.conf used to have

bitcoind.zmqpath=tcp://127.0.0.1:28332

however now it must have

bitcoind.zmqpubrawblock=tcp://127.0.0.1:28332
bitcoind.zmqpubrawtx=tcp://127.0.0.1:28333

???

but the sample lnd.conf file is incorrect?

https://github.com/lightningnetwork/lnd/blob/master/sample-lnd.conf

; bitcoind.zmqblockhost=tcp://127.0.0.1:28332
; bitcoind.zmqtxhost=tcp://127.0.0.1:28333

can submit PR if needed?

@wpaulino

This comment has been minimized.

Collaborator

wpaulino commented Aug 10, 2018

See #1717.

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