Key rotation: construct new HD chain based on the oldest possible key, a la upgrade, with a fresh random HD chain only being created if all random keys are rotating.

core/src/main/java/org/bitcoinj/core/Wallet.java

@@ -4273,15 +4273,12 @@ public class Wallet extends BaseTaggableObject implements Serializable, BlockCha
     }
 
     /**
-     * <p>When a key rotation time is set, and money controlled by keys created before the given timestamp T will be
+     * <p>When a key rotation time is set, any money controlled by keys created before the given timestamp T will be
      * automatically respent to any key that was created after T. This can be used to recover from a situation where
      * a set of keys is believed to be compromised. You can stop key rotation by calling this method again with zero
-     * as the argument, or by using {@link #setKeyRotationEnabled(boolean)}. Once set up, calling
+     * as the argument. Once set up, calling {@link #maybeDoMaintenance(org.spongycastle.crypto.params.KeyParameter, boolean)}
-     * {@link #maybeDoMaintenance(org.spongycastle.crypto.params.KeyParameter, boolean)} will create and possibly
+     * will create and possibly send rotation transactions: but it won't be done automatically (because you might have
-     * send rotation transactions: but it won't be done automatically (because you might have to ask for the users
+     * to ask for the users password).</p>
-     * password).</p>
-     *
-     * <p>Note that this method won't do anything unless you call {@link #setKeyRotationEnabled(boolean)} first.</p>
      *
      * <p>The given time cannot be in the future.</p>
      */
@@ -4311,7 +4308,7 @@ public class Wallet extends BaseTaggableObject implements Serializable, BlockCha
      * @param andSend if true, send the transactions via the tx broadcaster and return them, if false just return them.
      * @return A list of transactions that the wallet just made/will make for internal maintenance. Might be empty.
      */
