chainreg: use getnetworkinfo to count outbound peers (bitcoind backend)#10686
chainreg: use getnetworkinfo to count outbound peers (bitcoind backend)#10686starius wants to merge 5 commits intolightningnetwork:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the efficiency of chain backend health checks when using bitcoind by utilizing a lighter RPC call for monitoring outbound peer connections. Additionally, it refactors existing code to leverage structured RPC methods, reducing reliance on raw JSON parsing. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the retrieval of bitcoind network information and ZMQ notifications by replacing raw RPC requests and manual JSON unmarshaling with typed methods from the rpcclient. It also introduces a bitcoind-specific outbound peer check and extracts the peer count warning logic into a shared helper function. I have no feedback to provide.
|
I don't really know anything about the behavior of Here is the latency data I do have if it's of any use (frequency + seconds): |
|
By the way since you are looking at |
|
@jogc Thanks for sharing the latency data! If I read it correctly,
Can you elaborate on that, please? That PR replaced an error with a warning. I just refactored the checking code there, to use a higher level method of rpcclient instead of RawRequest. Do you think that code can be further improved? |
d8b3ec9 to
bccfaea
Compare
No. There are not enough requests recorded to say anything regarding
Sorry I meant #10568 |
Simplify the code. Use rpcclient's typed GetNetworkInfo call in getBitcoindHealthCheckCmd instead of a raw JSON-RPC request and manual JSON decoding.
Simplify the code. Use rpcclient's GetZmqNotifications method instead of a raw request and manual unmarshalling when validating bitcoind ZMQ subscriptions. The typed result already parses notification addresses, so the extra per-entry URL parsing is removed.
Use getnetworkinfo.connections_out for bitcoind outbound peer checks instead of getpeerinfo. This keeps the isolation-safety signal while avoiding heavier per-peer work. This helper is bitcoind-specific, btcd does not currently implement it.
The previous warning text ("unable to subscribe to zmq ... events") suggested
that lnd failed to create the ZMQ connection, when in reality it only means the
configured port differs from what bitcoind reports via getzmqnotifications.
Reword both messages to say "port mismatch" and tell to verify the port.
Fixes lightningnetwork#10568
Document that bitcoind outbound peer health checks now use getnetworkinfo.connections_out instead of getpeerinfo. Also mention that texts of zmq port mismatch warnings were fixed.
bccfaea to
4bf9810
Compare
I'm judging from Bitcoin Core code. Latency of Here is AI provided table comparing latencies of these two calls based on recent bitcoin core version (0831173c0171de33f95b96324db4041c4799b163).
I fixed the issue with warnings' texts in another commit. |
Change Description
Use
getnetworkinfo.connections_outfor bitcoind outbound peer checks instead ofgetpeerinfo. This keeps the isolation-safety signal while avoiding heavier per-peer work. This helper is bitcoind-specific, btcd does not currently implement it.Also made two unrelated cleanups to use structured rpcclient methods instead of RawRequest in the same file.
See #10641 This should partially address it, switching to lighter calls for health-checks. CC @jogc
Fixes #10568
Steps to Test
Verification done in signet:
lncli getinfoworks and shows correctblock_height.grep -nE "Health check: chain backend failed after|SRVR: Health check|timed out after" ~/.lnd/logs/bitcoin/signet/lnd.logreturns nothinggrep -nE "insufficient number of connected outbound peers|Skipping chain backend peer check" ~/.lnd/logs/bitcoin/signet/lnd.logreturns nothingAlso tested
loop outandloop inon that node.Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.