lncli unlock: wait until daemon can unlock#10536
lncli unlock: wait until daemon can unlock#10536starius wants to merge 4 commits intolightningnetwork:masterfrom
Conversation
Summary of ChangesHello @starius, 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 significantly enhances the robustness of 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. 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
|
🟡 PR Severity: MEDIUM
🟡 Medium (1 file)
AnalysisThis PR modifies the The change adds 104 lines to improve the user experience by waiting until the daemon is ready to accept unlock commands, rather than immediately failing. This is a focused, single-file change to CLI tooling. Severity determination:
Review focus: Ensure the retry logic handles edge cases properly (timeouts, connection failures, daemon state transitions). To override, add a |
![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 addresses an important issue where lncli unlock could fail if run too quickly during lnd startup. The approach of using the StateService to wait for the correct wallet state is solid and well-implemented. The new functions are clear and well-commented.
However, there are a couple of areas for improvement:
- Testing: The PR does not include any tests. Given that this change fixes a race condition and introduces new state-dependent logic, adding unit or integration tests would be crucial to prevent future regressions and ensure the fix is robust. The style guide also emphasizes testing.
- Error Handling: The error handling for the stream closing prematurely could be more user-friendly.
Overall, this is a good fix that improves the robustness of lncli unlock.
🟠 PR Severity: HIGH
🟠 High (1 file)
🟢 Low (1 file)
🧪 Test Files (excluded from severity)
AnalysisThis PR modifies the The PR is classified as HIGH because:
While this is a CLI-side change (not core wallet logic), it still warrants review by an engineer knowledgeable about the wallet unlocking workflow to ensure the retry logic and error handling are correct. To override, add a |
I added unit tests in a separate commit.
Added handling of io.EOF. |
🟠 PR Severity: HIGH
🟠 High (1 file)
🟢 Low (2 files)
AnalysisThis PR modifies the wallet unlock CLI command to wait until the daemon is ready to accept unlock requests. The changes touch authentication/security-related code in the walletunlocker command implementation, which falls into the HIGH severity category requiring a knowledgeable engineer to review. The PR adds retry logic and proper timing handling to the unlock command, which is critical for wallet security operations. While the majority of additions are in test files (522 lines), the core implementation changes (131 lines in cmd_walletunlocker.go) warrant careful review to ensure the unlock flow works correctly and doesn't introduce timing vulnerabilities or race conditions. Key concerns:
To override, add a `severity-override-{critical,high,medium,low}` label. |
starius
force-pushed
the
unlock-wait
branch
2 times, most recently
from
41cd873 to
2c8b2ee
Compare
🟠 PR Severity: HIGH
🟠 High (1 file)
🟢 Low (2 files)
🧪 Test Files (1 file, excluded from classification)
AnalysisThis PR enhances the Severity determination:
Review considerations:
To override, add a |
|
Rebased |
🟡 PR Severity: MEDIUM
🟡 Medium (1 file)
🟢 Low (2 files)
📋 Test Files (1 file)
AnalysisThis PR modifies the wallet unlock CLI command to wait until the daemon is ready to accept unlock requests. The changes are primarily in the CLI command layer ( The PR adds retry logic and connection handling to the No severity bump criteria met:
To override, add a |
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
The test cases wait_locked_non_existing (lines 365-378) and wait_unlocked_non_existing (lines 438-453) appear to be testing the exact same code path.
Both have only one state stream returning NON_EXISTING, which means the error occurs during the first "wait for locked" phase, before any unlock is attempted. The expectUnlockCalls: 0 and expectSubscribeCalls: 1 confirm this.
To actually test "wait_unlocked_non_existing" (receiving NON_EXISTING after the unlock call), it would need two streams:
{
name: "wait_unlocked_non_existing",
readPasswordRet: []byte("pw"),
stateStreams: []stateStreamSpec{
{
states: []lnrpc.WalletState{
lnrpc.WalletState_LOCKED, // First wait succeeds
},
},
{
states: []lnrpc.WalletState{
lnrpc.WalletState_NON_EXISTING, // Second wait fails
},
},
},
expectUnlockCalls: 1, // Unlock was called
expectSubscribeCalls: 2, // Two streams used
// ...
}This looks like it might be a copy-paste issue where the second stream was forgotten.
🟠 PR Severity: HIGH
🟠 High (1 file)
🟢 Test/Documentation (3 files)
AnalysisThis PR addresses issue #7749 where wallet unlock requests could be lost during slow LND startup. The change modifies the wallet unlock command flow to:
Severity Rationale:
Key Review Points:
To override, add a |
Great catch! I fixed "wait_locked_non_existing" test case and also added comments before test cases to explain what happens in each test case. |
|
needs a rebase |
Use the StateService stream to wait for LOCKED before sending the unlock request, then wait for UNLOCKED/RPC_ACTIVE before reporting success. If the state shows the wallet is already unlocked, skip sending the unlock request and return an error immediately. This avoids lost unlocks during slow startup. Fix lightningnetwork#7749
Add table-driven unit tests for unlock() that exercise success and error paths, cover flag and arg handling.
|
Rebased |
🟠 PR Severity: HIGH
🟠 High (1 file)
🟢 Low (2 files)
ℹ️ Excluded from severity calculation
AnalysisThis PR modifies the wallet unlocking command logic in While the majority of additions are test code (661 lines), the core logic change in The PR does not meet the criteria for a severity bump (not >20 files, not >500 non-test lines, not multiple critical packages). Recommendation: Review should focus on:
To override, add a |
3 participants
Change Description
Use the StateService stream to wait for LOCKED before sending the unlock request, then wait for UNLOCKED/RPC_ACTIVE before reporting success. If the state shows the wallet is already unlocked, skip sending the unlock request and return an error immediately.
This avoids lost unlocks during slow startup. Fix #7749
Steps to Test
lncli unlockIn my case I see this:
When LND is already unlocked, I see this:
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.