From 630b36c5c82dfb466840bfc442cd82ebea8ce890 Mon Sep 17 00:00:00 2001 From: Mike Hearn Date: Thu, 25 Jul 2013 18:11:53 +0200 Subject: [PATCH] Payment channels: tweaks to channel resume behaviour. Don't create a new channel automatically when the client wants to resume but there's already an open connection using that contract. Instead, disconnect the other client. This fixes unintuitive behaviour that could occur if a TCP connection silently died and the server didn't notice. --- .../channels/PaymentChannelClient.java | 2 +- .../channels/PaymentChannelServer.java | 27 ++++---- .../channels/PaymentChannelServerState.java | 8 ++- .../StoredPaymentChannelServerStates.java | 6 +- .../channels/StoredServerChannel.java | 26 ++++++-- .../channels/ChannelConnectionTest.java | 66 ++++--------------- 6 files changed, 56 insertions(+), 79 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 eabd2e97..7da9fb23 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 @@ -180,7 +180,7 @@ public class PaymentChannelClient { @GuardedBy("lock") private void receiveChannelOpen() throws VerificationException { - checkState(step == InitStep.WAITING_FOR_CHANNEL_OPEN || (step == InitStep.WAITING_FOR_INITIATE && storedChannel != null)); + checkState(step == InitStep.WAITING_FOR_CHANNEL_OPEN || (step == InitStep.WAITING_FOR_INITIATE && storedChannel != null), step); log.info("Got CHANNEL_OPEN message, ready to pay"); if (step == InitStep.WAITING_FOR_INITIATE) diff --git a/core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelServer.java b/core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelServer.java index 3ca20eac..13d886ae 100644 --- a/core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelServer.java +++ b/core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelServer.java @@ -156,18 +156,21 @@ public class PaymentChannelServer { if (channels != null) { StoredServerChannel storedServerChannel = channels.getChannel(contractHash); if (storedServerChannel != null) { - if (storedServerChannel.setConnectedHandler(this)) { - log.info("Got resume version message, responding with VERSIONS and CHANNEL_OPEN"); - state = storedServerChannel.getOrCreateState(wallet, broadcaster); - step = InitStep.CHANNEL_OPEN; - conn.sendToClient(Protos.TwoWayChannelMessage.newBuilder() - .setType(Protos.TwoWayChannelMessage.MessageType.CHANNEL_OPEN) - .build()); - conn.channelOpen(contractHash); - return; - } else { - log.error(" ... but that contract is already in use."); + final PaymentChannelServer existingHandler = storedServerChannel.setConnectedHandler(this, false); + if (existingHandler != this) { + log.warn(" ... and that channel is already in use, disconnecting other user."); + existingHandler.close(); + storedServerChannel.setConnectedHandler(this, true); } + + log.info("Got resume version message, responding with VERSIONS and CHANNEL_OPEN"); + state = storedServerChannel.getOrCreateState(wallet, broadcaster); + step = InitStep.CHANNEL_OPEN; + conn.sendToClient(Protos.TwoWayChannelMessage.newBuilder() + .setType(Protos.TwoWayChannelMessage.MessageType.CHANNEL_OPEN) + .build()); + conn.channelOpen(contractHash); + return; } else { log.error(" ... but we do not have any record of that contract! Resume failed."); } @@ -367,7 +370,7 @@ public class PaymentChannelServer { if (channels != null) { StoredServerChannel storedServerChannel = channels.getChannel(state.getMultisigContract().getHash()); if (storedServerChannel != null) { - storedServerChannel.setConnectedHandler(null); + storedServerChannel.clearConnectedHandler(); } } } diff --git a/core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelServerState.java b/core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelServerState.java index 024660d5..776717d8 100644 --- a/core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelServerState.java +++ b/core/src/main/java/com/google/bitcoin/protocols/channels/PaymentChannelServerState.java @@ -475,9 +475,10 @@ public class PaymentChannelServerState { * a call to {@link PaymentChannelServerState#close()} completes successfully. A channel may only be stored after it * has fully opened (ie state == State.READY). * - * @param connectedHandler The {@link PaymentChannelClientState} object which is managing this object. This will + * @param connectedHandler Optional {@link PaymentChannelServer} object that manages this object. This will * set the appropriate pointer in the newly created {@link StoredServerChannel} before it is - * committed to wallet. + * committed to wallet. If set, closing the state object will propagate the close to the + * handler which can then do a TCP disconnect. */ public synchronized void storeChannelInWallet(@Nullable PaymentChannelServer connectedHandler) { checkState(state == State.READY); @@ -488,7 +489,8 @@ public class PaymentChannelServerState { StoredPaymentChannelServerStates channels = (StoredPaymentChannelServerStates) wallet.addOrGetExistingExtension(new StoredPaymentChannelServerStates(wallet, broadcaster)); storedServerChannel = new StoredServerChannel(this, multisigContract, clientOutput, refundTransactionUnlockTimeSecs, serverKey, bestValueToMe, bestValueSignature); - checkState(storedServerChannel.setConnectedHandler(connectedHandler)); + if (connectedHandler != null) + checkState(storedServerChannel.setConnectedHandler(connectedHandler, false) == connectedHandler); channels.putChannel(storedServerChannel); wallet.addOrUpdateExtension(channels); } diff --git a/core/src/main/java/com/google/bitcoin/protocols/channels/StoredPaymentChannelServerStates.java b/core/src/main/java/com/google/bitcoin/protocols/channels/StoredPaymentChannelServerStates.java index cd0288e0..1184dfd6 100644 --- a/core/src/main/java/com/google/bitcoin/protocols/channels/StoredPaymentChannelServerStates.java +++ b/core/src/main/java/com/google/bitcoin/protocols/channels/StoredPaymentChannelServerStates.java @@ -78,9 +78,9 @@ public class StoredPaymentChannelServerStates implements WalletExtension { lock.unlock(); } synchronized (channel) { - if (channel.connectedHandler != null) // connectedHandler will be reset to null in connectionClosed - channel.connectedHandler.close(); // Closes the actual connection, not the channel - try {//TODO add event listener to PaymentChannelServerStateManager + channel.closeConnectedHandler(); + try { + //TODO add event listener to PaymentChannelServerStateManager channel.getOrCreateState(wallet, broadcaster).close(); } catch (ValueOutOfRangeException e) { e.printStackTrace(); diff --git a/core/src/main/java/com/google/bitcoin/protocols/channels/StoredServerChannel.java b/core/src/main/java/com/google/bitcoin/protocols/channels/StoredServerChannel.java index 12f0c224..e6717636 100644 --- a/core/src/main/java/com/google/bitcoin/protocols/channels/StoredServerChannel.java +++ b/core/src/main/java/com/google/bitcoin/protocols/channels/StoredServerChannel.java @@ -38,7 +38,7 @@ public class StoredServerChannel { // In-memory pointer to the event handler which handles this channel if the client is connected. // Used as a flag to prevent duplicate connections and to disconnect the channel if its expire time approaches. - PaymentChannelServer connectedHandler = null; + private PaymentChannelServer connectedHandler = null; PaymentChannelServerState state = null; StoredServerChannel(PaymentChannelServerState state, Transaction contract, TransactionOutput clientOutput, @@ -63,13 +63,27 @@ public class StoredServerChannel { /** * Attempts to connect the given handler to this, returning true if it is the new handler, false if there was - * already one attached. A null connectedHandler clears this's connected handler no matter its current state. + * already one attached. */ - synchronized boolean setConnectedHandler(PaymentChannelServer connectedHandler) { - if (this.connectedHandler != null && connectedHandler != null) - return false; + synchronized PaymentChannelServer setConnectedHandler(PaymentChannelServer connectedHandler, boolean override) { + if (this.connectedHandler != null && !override) + return this.connectedHandler; this.connectedHandler = connectedHandler; - return true; + return connectedHandler; + } + + /** Clears a handler that was connected with setConnectedHandler. */ + synchronized void clearConnectedHandler() { + this.connectedHandler = null; + } + + /** + * If a handler is connected, call its {@link com.google.bitcoin.protocols.channels.PaymentChannelServer#close()} + * method thus disconnecting the TCP connection. + */ + synchronized void closeConnectedHandler() { + if (connectedHandler != null) + connectedHandler.close(); } /** diff --git a/core/src/test/java/com/google/bitcoin/protocols/channels/ChannelConnectionTest.java b/core/src/test/java/com/google/bitcoin/protocols/channels/ChannelConnectionTest.java index 25c05407..10a1dcc2 100644 --- a/core/src/test/java/com/google/bitcoin/protocols/channels/ChannelConnectionTest.java +++ b/core/src/test/java/com/google/bitcoin/protocols/channels/ChannelConnectionTest.java @@ -325,9 +325,7 @@ public class ChannelConnectionTest extends TestWithWallet { PaymentChannelClient openClient = client; ChannelTestUtils.RecordingPair openPair = pair; - // Now open up a new client with the same id and make sure it doesn't attempt to reopen the channel. - // If a client connects to a server with the same channel ID as one that's currently in use, the - // server responds by opening up a new channel instead of letting two client connections conflict. + // Now open up a new client with the same id and make sure the server disconnects the previous client. pair = ChannelTestUtils.makeRecorders(serverWallet, mockBroadcaster); client = new PaymentChannelClient(wallet, myKey, Utils.COIN, someServerId, pair.clientRecorder); server = pair.server; @@ -339,14 +337,13 @@ public class ChannelConnectionTest extends TestWithWallet { Protos.TwoWayChannelMessage msg = pair.clientRecorder.checkNextMsg(MessageType.CLIENT_VERSION); assertFalse(msg.getClientVersion().hasPreviousChannelContractHash()); } - - // Make sure the server won't allow two simultaneous opens either. It will try to reinitiate instead. + // Make sure the server allows two simultaneous opens. It will close the first and allow resumption of the second. pair = ChannelTestUtils.makeRecorders(serverWallet, mockBroadcaster); client = new PaymentChannelClient(wallet, myKey, Utils.COIN, someServerId, pair.clientRecorder); server = pair.server; client.connectionOpen(); server.connectionOpen(); - // Swap out the clients version message for a custom one that incorrectly tries to resume ... + // Swap out the clients version message for a custom one that tries to resume ... pair.clientRecorder.getNextMsg(); server.receiveMessage(Protos.TwoWayChannelMessage.newBuilder() .setType(MessageType.CLIENT_VERSION) @@ -354,48 +351,12 @@ public class ChannelConnectionTest extends TestWithWallet { .setPreviousChannelContractHash(ByteString.copyFrom(contractHash.getBytes())) .setMajor(0).setMinor(42)) .build()); - // We get the usual setup sequence. - client.receiveMessage(pair.serverRecorder.checkNextMsg(MessageType.SERVER_VERSION)); - client.receiveMessage(pair.serverRecorder.checkNextMsg(MessageType.INITIATE)); - server.receiveMessage(pair.clientRecorder.checkNextMsg(MessageType.PROVIDE_REFUND)); - client.receiveMessage(pair.serverRecorder.checkNextMsg(MessageType.RETURN_REFUND)); - broadcastTxPause.release(); - server.receiveMessage(pair.clientRecorder.checkNextMsg(MessageType.PROVIDE_CONTRACT)); - broadcasts.take(); - client.receiveMessage(pair.serverRecorder.checkNextMsg(MessageType.CHANNEL_OPEN)); - Sha256Hash secondContractHash = (Sha256Hash) pair.serverRecorder.q.take(); - pair.clientRecorder.checkOpened(); - assertNull(pair.serverRecorder.q.poll()); - assertNull(pair.clientRecorder.q.poll()); - client.close(); - client.connectionClosed(); - pair.server.close(); - pair.server.connectionClosed(); + // We get the usual resume sequence. + pair.serverRecorder.checkNextMsg(MessageType.SERVER_VERSION); + pair.serverRecorder.checkNextMsg(MessageType.CHANNEL_OPEN); + // Verify the previous one was closed. + openPair.serverRecorder.checkNextMsg(MessageType.CLOSE); - // Now open again with the same id and make sure it reopens the second (because the 1st is still open). - pair = ChannelTestUtils.makeRecorders(serverWallet, mockBroadcaster); - client = new PaymentChannelClient(wallet, myKey, Utils.COIN, someServerId, pair.clientRecorder); - server = pair.server; - client.connectionOpen(); - server.connectionOpen(); - { - Protos.TwoWayChannelMessage msg = pair.clientRecorder.checkNextMsg(MessageType.CLIENT_VERSION); - assertEquals(secondContractHash, new Sha256Hash(msg.getClientVersion().getPreviousChannelContractHash().toByteArray())); - server.receiveMessage(msg); - } - client.receiveMessage(pair.serverRecorder.checkNextMsg(MessageType.SERVER_VERSION)); - client.receiveMessage(pair.serverRecorder.checkNextMsg(MessageType.CHANNEL_OPEN)); - pair.clientRecorder.checkOpened(); - // Close it. - assertEquals(2, clientStoredChannels.mapChannels.size()); - broadcastTxPause.release(); - client.close(); - server.receiveMessage(pair.clientRecorder.checkNextMsg(MessageType.CLOSE)); - assertEquals(CloseReason.CLIENT_REQUESTED_CLOSE, pair.clientRecorder.q.take()); - server.connectionClosed(); - client.connectionClosed(); - - assertFalse(clientStoredChannels.getChannel(Sha256Hash.create(new byte[]{}), secondContractHash).active); assertTrue(clientStoredChannels.getChannel(Sha256Hash.create(new byte[]{}), contractHash).active); // And finally close the first channel too. @@ -406,13 +367,10 @@ public class ChannelConnectionTest extends TestWithWallet { Utils.rollMockClock(60 * 60 * 24 + 60*5); // Client announces refund 5 minutes after expire time StoredPaymentChannelClientStates newClientStates = new StoredPaymentChannelClientStates(wallet, mockBroadcaster); newClientStates.deserializeWalletExtension(wallet, clientStoredChannels.serializeWalletExtension()); - // Expect two pairs of contract/refund ... - for (int i = 0; i < 2; i++) { - broadcastTxPause.release(); - assertTrue(broadcasts.take().getOutput(0).getScriptPubKey().isSentToMultiSig()); - broadcastTxPause.release(); - assertEquals(TransactionConfidence.Source.SELF, broadcasts.take().getConfidence().getSource()); - } + broadcastTxPause.release(); + assertTrue(broadcasts.take().getOutput(0).getScriptPubKey().isSentToMultiSig()); + broadcastTxPause.release(); + assertEquals(TransactionConfidence.Source.SELF, broadcasts.take().getConfidence().getSource()); assertTrue(broadcasts.isEmpty()); assertTrue(newClientStates.mapChannels.isEmpty()); // Server also knows it's too late.