server: fix timestamp comparison in setSelfNode #10449

ziggie1984 · 2025-12-15T13:08:56Z

Fix bug where setSelfNode compared only the seconds component of
timestamps instead of the full timestamp. This caused the node to
attempt persisting an older timestamp than what existed in the
database during restart, resulting in "sql: no rows in result set"
errors.

Fixes a flake:

https://github.com/lightningnetwork/lnd/actions/runs/20183761355/job/57949717763

The issue occurred when:

Node starts with timestamp T1 (e.g., 10:00:05)
Runtime update creates newer timestamp T2 (e.g., 10:00:30)
Node restarts, time.Now() returns T3 (e.g., 10:01:02)
Comparison srcNode.LastUpdate.Second() >= nodeLastUpdate.Second()
evaluated 30 >= 2, causing nodeLastUpdate to be set to 10:00:31
This older timestamp failed the UpsertSourceNode WHERE clause

gemini-code-assist · 2025-12-15T13:09:10Z

Summary of Changes

Hello @ziggie1984, I'm Gemini Code Assist¹! 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 critical bug within the setSelfNode function that previously led to incorrect timestamp comparisons. The original logic only considered the seconds component, which could cause the system to attempt persisting an older timestamp than what was already in the database, resulting in 'sql: no rows in result set' errors during node restarts. The fix updates the comparison to evaluate the full timestamp, ensuring proper data handling and preventing these errors.

Highlights

Timestamp Comparison Fix: Corrected a bug in the setSelfNode function where only the seconds component of timestamps was compared, leading to incorrect chronological evaluation.
Full Timestamp Logic: The comparison logic has been updated to use !srcNode.LastUpdate.Before(nodeLastUpdate), ensuring that the full timestamp, including all components, is considered.
Error Resolution: This change resolves 'sql: no rows in result set' errors that previously occurred during node restarts when the system attempted to persist an older timestamp than what was already present in the database.

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.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

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. ↩

gemini-code-assist

Code Review

This pull request addresses a critical bug in timestamp comparison within the setSelfNode function. The original code incorrectly compared only the seconds component of timestamps, which could lead to using an older timestamp and causing errors on restart. The fix correctly uses time.Time.Before for a full timestamp comparison, which is the proper and idiomatic approach in Go. This change is accurate and effectively resolves the described issue. The code quality is good, and I have no further suggestions.

ellemouton

nice catch 🙏

ziggie1984 · 2025-12-15T18:46:35Z

test flake is already tracked here: #8460

starius

I think there is an issue in the PR.

Also I propose to document when the bug was introduced, commit 75895db ("lnd: use saved node ann config from previous run", 2024-06-09).

Also I propose to cover this logic with regression tests as it proved to be fragile.

starius · 2025-12-15T19:45:51Z

server.go

 		// just need to make sure that the new LastUpdate time is at
 		// least one second after the last update time.
