fix(connext): prevent duplicate htlcAccepted evts (#1854)

* fix(connext): prevent duplicate htlcAccepted evts

This prevents trying to settle an invoice more than once in case Connext
sends us duplicate transfer received events. Previously, ConnextClient
could emit the `htlcAccepted` event multiple times, resulting in one
successful settleInvoice call and at least one failed call, which would
then attempt to fail a completed deal and crash xud.

Closes #1851.

* refactor(swaps): relax faildeal complete check

This relaxes the assertion in `failDeal` to ensure we don't "fail" a
deal that has already been completed. Previously such a case would crash
xud, now it only prints an error message and does nothing. Although this
shouldn't happen and indicates a flaw with xud code should it arise, it
is not a critical error and does not risk funds or indicate that xud is
inoperable.

Author: sangaman

Diff for: lib/connextclient/ConnextClient.ts

@@ -167,7 +167,7 @@ class ConnextClient extends SwapClient {
     }
     const expectedIncomingTransfer = this.expectedIncomingTransfers.get(rHash);
     if (!expectedIncomingTransfer) {
-      this.logger.warn(`received unexpected incoming transfer created event with rHash ${rHash}`);
+      this.logger.warn(`received unexpected incoming transfer created event with rHash ${rHash}, units: ${units}, timelock ${timelock}, token address ${tokenAddress}, and paymentId ${paymentId}`);
       return;
     }
 
@@ -184,7 +184,18 @@ class ConnextClient extends SwapClient {
     ) {
       expectedIncomingTransfer.paymentId = paymentId;
       this.logger.debug(`accepting incoming transfer with rHash: ${rHash}, units: ${units}, timelock ${timelock}, currency ${currency}, and paymentId ${paymentId}`);
+      this.expectedIncomingTransfers.delete(rHash);
       this.emit('htlcAccepted', rHash, units, currency);
+    } else {
+      if (tokenAddress !== expectedTokenAddress) {
+        this.logger.warn(`incoming transfer for rHash ${rHash} with token address ${tokenAddress} does not match expected ${expectedTokenAddress}`);
+      }
+      if (units !== expectedUnits) {
+        this.logger.warn(`incoming transfer for rHash ${rHash} with value ${units} does not match expected ${expectedUnits}`);
+      }
+      if (timelock !== expectedTimelock) {
+        this.logger.warn(`incoming transfer for rHash ${rHash} with time lock ${timelock} does not match expected ${expectedTimelock}`);
+      }
     }
   }
 
@@ -413,7 +424,6 @@ class ConnextClient extends SwapClient {
       assetId,
       preImage: `0x${rPreimage}`,
     });