-    public ListenableFuture<List<Transaction>> maybeDoMaintenance(@Nullable KeyParameter aesKey, boolean andSend) {
+    public ListenableFuture<List<Transaction>> maybeDoMaintenance(@Nullable KeyParameter aesKey, boolean andSend) throws DeterministicUpgradeRequiresPassword {
         List<Transaction> txns;
         lock.lock();
         try {
@@ -4347,7 +4344,7 @@ public class Wallet extends BaseTaggableObject implements Serializable, BlockCha
     }
 
     // Checks to see if any coins are controlled by rotating keys and if so, spends them.
-    private List<Transaction> maybeRotateKeys(@Nullable KeyParameter aesKey) {
+    private List<Transaction> maybeRotateKeys(@Nullable KeyParameter aesKey) throws DeterministicUpgradeRequiresPassword {
         checkState(lock.isHeldByCurrentThread());
         List<Transaction> results = Lists.newLinkedList();
         // TODO: Handle chain replays here.
@@ -4363,8 +4360,14 @@ public class Wallet extends BaseTaggableObject implements Serializable, BlockCha
             }
         }
         if (allChainsRotating) {
-            log.info("All HD chains are currently rotating, creating a new one");
+            log.info("All HD chains are currently rotating, attempting to create a new one from the next oldest non-rotating key material ...");
-            keychain.createAndActivateNewHDChain();
+            try {
+                keychain.upgradeToDeterministic(keyRotationTimestamp, aesKey);
+                log.info(" ... upgraded to HD again, based on next best oldest key.");
+            } catch (AllRandomKeysRotating rotating) {
+                log.info(" ... no non-rotating random keys available, generating entirely new HD tree: backup required after this.");
+                keychain.createAndActivateNewHDChain();
+            }
         }
 
         // Because transactions are size limited, we might not be able to re-key the entire wallet in one go. So

core/src/main/java/org/bitcoinj/wallet/AllRandomKeysRotating.java

@@ -0,0 +1,7 @@
+package org.bitcoinj.wallet;
+
+/**
+ * Indicates that an attempt was made to upgrade a random wallet to deterministic, but there were no non-rotating
+ * random keys to use as source material for the seed. Add a non-compromised key first!
+ */
+public class AllRandomKeysRotating  extends RuntimeException {}

core/src/main/java/org/bitcoinj/wallet/KeyChainGroup.java

@@ -630,12 +630,14 @@ public class KeyChainGroup implements KeyBag {
      *         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 {
+    public DeterministicKeyChain upgradeToDeterministic(long keyRotationTimeSecs, @Nullable KeyParameter aesKey) throws DeterministicUpgradeRequiresPassword, AllRandomKeysRotating {
-        checkState(chains.isEmpty());
         checkState(basic.numKeys() > 0);
         checkArgument(keyRotationTimeSecs >= 0);
-        ECKey keyToUse = basic.findOldestKeyAfter(keyRotationTimeSecs);
+        // Subtract one because the key rotation time might have been set to the creation time of the first known good
-        checkArgument(keyToUse != null, "All keys are considered rotating, so we cannot upgrade deterministically.");
+        // key, in which case, that's the one we want to find.
+        ECKey keyToUse = basic.findOldestKeyAfter(keyRotationTimeSecs - 1);
+        if (keyToUse == null)
+            throw new AllRandomKeysRotating();
 
         if (keyToUse.isEncrypted()) {
             if (aesKey == null) {
@@ -658,7 +660,12 @@ public class KeyChainGroup implements KeyBag {
             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");
+        if (chains.isEmpty()) {
+            log.info("Auto-upgrading pre-HD wallet to HD!");
+        } else {
+            log.info("Wallet with existing HD chain is being re-upgraded due to change in key rotation time.");
+        }
+        log.info("Instantiating new HD chain using oldest non-rotating private key (address: {})", keyToUse.toAddress(params));
         byte[] entropy = checkNotNull(keyToUse.getSecretBytes());
         // Private keys should be at least 128 bits long.
         checkState(entropy.length >= DeterministicSeed.DEFAULT_SEED_ENTROPY_BITS / 8);

core/src/test/java/org/bitcoinj/core/WalletTest.java

@@ -78,6 +78,7 @@ public class WalletTest extends TestWithWallet {
     @Override
     public void setUp() throws Exception {
         super.setUp();
+        // TODO: Move these fields into the right tests so we don't create two wallets for every test case.
         encryptedWallet = new Wallet(params);
         myEncryptedAddress = encryptedWallet.freshReceiveKey().toAddress(params);
         encryptedWallet.encrypt(PASSWORD1);
@@ -96,7 +97,6 @@ public class WalletTest extends TestWithWallet {
         createMarriedWallet(threshold, numKeys, true);
     }
 
-    
     private void createMarriedWallet(int threshold, int numKeys, boolean addSigners) throws BlockStoreException {
         wallet = new Wallet(params);
         blockStore = new MemoryBlockStore(params);
@@ -2302,11 +2302,8 @@ public class WalletTest extends TestWithWallet {
         assertEquals(0, broadcaster.size());
         assertFalse(wallet.isKeyRotating(key1));
 
-        // We got compromised! We have an old style random-only wallet. So let's upgrade to HD: for that we need a fresh
+        // We got compromised!
-        // random key that's not rotating as the wallet won't create a new seed for us, it'll just refuse to upgrade.
         Utils.rollMockClock(1);
-        ECKey key3 = new ECKey();
-        wallet.importKey(key3);
         wallet.setKeyRotationTime(compromiseTime);
         assertTrue(wallet.isKeyRotating(key1));
         wallet.maybeDoMaintenance(null, true);
@@ -2381,6 +2378,42 @@ public class WalletTest extends TestWithWallet {
         assertNotEquals(watchKey1, watchKey2);
     }
 
+    @SuppressWarnings("ConstantConditions")
+    @Test
+    public void keyRotationHD2() throws Exception {
+        // Check we handle the following scenario: a weak random key is created, then some good random keys are created
+        // but the weakness of the first isn't known yet. The wallet is upgraded to HD based on the weak key. Later, we
+        // find out about the weakness and set the rotation time to after the bad key's creation date. A new HD chain
+        // should be created based on the oldest known good key and the old chain + bad random key should rotate to it.
+
+        // We fix the private keys just to make the test deterministic (last byte differs).
+        Utils.setMockClock();
+        ECKey badKey = ECKey.fromPrivate(Utils.HEX.decode("00905b93f990267f4104f316261fc10f9f983551f9ef160854f40102eb71cffdbb"));
+        badKey.setCreationTimeSeconds(Utils.currentTimeSeconds());
+        Utils.rollMockClock(86400);
+        ECKey goodKey = ECKey.fromPrivate(Utils.HEX.decode("00905b93f990267f4104f316261fc10f9f983551f9ef160854f40102eb71cffdcc"));
+        goodKey.setCreationTimeSeconds(Utils.currentTimeSeconds());
+
+        // Do an upgrade based on the bad key.
+        KeyChainGroup kcg = new KeyChainGroup(params);
+        kcg.importKeys(badKey, goodKey);
+        Utils.rollMockClock(86400);
+        wallet = new Wallet(params, kcg);   // This avoids the automatic HD initialisation
+        wallet.upgradeToDeterministic(null);
+        DeterministicKey badWatchingKey = wallet.getWatchingKey();
+        assertEquals(badKey.getCreationTimeSeconds(), badWatchingKey.getCreationTimeSeconds());
+        sendMoneyToWallet(wallet, CENT, badWatchingKey.toAddress(params), AbstractBlockChain.NewBlockType.BEST_CHAIN);
+
+        // Now we set the rotation time to the time we started making good keys. This should create a new HD chain.
+        wallet.setKeyRotationTime(goodKey.getCreationTimeSeconds());
+        List<Transaction> txns = wallet.maybeDoMaintenance(null, false).get();
+        assertEquals(1, txns.size());
+        Address output = txns.get(0).getOutput(0).getAddressFromP2PKHScript(params);
+        ECKey usedKey = wallet.findKeyFromPubHash(output.getHash160());
+        assertEquals(goodKey.getCreationTimeSeconds(), usedKey.getCreationTimeSeconds());
+        assertEquals("mrM3TpCnav5YQuVA1xLercCGJH4DXujMtv", usedKey.toAddress(params).toString());
+    }
+
     @Test(expected = IllegalArgumentException.class)
     public void importOfHDKeyForbidden() throws Exception {
         wallet.importKey(wallet.freshReceiveKey());