-		if srcNode.LastUpdate.Second() >= nodeLastUpdate.Second() {
+		if !srcNode.LastUpdate.Before(nodeLastUpdate) {


What if srcNode.LastUpdate.Before(nodeLastUpdate) but at the same time srcNode.LastUpdate.Unix() == nodeLastUpdate.Unix() (i.e. they fall into the same second)?

IMHO in this case get a violation of the "at least one second after" requirement of the comment above and [BOLT‑07 (P2P Node and Channel Discovery)]:

MUST set timestamp to be greater than that of any previous node_announcement it has previously created

IMHO the proper fix is:

if srcNode.LastUpdate.Unix() >= nodeLastUpdate.Unix() {

@gemini-code-assist can you explain what the problem is with the current fix ?

The problem with the current fix, !srcNode.LastUpdate.Before(nodeLastUpdate), as pointed out by @starius, lies in how time.Time.Before() handles sub-second precision compared to the stated requirement of being "at least one second after".

The code comment explicitly states: "just need to make sure that the new LastUpdate time is at least one second after the last update time."

Consider the following scenario:

srcNode.LastUpdate (the existing timestamp in the DB) = 2025-12-15 19:00:00.500000000 UTC

nodeLastUpdate (the new proposed timestamp) = 2025-12-15 19:00:00.800000000 UTC

With the current fix:

srcNode.LastUpdate.Before(nodeLastUpdate) evaluates to true (0.5s is indeed before 0.8s).

Therefore, the condition !srcNode.LastUpdate.Before(nodeLastUpdate) evaluates to false.

The if block is skipped, and nodeLastUpdate remains 2025-12-15 19:00:00.800000000 UTC.

However, 2025-12-15 19:00:00.800000000 UTC is only 0.3 seconds after srcNode.LastUpdate, which violates the requirement of being "at least one second after".

@starius's suggestion, srcNode.LastUpdate.Unix() >= nodeLastUpdate.Unix(), addresses this by comparing the timestamps at second-level granularity. Let's re-evaluate with the same example:

srcNode.LastUpdate.Unix() (seconds part of 19:00:00.5s) = 1765834800

nodeLastUpdate.Unix() (seconds part of 19:00:00.8s) = 1765834800

With @starius's suggested fix:

The condition srcNode.LastUpdate.Unix() >= nodeLastUpdate.Unix() evaluates to 1765834800 >= 1765834800, which is true.

The if block would execute: nodeLastUpdate = srcNode.LastUpdate.Add(time.Second).

nodeLastUpdate would then become 2025-12-15 19:00:01.500000000 UTC.

This new nodeLastUpdate (19:00:01.5s) is correctly "at least one second after" srcNode.LastUpdate (19:00:00.5s), aligning with the stated intent. This approach also correctly handles the original issue described in the PR where only the .Second() component was compared.

Therefore, @starius's suggestion provides a more robust solution that adheres to the "at least one second after" logic, considering the practical granularity of such timestamp comparisons.

Suggested change

if !srcNode.LastUpdate.Before(nodeLastUpdate) {

if srcNode.LastUpdate.Unix() >= nodeLastUpdate.Unix() {

ziggie1984 · 2025-12-15T22:24:04Z

/gemini review

gemini-code-assist

Code Review

This pull request correctly fixes a bug in setSelfNode where an incorrect timestamp comparison could lead to startup failures. The change replaces the flawed comparison of only the seconds component with a full Unix timestamp comparison by introducing a new helper function, needsTimestampIncrement. This approach is clean and improves code clarity. The accompanying new test, TestNodeAnnouncementTimestampComparison, is thorough, covering a good range of edge cases including clock skew, which ensures the fix is robust. The release note is also clear and accurately describes the change. Overall, this is a high-quality contribution.

ziggie1984 · 2025-12-15T22:47:14Z

server.go

+// how timestamps are stored in the database (both SQL and KV stores truncate
+// to second precision). This ensures BOLT-07 compliance, which requires node
+// announcements to have strictly increasing timestamps.
+func needsTimestampIncrement(persistedTime, currentTime time.Time) bool {


move this in a separate function so that I can test it as well.

I propose to include the if into the function. So it returns the final nodeLastUpdate value.

starius

LGTM! 🏆
I left few comments with refactoring and tests proposals.

starius · 2025-12-16T04:19:24Z

server.go

+// how timestamps are stored in the database (both SQL and KV stores truncate
+// to second precision). This ensures BOLT-07 compliance, which requires node
+// announcements to have strictly increasing timestamps.
+func needsTimestampIncrement(persistedTime, currentTime time.Time) bool {


I propose to include the if into the function. So it returns the final nodeLastUpdate value.

server.go

server_test.go

Fix bug where setSelfNode compared only the seconds component of timestamps instead of the full timestamp. This caused the node to attempt persisting an older timestamp than what existed in the database during restart, resulting in "sql: no rows in result set" errors.

github-actions · 2025-12-16T08:58:50Z

Successfully created backport PR for v0.20.x-branch:

[v0.20.x-branch] Backport #10449: server: fix timestamp comparison in setSelfNode #10451

gemini-code-assist bot reviewed Dec 15, 2025

View reviewed changes

ziggie1984 self-assigned this Dec 15, 2025

ziggie1984 added back port candidate pr which should be back ported to last major release backport-v0.20.x-branch This label is used to trigger the creation of a backport PR to the branch `v0.20.x-branch`. labels Dec 15, 2025

ziggie1984 marked this pull request as ready for review December 15, 2025 13:23

ziggie1984 requested review from ellemouton and yyforyongyu December 15, 2025 13:23

ziggie1984 added this to lnd v0.20 Dec 15, 2025

ziggie1984 added this to the v0.20.1 milestone Dec 15, 2025

ziggie1984 moved this to In review in lnd v0.20 Dec 15, 2025

ellemouton approved these changes Dec 15, 2025

View reviewed changes

ziggie1984 requested review from starius and removed request for yyforyongyu December 15, 2025 18:30

starius requested changes Dec 15, 2025

View reviewed changes

ziggie1984 force-pushed the bugfix/setsource-node-sql branch from 0a67307 to 90f4999 Compare December 15, 2025 22:11

gemini-code-assist bot reviewed Dec 15, 2025

View reviewed changes

ziggie1984 requested a review from starius December 15, 2025 22:45

ziggie1984 commented Dec 15, 2025

View reviewed changes

starius approved these changes Dec 16, 2025

View reviewed changes

ziggie1984 added 2 commits December 16, 2025 08:21

docs: add release-notes for LND 20.1

e5b0704

ziggie1984 force-pushed the bugfix/setsource-node-sql branch from 90f4999 to e5b0704 Compare December 16, 2025 07:21

ziggie1984 requested a review from ellemouton December 16, 2025 07:22

yyforyongyu approved these changes Dec 16, 2025

View reviewed changes

yyforyongyu merged commit 91423ee into lightningnetwork:master Dec 16, 2025
38 of 41 checks passed

github-project-automation bot moved this from In review to Done in lnd v0.20 Dec 16, 2025

github-actions bot mentioned this pull request Dec 16, 2025

[v0.20.x-branch] Backport #10449: server: fix timestamp comparison in setSelfNode #10451

Open

	if !srcNode.LastUpdate.Before(nodeLastUpdate) {
	if srcNode.LastUpdate.Unix() >= nodeLastUpdate.Unix() {

server: fix timestamp comparison in setSelfNode #10449

server: fix timestamp comparison in setSelfNode #10449

Conversation

ziggie1984 commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot commented Dec 15, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

ziggie1984 commented Dec 15, 2025

Uh oh!

starius left a comment

Choose a reason for hiding this comment

Uh oh!

starius Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

ziggie1984 Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

ziggie1984 commented Dec 15, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

ziggie1984 Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

starius Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

starius left a comment

Choose a reason for hiding this comment

Uh oh!

starius Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ziggie1984 commented Dec 15, 2025 •

edited

Loading