HD Wallets: implement auto upgrade behaviour and refresh the design doc.

HD Wallets TODO.txt

@@ -2,7 +2,6 @@
 - Store the account key creation time for an HD hierarchy.
 - Support seeds up to 512 bits in size. Test compatibility with greenaddress, at least for some keys.
 - Support for key rotation
-- Support for auto upgrade
 - Calculate lookahead keys on a background thread.
 - Redo internals of DKC to support arbitrary tree structures.
 - Add a REFUND key purpose and map to the receive tree (for now).

core/src/main/java/com/google/bitcoin/core/Wallet.java

@@ -343,7 +343,20 @@ public class Wallet extends BaseTaggableObject implements Serializable, BlockCha
     public List<DeterministicKey> freshKeys(KeyChain.KeyPurpose purpose, int numberOfKeys) {
         lock.lock();
         try {
-            List<DeterministicKey> keys = keychain.freshKeys(purpose, numberOfKeys);
+            List<DeterministicKey> keys;
+            try {
+                keys = keychain.freshKeys(purpose, numberOfKeys);
+            } catch (DeterministicUpgradeRequiredException e) {
+                log.info("Attempt to request a fresh HD key on a non-upgraded wallet, trying to upgrade ...");
+                try {
+                    upgradeToDeterministic(null);
+                    keys = keychain.freshKeys(purpose, numberOfKeys);
+                } catch (DeterministicUpgradeRequiresPassword e2) {
+                    // Nope, can't do it. Rethrow the original exception.
+                    log.error("Failed to auto upgrade because wallet is encrypted, giving up. You should call wallet.upgradeToDeterministic yourself to avoid this situation.");
+                    throw e;
+                }
+            }
             // Do we really need an immediate hard save? Arguably all this is doing is saving the 'current' key
             // and that's not quite so important, so we could coalesce for more performance.
             saveNow();
@@ -383,6 +396,37 @@ public class Wallet extends BaseTaggableObject implements Serializable, BlockCha
         return freshAddress(KeyChain.KeyPurpose.RECEIVE_FUNDS);
     }
 
+
+    /**
+     * Upgrades the wallet to be deterministic (BIP32). You should call this, possibly providing the users encryption
+     * key, after loading a wallet produced by previous versions of bitcoinj. If the wallet is encrypted the key
+     * <b>must</b> be provided, due to the way the seed is derived deterministically from private key bytes: failing
+     * to do this will result in an exception being thrown. For non-encrypted wallets, the upgrade will be done for
+     * you automatically the first time a new key is requested (this happens when spending due to the change address).
+     */
+    public void upgradeToDeterministic(@Nullable KeyParameter aesKey) throws DeterministicUpgradeRequiresPassword {
+        lock.lock();
+        try {
+            keychain.upgradeToDeterministic(vKeyRotationEnabled ? vKeyRotationTimestamp : 0, aesKey);
+        } finally {
+            lock.unlock();
+        }
+    }
+
+    /**
+     * Returns true if the wallet contains random keys and no HD chains, in which case you should call
+     * {@link #upgradeToDeterministic(org.spongycastle.crypto.params.KeyParameter)} before attempting to do anything
+     * that would require a new address or key.
+     */
+    public boolean isDeterministicUpgradeRequired() {
+        lock.lock();
+        try {
+            return keychain.isDeterministicUpgradeRequired();
+        } finally {
+            lock.unlock();
+        }
+    }
+
     /**
      * Returns a snapshot of the watched scripts. This view is not live.
      */

core/src/main/java/com/google/bitcoin/wallet/BasicKeyChain.java

@@ -541,4 +541,30 @@ public class BasicKeyChain implements EncryptableKeyChain {
     public int numBloomFilterEntries() {
         return numKeys() * 2;
     }
+
+
+    ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+    //
+    // Key rotation support
+    //
+    ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+
+    /** Returns the first ECKey created after the given UNIX time, or null if there is none. */
+    @Nullable
+    public ECKey findOldestKeyAfter(long timeSecs) {
+        lock.lock();
+        try {
+            ECKey oldest = null;
+            for (ECKey key : hashToKeys.values()) {
+                final long keyTime = key.getCreationTimeSeconds();
+                if (keyTime > timeSecs) {
+                    if (oldest == null || oldest.getCreationTimeSeconds() > keyTime)
+                        oldest = key;
+                }
+            }
+            return oldest;
+        } finally {
+            lock.unlock();
+        }
+    }
 }

core/src/main/java/com/google/bitcoin/wallet/DeterministicKeyChain.java

@@ -852,7 +852,7 @@ public class DeterministicKeyChain implements EncryptableKeyChain {
         }
     }
 
