Immediately advance current addresses instead of doing it lazily. This avoids a bug whereby an app might quit after using a change address, thus currentKey(CHANGE) == null and it gets reset to the last used address when the wallet is round-tripped.

Unit tests didn't catch this because they didn't simulate the app terminating after the send, and weren't explicitly checking that the change address was different, so improve tests to do those things.

Additionally implement marking as used for married wallets.

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

@@ -939,6 +939,9 @@ public class Wallet extends BaseTaggableObject implements Serializable, BlockCha
                     } else if (script.isSentToAddress()) {
                         byte[] pubkeyHash = script.getPubKeyHash();
                         keychain.markPubKeyHashAsUsed(pubkeyHash);
+                    } else if (script.isPayToScriptHash() && keychain.isMarried()) {
+                        Address a = Address.fromP2SHScript(tx.getParams(), script);
+                        keychain.markP2SHAddressAsUsed(a);
                     }
                 } catch (ScriptException e) {
                     // Just means we didn't understand the output of this transaction: ignore it.

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

@@ -69,9 +69,10 @@ public class KeyChainGroup implements KeyBag {
     private BasicKeyChain basic;
     private NetworkParameters params;
     protected final LinkedList<DeterministicKeyChain> chains;
+    // currentKeys is used for normal, non-multisig/married wallets. currentAddresses is used when we're handing out
+    // P2SH addresses. They're mutually exclusive.
     private final EnumMap<KeyChain.KeyPurpose, DeterministicKey> currentKeys;
-
+    private final EnumMap<KeyChain.KeyPurpose, Address> currentAddresses;
-    private EnumMap<KeyChain.KeyPurpose, Address> currentAddresses;
     @Nullable private KeyCrypter keyCrypter;
     private int lookaheadSize = -1;
     private int lookaheadThreshold = -1;
@@ -358,6 +359,22 @@ public class KeyChainGroup implements KeyBag {
         return null;
     }
 
+    public void markP2SHAddressAsUsed(Address address) {
+        checkState(isMarried());
+        checkArgument(address.isP2SHAddress());
+        RedeemData data = findRedeemDataFromScriptHash(address.getHash160());
+        if (data == null)
+            return;   // Not our P2SH address.
+        for (ECKey key : data.keys) {
+            for (DeterministicKeyChain chain : chains) {
+                DeterministicKey k = chain.findKeyFromPubKey(key.getPubKey());
+                if (k == null) continue;
+                chain.markKeyAsUsed(k);
+                maybeMarkCurrentAddressAsUsed(address);
+            }
+        }
+    }
+
     @Nullable
     @Override
     public ECKey findKeyFromPubHash(byte[] pubkeyHash) {
@@ -385,12 +402,27 @@ public class KeyChainGroup implements KeyBag {
         }
     }
 
+    /** If the given P2SH address is "current", advance it to a new one. */
+    private void maybeMarkCurrentAddressAsUsed(Address address) {
+        checkState(isMarried());
+        checkArgument(address.isP2SHAddress());
+        for (Map.Entry<KeyChain.KeyPurpose, Address> entry : currentAddresses.entrySet()) {
+            if (entry.getValue() != null && entry.getValue().equals(address)) {
+                log.info("Marking P2SH address as used: {}", address);
+                currentAddresses.put(entry.getKey(), freshAddress(entry.getKey()));
+                return;
+            }
+        }
+    }
+
     /** If the given key is "current", advance the current key to a new one. */
     private void maybeMarkCurrentKeyAsUsed(DeterministicKey key) {
+        // It's OK for currentKeys to be empty here: it means we're a married wallet and the key may be a part of a
+        // rotating chain.
         for (Map.Entry<KeyChain.KeyPurpose, DeterministicKey> entry : currentKeys.entrySet()) {
             if (entry.getValue() != null && entry.getValue().equals(key)) {
                 log.info("Marking key as used: {}", key);
-                currentKeys.put(entry.getKey(), null);
+                currentKeys.put(entry.getKey(), freshKey(entry.getKey()));
                 return;
             }
         }

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

@@ -23,6 +23,7 @@ import org.bitcoinj.signers.StatelessTransactionSigner;
 import org.bitcoinj.signers.TransactionSigner;
 import org.bitcoinj.store.BlockStoreException;
 import org.bitcoinj.store.MemoryBlockStore;
+import org.bitcoinj.store.UnreadableWalletException;
 import org.bitcoinj.store.WalletProtobufSerializer;
 import org.bitcoinj.testing.*;
 import org.bitcoinj.utils.ExchangeRate;
@@ -348,7 +349,6 @@ public class WalletTest extends TestWithWallet {
         assertEquals("Wrong number of ALL.3", 1, wallet.getTransactions(true).size());
         assertEquals(TransactionConfidence.Source.SELF, t2.getConfidence().getSource());
         assertEquals(Transaction.Purpose.USER_PAYMENT, t2.getPurpose());
-        assertEquals(wallet.getChangeAddress(), t2.getOutput(1).getScriptPubKey().getToAddress(params));
 
         // Do some basic sanity checks.
         basicSanityChecks(wallet, t2, destination);
@@ -422,16 +422,18 @@ public class WalletTest extends TestWithWallet {
     }
 
     private void spendUnconfirmedChange(Wallet wallet, Transaction t2, KeyParameter aesKey) throws Exception {
+        if (wallet.getTransactionSigners().size() == 1)   // don't bother reconfiguring the p2sh wallet
+            wallet = roundTrip(wallet);
         Coin v3 = valueOf(0, 49);
         assertEquals(v3, wallet.getBalance());
         Wallet.SendRequest req = Wallet.SendRequest.to(new ECKey().toAddress(params), valueOf(0, 48));
         req.aesKey = aesKey;
-        Address a = req.changeAddress = new ECKey().toAddress(params);
         req.ensureMinRequiredFee = false;
         req.shuffleOutputs = false;
         wallet.completeTx(req);
         Transaction t3 = req.tx;
-        assertEquals(a, t3.getOutput(1).getScriptPubKey().getToAddress(params));
+        assertNotEquals(t2.getOutput(1).getScriptPubKey().getToAddress(params),
+                        t3.getOutput(1).getScriptPubKey().getToAddress(params));
         assertNotNull(t3);
         wallet.commitTx(t3);
         assertTrue(wallet.isConsistent());
@@ -2387,8 +2389,7 @@ public class WalletTest extends TestWithWallet {
         // We don't attempt to race an attacker against unconfirmed transactions.
 
         // Now round-trip the wallet and check the protobufs are storing the data correctly.
-        Protos.Wallet protos = new WalletProtobufSerializer().walletToProto(wallet);
+        wallet = roundTrip(wallet);
-        wallet = new WalletProtobufSerializer().readWallet(params, null, protos);
 
         tx = wallet.getTransaction(tx.getHash());
         checkNotNull(tx);
@@ -2403,6 +2404,11 @@ public class WalletTest extends TestWithWallet {
         assertArrayEquals(address.getHash160(), tx.getOutput(0).getScriptPubKey().getPubKeyHash());
     }
 
+    private Wallet roundTrip(Wallet wallet) throws UnreadableWalletException {
+        Protos.Wallet protos = new WalletProtobufSerializer().walletToProto(wallet);
+        return new WalletProtobufSerializer().readWallet(params, null, protos);
+    }
+
     @Test
     public void keyRotationHD() throws Exception {
         // Test that if we rotate an HD chain, a new one is created and all arrivals on the old keys are moved.
@@ -2713,8 +2719,7 @@ public class WalletTest extends TestWithWallet {
         TransactionSigner signer = new NopTransactionSigner(true);
         wallet.addTransactionSigner(signer);
         assertEquals(2, wallet.getTransactionSigners().size());
-        Protos.Wallet protos = new WalletProtobufSerializer().walletToProto(wallet);
+        wallet = roundTrip(wallet);
-        wallet = new WalletProtobufSerializer().readWallet(params, null, protos);
         assertEquals(2, wallet.getTransactionSigners().size());
         assertTrue(wallet.getTransactionSigners().get(1).isReady());
     }