Skip to content
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.

Sign up for GitHub

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

Jump to bottom

lnd+walletunlocker: make unlock/init operations synchronous #4349

Closed
wants to merge 1 commit into from
Closed

lnd+walletunlocker: make unlock/init operations synchronous #4349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions lnd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ func Main(cfg *Config, lisCfg ListenerCfg, shutdownChan <-chan struct{}) error {
params, err := waitForWalletPassword(
cfg, cfg.RESTListeners, serverOpts, restDialOpts,
restProxyDest, tlsCfg, walletUnlockerListeners,
shutdownChan,
)
if err != nil {
err := fmt.Errorf("unable to set up wallet password "+
Expand Down Expand Up @@ -966,7 +967,8 @@ type WalletUnlockParams struct {
func waitForWalletPassword(cfg *Config, restEndpoints []net.Addr,
serverOpts []grpc.ServerOption, restDialOpts []grpc.DialOption,
restProxyDest string, tlsConf *tls.Config,
getListeners rpcListeners) (*WalletUnlockParams, error) {
getListeners rpcListeners,
shutdownChan <-chan struct{}) (*WalletUnlockParams, error) {

// Start a gRPC server listening for HTTP/2 connections, solely used
// for getting the encryption password from the client.
Expand Down Expand Up @@ -996,7 +998,7 @@ func waitForWalletPassword(cfg *Config, restEndpoints []net.Addr,
}
pwService := walletunlocker.New(
chainConfig.ChainDir, activeNetParams.Params, !cfg.SyncFreelist,
macaroonFiles,
macaroonFiles, shutdownChan,
)
lnrpc.RegisterWalletUnlockerServer(grpcServer, pwService)

Expand Down Expand Up @@ -1113,6 +1115,10 @@ func waitForWalletPassword(cfg *Config, restEndpoints []net.Addr,
return nil, err
}

// Now that the wallet has been initialized, we'll close the
// done channel so the call can unblock.
close(initMsg.Done)

return &WalletUnlockParams{
Password: password,
Birthday: birthday,
Expand All @@ -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:

Copy link
Contributor

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.

// Now that we have the parameters, we'll close the done
// channel to allow other operations for the unlocker service.
//
// TODO(roasbeef): push down further?
close(unlockMsg.Done)

return &WalletUnlockParams{
Password: unlockMsg.Passphrase,
RecoveryWindow: unlockMsg.RecoveryWindow,
Expand Down
76 changes: 72 additions & 4 deletions walletunlocker/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"errors"
"fmt"
"os"
"sync"
"time"

"github.com/btcsuite/btcd/chaincfg"
Expand Down Expand Up @@ -54,6 +55,10 @@ type WalletInitMsg struct {
// ChanBackups a set of static channel backups that should be received
// after the wallet has been initialized.
ChanBackups ChannelsToRecover

// Done is a channel that should be closed once the wallet has been
// fully initialized.
Done chan struct{}
}

// WalletUnlockMsg is a message sent by the UnlockerService when a user wishes
Expand Down Expand Up @@ -81,6 +86,10 @@ type WalletUnlockMsg struct {
// ChanBackups a set of static channel backups that should be received
// after the wallet has been unlocked.
ChanBackups ChannelsToRecover

// Done is a channel that should be closed once the wallet has been
// fully unlocked.
Done chan struct{}
}

// UnlockerService implements the WalletUnlocker service used to provide lnd
Expand All @@ -100,18 +109,26 @@ type UnlockerService struct {
noFreelistSync bool
netParams *chaincfg.Params
macaroonFiles []string

quitChan <-chan struct{}

// This mutex is only used to guard concurrent access to the external
// RPC calls. This ensures that we don't allow multiple callers to
// init/unlock the wallet.
sync.Mutex
Roasbeef marked this conversation as resolved.
Show resolved Hide resolved
}

// New creates and returns a new UnlockerService.
func New(chainDir string, params *chaincfg.Params, noFreelistSync bool,
macaroonFiles []string) *UnlockerService {
macaroonFiles []string, quitChan <-chan struct{}) *UnlockerService {

return &UnlockerService{
InitMsgs: make(chan *WalletInitMsg, 1),
UnlockMsgs: make(chan *WalletUnlockMsg, 1),
chainDir: chainDir,
netParams: params,
macaroonFiles: macaroonFiles,
quitChan: quitChan,
}
}

Expand Down Expand Up @@ -242,6 +259,9 @@ func extractChanBackups(chanBackups *lnrpc.ChanBackupSnapshot) *ChannelsToRecove
func (u *UnlockerService) InitWallet(ctx context.Context,
in *lnrpc.InitWalletRequest) (*lnrpc.InitWalletResponse, error) {

u.Lock()
defer u.Unlock()

// Make sure the password meets our constraints.
password := in.WalletPassword
if err := ValidatePassword(password); err != nil {
Expand Down Expand Up @@ -293,6 +313,7 @@ func (u *UnlockerService) InitWallet(ctx context.Context,
Passphrase: password,
WalletSeed: cipherSeed,
RecoveryWindow: uint32(recoveryWindow),
Done: make(chan struct{}),
}

// Before we return the unlock payload, we'll check if we can extract
Expand All @@ -302,7 +323,19 @@ func (u *UnlockerService) InitWallet(ctx context.Context,
initMsg.ChanBackups = *chansToRestore
}

u.InitMsgs <- initMsg
select {
Copy link
Contributor

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?

Copy link
Contributor

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

case u.InitMsgs <- initMsg:
case <-u.quitChan:
return nil, fmt.Errorf("server shutting down")
}

// As we want to avoid a possible deadlock scenario, we'll wait the
// daemon to respond that the wallet has been initialized.
select {
case <-initMsg.Done:
case <-u.quitChan:
return nil, fmt.Errorf("server shutting down")
}

return &lnrpc.InitWalletResponse{}, nil
}
Expand All @@ -313,6 +346,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()
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

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.


password := in.WalletPassword
recoveryWindow := uint32(in.RecoveryWindow)

Expand Down Expand Up @@ -346,6 +382,7 @@ func (u *UnlockerService) UnlockWallet(ctx context.Context,
Passphrase: password,
RecoveryWindow: recoveryWindow,
Wallet: unlockedWallet,
Done: make(chan struct{}),
}

// Before we return the unlock payload, we'll check if we can extract
Expand All @@ -358,7 +395,19 @@ func (u *UnlockerService) UnlockWallet(ctx context.Context,
// At this point we was able to open the existing wallet with the
// provided password. We send the password over the UnlockMsgs
// channel, such that it can be used by lnd to open the wallet.
u.UnlockMsgs <- walletUnlockMsg
select {
case u.UnlockMsgs <- walletUnlockMsg:
case <-u.quitChan:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is never closed.

Copy link
Contributor

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.

return nil, fmt.Errorf("server shutting down")
}

// As we want to avoid a possible deadlock scenario, we'll wait the
// daemon to respond that the wallet has been unlocked.
select {
case <-walletUnlockMsg.Done:
case <-u.quitChan:
return nil, fmt.Errorf("server shutting down")
}

return &lnrpc.UnlockWalletResponse{}, nil
}
Expand All @@ -369,6 +418,9 @@ func (u *UnlockerService) UnlockWallet(ctx context.Context,
func (u *UnlockerService) ChangePassword(ctx context.Context,
in *lnrpc.ChangePasswordRequest) (*lnrpc.ChangePasswordResponse, error) {

u.Lock()
defer u.Unlock()

netDir := btcwallet.NetworkDir(u.chainDir, u.netParams)
loader := wallet.NewLoader(u.netParams, netDir, u.noFreelistSync, 0)

Expand Down Expand Up @@ -431,7 +483,23 @@ func (u *UnlockerService) ChangePassword(ctx context.Context,

// Finally, send the new password across the UnlockPasswords channel to
// automatically unlock the wallet.
u.UnlockMsgs <- &WalletUnlockMsg{Passphrase: in.NewPassword}
unlockMsg := &WalletUnlockMsg{
Passphrase: in.NewPassword,
Done: make(chan struct{}),
}
select {
case u.UnlockMsgs <- unlockMsg:
case <-u.quitChan:
return nil, fmt.Errorf("server shutting down")
}

// As we want to avoid a possible deadlock scenario, we'll wait the
// daemon to respond that the wallet has been unlocked.
select {
case <-unlockMsg.Done:
case <-u.quitChan:
return nil, fmt.Errorf("server shutting down")
}

return &lnrpc.ChangePasswordResponse{}, nil
}
Expand Down
Loading