-    this.expectedIncomingTransfers.delete(rHash);
   }
 
   public removeInvoice = async (rHash: string) => {

Diff for: lib/swaps/Swaps.ts

@@ -726,8 +726,6 @@ class Swaps extends EventEmitter {
       await swapClient.settleInvoice(rHash, rPreimage, currency).catch(this.logger.error);
     } else if (deal.state === SwapState.Active) {
       // we check that the deal is still active before we try to settle the invoice
-      // if the swap has already been failed, then we leave the swap recovery module
-      // to attempt to settle the invoice and claim funds rather than do it here
       try {
         await swapClient.settleInvoice(rHash, rPreimage, currency);
       } catch (err) {
@@ -1226,7 +1224,10 @@ class Swaps extends EventEmitter {
       /** An optional reqId in case the SwapFailedPacket is in response to a swap request. */
       reqId?: string,
     }) => {
-    assert(deal.state !== SwapState.Completed, 'Can not fail a completed deal.');
+    if (deal.state === SwapState.Completed) {
+      this.logger.error(`Can not fail completed deal ${deal.rHash}`);
+      return;
+    }
 
     // If we are already in error state and got another error report we
     // aggregate all error reasons by concatenation

Diff for: test/simulation/custom-xud.patch

@@ -1,5 +1,5 @@
 diff --git a/lib/Xud.ts b/lib/Xud.ts
-index 52ed8b5b4..93c5029b3 100644
+index 52ed8b5b..93c5029b 100644
 --- a/lib/Xud.ts
 +++ b/lib/Xud.ts
 @@ -87,6 +87,11 @@ class Xud extends EventEmitter {
@@ -15,7 +15,7 @@ index 52ed8b5b4..93c5029b3 100644
        if (!this.config.rpc.disable) {
          // start rpc server first, it will respond with UNAVAILABLE error
 diff --git a/lib/swaps/SwapRecovery.ts b/lib/swaps/SwapRecovery.ts
-index 3759f6a35..4089dc944 100644
+index 3759f6a3..4089dc94 100644
 --- a/lib/swaps/SwapRecovery.ts
 +++ b/lib/swaps/SwapRecovery.ts
 @@ -29,7 +29,18 @@ class SwapRecovery extends EventEmitter {
@@ -39,12 +39,12 @@ index 3759f6a35..4089dc944 100644
    }
 
 diff --git a/lib/swaps/Swaps.ts b/lib/swaps/Swaps.ts
-index 3a2367f52..ec1712119 100644
+index 0018b386..388e1638 100644
 --- a/lib/swaps/Swaps.ts
 +++ b/lib/swaps/Swaps.ts
-@@ -729,6 +729,24 @@ class Swaps extends EventEmitter {
-       // if the swap has already been failed, then we leave the swap recovery module
-       // to attempt to settle the invoice and claim funds rather than do it here
+@@ -727,6 +727,24 @@ class Swaps extends EventEmitter {
+     } else if (deal.state === SwapState.Active) {
+       // we check that the deal is still active before we try to settle the invoice
        try {
 +        if (rPreimage === '') {
 +          this.logger.info('NOT SETTLING INVOICE');
@@ -67,7 +67,7 @@ index 3a2367f52..ec1712119 100644
          await swapClient.settleInvoice(rHash, rPreimage, currency);
        } catch (err) {
          this.logger.error(`could not settle invoice for deal ${rHash}`, err);
-@@ -749,7 +767,9 @@ class Swaps extends EventEmitter {
+@@ -747,7 +765,9 @@ class Swaps extends EventEmitter {
                } catch (err) {
                  this.logger.error(`could not settle invoice for deal ${rHash}`, err);
                }
@@ -78,7 +78,7 @@ index 3a2367f52..ec1712119 100644
            });
            await settleRetryPromise;
          } else {
-@@ -773,6 +793,16 @@ class Swaps extends EventEmitter {
+@@ -771,6 +791,16 @@ class Swaps extends EventEmitter {
     * accepted, initiates the swap.
     */
    private handleSwapAccepted = async (responsePacket: packets.SwapAcceptedPacket, peer: Peer) => {
@@ -95,7 +95,7 @@ index 3a2367f52..ec1712119 100644
      assert(responsePacket.body, 'SwapAcceptedPacket does not contain a body');
      const { quantity, rHash, makerCltvDelta } = responsePacket.body;
      const deal = this.getDeal(rHash);
-@@ -860,6 +890,11 @@ class Swaps extends EventEmitter {
+@@ -858,6 +888,11 @@ class Swaps extends EventEmitter {
 
      try {
        await makerSwapClient.sendPayment(deal);
@@ -107,7 +107,7 @@ index 3a2367f52..ec1712119 100644
      } catch (err) {
        // first we must handle the edge case where the maker has paid us but failed to claim our payment
        // in this case, we've already marked the swap as having been paid and completed
-@@ -1041,6 +1076,18 @@ class Swaps extends EventEmitter {
+@@ -1039,6 +1074,18 @@ class Swaps extends EventEmitter {
 
        this.logger.debug('Executing maker code to resolve hash');
 
@@ -126,7 +126,7 @@ index 3a2367f52..ec1712119 100644
        const swapClient = this.swapClientManager.get(deal.takerCurrency)!;
 
        // we update the phase persist the deal to the database before we attempt to send payment
-@@ -1051,6 +1098,13 @@ class Swaps extends EventEmitter {
+@@ -1049,6 +1096,13 @@ class Swaps extends EventEmitter {
        assert(deal.state !== SwapState.Error, `cannot send payment for failed swap ${deal.rHash}`);
 
        try {
@@ -140,7 +140,7 @@ index 3a2367f52..ec1712119 100644
          deal.rPreimage = await swapClient.sendPayment(deal);
        } catch (err) {
          this.logger.debug(`sendPayment in resolveHash for swap ${deal.rHash} failed due to ${err.message}`);
-@@ -1128,10 +1182,21 @@ class Swaps extends EventEmitter {
+@@ -1126,10 +1180,21 @@ class Swaps extends EventEmitter {
          }
        }
 
@@ -163,7 +163,7 @@ index 3a2367f52..ec1712119 100644
        return deal.rPreimage;
      } else {
        // If we are here we are the taker
-@@ -1139,6 +1204,16 @@ class Swaps extends EventEmitter {
+@@ -1137,6 +1202,16 @@ class Swaps extends EventEmitter {
        assert(htlcCurrency === undefined || htlcCurrency === deal.takerCurrency, 'incoming htlc does not match expected deal currency');
        this.logger.debug('Executing taker code to resolve hash');
 
@@ -180,7 +180,7 @@ index 3a2367f52..ec1712119 100644
        return deal.rPreimage;
      }
    }
-@@ -1302,8 +1377,11 @@ class Swaps extends EventEmitter {
+@@ -1305,8 +1380,11 @@ class Swaps extends EventEmitter {
          swapClient.removeInvoice(deal.rHash).catch(this.logger.error); // we don't need to await the remove invoice call
        }
      } else if (deal.phase === SwapPhase.SendingPayment) {
@@ -194,7 +194,7 @@ index 3a2367f52..ec1712119 100644
      }
 
      this.logger.trace(`emitting swap.failed event for ${deal.rHash}`);
-@@ -1367,9 +1445,14 @@ class Swaps extends EventEmitter {
+@@ -1370,9 +1448,14 @@ class Swaps extends EventEmitter {
 
          if (deal.role === SwapRole.Maker) {
            // the maker begins execution of the swap upon accepting the deal