From 1d7f2eb00bd6116adb550efbf0ec892cc0e2cad8 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 16 Jul 2013 14:56:25 +0200 Subject: [PATCH] Fix channel client state saving to save earlier (and be secure) --- .../channels/PaymentChannelClient.java | 6 ++- .../channels/PaymentChannelClientState.java | 47 ++++++++++++++----- .../channels/PaymentChannelStateTest.java | 14 +++++- 3 files changed, 52 insertions(+), 15 deletions(-) diff --git a/core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelClient.java b/core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelClient.java index f7da556e..a76a53f2 100644 --- a/core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelClient.java +++ b/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 diff --git a/core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelClientState.java b/core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelClientState.java index 67c0122a..636d9c2e 100644 --- a/core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelClientState.java +++ b/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 - * and can provide the server with the multi-sig contract (via {@link PaymentChannelClientState#getMultisigContract()}) - * safely. + * You must then call {@link PaymentChannelClientState#storeChannelInWallet(Sha256Hash)} to store the refund transaction + * in the wallet, protecting you against a malicious server attempting to destroy all your coins. At this point, you can + * provide the server with the multi-sig contract (via {@link PaymentChannelClientState#getMultisigContract()}) safely. *

*/ 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.PROVIDE_MULTISIG_CONTRACT_TO_SERVER; + state = State.SAVE_STATE_IN_WALLET; } 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); + } + /** *

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) - 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); + try { + wallet.commitTx(multisigContract); + } catch (VerificationException e) { + throw new RuntimeException(e); // We created it + } + state = State.PROVIDE_MULTISIG_CONTRACT_TO_SERVER; } /** diff --git a/core/src/test/java/com/google/bitcoin/protocols/channels/PaymentChannelStateTest.java b/core/src/test/java/com/google/bitcoin/protocols/channels/PaymentChannelStateTest.java index f98679da..11a6e906 100644 --- a/core/src/test/java/com/google/bitcoin/protocols/channels/PaymentChannelStateTest.java +++ b/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.