-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lnd+walletunlocker: make unlock/init operations synchronous #4349
Conversation
As is, this makes unlock/init fully synchronous. If we want to leave it as async, then a goroutine can be added in the unlocker service to release the mutex in the background after the operation is complete. |
@@ -313,6 +342,9 @@ func (u *UnlockerService) InitWallet(ctx context.Context, | |||
func (u *UnlockerService) UnlockWallet(ctx context.Context, | |||
in *lnrpc.UnlockWalletRequest) (*lnrpc.UnlockWalletResponse, error) { | |||
|
|||
u.Lock() | |||
defer u.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may also need another signal/bool at the UnlockerService
level that indicates the wallet has been initialized/unlocked so that we can check it here and everywhere else, otherwise it seems like we still risk attempting to open the wallet twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? Push things down even further? With where things are atm, we won't return back to the caller until the wallet has been fully initialized. However, for unlock we return a bit earlier once we have all the credentials. In my testing, the concurrent init was was ended up tripping things up.
For unlock, things only become "fully finalized" once we create the chain control.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, as soon as this method returns, unlocking isn't even possible since the listener service only has a lifetime of this call.
In this commit, we fix a deadlock that can happen if a user attempts to init then rapidly unlock a wallet right after. In my profiles, it seems the lnd gets caught up on the bbolt flock, which deadlocks the entire process. We fix this issue by making the Init/Unlock calls now fully synchronous. Only a single outstanding request can exist across the entire wallet unlocker service now. Fixes lightningnetwork#4330. Fixes lightningnetwork#3631.
3b4f025
to
d533851
Compare
@@ -472,7 +504,20 @@ func TestChangeWalletPassword(t *testing.T) { | |||
t.Fatalf("expected to receive password %x, got %x", | |||
testPassword, unlockMsg.Passphrase) | |||
} | |||
|
|||
// We'll now close the done channel to unlock the service to be | |||
// able to accept another request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from your comment on the pr it seems the unlocker can only process one request, but several comments in the code refer to processing more requests. is it the case that the wallet unlocker could process more requests, but we only allow one due to the defer cancel()
in waitForWalletPassword
?
@@ -302,7 +323,19 @@ func (u *UnlockerService) InitWallet(ctx context.Context, | |||
initMsg.ChanBackups = *chansToRestore | |||
} | |||
|
|||
u.InitMsgs <- initMsg | |||
select { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add helper to avoid having 3 copies of the same code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guess there are really two versions, one for init and one for unlock...
u.UnlockMsgs <- walletUnlockMsg | ||
select { | ||
case u.UnlockMsgs <- walletUnlockMsg: | ||
case <-u.quitChan: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is never closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, the main shutdown chan is passed to it.
@@ -1124,6 +1130,13 @@ func waitForWalletPassword(cfg *Config, restEndpoints []net.Addr, | |||
// The wallet has already been created in the past, and is simply being | |||
// unlocked. So we'll just return these passphrases. | |||
case unlockMsg := <-pwService.UnlockMsgs: | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was still able to reproduce the issue with the current changes. The issue doesn't seem to be related to the init call being asynchronous, but rather with the UnlockerService
having a queued UnlockWallet
call that it doesn't cancel after InitWallet
has been called. If we had a UnlockerService.Quit
method, we could call it here and check that it's been closed after the lock has been acquired at the UnlockerService
level.
Replaced by #4985 |
In this commit, we fix a deadlock that can happen if a user attempts to
init then rapidly unlock a wallet right after. In my profiles, it seems
the lnd gets caught up on the bbolt flock, which deadlocks the entire
process. We fix this issue by making the Init/Unlock calls now fully
synchronous. Only a single outstanding request can exist across the
entire wallet unlocker service now.
Fixes #4340.
Fixes #3631.