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

chainntnfs+lnrpc/chainrpc: add ChainNotifier RPC subserver #2314

Merged
merged 36 commits into from Jan 21, 2019

Conversation

4 participants
@wpaulino
Copy link
Collaborator

wpaulino commented Dec 11, 2018

In this PR, we introduce a new RPC subserver for lnd's ChainNotifier as defined in chainnotifier.proto. This new subserver has its own macaroon state and gives users a simplified
interface for a gRPC service that is able to dispatch notifications for blocks, transaction/script confirmations, and output/script spends.

In order to satisfy some of the requirements of the service, some changes were necessary to lnd's ChainNotifier. These changes include:

  • Dispatching confirmation and spend notifications for scripts (achieved by #2291).
  • Dispatching a block notification for the current tip upon registration.
  • Adding a Done channel to signal when notifications are no longer under the risk of being reorged out of the chain.

In order to enable this service, lnd must be built with the chainrpc build tag.

Depends on #2291.

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Dec 11, 2018

It doesn't build:

joost@joost-ThinkPad-T440s $ make  tags="signrpc walletrpc chainrpc"
 Building debug lnd and lncli.
GO111MODULE=on go build -v -tags="dev signrpc walletrpc chainrpc" -o lnd-debug -ldflags "-X github.com/lightningnetwork/lnd/build.Commit=v0.5.1-beta-172-g45fad52f1e7ddf027c10c926fc6065198945c0a3" github.com/lightningnetwork/lnd
github.com/lightningnetwork/lnd/lnrpc/chainrpc
# github.com/lightningnetwork/lnd/lnrpc/chainrpc
lnrpc/chainrpc/chainnotifer_server.go:209:2: undefined: confEvent
lnrpc/chainrpc/chainnotifer_server.go:209:13: undefined: err
lnrpc/chainrpc/chainnotifer_server.go:212:5: undefined: err
lnrpc/chainrpc/chainnotifer_server.go:213:10: undefined: err
lnrpc/chainrpc/chainnotifer_server.go:215:8: undefined: confEvent
lnrpc/chainrpc/chainnotifer_server.go:224:20: select case must be receive, send or assign recv
lnrpc/chainrpc/chainnotifer_server.go:224:25: undefined: confEvent
lnrpc/chainrpc/chainnotifer_server.go:251:14: select case must be receive, send or assign recv
lnrpc/chainrpc/chainnotifer_server.go:251:19: undefined: confEvent
lnrpc/chainrpc/chainnotifer_server.go:266:19: undefined: confEvent
lnrpc/chainrpc/chainnotifer_server.go:266:19: too many errors
Makefile:99: recipe for target 'build' failed
make: *** [build] Error 2

@wpaulino wpaulino force-pushed the wpaulino:chainnotifier-subserver branch from 45fad52 to 431dbf1 Dec 11, 2018

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Dec 11, 2018

Reminder: immediately send current block after RegisterBlockEpochNtfn

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Dec 11, 2018

Ntfr log not enabled in main log.go.

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Dec 11, 2018

Crash when spend ntfn without tx hash comes in:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0xc82b05]

goroutine 384 [running]:
github.com/lightningnetwork/lnd/lnrpc/chainrpc.(*Server).RegisterSpendNtfn(0xc000010e10, 0xc010568e10, 0x12b87a0, 0xc010639d10, 0x0, 0x0)
	/home/joost/lightninglabs/lnd/lnrpc/chainrpc/chainnotifer_server.go:304 +0x55
github.com/lightningnetwork/lnd/lnrpc/chainrpc._ChainNotifier_RegisterSpendNtfn_Handler(0xf85440, 0xc000010e10, 0x12b7a20, 0xc0000d1550, 0x1adb980, 0xc010568de0)
	/home/joost/lightninglabs/lnd/lnrpc/chainrpc/chainnotifier.pb.go:758 +0x109
