backport[0.20.x]: graph/db: fix SetSourceNode no rows error#10418
Conversation
Summary of ChangesHello @ellemouton, 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 backports a critical fix to resolve a race condition that could hinder a node's startup process. The issue arose when multiple internal processes attempted to update the node's own announcement in the database simultaneously, leading to failures if timestamps were not strictly increasing. The solution involves implementing a more lenient upsert operation specifically for the source node, ensuring that all parameter changes persist even when concurrent updates occur with the same timestamp, thereby improving node stability and reliability. Highlights
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. 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request backports a fix for a race condition that could occur when updating the source node. The fix introduces a new, less strict SQL upsert query (UpsertSourceNode) that allows updates with the same timestamp, resolving the issue of concurrent updates failing. A new test case, TestSetSourceNodeSameTimestamp, is added to verify this behavior. The changes also include a refactoring of the node upsert logic in graph/db/sql_store.go to reduce code duplication and improve maintainability. The fix is correct and the changes are well-implemented. I have one suggestion to further improve the readability of the refactored code.
This commit adds TestSetSourceNodeSameTimestamp to demonstrate the current behavior when SetSourceNode is called with the same last update timestamp. The test reveals a difference between the SQL and bbolt implementations: - SQL store returns sql.ErrNoRows when attempting to update with the same timestamp, as the upsert query's UPDATE clause requires the new timestamp to be strictly greater than the existing one - bbolt store silently ignores stale updates and returns no error This behavior is important to document because our own node announcements may change quickly with the same timestamp, unlike announcements from other nodes where same timestamp typically means identical parameters.
This query is less strict in terms of the latest update timestamp field. We want to be less strict with our own node data since we always want our own updates recorded.
This commit fixes a race condition where multiple goroutines call SetSourceNode concurrently during startup, causing sql.ErrNoRows errors. The race occurs when multiple code paths (setSelfNode, createNewHiddenService, RPC updates) read the same old timestamp, independently increment it to the same new value (T+1), and race to write. The fix uses the new UpsertSourceNode SQL query (without strict timestamp constraint) instead of UpsertNode. This allows last-write-wins semantics for our own node, ensuring all parameter changes persist even when timestamps collide. Refactored sql_store.go for reusability: - upsertNodeAncillaryData: common logic for features/addresses/extras - populateNodeParams: common parameter building with callback pattern - buildNodeUpsertParams: builds params for strict UpsertNode - buildSourceNodeUpsertParams: builds params for lenient UpsertSourceNode - upsertSourceNode: new function using lenient query Updated TestSetSourceNodeSameTimestamp to verify that concurrent updates with the same timestamp now succeed and parameter changes persist. Fixes the itest error: "unable to upsert source node: upserting node(...): sql: no rows in result set"
ellemouton
force-pushed
the
fixSourceNodeSetting-v0.20.x
branch
from
724bf99 to
2590593
Compare
yyforyongyu
merged commit 5bfcdf9
into
lightningnetwork:v0.20.x-branch
2 participants
backport: #10371