peer: fix flaky nil pointer dereference in PingManager#10660
peer: fix flaky nil pointer dereference in PingManager#10660ellemouton wants to merge 2 commits intolightningnetwork:masterfrom
Conversation
The condition guarding the early return used && when it should have used ||. When BestBlockHeader returns an error with a nil header, the old code only short-circuited if the nil header equalled lastBlockHeader. Otherwise it fell through to header.Serialize(), causing a nil pointer dereference panic. Change the condition to return the cached serialized header whenever there is an error OR when the header is unchanged.
The test Config in createTestPeer left BestBlockView nil. When the PingManager's timer fires during a test, it calls BestBlockView.BestBlockHeader() which panics on the nil receiver. This was a flaky failure since it depended on a race between the timer and test completion. Add a trivial mockBestBlockView that returns an empty block header and wire it into the test Config.
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 addresses a flaky nil pointer dereference panic observed in the 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 effectively addresses a flaky nil pointer dereference in the peer package test suite. The fix is twofold: correcting a logical error in newPingPayload by changing an && to an ||, and providing a mock BestBlockView in the test configuration to prevent panics during test runs. Both changes are clear, correct, and well-supported by the detailed description. I have a couple of minor suggestions regarding documentation to better align with the repository's style guide.
Summary
Fixes a flaky nil pointer dereference panic in the
peerpackage testsuite, observed in CI:
The panic:
Root Cause
Two issues contribute to this crash:
Logic bug in
newPingPayload: The early-return condition on errorfrom
BestBlockView.BestBlockHeader()used&&instead of||:Missing
BestBlockViewin test config:createTestPeerconstructeda
Configwith a nilBestBlockView. When the PingManager's intervaltimer fires during a test, it invokes
BestBlockView.BestBlockHeader()on a nil receiver → panic. The flakiness is a race between the timer
firing and the test completing.
Fixes
&&→||logic so that any error fromBestBlockHeader()correctly returns the cached serialized headerinstead of falling through to dereference a nil pointer.
mockBestBlockViewto the test peer config sothat the PingManager can safely fire during tests.