-    /** Returns the seed or null if this chain is encrypted or watching. */
+    /** Returns the seed or null if this chain is a watching chain. */
     @Nullable
     public DeterministicSeed getSeed() {
         lock.lock();

core/src/main/java/com/google/bitcoin/wallet/DeterministicSeed.java

@@ -23,6 +23,7 @@ import org.spongycastle.crypto.params.KeyParameter;
 
 import javax.annotation.Nullable;
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.List;
 
 import static com.google.bitcoin.core.Utils.HEX;
@@ -149,4 +150,29 @@ public class DeterministicSeed implements EncryptableItem {
     public List<String> toMnemonicCode() {
         return toMnemonicCode(getCachedMnemonicCode());
     }
+
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+
+        DeterministicSeed seed = (DeterministicSeed) o;
+
+        if (creationTimeSeconds != seed.creationTimeSeconds) return false;
+        if (encryptedSeed != null) {
+            if (seed.encryptedSeed == null) return false;
+            if (!encryptedSeed.equals(seed.encryptedSeed)) return false;
+        } else {
+            if (!Arrays.equals(unencryptedSeed, seed.unencryptedSeed)) return false;
+        }
+
+        return true;
+    }
+
+    @Override
+    public int hashCode() {
+        int result = encryptedSeed != null ? encryptedSeed.hashCode() : Arrays.hashCode(unencryptedSeed);
+        result = 31 * result + (int) (creationTimeSeconds ^ (creationTimeSeconds >>> 32));
+        return result;
+    }
 }

core/src/main/java/com/google/bitcoin/wallet/DeterministicUpgradeRequiredException.java

@@ -0,0 +1,7 @@
+package com.google.bitcoin.wallet;
+
+/**
+ * Indicates that an attempt was made to use HD wallet features on a wallet that was deserialized from an old,
+ * pre-HD random wallet without calling upgradeToDeterministic() beforehand.
+ */
+public class DeterministicUpgradeRequiredException extends RuntimeException {}

core/src/main/java/com/google/bitcoin/wallet/DeterministicUpgradeRequiresPassword.java

@@ -0,0 +1,8 @@
+package com.google.bitcoin.wallet;
+
+/**
+ * Indicates that the pre-HD random wallet is encrypted, so you should try the upgrade again after getting the
+ * users password. This is required because HD wallets are upgraded from random using the private key bytes of
+ * the oldest non-rotating key, in order to make the upgrade process itself deterministic.
+ */
+public class DeterministicUpgradeRequiresPassword extends Exception {}

core/src/main/java/com/google/bitcoin/wallet/KeyChainGroup.java

@@ -28,6 +28,8 @@ import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import org.bitcoinj.wallet.Protos;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.spongycastle.crypto.params.KeyParameter;
 
 import javax.annotation.Nullable;
