Skip to content

peer: fix flaky nil pointer dereference in PingManager#10660

Open
ellemouton wants to merge 2 commits intolightningnetwork:masterfrom
ellemouton:fix/peer-ping-nil-deref
Open

peer: fix flaky nil pointer dereference in PingManager#10660
ellemouton wants to merge 2 commits intolightningnetwork:masterfrom
ellemouton:fix/peer-ping-nil-deref

Conversation

@ellemouton
Copy link
Collaborator

Summary

Fixes a flaky nil pointer dereference panic in the peer package test
suite, observed in CI:

The panic:

panic: runtime error: invalid memory address or nil pointer dereference
goroutine 577 [running]:
github.com/lightningnetwork/lnd/peer.NewBrontide.func1()
    peer/brontide.go:736
github.com/lightningnetwork/lnd/peer.(*PingManager).pingHandler(0xc00034e150)
    peer/ping_manager.go:175

Root Cause

Two issues contribute to this crash:

  1. Logic bug in newPingPayload: The early-return condition on error
    from BestBlockView.BestBlockHeader() used && instead of ||:

    // Before: only returns cached header if err != nil AND header == lastBlockHeader.
    // When header is nil (due to error) and lastBlockHeader is non-nil, falls
    // through to header.Serialize() → nil deref.
    if err != nil && header == lastBlockHeader {
    
    // After: returns cached header on any error OR when header is unchanged.
    if err != nil || header == lastBlockHeader {
  2. Missing BestBlockView in test config: createTestPeer constructed
    a Config with a nil BestBlockView. When the PingManager's interval
    timer 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

  • Commit 1: Fix the &&|| logic so that any error from
    BestBlockHeader() correctly returns the cached serialized header
    instead of falling through to dereference a nil pointer.
  • Commit 2: Add a mockBestBlockView to the test peer config so
    that the PingManager can safely fire during tests.

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.
@gemini-code-assist
Copy link

Summary of Changes

Hello, 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 peer package's test suite. The fix involves resolving a logical error in how block headers are handled when an error occurs or the header is unchanged, and ensuring that test configurations properly initialize the BestBlockView to prevent calls on a nil receiver. These changes enhance the stability and reliability of the test suite by eliminating a race condition related to the PingManager's operations.

Highlights

  • Logic Bug Fix: Corrected a logical operator from && to || in the newPingPayload function within peer/brontide.go. This change ensures that any error from BestBlockHeader() or an unchanged header correctly returns the cached serialized header, preventing a nil pointer dereference.
  • Test Configuration Enhancement: Introduced a mockBestBlockView in peer/test_utils.go and integrated it into the createTestPeer function. This provides a non-nil BestBlockView to the PingManager during tests, resolving a flaky nil pointer dereference that occurred when the PingManager's interval timer fired.

🧠 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 Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@ellemouton ellemouton self-assigned this Mar 24, 2026
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant