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

wpaulino · 2018-12-11T02:38:50Z

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 chainntnfs: add support for detecting confirmations and spends of output scripts #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 · 2018-12-11T10:14:32Z

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

joostjager · 2018-12-11T11:20:16Z

Reminder: immediately send current block after RegisterBlockEpochNtfn

joostjager · 2018-12-11T12:17:53Z

Ntfr log not enabled in main log.go.

joostjager · 2018-12-11T12:27:12Z

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 · 2018-12-11T15:59:22Z

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

Roasbeef

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.

chainntnfs/txnotifier.go

chainntnfs/height_hint_cache.go

chainntnfs/neutrinonotify/neutrino.go

lnrpc/chainrpc/config_active.go

lnrpc/chainrpc/chainnotifer_server.go

joostjager · 2018-12-18T10:19:24Z

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 · 2018-12-18T10:20:31Z

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

Roasbeef · 2018-12-18T22:26:32Z

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.

log.go

cfromknecht

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

chainntnfs/txnotifier.go

chainntnfs/btcdnotify/btcd.go

chainntnfs/neutrinonotify/neutrino.go

chainntnfs/interface.go

lnrpc/chainrpc/chainnotifier.proto

joostjager · 2019-01-14T08:09:07Z

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

Roasbeef

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.

chainntnfs/txnotifier.go

chainntnfs/btcdnotify/btcd.go

Roasbeef · 2019-01-17T05:51:47Z

chainntnfs/bitcoindnotify/bitcoind.go

+				// known block, then we'll immediately dispatch
+				// a notification for the current tip.
+				if msg.bestBlock == nil {
+					b.notifyBlockEpochClient(


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.

lnrpc/chainrpc/chainnotifier.proto

Roasbeef

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

…ctTip

…tion

wpaulino · 2019-01-21T22:49:56Z

Rebased.

Roasbeef

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.

wpaulino force-pushed the chainnotifier-subserver branch from 45fad52 to 431dbf1 Compare December 11, 2018 10:27

wpaulino force-pushed the chainnotifier-subserver branch from df122a4 to a5da112 Compare December 11, 2018 23:42

Roasbeef requested changes Dec 18, 2018

View reviewed changes

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

wpaulino force-pushed the chainnotifier-subserver branch from a5da112 to e3ecc52 Compare January 4, 2019 00:11

Roasbeef requested a review from cfromknecht January 8, 2019 01:01

joostjager reviewed Jan 8, 2019

View reviewed changes

log.go Show resolved Hide resolved

cfromknecht reviewed Jan 9, 2019

View reviewed changes

wpaulino force-pushed the chainnotifier-subserver branch from e3ecc52 to 2831eab Compare January 12, 2019 03:37

wpaulino force-pushed the chainnotifier-subserver branch from 2831eab to dc63149 Compare January 15, 2019 07:08

Roasbeef added this to the 0.6 milestone Jan 16, 2019

wpaulino force-pushed the chainnotifier-subserver branch from dc63149 to ff24faa Compare January 16, 2019 05:01

Roasbeef reviewed Jan 17, 2019

View reviewed changes

Roasbeef previously approved these changes Jan 19, 2019

View reviewed changes

wpaulino added 7 commits January 21, 2019 13:57

build: update deps to support detecting script spends within rescans

cdf2a65

chainntnfs/txnotifier: add ConfRequest and SpendRequest structs

de0adc4

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/txnotifier: support registration of scripts for confirmations

0f4ff2d

chainntnfs/txnotifier: support registration of scripts for spends

b579c23

chainntnfs/txnotifier: notify confirmations and spends of scripts at tip

83c8ccb

wpaulino added 22 commits January 21, 2019 13:57

multi: update to latest ChainNotifier impl New method

c72da48

chainntnfs/interface: update docs to note support for scripts

72cc843

chainntnfs: modify GetTestTxidAndScript to generate new P2PKH scripts

83c7f20

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.

chainntnfs/interface_test: add script dispatch test cases

5ab30cf

chainntnfs: include transaction in confirmation details

50650d0

chainntnfs: declare common ErrChainNotifierShuttingDown for all backends

be2c321

chainntnfs: add support to cancel pending registered confirmation

0c579b1

chainntnfs/txnotifier: track reorg risk for transactions within Conne…

52a80f2

…ctTip

chainntnfs: add Done chan to events to signal reorg safety

060f2f7

chainntnfs/neutrinonotify: track best block hash

568f827

chainntnfs: dispatch block notification for current tip upon registra…

26d53c6

…tion

lnrpc/chainrpc: add ChainNotifier subserver protos

d06f6c1

lnrpc/chainrpc: add ChainNotifier subserver Config

5e98af8

lnrpc/chainrpc: add ChainNotifier subserver implementation

1f8eef2

lnrpc/chainrpc: add ChainNotifier subserver registration driver

3d91aa2

config: add ChainRPC config

59e2be5

wpaulino dismissed Roasbeef’s stale review via 59e2be5 January 21, 2019 22:02

wpaulino force-pushed the chainnotifier-subserver branch from ff24faa to 59e2be5 Compare January 21, 2019 22:02

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

Roasbeef approved these changes Jan 21, 2019

View reviewed changes

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

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

wpaulino deleted the chainnotifier-subserver branch January 21, 2019 23:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wpaulino commented Dec 11, 2018 •

edited

joostjager commented Dec 11, 2018

joostjager commented Dec 11, 2018

joostjager commented Dec 11, 2018

joostjager commented Dec 11, 2018 •

edited

joostjager commented Dec 11, 2018

Roasbeef left a comment

joostjager commented Dec 18, 2018

joostjager commented Dec 18, 2018

Roasbeef commented Dec 18, 2018

cfromknecht left a comment

joostjager commented Jan 14, 2019

Roasbeef left a comment

Roasbeef Jan 17, 2019

Roasbeef left a comment

wpaulino commented Jan 21, 2019

Roasbeef left a comment

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

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

Conversation

wpaulino commented Dec 11, 2018 • edited

joostjager commented Dec 11, 2018

joostjager commented Dec 11, 2018

joostjager commented Dec 11, 2018

joostjager commented Dec 11, 2018 • edited

joostjager commented Dec 11, 2018

Roasbeef left a comment

Choose a reason for hiding this comment

joostjager commented Dec 18, 2018

joostjager commented Dec 18, 2018

Roasbeef commented Dec 18, 2018

cfromknecht left a comment

Choose a reason for hiding this comment

joostjager commented Jan 14, 2019

Roasbeef left a comment

Choose a reason for hiding this comment

Roasbeef Jan 17, 2019

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

wpaulino commented Jan 21, 2019

Roasbeef left a comment

Choose a reason for hiding this comment

wpaulino commented Dec 11, 2018 •

edited

joostjager commented Dec 11, 2018 •

edited