Fix channel client state saving to save earlier (and be secure)

core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelClient.java

@@ -166,6 +166,10 @@ public class PaymentChannelClient {
         state.provideRefundSignature(returnedRefund.getSignature().toByteArray());
         step = InitStep.WAITING_FOR_CHANNEL_OPEN;
 
+        // Before we can send the server the contract (ie send it to the network), we must ensure that our refund
+        // transaction is safely in the wallet - thus we store it (this also keeps it up-to-date when we pay)
+        state.storeChannelInWallet(serverId);
+
         Protos.ProvideContract.Builder provideContractBuilder = Protos.ProvideContract.newBuilder()
                 .setTx(ByteString.copyFrom(state.getMultisigContract().bitcoinSerialize()));
 
@@ -182,8 +186,6 @@ public class PaymentChannelClient {
 
         if (step == InitStep.WAITING_FOR_INITIATE)
             state = new PaymentChannelClientState(storedChannel, wallet);
-        // Let state know its wallet id so it gets stored and stays up-to-date
-        state.storeChannelInWallet(serverId);
         step = InitStep.CHANNEL_OPEN;
         // channelOpen should disable timeouts, but
         // TODO accomodate high latency between PROVIDE_CONTRACT and here

core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelClientState.java

@@ -23,6 +23,7 @@ import com.google.bitcoin.core.*;
 import com.google.bitcoin.crypto.TransactionSignature;
 import com.google.bitcoin.script.Script;
 import com.google.bitcoin.script.ScriptBuilder;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import org.slf4j.Logger;
@@ -55,9 +56,9 @@ import static com.google.common.base.Preconditions.*;
  * exception will be thrown at this point. Once this is done, call
  * {@link PaymentChannelClientState#getIncompleteRefundTransaction()} and pass the resultant transaction through to the
  * server. Once you have retrieved the signature, use {@link PaymentChannelClientState#provideRefundSignature(byte[])}.
- * If no exception is thrown at this point, we are secure against a malicious server attempting to destroy all our coins
+ * You must then call {@link PaymentChannelClientState#storeChannelInWallet(Sha256Hash)} to store the refund transaction
- * and can provide the server with the multi-sig contract (via {@link PaymentChannelClientState#getMultisigContract()})
+ * in the wallet, protecting you against a malicious server attempting to destroy all your coins. At this point, you can
- * safely.
+ * provide the server with the multi-sig contract (via {@link PaymentChannelClientState#getMultisigContract()}) safely.
  * </p>
  */
 public class PaymentChannelClientState {
@@ -92,6 +93,7 @@ public class PaymentChannelClientState {
         NEW,
         INITIATED,
         WAITING_FOR_SIGNED_REFUND,
+        SAVE_STATE_IN_WALLET,
         PROVIDE_MULTISIG_CONTRACT_TO_SERVER,
         READY,
         EXPIRED
@@ -256,8 +258,7 @@ public class PaymentChannelClientState {
         TransactionInput refundInput = refundTx.getInput(0);
         refundInput.setScriptSig(scriptSig);
         refundInput.verify(multisigContractOutput);
-        wallet.commitTx(multisigContract);
+        state = State.SAVE_STATE_IN_WALLET;
-        state = State.PROVIDE_MULTISIG_CONTRACT_TO_SERVER;
     }
 
     private synchronized Transaction makeUnsignedChannelContract(BigInteger valueToMe) throws ValueOutOfRangeException {
@@ -346,6 +347,27 @@ public class PaymentChannelClientState {
         storedChannel = null;
     }
 
+    /**
+     * Skips saving state in the wallet for testing
+     */
+    @VisibleForTesting synchronized void fakeSave() {
+        try {
+            wallet.commitTx(multisigContract);
+        } catch (VerificationException e) {
+            throw new RuntimeException(e); // We created it
+        }
+        state = State.PROVIDE_MULTISIG_CONTRACT_TO_SERVER;
+    }
+
+    @VisibleForTesting synchronized void doStoreChannelInWallet(Sha256Hash id) {
+        StoredPaymentChannelClientStates channels = (StoredPaymentChannelClientStates)
+                wallet.getExtensions().get(StoredPaymentChannelClientStates.EXTENSION_ID);
+        checkState(channels.getChannel(id, multisigContract.getHash()) == null);
+        storedChannel = new StoredClientChannel(id, multisigContract, refundTx, myKey, valueToMe, refundFees);
+        channels.putChannel(storedChannel);
+        wallet.addOrUpdateExtension(channels);
+    }
+
     /**
      * <p>Stores this channel's state in the wallet as a part of a {@link StoredPaymentChannelClientStates} wallet
      * extension and keeps it up-to-date each time payment is incremented. This allows the
@@ -359,18 +381,19 @@ public class PaymentChannelClientState {
      *           unique.
      */
     public synchronized void storeChannelInWallet(Sha256Hash id) {
-        checkState(state == State.READY && id != null);
+        checkState(state == State.SAVE_STATE_IN_WALLET && id != null);
         if (storedChannel != null) {
             checkState(storedChannel.id.equals(id));
             return;
         }
+        doStoreChannelInWallet(id);
 
-        StoredPaymentChannelClientStates channels = (StoredPaymentChannelClientStates)
+        try {
-                                            wallet.getExtensions().get(StoredPaymentChannelClientStates.EXTENSION_ID);
+            wallet.commitTx(multisigContract);
-        checkState(channels.getChannel(id, multisigContract.getHash()) == null);
+        } catch (VerificationException e) {
-        storedChannel = new StoredClientChannel(id, multisigContract, refundTx, myKey, valueToMe, refundFees);
+            throw new RuntimeException(e); // We created it
-        channels.putChannel(storedChannel);
+        }
-        wallet.addOrUpdateExtension(channels);
+        state = State.PROVIDE_MULTISIG_CONTRACT_TO_SERVER;
     }
 
     /**

core/src/test/java/com/google/bitcoin/protocols/channels/PaymentChannelStateTest.java

@@ -125,6 +125,8 @@ public class PaymentChannelStateTest extends TestWithWallet {
         assertEquals(PaymentChannelServerState.State.WAITING_FOR_MULTISIG_CONTRACT, serverState.getState());
         // This verifies that the refund can spend the multi-sig output when run.
         clientState.provideRefundSignature(refundSig);
+        assertEquals(PaymentChannelClientState.State.SAVE_STATE_IN_WALLET, clientState.getState());
+        clientState.fakeSave();
         assertEquals(PaymentChannelClientState.State.PROVIDE_MULTISIG_CONTRACT_TO_SERVER, clientState.getState());
 
         // Validate the multisig contract looks right.
@@ -245,6 +247,8 @@ public class PaymentChannelStateTest extends TestWithWallet {
         assertEquals(PaymentChannelServerState.State.WAITING_FOR_MULTISIG_CONTRACT, serverState.getState());
         // This verifies that the refund can spend the multi-sig output when run.
         clientState.provideRefundSignature(refundSig);
+        assertEquals(PaymentChannelClientState.State.SAVE_STATE_IN_WALLET, clientState.getState());
+        clientState.fakeSave();
         assertEquals(PaymentChannelClientState.State.PROVIDE_MULTISIG_CONTRACT_TO_SERVER, clientState.getState());
 
         // Validate the multisig contract looks right.
@@ -286,7 +290,7 @@ public class PaymentChannelStateTest extends TestWithWallet {
         Utils.rollMockClock(60 * 60 * 2 + 60 * 5);
 
         // Now store the client state in a stored state object which handles the rebroadcasting
-        clientState.storeChannelInWallet(Sha256Hash.create(new byte[]{}));
+        clientState.doStoreChannelInWallet(Sha256Hash.create(new byte[]{}));
         TxFuturePair clientBroadcastedMultiSig = broadcasts.take();
         TxFuturePair broadcastRefund = broadcasts.take();
         assertEquals(clientBroadcastedMultiSig.tx.getHash(), multisigContract.getHash());
@@ -408,6 +412,8 @@ public class PaymentChannelStateTest extends TestWithWallet {
         try { clientState.getCompletedRefundTransaction(); fail(); } catch (IllegalStateException e) {}
         clientState.provideRefundSignature(refundSigCopy);
         try { clientState.provideRefundSignature(refundSigCopy); fail(); } catch (IllegalStateException e) {}
+        assertEquals(PaymentChannelClientState.State.SAVE_STATE_IN_WALLET, clientState.getState());
+        clientState.fakeSave();
         assertEquals(PaymentChannelClientState.State.PROVIDE_MULTISIG_CONTRACT_TO_SERVER, clientState.getState());
 
         try { clientState.incrementPaymentBy(BigInteger.ONE); fail(); } catch (IllegalStateException e) {}
@@ -574,6 +580,8 @@ public class PaymentChannelStateTest extends TestWithWallet {
         assertEquals(PaymentChannelServerState.State.WAITING_FOR_MULTISIG_CONTRACT, serverState.getState());
         // This verifies that the refund can spend the multi-sig output when run.
         clientState.provideRefundSignature(refundSig);
+        assertEquals(PaymentChannelClientState.State.SAVE_STATE_IN_WALLET, clientState.getState());
+        clientState.fakeSave();
         assertEquals(PaymentChannelClientState.State.PROVIDE_MULTISIG_CONTRACT_TO_SERVER, clientState.getState());
 
         // Get the multisig contract
@@ -648,6 +656,8 @@ public class PaymentChannelStateTest extends TestWithWallet {
         assertEquals(PaymentChannelServerState.State.WAITING_FOR_MULTISIG_CONTRACT, serverState.getState());
         // This verifies that the refund can spend the multi-sig output when run.
         clientState.provideRefundSignature(refundSig);
+        assertEquals(PaymentChannelClientState.State.SAVE_STATE_IN_WALLET, clientState.getState());
+        clientState.fakeSave();
         assertEquals(PaymentChannelClientState.State.PROVIDE_MULTISIG_CONTRACT_TO_SERVER, clientState.getState());
 
         // Validate the multisig contract looks right.
@@ -727,6 +737,8 @@ public class PaymentChannelStateTest extends TestWithWallet {
         assertEquals(PaymentChannelServerState.State.WAITING_FOR_MULTISIG_CONTRACT, serverState.getState());
         // This verifies that the refund can spend the multi-sig output when run.
         clientState.provideRefundSignature(refundSig);
+        assertEquals(PaymentChannelClientState.State.SAVE_STATE_IN_WALLET, clientState.getState());
+        clientState.fakeSave();
         assertEquals(PaymentChannelClientState.State.PROVIDE_MULTISIG_CONTRACT_TO_SERVER, clientState.getState());
 
         // Validate the multisig contract looks right.