Skip to content
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

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

Closed

Conversation

orbitalturtle
Copy link
Contributor

Normally Bitcoin Core block notifications come in via ZMQ. This PR adds an option for polling for blocks via RPC when using Bitcoin Core as the backend for LND. To use, the flag --rpcpolling needs to be set for lnd

Requires btcsuite/btcwallet#784

@guggero guggero self-requested a review January 3, 2022 13:17
@Roasbeef
Copy link
Member

Roasbeef commented Jan 3, 2022

Great PR! Will need to ensure we did an adequate amount of testing for the combined lnd+btcwallet PR to make sure we don't introduce any unexpected regressions in stability/performance, given we've been using the ZMQ code paths since lnd was created.

@Roasbeef Roasbeef added this to the v0.15.0 milestone Jan 3, 2022
@Roasbeef Roasbeef added bitcoind Bitcoin Core backend rpc Related to the RPC interface labels Jan 3, 2022
@Roasbeef
Copy link
Member

Roasbeef commented Jan 3, 2022

One other important difference is that in the event of a re-org, ZMQ as is, will give you each disconnected block, then the set of connected blocks. An implementation based on polling will need to do the extra work to deliver blocks in the same order, so we don't need to modify any of the logic in the chain notifier.

As is, I don't see the PR updating the notifier or itests to exercise the new operating mode.

@ellemouton
Copy link
Collaborator

ellemouton commented Jan 19, 2022

Did a review of the btcwallet PR. See my latest comment here

in the event of a re-org, ZMQ as is, will give you each disconnected block, then the set of connected blocks

Interestingly, this seems to not be the case. ZMQ seems to be height based. So in the event of a reorg, it will just send you the blocks of the heights it has not sent to you yet. Then we externally, here, notice that it is a re-org and deal with it there.

I put together a playground & script for verification of this: https://github.com/ellemouton/playgrounds/tree/master/zmq_reorg

@orbitalturtle orbitalturtle force-pushed the bitcoind-rpc-poll branch 2 times, most recently from c41916a to 92e7795 Compare January 24, 2022 19:45
@orbitalturtle
Copy link
Contributor Author

Just FYI, I made some changes here based on @ellemouton's feedback on the btcwallet change. Also added some unit tests which further show the blocks being notified in order in the event of a reorg

@Roasbeef for the itests, do you mean adding a new mode which replays all of the itests on top of this new --rpcpolling flag? Like how all the itests are replayed with the --txindex flag? Just want to make sure before I get too into the weeds with that

@Roasbeef
Copy link
Member

Roasbeef commented Jan 24, 2022

@Roasbeef for the itests, do you mean adding a new mode which replays all of the itests on top of this new --rpcpolling flag?

Yep, this can effectively be considered a new backend, just like we run the itests for: neutrino, btcd, and bitcoind-zmq. In addition, we want to mirror coverage across the: wallet, notifier, and chainview (I see the chain view coverage in this PR so far). The notifier tests are important as this is where some of the basic re-org behavior is tested.

@orbitalturtle orbitalturtle force-pushed the bitcoind-rpc-poll branch 2 times, most recently from 79fcb03 to 7105d49 Compare January 31, 2022 05:44
@orbitalturtle
Copy link
Contributor Author

@Roasbeef That makes sense! Added those tests

@Roasbeef Roasbeef added this to In progress in v0.15.0-beta via automation Feb 2, 2022
@Roasbeef Roasbeef moved this from In progress to Review in progress in v0.15.0-beta Feb 2, 2022
@ellemouton
Copy link
Collaborator

Just posting an update here to keep the bot happy: currently waiting on updates in the dependant PR before reviewing this one

@Roasbeef Roasbeef added the P2 should be fixed if one has time label Feb 14, 2022
@guggero
Copy link
Collaborator

guggero commented Feb 21, 2022

Going to remove my request for review until the btcwallet PR is in a reviewable state.

@guggero guggero removed their request for review February 21, 2022 11:13
@lightninglabs-deploy
Copy link

@ellemouton: review reminder

@ellemouton ellemouton removed their request for review March 22, 2022 05:19
@ellemouton
Copy link
Collaborator

closing this as it has been replaced by #6345 which does retain some of the commits from this pr. Thanks again @orbitalturtle !

@ellemouton ellemouton closed this Mar 22, 2022
v0.15.0-beta automation moved this from Review in progress to Done Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bitcoind Bitcoin Core backend P2 should be fixed if one has time rpc Related to the RPC interface
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants