accounts: fix data race when key is locked after the unlock timeout

While here, also improve the docs and speed up the tests.
The tests used the scrypt keystore with ridiculous settins and took 20s
each.

2 files changed, 45 insertions, 20 deletions

diff --git a/accounts/account_manager.go b/accounts/account_manager.go
index 2a9d9b0ae..c6c2787dc 100644
--- a/accounts/account_manager.go
+++ b/accounts/account_manager.go
@@ -78,8 +78,8 @@ func (am *Manager) DeleteAccount(address common.Address, auth string) error {
 
 func (am *Manager) Sign(a Account, toSign []byte) (signature []byte, err error) {
 	am.mutex.RLock()
+	defer am.mutex.RUnlock()
 	unlockedKey, found := am.unlocked[a.Address]
-	am.mutex.RUnlock()
 	if !found {
 		return nil, ErrLocked
 	}
@@ -87,14 +87,17 @@ func (am *Manager) Sign(a Account, toSign []byte) (signature []byte, err error)
 	return signature, err
 }
 
-// unlock indefinitely
+// Unlock unlocks the given account indefinitely.
 func (am *Manager) Unlock(addr common.Address, keyAuth string) error {
 	return am.TimedUnlock(addr, keyAuth, 0)
 }
 
-// Unlock unlocks the account with the given address. The account
-// stays unlocked for the duration of timeout
-// it timeout is 0 the account is unlocked for the entire session
+// TimedUnlock unlocks the account with the given address. The account
+// stays unlocked for the duration of timeout. A timeout of 0 unlocks the account
+// until the program exits.
+//
+// If the accout is already unlocked, TimedUnlock extends or shortens
+// the active unlock timeout.
 func (am *Manager) TimedUnlock(addr common.Address, keyAuth string, timeout time.Duration) error {
 	key, err := am.keyStore.GetKey(addr, keyAuth)
 	if err != nil {
diff --git a/accounts/accounts_test.go b/accounts/accounts_test.go
index 9d09156af..0be8c50bd 100644
--- a/accounts/accounts_test.go
+++ b/accounts/accounts_test.go
@@ -23,9 +23,10 @@ import (
 	"time"
 
 	"github.com/ethereum/go-ethereum/crypto"
-	"github.com/ethereum/go-ethereum/crypto/randentropy"
 )
 
+var testSigData = make([]byte, 32)
+
 func TestSign(t *testing.T) {
 	dir, ks := tmpKeyStore(t, crypto.NewKeyStorePlain)
 	defer os.RemoveAll(dir)
@@ -33,26 +34,24 @@ func TestSign(t *testing.T) {
 	am := NewManager(ks)
 	pass := "" // not used but required by API
 	a1, err := am.NewAccount(pass)
-	toSign := randentropy.GetEntropyCSPRNG(32)
 	am.Unlock(a1.Address, "")
 
-	_, err = am.Sign(a1, toSign)
+	_, err = am.Sign(a1, testSigData)
 	if err != nil {
 		t.Fatal(err)
 	}
 }
 
 func TestTimedUnlock(t *testing.T) {
-	dir, ks := tmpKeyStore(t, crypto.NewKeyStorePassphrase)
+	dir, ks := tmpKeyStore(t, crypto.NewKeyStorePlain)
 	defer os.RemoveAll(dir)
 
 	am := NewManager(ks)
 	pass := "foo"
 	a1, err := am.NewAccount(pass)
-	toSign := randentropy.GetEntropyCSPRNG(32)
 
 	// Signing without passphrase fails because account is locked
-	_, err = am.Sign(a1, toSign)
+	_, err = am.Sign(a1, testSigData)
 	if err != ErrLocked {
 		t.Fatal("Signing should've failed with ErrLocked before unlocking, got ", err)
 	}
@@ -63,28 +62,26 @@ func TestTimedUnlock(t *testing.T) {
 	}
 
 	// Signing without passphrase works because account is temp unlocked
-	_, err = am.Sign(a1, toSign)
+	_, err = am.Sign(a1, testSigData)
 	if err != nil {
 		t.Fatal("Signing shouldn't return an error after unlocking, got ", err)
 	}
 
 	// Signing fails again after automatic locking
 	time.Sleep(150 * time.Millisecond)
-	_, err = am.Sign(a1, toSign)
+	_, err = am.Sign(a1, testSigData)
 	if err != ErrLocked {
 		t.Fatal("Signing should've failed with ErrLocked timeout expired, got ", err)
 	}
-
 }
 
 func TestOverrideUnlock(t *testing.T) {
-	dir, ks := tmpKeyStore(t, crypto.NewKeyStorePassphrase)
+	dir, ks := tmpKeyStore(t, crypto.NewKeyStorePlain)
 	defer os.RemoveAll(dir)
 
 	am := NewManager(ks)
 	pass := "foo"
 	a1, err := am.NewAccount(pass)
-	toSign := randentropy.GetEntropyCSPRNG(32)
 
 	// Unlock indefinitely
 	if err = am.Unlock(a1.Address, pass); err != nil {
@@ -92,7 +89,7 @@ func TestOverrideUnlock(t *testing.T) {
 	}
 
 	// Signing without passphrase works because account is temp unlocked
-	_, err = am.Sign(a1, toSign)
+	_, err = am.Sign(a1, testSigData)
 	if err != nil {
 		t.Fatal("Signing shouldn't return an error after unlocking, got ", err)
 	}
@@ -103,20 +100,45 @@ func TestOverrideUnlock(t *testing.T) {
 	}
 
 	// Signing without passphrase still works because account is temp unlocked
-	_, err = am.Sign(a1, toSign)
+	_, err = am.Sign(a1, testSigData)
 	if err != nil {
 		t.Fatal("Signing shouldn't return an error after unlocking, got ", err)
 	}
 
 	// Signing fails again after automatic locking
 	time.Sleep(150 * time.Millisecond)
-	_, err = am.Sign(a1, toSign)
+	_, err = am.Sign(a1, testSigData)
 	if err != ErrLocked {
 		t.Fatal("Signing should've failed with ErrLocked timeout expired, got ", err)
 	}
 }
 
-//
+// This test should fail under -race if signing races the expiration goroutine.
+func TestSignRace(t *testing.T) {
+	dir, ks := tmpKeyStore(t, crypto.NewKeyStorePlain)
+	defer os.RemoveAll(dir)
+
+	// Create a test account.
+	am := NewManager(ks)
+	a1, err := am.NewAccount("")
+	if err != nil {
+		t.Fatal("could not create the test account", err)
+	}
+
+	if err := am.TimedUnlock(a1.Address, "", 15*time.Millisecond); err != nil {
+		t.Fatalf("could not unlock the test account", err)
+	}
+	end := time.Now().Add(80 * time.Millisecond)
+	for time.Now().Before(end) {
+		if _, err := am.Sign(a1, testSigData); err == ErrLocked {
+			return
+		} else if err != nil {
+			t.Errorf("Sign error: %v", err)
+			return
+		}
+	}
+	t.Errorf("Account did not lock within the timeout")
+}
 
 func tmpKeyStore(t *testing.T, new func(string) crypto.KeyStore) (string, crypto.KeyStore) {
 	d, err := ioutil.TempDir("", "eth-keystore-test")