google.golang.org/grpc.(*Server).processStreamingRPC(0xc0104db080, 0x12ba1e0, 0xc0105

After setting outpoint hard to nil in the sub server, this pr seems to work for me!

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Dec 11, 2018

ctrl-c on lnd:

2018-12-11 16:58:14.343 [INF] RPCS: Stopping RPC Server
2018-12-11 16:58:14.343 [INF] RPCS: Stopping WalletKitRPC Sub-RPC Server
2018-12-11 16:58:14.343 [INF] RPCS: Stopping ChainRPC Sub-RPC Server
2018-12-11 16:58:14.343 [INF] LTND: Shutdown complete
panic: close of nil channel

goroutine 1 [running]:
github.com/lightningnetwork/lnd/lnrpc/chainrpc.(*Server).Stop(0xc0000cebe0, 0x104c963, 0x1a)
	/home/joost/lightninglabs/lnd/lnrpc/chainrpc/chainnotifer_server.go:163 +0x42
main.(*rpcServer).Stop(0xc000560b00, 0xc000449dd8, 0x1)
	/home/joost/lightninglabs/lnd/rpcserver.go:587 +0x1c7
main.lndMain(0x0, 0x0)
	/home/joost/lightninglabs/lnd/lnd.go:397 +0x1002
main.main()
	/home/joost/lightninglabs/lnd/lnd.go:403 +0x26

@wpaulino wpaulino force-pushed the wpaulino:chainnotifier-subserver branch from df122a4 to a5da112 Dec 11, 2018

@Roasbeef
Copy link
Member

Roasbeef left a comment

Solid PR, +1 for the embedded bug fix along the way. Could likely have been broken up into two distinct PRs, but too late for that as review/testing has already begun on this instance. The amount of duplication throughout the PR is rather painful...and something we're all aware of as far as a major issue within the notifier package in general. Not saying we need to address this now, but it's apparent that most of the size of this PR is due to all changes being 3x'd (sometimes more) across the sub-packages.

Occurs to me that we could even start to use this sub-server within the integration tests to obtain a very tight hook w.r.t when lnd see certain transactions/blocks.

Show resolved Hide resolved chainntnfs/txnotifier.go Outdated
Show resolved Hide resolved chainntnfs/txnotifier.go
Show resolved Hide resolved chainntnfs/txnotifier.go Outdated
Show resolved Hide resolved chainntnfs/txnotifier.go
Show resolved Hide resolved chainntnfs/height_hint_cache.go Outdated
Show resolved Hide resolved chainntnfs/neutrinonotify/neutrino.go
Show resolved Hide resolved lnrpc/chainrpc/config_active.go Outdated
Show resolved Hide resolved lnrpc/chainrpc/chainnotifer_server.go
Show resolved Hide resolved lnrpc/chainrpc/chainnotifer_server.go Outdated
Show resolved Hide resolved lnrpc/chainrpc/chainnotifer_server.go

@Roasbeef Roasbeef added this to In progress in High Priority via automation Dec 18, 2018

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Dec 18, 2018

If there is so much duplication, can't more be extracted into reusable methods first and build the new functionality on top of that?

dupl -t 80

found 2 clones:
  bitcoindnotify/bitcoind.go:332,361
  btcdnotify/btcd.go:381,410
found 2 clones:
  bitcoindnotify/bitcoind.go:228,261
  btcdnotify/btcd.go:296,329
found 3 clones:
  bitcoindnotify/bitcoind.go:199,225
  btcdnotify/btcd.go:268,293
  neutrinonotify/neutrino.go:282,307
found 2 clones:
  txnotifier.go:499,527
  txnotifier.go:821,849
found 2 clones:
  bitcoindnotify/bitcoind.go:478,637
  btcdnotify/btcd.go:529,689
found 2 clones:
  height_hint_cache.go:119,233
  height_hint_cache.go:236,351
found 3 clones:
  bitcoindnotify/bitcoind.go:1027,1106
  btcdnotify/btcd.go:1041,1122
  neutrinonotify/neutrino.go:946,1025
found 2 clones:
  bitcoindnotify/bitcoind_test.go:32,49
  btcdnotify/btcd_test.go:30,47
found 2 clones:
  bitcoindnotify/bitcoind.go:282,318
  btcdnotify/btcd.go:331,367
found 2 clones:
  bitcoindnotify/bitcoind.go:735,749
  btcdnotify/btcd.go:796,810

Found total 10 clone groups.

This is just automated output, if the code is slightly different it isn't reported by dupl.

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Dec 18, 2018

Tried to build several commits with git bisect and manually too. Seems that many of them aren't building.

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Dec 18, 2018

If there is so much duplication, can't more be extracted into reusable methods first and build the new functionality on top of that?

Yes we plan to do that in the future, and have outlined what that would look like, but it's a much larger change the would modify the entire package vs this change that adds some new functionality. Was just pointing it out in the diff really, wasn't an attempt to convey that we should do it immediately.

@wpaulino wpaulino force-pushed the wpaulino:chainnotifier-subserver branch from a5da112 to e3ecc52 Jan 4, 2019

@Roasbeef Roasbeef requested a review from cfromknecht Jan 8, 2019

Show resolved Hide resolved log.go
@cfromknecht
Copy link
Collaborator

cfromknecht left a comment

great changes and refactoring! some comments from an initial pass, will circle back for a more thorough review 👍

Show resolved Hide resolved chainntnfs/txnotifier.go Outdated
Show resolved Hide resolved chainntnfs/txnotifier.go
Show resolved Hide resolved chainntnfs/txnotifier.go Outdated
Show resolved Hide resolved chainntnfs/btcdnotify/btcd.go Outdated
Show resolved Hide resolved chainntnfs/neutrinonotify/neutrino.go Outdated
Show resolved Hide resolved chainntnfs/interface.go
Show resolved Hide resolved lnrpc/chainrpc/chainnotifier.proto

@wpaulino wpaulino force-pushed the wpaulino:chainnotifier-subserver branch from e3ecc52 to 2831eab Jan 12, 2019

@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Jan 14, 2019

Still many commits failing:
git rev-list upstream/master..HEAD | xargs -n1 -I{} sh -c 'echo ---- {} ---- && git checkout {} && make'

@wpaulino wpaulino force-pushed the wpaulino:chainnotifier-subserver branch from 2831eab to dc63149 Jan 15, 2019

@Roasbeef Roasbeef added this to the 0.6 milestone Jan 16, 2019

@wpaulino wpaulino force-pushed the wpaulino:chainnotifier-subserver branch from dc63149 to ff24faa Jan 16, 2019

@Roasbeef
Copy link
Member

Roasbeef left a comment

Looking pretty good now, running on multiple nodes, just one notable comment I plan on investigating before we start merging this through. Also needs a rebase.

Show resolved Hide resolved chainntnfs/txnotifier.go
Show resolved Hide resolved chainntnfs/btcdnotify/btcd.go
// known block, then we'll immediately dispatch
// a notification for the current tip.
if msg.bestBlock == nil {
b.notifyBlockEpochClient(

This comment has been minimized.

@Roasbeef

Roasbeef Jan 17, 2019

Member

How do the current sub-systems handle this extra notification? In operation things seem to be fine which is a good sign, but will take a look at the existing block epoch handling to ensure we're not missing any edge cases.

Show resolved Hide resolved lnrpc/chainrpc/chainnotifier.proto
@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 💎

Been testing on multiple nodes for the past week with no immediate issues. Once this is in, I'd say we need to put a hold on all other non abstraction adding changes to this section of the codebase. At the end of the day, there should be a single notifier, with distinct chain backends that simply notify us of new blocks, and give us a consistent view of re-orgs.

Needs a rebase.

wpaulino added some commits Dec 7, 2018

chainntnfs/txnotifier: add ConfRequest and SpendRequest structs
These structs allow us to request notifications for either
txids/outpoints or output scripts. We'll be using these in later commits
to index confirmation and spend requests within the TxNotifier.
chainntnfs: support caching confirm/spend hints for scripts
In this commit, we refactor the HeightHintCache and its underlying
interfaces to be able to manipulate hints for ConfRequests and
SpendRequests. By doing so, we'll be able to manipulate hints for
scripts if the request includes either a zero hash or a zero outpoint.
chainntnfs/txnotifer: detect confirmations and spends of scripts at tip
In this commit, we modify the TxNotifier's ConnectTip method to also
detect whether a registered script has been confirmed or spent on-chain
by a transaction in the connected block. Once detected, notifications
for them will be queued up for dispatch as with txids/outpoints.

We've also refactored the ConnectTip method into smaller and reusable
methods, which will serve useful later.

wpaulino added some commits Dec 7, 2018

chainntnfs/btcdnotify: support registration for script confirmations
In this commit, we extend the BtcdNotifier to support registering
scripts for confirmation notifications. Once the script has been
detected as confirmed within the chain, a confirmation notification will
be dispatched to through the Confirmed channel of the ConfirmationEvent
returned upon registration.

For scripts that have confirmed in the past, the `historicalConfDetails`
method has been modified to skip the txindex and go straight to scanning
the chain manually if confirmation request is for a script. When
scanning the chain, we'll determine whether the script has been
confirmed by locating the script in an output of a confirmed
transaction.

For scripts that have yet to confirm, they will be properly tracked
within the TxNotifier.
chainntnfs/bitcoindnotify: support registration for script confirmations
In this commit, we extend the BitcoindNotifier to support registering
scripts for confirmation notifications. Once the script has been
detected as confirmed within the chain, a confirmation notification will
be dispatched to through the Confirmed channel of the ConfirmationEvent
returned upon registration.

For scripts that have confirmed in the past, the `historicalConfDetails`
method has been modified to skip the txindex and go straight to scanning
the chain manually if confirmation request is for a script. When
scanning the chain, we'll determine whether the script has been
confirmed by locating the script in an output of a confirmed
transaction.

For scripts that have yet to confirm, they will be properly tracked
within the TxNotifier.
chainntnfs/neutrinonotify: support registration for script confirmations
In this commit, we extend the NeutrinoNotifier to support registering
scripts for confirmation notifications. Once the script has been
detected as confirmed within the chain, a confirmation notification will
be dispatched to through the Confirmed channel of the ConfirmationEvent
returned upon registration.

For scripts that have confirmed in the past, the `historicalConfDetails`
method has been modified to determine whether the script has been
confirmed by locating the script in an output of a confirmed
transaction.

For scripts that have yet to confirm, a filter update is sent to the
underlying rescan to ensure that we match and dispatch on the script
when processing new blocks.

Along the way, we also address an issue where we'd miss detecting that a
transaction/script has confirmed in the future due to not receiving a
historical dispatch request from the underlying txNotifier. To fix this,
we ensure that we always update our filters to detect the confirmation
at tip, regardless of whether a historical rescan was detected or not.
chainntnfs/btcdnotify: support registration for script spends
In this commit, we extend the BtcdNotifier to support registering
scripts for spends notifications. Once the script has been detected as
spent within the chain, a spend notification will be dispatched through
the Spend channel of the SpendEvent returned upon registration.

For scripts that have been spent in the past, the rescan logic has been
modified to match on the script rather than the outpoint. This is done
by encoding the script as an address.

For scripts that are unspent, a request to the backend will be sent to
alert the BtcdNotifier of when the script was spent by a transaction. To
make this request we encode the script as an address, as this is what
the backend uses to detect the spend. The transaction will then be
proxied through the txUpdates concurrent queue, which will hand it off
to the underlying txNotifier and dispatch spend notifications to the
relevant clients.

Along the way, we also address an issue where we'd miss detecting that
an outpoint/script has been spent in the future due to not receiving a
historical dispatch request from the underlying txNotifier. To fix this,
we ensure that we always request the backend to notify us of the spend
once it detects it at tip, regardless of whether a historical rescan was
detected or not.
chainntnfs/bitcoindnotify: support registration for script spends
In this commit, we extend the BitcoindNotifier to support registering
scripts for spends notifications. Once the script has been detected as
spent within the chain, a spend notification will be dispatched through
the Spend channel of the SpendEvent returned upon registration.

For scripts that have been spent in the past, the rescan logic has been
modified to match on the script rather than the outpoint. This is done
by re-deriving the script of the output a transaction input is spending
and checking whether it matches ours.

For scripts that are unspent, a request to the backend will be sent to
alert the BitcoindNotifier of when the script was spent by a
transaction. To make this request we encode the script as an address, as
this is what the backend uses to detect the spend. The transaction will
then be proxied through the txUpdates concurrent queue, which will hand
it off to the underlying txNotifier and dispatch spend notifications to
the relevant clients.

Along the way, we also address an issue where we'd miss detecting that
an outpoint/script has been spent in the future due to not receiving a
historical dispatch request from the underlying txNotifier. To fix this,
we ensure that we always request the backend to notify us of the spend
once it detects it at tip, regardless of whether a historical rescan was
detected or not.
chainntnfs/neutrinonotify: support registration for script spends
In this commit, we extend the NeutrinoNotifier to support registering
scripts for spends notifications. Once the script has been detected as
spent within the chain, a spend notification will be dispatched through
the Spend channel of the SpendEvent returned upon registration.

For scripts that have been spent in the past, the rescan logic has been
modified to match on the script rather than the outpoint. A concurrent
queue for relevant transactions has been added to proxy notifications
from the underlying rescan to the txNotifier. This is needed for
scripts, as we cannot perform a historical rescan for scripts through
`GetUtxo`, like we do with outpoints.

For scripts that are unspent, a filter update is sent to the underlying
rescan to ensure that we match and dispatch on the script when
processing new blocks.

Along the way, we also address an issue where we'd miss detecting that
an outpoint/script has been spent in the future due to not receiving a
historical dispatch request from the underlying txNotifier. To fix this,
we ensure that we always request the backend to notify us of the spend
once it detects it at tip, regardless of whether a historical rescan was
detected or not.
chainntnfs: modify GetTestTxidAndScript to generate new P2PKH scripts
In this commit, we modify GetTestTxidAndScript to generate new P2PKH
scripts. This is needed to properly test confirmations and spends of
unique scripts on-chain within the set of interface-level test cases.

@wpaulino wpaulino force-pushed the wpaulino:chainnotifier-subserver branch from ff24faa to 59e2be5 Jan 21, 2019

High Priority automation moved this from In progress to Needs review Jan 21, 2019

@wpaulino

This comment has been minimized.

Copy link
Collaborator Author

wpaulino commented Jan 21, 2019

Rebased.

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 🛡

PR shown to be functional with no major regressions in testing, landing this now so we can move on with our other projects that depend on this. Any additional review comments can be executed as follow up PRs, with any major changes blocked on further refactoring in this area.

@Roasbeef Roasbeef merged commit 375be93 into lightningnetwork:master Jan 21, 2019

1 of 2 checks passed

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

High Priority automation moved this from Needs review to Done Jan 21, 2019

@wpaulino wpaulino deleted the wpaulino:chainnotifier-subserver branch Jan 21, 2019

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