multi: bitcoind rpc polling option #6345

ellemouton · 2022-03-18T12:46:21Z

replaces #6117
Depends on btcsuite/btcwallet#795

Thanks to @orbitalturtle for the original lnd and btcwallet PRs 🚀

Note, this PR also adds a new itest backend.

guggero

Very cool to see all unit and integration tests being green with this new "backend" type 🎉

LGTM pending a few nits and the merge of the dependent PR.

routing/chainview/interface_test.go

chainreg/chainregistry.go

chainntnfs/bitcoindnotify/bitcoind.go

ellemouton · 2022-03-31T07:53:50Z

updated both PRs 👍

guggero

Very nice 🎉

lightninglabs-deploy · 2022-04-07T11:20:28Z

@carlaKC: review reminder

Roasbeef · 2022-04-13T23:10:09Z

routing/chainview/interface_test.go

+			}
+
+			// Wait for the bitcoind instance to start up.
+			time.Sleep(time.Second)


Can we tighten this check up here with something like a wait.Predicate? Otherwise I fear this'll end up flaking a lot on CI.

i havent yet been able to figure out how to completely get rid of the time.Sleep even when adding the wait.NoError. Seems like needs to happen between starting bitcoind and calling calling Start on the chainView. This seems to be the case for the ZMQ connection too (in multiple places in the code base) so is not specific to this PR so wondering if i can add a todo for now?

What about instead doing a mini poll here until we get a valid response from the bitcoind connection?

yeah i did try this before but it consistently flakes on my machine.
I pushed up the patch to demonstrate the flake now but I see it does not happen on github. So it might be a MacOS thing or something specific to my machine or my bitcoind installation...

@Roasbeef , could you try run this test on your machine to see if it flakes?

lnwallet/test/test_interface.go

chainreg/chainregistry.go

ellemouton · 2022-04-19T18:52:01Z

erg, must have broken something today while updating. Will find and fix tomorrow

Roasbeef · 2022-05-03T22:52:48Z

I'm assuming this isn't ready for a re-review?

ellemouton · 2022-05-05T14:07:23Z

I'm assuming this isn't ready for a re-review?

it is but there are 2 comments from the review that i have not been able to address. Have left comments on those 2 points.

Roasbeef · 2022-05-11T00:01:10Z

Needs a rebase!

Roasbeef · 2022-05-11T00:03:16Z

I get this segfault when I try to run the tests on my machine:

    interface_test.go:1132: Testing 'bitcoind_zmq' implementation of FilteredChainView
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x40 pc=0x100d58710]

goroutine 166 [running]:
github.com/btcsuite/btcd/rpcclient.(*Client).handleSendPostMessage(0x140001f2000, 0x140000f8280)
	/Users/roasbeef/gocode/pkg/mod/github.com/btcsuite/btcd@v0.22.0-beta.0.20220413172512-bf64c8bdbbbf/rpcclient/infrastructure.go:814 +0x480
github.com/btcsuite/btcd/rpcclient.(*Client).sendPostHandler(0x140001f2000)
	/Users/roasbeef/gocode/pkg/mod/github.com/btcsuite/btcd@v0.22.0-beta.0.20220413172512-bf64c8bdbbbf/rpcclient/infrastructure.go:861 +0x30
created by github.com/btcsuite/btcd/rpcclient.(*Client).start
	/Users/roasbeef/gocode/pkg/mod/github.com/btcsuite/btcd@v0.22.0-beta.0.20220413172512-bf64c8bdbbbf/rpcclient/infrastructure.go:1134 +0x11c
exit status 2

Roasbeef · 2022-05-11T00:10:17Z

AFAICT, this happens because the response is nil at that point, and I think the backoff has fully expired. This happens for all the bitcoind tests (not just this new one).

Tried to see if btcsuite/btcd#1856 helped, but didn't see to do anything. Perhaps

Roasbeef · 2022-05-11T00:15:46Z

AFAICT, when I ran it locally, bitcoind actually didn't fully stand up for some reason. There does appears to be an issue in the rpcclient library though, the loop exists but the response is nil, which causes that panic cc @bhandras.

Roasbeef · 2022-05-11T00:51:33Z

So turns out my bitcoind install locally was borked (some dynlib thing w/ openssl, had to reinstall). After fixing that, everything works fine!

I realized that though we check that error output, the process can still fail to start. So we can do another check to make sure cmd.Process is set after Start there. If it isn't, then something messed up. I was able to debug this by threading the cmd.Stdout/Stderr into os.Stdout/Stderr.

Roasbeef

LGTM 💰

routing/chainview/interface_test.go

Update go.mod to point to latest btcwallet version.

Make the Bitcoind ZMQReadDeadline option configurable.

Add a new chainview interface test that runds the chainview tests against a bitcoind node that we are getting block and tx notifications from using the rpc interface.

ellemouton requested a review from guggero March 18, 2022 12:46

Roasbeef added this to the v0.15.0 milestone Mar 18, 2022

Roasbeef added this to In progress in v0.15.0-beta via automation Mar 18, 2022

Roasbeef added the bitcoind Bitcoin Core backend label Mar 18, 2022

