Skip to content

Fix flake in TestChangeWalletPasswordStateless #10075

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
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
93 changes: 56 additions & 37 deletions walletunlocker/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func openOrCreateTestMacStore(tempDir string, pw *[]byte,
return store, nil
}

// TestGenSeedUserEntropy tests that the gen seed method generates a valid
// TestGenSeed tests that the gen seed method generates a valid
// cipher seed mnemonic phrase and user provided source of entropy.
func TestGenSeed(t *testing.T) {
t.Parallel()
Expand Down Expand Up @@ -173,8 +173,8 @@ func TestGenSeed(t *testing.T) {
require.NoError(t, err)
}

// TestGenSeedInvalidEntropy tests that the gen seed method generates a valid
// cipher seed mnemonic pass phrase even when the user doesn't provide its own
// TestGenSeedGenerateEntropy tests that the gen seed method generates a valid
// cipher seed mnemonic passphrase even when the user doesn't provide its own
// source of entropy.
func TestGenSeedGenerateEntropy(t *testing.T) {
t.Parallel()
Expand Down Expand Up @@ -318,7 +318,7 @@ func TestInitWallet(t *testing.T) {
require.Error(t, err)
}

// TestInitWalletInvalidCipherSeed tests that if we attempt to create a wallet
// TestCreateWalletInvalidEntropy tests that if we attempt to create a wallet
// with an invalid cipher seed, then we'll receive an error.
func TestCreateWalletInvalidEntropy(t *testing.T) {
t.Parallel()
Expand All @@ -344,7 +344,7 @@ func TestCreateWalletInvalidEntropy(t *testing.T) {
require.Error(t, err)
}

// TestUnlockWallet checks that trying to unlock non-existing wallet fail, that
// TestUnlockWallet checks that trying to unlock non-existing wallet fails, that
// unlocking existing wallet with wrong passphrase fails, and that unlocking
// existing wallet with correct passphrase succeeds.
func TestUnlockWallet(t *testing.T) {
Expand Down Expand Up @@ -491,7 +491,7 @@ func TestChangeWalletPasswordNewRootKey(t *testing.T) {
// password that meets the length requirement, the password change
// should succeed.
errChan := make(chan error, 1)
go doChangePassword(service, testDir, req, errChan)

Choose a reason for hiding this comment

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

high

Adding a select block with a timeout ensures the test doesn't hang indefinitely if the doChangePassword goroutine fails to complete, improving test reliability.

	select {
	case err := <-errChan:
		t.Fatalf("ChangePassword call failed: %v", err)
	case <-time.After(defaultTestTimeout):
		t.Fatalf("password not received")
	}

Copy link
Member Author

Choose a reason for hiding this comment

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

@gemini-code-assist I think it's done below, where this select is added to make sure it doesn't hang.

Choose a reason for hiding this comment

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

You are absolutely right, @yyforyongyu! My apologies for missing that. The select blocks added in DIFF 6 and DIFF 9 effectively address the potential for the test to hang by waiting for the doChangePassword goroutine to complete. This resolves the concern perfectly. Thank you for pointing that out!

Choose a reason for hiding this comment

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

medium

The testDir argument is no longer needed in doChangePassword function, since the function no longer performs file operations. Removing it simplifies the function call and reduces the chance of passing incorrect or irrelevant data.

Suggested change
go doChangePassword(service, testDir, req, errChan)
go doChangePassword(service, req, errChan)

Copy link
Member Author

Choose a reason for hiding this comment

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

@gemini-code-assist It's removed?

Choose a reason for hiding this comment

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

Yes, that's correct. Looking at DIFF 9 and DIFF 10 in the provided patches, the testDir parameter has been removed from the doChangePassword function's signature, and all its usages within the function body have also been removed. This is why it's no longer necessary to pass it in the function call, simplifying the code as mentioned in my previous comment.

go doChangePassword(service, req, errChan)

// The new password should be sent over the channel.
select {
Expand All @@ -510,6 +510,15 @@ func TestChangeWalletPasswordNewRootKey(t *testing.T) {
t.Fatalf("password not received")
}

// Wait for the doChangePassword goroutine to finish.
select {
case err := <-errChan:
require.NoError(t, err, "ChangePassword call failed")

case <-time.After(defaultTestTimeout):
t.Fatalf("ChangePassword timed out")
}

// The files should no longer exist.
for _, tempFile := range tempFiles {
f, err := os.Open(tempFile)
Expand All @@ -518,11 +527,17 @@ func TestChangeWalletPasswordNewRootKey(t *testing.T) {
t.Fatal("file exists but it shouldn't")
}
}

// Close the old db first.
require.NoError(t, store.Backend.Close())

// Check that the new password can be used to open the db.
assertPasswordChanged(t, testDir, req.NewPassword)
}

// TestChangeWalletPasswordStateless checks that trying to change the password
// of an existing wallet that was initialized stateless works when when the
// --stateless_init flat is set. Also checks that if no password is given,
// of an existing wallet that was initialized stateless works when the
// --stateless_init flag is set. Also checks that if no password is given,
// the default password is used.
func TestChangeWalletPasswordStateless(t *testing.T) {
t.Parallel()
Expand Down Expand Up @@ -594,7 +609,7 @@ func TestChangeWalletPasswordStateless(t *testing.T) {
// async and then wait for the unlock message to arrive so we can send
// back a fake macaroon.
errChan := make(chan error, 1)
go doChangePassword(service, testDir, req, errChan)
go doChangePassword(service, req, errChan)

// Password and recovery window should be sent over the channel.
select {
Expand All @@ -612,9 +627,24 @@ func TestChangeWalletPasswordStateless(t *testing.T) {
case <-time.After(defaultTestTimeout):
t.Fatalf("password not received")
}

// Wait for the doChangePassword goroutine to finish.
select {
case err := <-errChan:
require.NoError(t, err, "ChangePassword call failed")

case <-time.After(defaultTestTimeout):
t.Fatalf("ChangePassword timed out")
}

// Close the old db first.
require.NoError(t, store.Backend.Close())

// Check that the new password can be used to open the db.
assertPasswordChanged(t, testDir, req.NewPassword)
}

func doChangePassword(service *walletunlocker.UnlockerService, testDir string,
func doChangePassword(service *walletunlocker.UnlockerService,
req *lnrpc.ChangePasswordRequest, errChan chan error) {

// When providing the correct wallet's current password and a new
Expand All @@ -630,37 +660,26 @@ func doChangePassword(service *walletunlocker.UnlockerService, testDir string,
if !bytes.Equal(response.AdminMacaroon, testMac) {
errChan <- fmt.Errorf("mismatched macaroon: expected %x, got "+
"%x", testMac, response.AdminMacaroon)
return
}

// Close the macaroon DB and try to open it and read the root key with
// the new password.
close(errChan)
}

// assertPasswordChanged asserts that the new password can be used to open the
// store.
func assertPasswordChanged(t *testing.T, testDir string, newPassword []byte) {
// Open it and read the root key with the new password.
store, err := openOrCreateTestMacStore(
testDir, &testPassword, testNetParams,
testDir, &newPassword, testNetParams,
)
if err != nil {
errChan <- fmt.Errorf("could not create test store: %w", err)
return
}
_, _, err = store.RootKey(defaultRootKeyIDContext)
if err != nil {
errChan <- fmt.Errorf("could not get root key: %w", err)
return
}
require.NoError(t, err)

// Do cleanup now. Since we are in a go func, the defer at the top of
// the outer would not work, because it would delete the directory
// before we could check the content in here.
err = store.Close()
if err != nil {
errChan <- fmt.Errorf("could not close store: %w", err)
return
}
// Assert that we can read the root key.
_, _, err = store.RootKey(defaultRootKeyIDContext)
require.NoError(t, err)

// The backend database isn't closed automatically if the store is
// closed, do that now manually.
err = store.Backend.Close()
if err != nil {
errChan <- fmt.Errorf("could not close db: %w", err)
return
}
// Close the db once done.
require.NoError(t, store.Close())
require.NoError(t, store.Backend.Close())
}
Loading