@@ -54,6 +56,7 @@ import static com.google.common.base.Preconditions.*;
  * combining their responses together when necessary.</p>
  */
 public class KeyChainGroup {
+    private static final Logger log = LoggerFactory.getLogger(KeyChainGroup.class);
     private BasicKeyChain basic;
     private final List<DeterministicKeyChain> chains;
     private final EnumMap<KeyChain.KeyPurpose, DeterministicKey> currentKeys;
@@ -61,7 +64,7 @@ public class KeyChainGroup {
     private int lookaheadSize = -1;
     private int lookaheadThreshold = -1;
 
-    /** Creates a keychain group with no basic chain, and a single randomly initialized HD chain. */
+    /** Creates a keychain group with no basic chain, and a single, lazily created HD chain. */
     public KeyChainGroup() {
         this(null, new ArrayList<DeterministicKeyChain>(1), null, null);
     }
@@ -99,6 +102,7 @@ public class KeyChainGroup {
     }
 
     private void createAndActivateNewHDChain() {
+        // We can't do auto upgrade here because we don't know the rotation time, if any.
         final DeterministicKeyChain chain = new DeterministicKeyChain(new SecureRandom());
         for (ListenerRegistration<KeyChainEventListener> registration : basic.getListeners())
             chain.addEventListener(registration.listener, registration.executor);
@@ -137,7 +141,7 @@ public class KeyChainGroup {
      * to someone who wishes to send money.
      */
     public DeterministicKey freshKey(KeyChain.KeyPurpose purpose) {
-        return freshKeys(purpose,1).get(0);
+        return freshKeys(purpose, 1).get(0);
     }
 
     /**
@@ -164,8 +168,16 @@ public class KeyChainGroup {
 
     /** Returns the key chain that's used for generation of fresh/current keys. This is always the newest HD chain. */
     public DeterministicKeyChain getActiveKeyChain() {
-        if (chains.isEmpty())
+        if (chains.isEmpty()) {
+            if (basic.numKeys() > 0) {
+                log.warn("No HD chain present but random keys are: you probably deserialized an old wallet.");
+                // If called from the wallet (most likely) it'll try to upgrade us, as it knows the rotation time
+                // but not the password.
+                throw new DeterministicUpgradeRequiredException();
+            }
+            // Otherwise we have no HD chains and no random keys: we are a new born! So a random seed is fine.
             createAndActivateNewHDChain();
+        }
         return chains.get(chains.size() - 1);
     }
 
@@ -228,7 +240,7 @@ public class KeyChainGroup {
     public boolean checkAESKey(KeyParameter aesKey) {
         checkState(keyCrypter != null, "Not encrypted");
         if (basic.numKeys() > 0)
-            return basic.checkAESKey(aesKey) && getActiveKeyChain().checkAESKey(aesKey);
+            return basic.checkAESKey(aesKey);
         return getActiveKeyChain().checkAESKey(aesKey);
     }
 
@@ -323,7 +335,9 @@ public class KeyChainGroup {
      * Encrypt the keys in the group using the KeyCrypter and the AES key. A good default KeyCrypter to use is
      * {@link com.google.bitcoin.crypto.KeyCrypterScrypt}.
      *
-     * @throws com.google.bitcoin.crypto.KeyCrypterException Thrown if the wallet encryption fails for some reason, leaving the group unchanged.
+     * @throws com.google.bitcoin.crypto.KeyCrypterException Thrown if the wallet encryption fails for some reason,
+     *         leaving the group unchanged.
+     * @throws DeterministicUpgradeRequiredException Thrown if there are random keys but no HD chain.
      */
     public void encrypt(KeyCrypter keyCrypter, KeyParameter aesKey) {
         checkNotNull(keyCrypter);
@@ -331,13 +345,13 @@ public class KeyChainGroup {
         // This code must be exception safe.
         BasicKeyChain newBasic = basic.toEncrypted(keyCrypter, aesKey);
         List<DeterministicKeyChain> newChains = new ArrayList<DeterministicKeyChain>(chains.size());
-        // If the user is trying to encrypt us before ever asking for a key, we might not have lazy created an HD chain
+        if (chains.isEmpty() && basic.numKeys() == 0) {
-        // yet. So do it now.
+            // No HD chains and no random keys: encrypting an entirely empty keychain group. But we can't do that, we
-        if (chains.isEmpty())
+            // must have something to encrypt: so instantiate a new HD chain here.
             createAndActivateNewHDChain();
+        }
         for (DeterministicKeyChain chain : chains)
             newChains.add(chain.toEncrypted(keyCrypter, aesKey));
-
         this.keyCrypter = keyCrypter;
         basic = newBasic;
         chains.clear();
@@ -447,12 +461,8 @@ public class KeyChainGroup {
         BasicKeyChain basicKeyChain = BasicKeyChain.fromProtobufUnencrypted(keys);
         List<DeterministicKeyChain> chains = DeterministicKeyChain.fromProtobuf(keys, null);
         EnumMap<KeyChain.KeyPurpose, DeterministicKey> currentKeys = null;
-
+        if (!chains.isEmpty())
-        if (chains.isEmpty()) {
-            // TODO: Old bag-of-keys style wallet only! Auto-upgrade time!
-        } else {
             currentKeys = createCurrentKeysMap(chains);
-        }
         return new KeyChainGroup(basicKeyChain, chains, currentKeys, null);
     }
 
@@ -461,15 +471,78 @@ public class KeyChainGroup {
         BasicKeyChain basicKeyChain = BasicKeyChain.fromProtobufEncrypted(keys, crypter);
         List<DeterministicKeyChain> chains = DeterministicKeyChain.fromProtobuf(keys, crypter);
         EnumMap<KeyChain.KeyPurpose, DeterministicKey> currentKeys = null;
-
+        if (!chains.isEmpty())
-        if (chains.isEmpty()) {
-            // TODO: Old bag-of-keys style wallet only! Auto-upgrade time!
-        } else {
             currentKeys = createCurrentKeysMap(chains);
-        }
         return new KeyChainGroup(basicKeyChain, chains, currentKeys, crypter);
     }
 
+    /**
+     * If the key chain contains only random keys and no deterministic key chains, this method will create a chain
+     * based on the oldest non-rotating private key (i.e. the seed is derived from the old wallet).
+     *
+     * @param keyRotationTimeSecs If non-zero, UNIX time for which keys created before this are assumed to be
+     *                            compromised or weak, those keys will not be used for deterministic upgrade.
+     * @param aesKey If non-null, the encryption key the keychain is encrypted under. If the keychain is encrypted
+     *               and this is not supplied, an exception is thrown letting you know you should ask the user for
+     *               their password, turn it into a key, and then try again.
+     * @throws java.lang.IllegalStateException if there is already a deterministic key chain present or if there are
+     *                                         no random keys (i.e. this is not an upgrade scenario), or if aesKey is
+     *                                         provided but the wallet is not encrypted.
+     * @throws java.lang.IllegalArgumentException if the rotation time specified excludes all keys.
+     * @throws com.google.bitcoin.wallet.DeterministicUpgradeRequiresPassword if the key chain group is encrypted
+     *         and you should provide the users encryption key.
+     * @return the DeterministicKeyChain that was created by the upgrade.
+     */
+    public DeterministicKeyChain upgradeToDeterministic(long keyRotationTimeSecs, @Nullable KeyParameter aesKey) throws DeterministicUpgradeRequiresPassword {
+        checkState(chains.isEmpty());
+        checkState(basic.numKeys() > 0);
+        checkArgument(keyRotationTimeSecs >= 0);
+        ECKey keyToUse = basic.findOldestKeyAfter(keyRotationTimeSecs);
+        checkArgument(keyToUse != null, "All keys are considered rotating, so we cannot upgrade deterministically.");
+
+        if (keyToUse.isEncrypted()) {
+            if (aesKey == null) {
+                // We can't auto upgrade because we don't know the users password at this point. We throw an
+                // exception so the calling code knows to abort the load and ask the user for their password, they can
+                // then try loading the wallet again passing in the AES key.
+                //
+                // There are a few different approaches we could have used here, but they all suck. The most obvious
+                // is to try and be as lazy as possible, running in the old random-wallet mode until the user enters
+                // their password for some other reason and doing the upgrade then. But this could result in strange
+                // and unexpected UI flows for the user, as well as complicating the job of wallet developers who then
+                // have to support both "old" and "new" UI modes simultaneously, switching them on the fly. Given that
+                // this is a one-off transition, it seems more reasonable to just ask the user for their password
+                // on startup, and then the wallet app can have all the widgets for accessing seed words etc active
+                // all the time.
+                throw new DeterministicUpgradeRequiresPassword();
+            }
+            keyToUse = keyToUse.decrypt(aesKey);
+        } else if (aesKey != null) {
+            throw new IllegalStateException("AES Key was provided but wallet is not encrypted.");
+        }
+
+        log.info("Auto-upgrading pre-HD wallet using oldest non-rotating private key");
+        byte[] seed = checkNotNull(keyToUse.getSecretBytes());
+        // Private keys should be at least 128 bits long.
+        checkState(seed.length >= 128 / 8);
+        // We reduce the entropy here to 128 bits because people like to write their seeds down on paper, and 128
+        // bits should be sufficient forever unless the laws of the universe change or ECC is broken; in either case
+        // we all have bigger problems.
+        seed = Arrays.copyOfRange(seed, 0, 128 / 8);    // final argument is exclusive range.
+        checkState(seed.length == 128 / 8);
+        DeterministicKeyChain chain = new DeterministicKeyChain(seed, keyToUse.getCreationTimeSeconds());
+        if (aesKey != null) {
+            chain = chain.toEncrypted(checkNotNull(basic.getKeyCrypter()), aesKey);
+        }
+        chains.add(chain);
+        return chain;
+    }
+
+    /** Returns true if the group contains random keys but no HD chains. */
+    public boolean isDeterministicUpgradeRequired() {
+        return basic.numKeys() > 0 && chains.isEmpty();
+    }
+
     private static EnumMap<KeyChain.KeyPurpose, DeterministicKey> createCurrentKeysMap(List<DeterministicKeyChain> chains) {
         DeterministicKeyChain activeChain = chains.get(chains.size() - 1);
 

core/src/test/java/com/google/bitcoin/core/WalletTest.java

@@ -2378,4 +2378,41 @@ public class WalletTest extends TestWithWallet {
         wallet.freshReceiveKey();
         assertEquals(6, keys.size());
     }
+
+    @Test
+    public void upgradeToHDUnencrypted() throws Exception {
+        // This isn't very deep because most of it is tested in KeyChainGroupTest and Wallet just forwards most logic
+        // there. We're mostly concerned with the slightly different auto upgrade logic: KeyChainGroup won't do an
+        // on-demand auto upgrade of the wallet to HD even in the unencrypted case, because the key rotation time is
+        // a property of the Wallet, not the KeyChainGroup (it should perhaps be moved at some point - it doesn't matter
+        // much where it goes). Wallet on the other hand will try to auto-upgrade you when possible.
+
+        // Create an old-style random wallet.
+        wallet = new Wallet(params);
+        wallet.importKey(new ECKey());
+        wallet.importKey(new ECKey());
+        assertTrue(wallet.isDeterministicUpgradeRequired());
+        // Use an HD feature.
+        wallet.freshReceiveKey();
+        assertFalse(wallet.isDeterministicUpgradeRequired());
+    }
+
+    public void upgradeToHDEncrypted() throws Exception {
+        // Create an old-style random wallet.
+        wallet = new Wallet(params);
+        wallet.importKey(new ECKey());
+        wallet.importKey(new ECKey());
+        assertTrue(wallet.isDeterministicUpgradeRequired());
+        KeyCrypter crypter = new KeyCrypterScrypt();
+        KeyParameter aesKey = crypter.deriveKey("abc");
+        wallet.encrypt(crypter, aesKey);
+        try {
+            wallet.freshReceiveKey();
+        } catch (DeterministicUpgradeRequiredException e) {
+            // Expected.
+        }
+        wallet.upgradeToDeterministic(aesKey);
+        assertFalse(wallet.isDeterministicUpgradeRequired());
+        wallet.freshReceiveKey();  // works.
+    }
 }

core/src/test/java/com/google/bitcoin/wallet/BasicKeyChainTest.java

@@ -253,4 +253,19 @@ public class BasicKeyChainTest {
         ECKey key3 = new ECKey();
         assertFalse(filter.contains(key3.getPubKey()));
     }
+
+    @Test
+    public void oldestKeyAfter() throws Exception {
+        Utils.setMockClock();
+        long now = Utils.currentTimeSeconds();
+        final ECKey key1 = new ECKey();
+        Utils.rollMockClock(86400);
+        final ECKey key2 = new ECKey();
+        final ArrayList<ECKey> keys = Lists.newArrayList(key1, key2);
+        assertEquals(2, chain.importKeys(keys));
+
+        assertNull(chain.findOldestKeyAfter(now + 86400 * 2));
+        assertEquals(key1, chain.findOldestKeyAfter(now - 1));
+        assertEquals(key2, chain.findOldestKeyAfter(now + 86400 - 1));
+    }
 }

core/src/test/java/com/google/bitcoin/wallet/KeyChainGroupTest.java

@@ -30,12 +30,14 @@ import org.bitcoinj.wallet.Protos;
 import org.junit.Before;
 import org.junit.Test;
 import org.spongycastle.crypto.params.KeyParameter;
+import org.spongycastle.util.Arrays;
 
 import java.util.List;
 import java.util.concurrent.atomic.AtomicReference;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 import static org.junit.Assert.*;
+import static org.junit.Assert.assertArrayEquals;
 
 public class KeyChainGroupTest {
     // Number of initial keys in this tests HD wallet, including interior keys.
@@ -326,4 +328,89 @@ public class KeyChainGroupTest {
         ECKey key2 = group2.freshKey(KeyChain.KeyPurpose.RECEIVE_FUNDS);
         assertEquals(key1, key2);
     }
+
+    @Test(expected = DeterministicUpgradeRequiredException.class)
+    public void deterministicUpgradeRequired() throws Exception {
+        // Check that if we try to use HD features in a KCG that only has random keys, we get an exception.
+        group = new KeyChainGroup();
+        group.importKeys(new ECKey(), new ECKey());
+        assertTrue(group.isDeterministicUpgradeRequired());
+        group.freshKey(KeyChain.KeyPurpose.RECEIVE_FUNDS);   // throws
+    }
+
+    @Test
+    public void deterministicUpgradeUnencrypted() throws Exception {
+        // Check that a group that contains only random keys has its HD chain created using the private key bytes of
+        // the oldest random key, so upgrading the same wallet twice gives the same outcome.
+        group = new KeyChainGroup();
+        group.setLookaheadSize(LOOKAHEAD_SIZE);   // Don't want slow tests.
+        ECKey key1 = new ECKey();
+        Utils.rollMockClock(86400);
+        ECKey key2 = new ECKey();
+        group.importKeys(key2, key1);
+
+        List<Protos.Key> protobufs = group.serializeToProtobuf();
+        group.upgradeToDeterministic(0, null);
+        assertFalse(group.isDeterministicUpgradeRequired());
+        DeterministicKey dkey1 = group.freshKey(KeyChain.KeyPurpose.RECEIVE_FUNDS);
+        DeterministicSeed seed1 = group.getActiveKeyChain().getSeed();
+        assertNotNull(seed1);
+
+        group = KeyChainGroup.fromProtobufUnencrypted(protobufs);
+        group.upgradeToDeterministic(0, null);  // Should give same result as last time.
+        DeterministicKey dkey2 = group.freshKey(KeyChain.KeyPurpose.RECEIVE_FUNDS);
+        DeterministicSeed seed2 = group.getActiveKeyChain().getSeed();
+        assertEquals(seed1, seed2);
+        assertEquals(dkey1, dkey2);
+
+        // Check we used the right (oldest) key despite backwards import order.
+        byte[] truncatedBytes = Arrays.copyOfRange(key1.getSecretBytes(), 0, 16);
+        assertArrayEquals(seed1.getSecretBytes(), truncatedBytes);
+    }
+
+    @Test
+    public void deterministicUpgradeRotating() throws Exception {
+        group = new KeyChainGroup();
+        group.setLookaheadSize(LOOKAHEAD_SIZE);   // Don't want slow tests.
+        long now = Utils.currentTimeSeconds();
+        ECKey key1 = new ECKey();
+        Utils.rollMockClock(86400);
+        ECKey key2 = new ECKey();
+        Utils.rollMockClock(86400);
+        ECKey key3 = new ECKey();
+        group.importKeys(key2, key1, key3);
+        group.upgradeToDeterministic(now + 10, null);
+        DeterministicSeed seed = group.getActiveKeyChain().getSeed();
+        assertNotNull(seed);
+        // Check we used the right key: oldest non rotating.
+        byte[] truncatedBytes = Arrays.copyOfRange(key2.getSecretBytes(), 0, 16);
+        assertArrayEquals(seed.getSecretBytes(), truncatedBytes);
+    }
+
+    @Test
+    public void deterministicUpgradeEncrypted() throws Exception {
+        group = new KeyChainGroup();
+        final ECKey key = new ECKey();
+        group.importKeys(key);
+        final KeyCrypterScrypt crypter = new KeyCrypterScrypt();
+        final KeyParameter aesKey = crypter.deriveKey("abc");
+        assertTrue(group.isDeterministicUpgradeRequired());
+        group.encrypt(crypter, aesKey);
+        assertTrue(group.isDeterministicUpgradeRequired());
+        try {
+            group.upgradeToDeterministic(0, null);
+            fail();
+        } catch (DeterministicUpgradeRequiresPassword e) {
+            // Expected.
+        }
+        group.upgradeToDeterministic(0, aesKey);
+        assertFalse(group.isDeterministicUpgradeRequired());
+        final DeterministicSeed deterministicSeed = group.getActiveKeyChain().getSeed();
+        assertNotNull(deterministicSeed);
+        assertTrue(deterministicSeed.isEncrypted());
+        byte[] seed = checkNotNull(group.getActiveKeyChain().toDecrypted(aesKey).getSeed()).getSecretBytes();
+        // Check we used the right key: oldest non rotating.
+        byte[] truncatedBytes = Arrays.copyOfRange(key.getSecretBytes(), 0, 16);
+        assertArrayEquals(seed, truncatedBytes);
+    }
 }

designdocs/Deterministic wallets.txt

@@ -25,29 +25,32 @@ Create a new KeyChain interface and provide BasicKeyChain, DeterministicKeyChain
 Wallets may contain multiple key chains. However only the last one is "active" in the sense that it will be used to
 create new keys. There's no way to change that.
 
-Wallet API changes to have an importKey method that works like addKey does today, and forwards to the basic key chain.
+The Wallet class has most key handling code refactored out into KeyChainGroup, which handles multiplexing a
-There's also a getKey method that forwards to the active key chain (which after upgrade will always be deterministic)
+BasicKeyChain (for random keys, if any), and zero or more DeterministicKeyChain. Wallet ends up just forwarding method
-and requests a key for a specific purpose, specified by an enum parameter. The getKey method supports requesting keys
+calls to this class most of the time. Thus in this section where the Wallet API is discussed, it can be assumed that
-for the following purposes:
+KeyChainGroup has the same API. Although individual key chain objects have their own locks and are expected to be thread
+safe, KeyChainGroup itself is not and is not exposed directly by Wallet: it's an implementation detail, and locked under
+the Wallet lock.
+
+The Wallet API changes to have an importKey method that works like addKey does today, and forwards to the BasicKeyChain.
+There's also a freshKey method that forwards to the active HD chain and requests a key for a specific purpose,
+specified by an enum parameter. The freshKey method supports requesting keys for the following purposes:
 
  - CHANGE
  - RECEIVE_FUNDS
 
 and may in future also have additional purposes like for micropayment channels, etc. These map to the notion of
-"accounts" as defined in the BIP32 spec, but otherwise should not be exposed in any user interfaces. getKey is not
+"accounts" as defined in the BIP32 spec, but otherwise should not be exposed in any user interfaces. freshKey is
-guaranteed to return a freshly generated key: it may return the same key repeatedly if the underlying keychain either
+guaranteed to return a freshly generated key: it will not return the same key repeatedly. There is also a currentKey
-does not support auto-extension (basic) or does not believe the key was used yet (deterministic). In cases where the
+method that returns a stable key suitable for display in the user interface: it will be changed automatically when
-user knows they need a fresh key even though earlier keys were not yet used, a newKey method takes the same purpose
+it's observed being used in a transaction.
-parameter as getKey, but tells the keychain to ignore usage heuristics and always generate a new key.
 
 There can be multiple key chains. There is always:
 
-<=1 basic key chain
+* 1 basic key chain, though it may be empty.
->=0 deterministic key chains
+* >=0 deterministic key chains
 
-Thus it's possible to have more than one deterministic key chain, but not more than one basic key chain. New wallets
+Thus it's possible to have more than one deterministic key chain, but not more than one basic key chain.
-will not have a basic key chain unless an attempt to import a key is made. Old wallets will have both a basic and
-(after upgrade) one deterministic key chain, and this is expected to be the normal state of operation.
 
 Multiple deterministic key chains become relevant when key rotation happens. Individual keys in a deterministic
 heirarchy do not rotate. Instead the rotation time is applied only to the seed. Either the whole key chain rotates or
@@ -198,21 +201,23 @@ private derivation (see the BIP32 spec for more information on this).
 Upgrade
 -------
 
-HD wallets are superior to regular wallets, there are no reasons why you would want not want to use them. Therefore
+HD wallets are strictly superior to old random wallets, thus by default all new wallets will be HD. The deterministic
-wallets generated by older versions of bitcoinj will be upgraded in place at the first opportunity. This process should
+key chain will be created on demand by the KeyChainGroup, which allows the default parameters like lookahead size to
-be transparent to end users. The wallet seed, which would normally be randomly generated, will for upgraded wallets
+be configured after construction of the wallet but before the DeterministicKeyChain is constructed.
-be set to the private key bytes of the oldest non-rotating key. This selection ensures that the key is least likely to
-be compromised and most likely to be backed up. Assuming the private key really is random, this gives security of the
-HD wallet as well. It also means that the user does not necessarily need to make a new backup after the upgrade,
-although creation of one would be recommended anyway.
 
-Encrypted wallets cannot be upgraded at load time. They must wait until the decryption key has been made available.
+For old wallets that contain random keys, attempts to use any methods that rely on an HD chain being present will
-This may happen on explicit decrypt by an end user, or more likely, the first time they spend money or click "add
+either automatically upgrade the wallet to HD, or if encrypted, throw an unchecked exception until the API user invokes
-address" with the new wallet app. The wallet class will have a maybeUpgradeToDeterministic method which will be called
+an upgrade method that takes the users encryption key. The upgrade will select the oldest non-rotating private key,
-at various places where private key bytes might become available, like just after deserialization or a method being
+truncate it to 128 bits and use that as the seed for the new HD chain. We truncate and thus lose entropy because
-called which has an AES key parameter. The method will check if an upgrade is necessary, and if so add a
+128 bits is more than enough, and people like to write down their seeds on paper. 128 bit seeds using the BIP 39
-DeterministicKeyChain to the internal list of chains.
+mnemonic code specification yields 12 words, which is a convenient size.
 
+As part of migrating to deterministic wallets, if the wallet is encrypted the wallet author is expected to test
+after load whether the wallet needs an upgrade, and call the upgrade method explicitly with the password.
+Note that attempting to create a spend will fail if the wallet is not upgraded, because it will attempt to retrieve a
+change key which is done deterministically in the new version of the code: thus an non-upgraded wallet is not very
+useful for more than viewing (unless the API caller explicitly overrides the change address behaviour using the
+relevant field in SendRequest of course).
 
 Test plan
 ---------

tools/src/main/java/com/google/bitcoin/tools/WalletTool.java

@@ -34,6 +34,8 @@ import com.google.bitcoin.uri.BitcoinURI;
 import com.google.bitcoin.uri.BitcoinURIParseException;
 import com.google.bitcoin.utils.BriefLogFormatter;
 import com.google.bitcoin.wallet.DeterministicSeed;
+import com.google.bitcoin.wallet.DeterministicUpgradeRequiredException;
+import com.google.bitcoin.wallet.DeterministicUpgradeRequiresPassword;
 import com.google.common.base.Charsets;
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
@@ -47,8 +49,10 @@ import joptsimple.util.DateConverter;
 import org.bitcoinj.wallet.Protos;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.spongycastle.crypto.params.KeyParameter;
 import org.spongycastle.util.encoders.Hex;
 
+import javax.annotation.Nullable;
 import java.io.BufferedInputStream;
 import java.io.File;
 import java.io.FileInputStream;
@@ -67,6 +71,7 @@ import java.util.logging.Level;
 import java.util.logging.LogManager;
 
 import static com.google.bitcoin.core.Coin.parseCoin;
+import static com.google.common.base.Preconditions.checkNotNull;
 
 /**
  * A command line tool for manipulating wallets and working with Bitcoin.
@@ -458,11 +463,9 @@ public class WalletTool {
                 wallet.allowSpendingUnconfirmedTransactions();
             }
             if (password != null) {
-                if (!wallet.checkPassword(password)) {
+                req.aesKey = passwordToKey(true);
-                    System.err.println("Password is incorrect.");
+                if (req.aesKey == null)
-                    return;
+                    return;  // Error message already printed.
-                }
-                req.aesKey = wallet.getKeyCrypter().deriveKey(password);
             }
             wallet.completeTx(req);
 
@@ -578,11 +581,9 @@ public class WalletTool {
             }
             final Wallet.SendRequest req = session.getSendRequest();
             if (password != null) {
-                if (!wallet.checkPassword(password)) {
+                req.aesKey = passwordToKey(true);
-                    System.err.println("Password is incorrect.");
+                if (req.aesKey == null)
-                    return;
+                    return;   // Error message already printed.
-                }
-                req.aesKey = wallet.getKeyCrypter().deriveKey(password);
             }
             wallet.completeTx(req);  // may throw InsufficientMoneyException.
             if (options.has("offline")) {
@@ -862,11 +863,38 @@ public class WalletTool {
                 log.info("Setting keychain lookahead size to {}", size);
                 wallet.setKeychainLookaheadSize(size);
             }
-            ECKey key = wallet.freshReceiveKey();
+            ECKey key;
+            try {
+                key = wallet.freshReceiveKey();
+            } catch (DeterministicUpgradeRequiredException e) {
+                try {
+                    KeyParameter aesKey = passwordToKey(false);
+                    wallet.upgradeToDeterministic(aesKey);
+                } catch (DeterministicUpgradeRequiresPassword e2) {
+                    System.err.println("This wallet must be upgraded to be deterministic, but it's encrypted: please supply the password and try again.");
+                    return;
+                }
+                key = wallet.freshReceiveKey();
+            }
             System.out.println(key.toAddress(params) + " " + key);
         }
     }
 
+    @Nullable
+    private static KeyParameter passwordToKey(boolean printError) {
+        if (password == null) {
+            if (printError)
+                System.err.println("You must provide a password.");
+            return null;
+        }
+        if (!wallet.checkPassword(password)) {
+            if (printError)
+                System.err.println("The password is incorrect.");
+            return null;
+        }
+        return checkNotNull(wallet.getKeyCrypter()).deriveKey(password);
+    }
+
     private static void importKey() {
         ECKey key;
         long creationTimeSeconds = getCreationTimeSeconds();
@@ -907,11 +935,10 @@ public class WalletTool {
         }
         try {
             if (wallet.isEncrypted()) {
-                if (password == null || !wallet.checkPassword(password)) {
+                KeyParameter aesKey = passwordToKey(true);
-                    System.err.println("The password is incorrect.");
+                if (aesKey == null)
-                    return;
+                    return;   // Error message already printed.
-                }
+                key = key.encrypt(checkNotNull(wallet.getKeyCrypter()), aesKey);
-                key = key.encrypt(wallet.getKeyCrypter(), wallet.getKeyCrypter().deriveKey(password));
             }
             wallet.importKey(key);
             System.out.println(key.toAddress(params) + " " + key);