fix(coinjoin): Reuse unused keys and prevent using the same key twice

core/src/main/java/org/bitcoinj/coinjoin/utils/KeyHolderStorage.java

@@ -18,7 +18,6 @@
 import com.google.common.collect.Lists;
 import org.bitcoinj.script.Script;
 import org.bitcoinj.utils.Threading;
-import org.bitcoinj.wallet.Wallet;
 import org.bitcoinj.wallet.WalletEx;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -40,7 +39,7 @@ public Script addKey(WalletEx wallet) {
         lock.lock();
         try {
             storage.add(keyHolder);
-            log.info("CKeyHolderStorage::addKey -- storage size {}", storage.size());
+            log.info("KeyHolderStorage.addKey -- storage size {}", storage.size());
         } finally {
             lock.unlock();
         }

core/src/main/java/org/bitcoinj/coinjoin/utils/ReserveDestination.java

@@ -20,13 +20,14 @@
 import org.bitcoinj.core.NoDestination;
 import org.bitcoinj.core.TransactionDestination;
 import org.bitcoinj.crypto.DeterministicKey;
-import org.bitcoinj.wallet.Wallet;
 import org.bitcoinj.wallet.WalletEx;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class ReserveDestination extends ReserveScript {
+    private static Logger log = LoggerFactory.getLogger(ReserveDestination.class);
     //! The wallet to reserve from
     protected final WalletEx wallet;
-    //LegacyScriptPubKeyMan* m_spk_man{nullptr};
 
     //! The index of the address's key in the keypool
     protected long index = -1;
@@ -42,15 +43,10 @@ public ReserveDestination(WalletEx wallet) {
         this.wallet = wallet;
     }
 
-    //! Destructor. If a key has been reserved and not KeepKey'ed, it will be returned to the keypool
-    protected void finalize() {
-        returnDestination();
-    }
-
     //! Reserve an address
     public TransactionDestination getReservedDestination(boolean internal) {
         if (index == -1) {
-            DeterministicKey key = (DeterministicKey) wallet.getCoinJoin().freshReceiveKey();
+            DeterministicKey key = wallet.getCoinJoin().getUnusedKey();
             if (key == null) {
                 return null;
             }
@@ -59,19 +55,41 @@ public TransactionDestination getReservedDestination(boolean internal) {
             this.internal = true;
         }
 
-        return KeyId.fromBytes(vchPubKey.getPubKeyHash());
+        address = KeyId.fromBytes(vchPubKey.getPubKeyHash());
+        return address;
     }
 
     //! Return reserved address
     void returnDestination() {
         // TODO tell the wallet to reserve the destination
+        if (vchPubKey != null) {
+            if (vchPubKey instanceof DeterministicKey) {
+                wallet.getCoinJoin().addUnusedKey((DeterministicKey) vchPubKey);
+            } else {
+                log.warn("cannot return key: {}", vchPubKey);
+            }
+        } else if (address instanceof KeyId){
+            wallet.getCoinJoin().addUnusedKey((KeyId) address);
+        } else if (!(address instanceof NoDestination)) {
+            log.warn("cannot return key: {}", address);
+        }
         index = -1;
         vchPubKey = null;
         address = NoDestination.get();
     }
     //! Keep the address. Do not return it's key to the keypool when this object goes out of scope
     public void keepDestination() {
         // TODO: tell the wallet to keep the destination
+        // TODO tell the wallet to reserve the destination
+        if (vchPubKey != null) {
+            if (vchPubKey instanceof DeterministicKey) {
+                wallet.getCoinJoin().removeUnusedKey(KeyId.fromBytes(vchPubKey.getPubKeyHash()));
+            }
+        } else if (address instanceof KeyId){
+            wallet.getCoinJoin().removeUnusedKey((KeyId) address);
+        } else if (!(address instanceof NoDestination)) {
+            log.warn("cannot keep key: {}", address);
+        }
         index = -1;
         vchPubKey = null;
         address = NoDestination.get();

core/src/main/java/org/bitcoinj/coinjoin/utils/TransactionBuilder.java

@@ -39,7 +39,7 @@
 
 import static com.google.common.base.Preconditions.checkState;
 
-public class TransactionBuilder {
+public class TransactionBuilder implements AutoCloseable {
     /// Wallet the transaction will be build for
     private final WalletEx wallet;
     /// See CTransactionBuilder() for initialization
@@ -97,11 +97,6 @@ public TransactionBuilder(WalletEx wallet, final CompactTallyItem tallyItem) {
         clear();
     }
 
-    @Override
-    protected void finalize() throws Throwable {
-        clear();
-    }
-
     /// Check it would be possible to add a single output with the amount amount. Returns true if its possible and false if not.
     public boolean couldAddOutput(Coin amountOutput) {
         if (amountOutput.isNegative()) {
@@ -204,7 +199,7 @@ public String toString() {
     }
 
     /// Clear the output vector and keep/return the included keys depending on the value of fKeepKeys
-    void clear() {
+    private void clear() {
         ArrayList<TransactionBuilderOutput> vecOutputsTmp;
 
         // Don't hold cs_outputs while clearing the outputs which might indirectly call lock cs_wallet
@@ -223,7 +218,8 @@ void clear() {
                 key.returnKey();
             }
         }
-        // Always return this key just to make sure..
+        System.out.println("returning: " + dummyReserveDestination.address);
+        // Always return this key
         dummyReserveDestination.returnDestination();
     }
     /// Get the total number of bytes used already by this transaction
@@ -309,4 +305,9 @@ static int calculateMaximumSignedTxSize(Transaction tx, Wallet wallet, List<Tran
     public Transaction getTransaction() {
         return transaction;
     }
+
+    @Override
+    public void close() {
+        clear();
+    }
 }

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

@@ -41,7 +41,7 @@
  * Implements basic keychain functionality for a keychain extension
  */
 abstract public class AbstractKeyChainGroupExtension implements KeyChainGroupExtension {
-    protected final ReentrantLock keyChainGroupLock = Threading.lock("keychaingroup");
+    protected final ReentrantLock keyChainGroupLock = Threading.lock("keychaingroup-extension");
     protected @Nullable Wallet wallet;
 
     protected AbstractKeyChainGroupExtension(Wallet wallet) {