Roasbeef requested review from arshbot and carlaKC and removed request for arshbot March 18, 2022 21:13

ellemouton mentioned this pull request Mar 22, 2022

Add option to poll for blocks/txs via RPC instead of using ZMQ #6117

Closed

guggero reviewed Mar 29, 2022

View reviewed changes

routing/chainview/interface_test.go Outdated Show resolved Hide resolved

chainreg/chainregistry.go Outdated Show resolved Hide resolved

chainntnfs/bitcoindnotify/bitcoind.go Outdated Show resolved Hide resolved

ellemouton force-pushed the bitcoind-rpc-polling branch 3 times, most recently from f52a0a3 to 3e0d297 Compare March 30, 2022 14:30

ellemouton requested a review from guggero March 31, 2022 07:53

guggero approved these changes Mar 31, 2022

View reviewed changes

ellemouton removed the request for review from carlaKC April 7, 2022 12:15

Roasbeef self-requested a review April 13, 2022 17:15

Roasbeef moved this from In progress to Review in progress in v0.15.0-beta Apr 13, 2022

Roasbeef requested changes Apr 13, 2022

View reviewed changes

ellemouton force-pushed the bitcoind-rpc-polling branch 7 times, most recently from 791907e to 3ab29c3 Compare April 19, 2022 18:26

ellemouton force-pushed the bitcoind-rpc-polling branch 2 times, most recently from 5d626ef to a820c9a Compare April 20, 2022 13:27

ellemouton force-pushed the bitcoind-rpc-polling branch from aeeeca9 to 537086a Compare May 2, 2022 15:22

ellemouton force-pushed the bitcoind-rpc-polling branch 6 times, most recently from eca4dad to bf356ac Compare May 4, 2022 12:20

ellemouton force-pushed the bitcoind-rpc-polling branch 2 times, most recently from 1bdab1a to 771d5c4 Compare May 6, 2022 10:33

ellemouton requested a review from Roasbeef May 6, 2022 11:23

Roasbeef mentioned this pull request May 11, 2022

rpcclient: fix crash in http retry handler btcsuite/btcd#1856

Merged

Roasbeef approved these changes May 11, 2022

View reviewed changes

routing/chainview/interface_test.go Show resolved Hide resolved

ellemouton and others added 8 commits May 11, 2022 08:45

multi: update btcwallet to v0.15.0

5a08788

Update go.mod to point to latest btcwallet version.

multi: make ZmqReadDeadline configurable

c76d04e

Make the Bitcoind ZMQReadDeadline option configurable.

routing/chainview: add bitcoind rpc polling test

33e1f9b

Add a new chainview interface test that runds the chainview tests against a bitcoind node that we are getting block and tx notifications from using the rpc interface.

lnwallet/test: test against bitcoind-rpc-polling backend

f766268

chainntnfs: test both zmq and rpc bitcoind modes

0459ccf

multi: add rpcpolling config options

7509af1

multi: new bitcoind rpcpolling backend for itests

e789107

docs: update release notes

a4bd389

ellemouton force-pushed the bitcoind-rpc-polling branch from 771d5c4 to a4bd389 Compare May 11, 2022 07:02

guggero merged commit 0051e39 into lightningnetwork:master May 11, 2022

v0.15.0-beta automation moved this from Reviewer approved + active testing to Done May 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: bitcoind rpc polling option #6345

multi: bitcoind rpc polling option #6345

ellemouton commented Mar 18, 2022 •

edited

guggero left a comment

ellemouton commented Mar 31, 2022

guggero left a comment

lightninglabs-deploy commented Apr 7, 2022

Roasbeef Apr 13, 2022

ellemouton May 4, 2022 •

edited

Roasbeef May 5, 2022

ellemouton May 6, 2022

ellemouton commented Apr 19, 2022

Roasbeef commented May 3, 2022

ellemouton commented May 5, 2022

Roasbeef commented May 11, 2022

Roasbeef commented May 11, 2022

Roasbeef commented May 11, 2022 •

edited

Roasbeef commented May 11, 2022

Roasbeef commented May 11, 2022

Roasbeef left a comment

multi: bitcoind rpc polling option #6345

multi: bitcoind rpc polling option #6345

Conversation

ellemouton commented Mar 18, 2022 • edited

guggero left a comment

Choose a reason for hiding this comment

ellemouton commented Mar 31, 2022

guggero left a comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Apr 7, 2022

Roasbeef Apr 13, 2022

Choose a reason for hiding this comment

ellemouton May 4, 2022 • edited

Choose a reason for hiding this comment

Roasbeef May 5, 2022

Choose a reason for hiding this comment

ellemouton May 6, 2022

Choose a reason for hiding this comment

ellemouton commented Apr 19, 2022

Roasbeef commented May 3, 2022

ellemouton commented May 5, 2022

Roasbeef commented May 11, 2022

Roasbeef commented May 11, 2022

Roasbeef commented May 11, 2022 • edited

Roasbeef commented May 11, 2022

Roasbeef commented May 11, 2022

Roasbeef left a comment

Choose a reason for hiding this comment

ellemouton commented Mar 18, 2022 •

edited

ellemouton May 4, 2022 •

edited

Roasbeef commented May 11, 2022 •

edited