Merge pull request #3191 from LefterisJP/workon_3121

Protect targets_to_identifiers_to_statuses with a lock
raiden-network · Jan 4, 2019 · b5ba8ae · b5ba8ae
2 parents 8f17677 + 46a8692
commit b5ba8ae
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 27 deletions.
diff --git a/docs/changelog.rst b/docs/changelog.rst
@@ -5,6 +5,7 @@ Changelog
 * :feature:`3217` If channel is already updated onchain don't call updateNonClosingBalanceProof.
 * :bug:`3216` If coming online after partner closed channel don't try to send updateNonClosingBalanceProof twice and crash Raiden.
 * :bug:`3211` If using parity and getting the already imported error, attempt to handle it and not crash the client.
+* :bug:`3121` If the same payment identifier is reused avoid a specific race condition that can crash Raiden.
 * :bug:`3201` Workaround for gas price strategy crashing Raiden with an Infura ethereum node.
 
 * :release:`0.100.1 <2018-12-21>`

diff --git a/raiden/raiden_event_handler.py b/raiden/raiden_event_handler.py
@@ -195,11 +195,12 @@ def handle_paymentsentsuccess(
             payment_sent_success_event: EventPaymentSentSuccess,
     ):
         target = payment_sent_success_event.target
-        payment_status = raiden.targets_to_identifiers_to_statuses[target].pop(
-            payment_sent_success_event.identifier,
-            None,
-        )
-        assert payment_status is not None
+        payment_identifier = payment_sent_success_event.identifier
+        payment_status = raiden.targets_to_identifiers_to_statuses[target].pop(payment_identifier)
+
+        # With the introduction of the lock we should always get
+        # here only once per identifier so payment_status should always exist
+        # see: https://github.com/raiden-network/raiden/pull/3191
         payment_status.payment_done.set(True)
 
     def handle_paymentsentfailed(
@@ -208,8 +209,9 @@ def handle_paymentsentfailed(
             payment_sent_failed_event: EventPaymentSentFailed,
     ):
         target = payment_sent_failed_event.target
+        payment_identifier = payment_sent_failed_event.identifier
         payment_status = raiden.targets_to_identifiers_to_statuses[target].pop(
-            payment_sent_failed_event.identifier,
+            payment_identifier,
             None,
         )
         # In the case of a refund transfer the payment fails earlier

diff --git a/raiden/raiden_service.py b/raiden/raiden_service.py
@@ -280,6 +280,7 @@ def __init__(
 
         self.event_poll_lock = gevent.lock.Semaphore()
         self.gas_reserve_lock = gevent.lock.Semaphore()
+        self.payment_identifier_lock = gevent.lock.Semaphore()
 
     def start(self):
         """ Start the node synchronously. Raises directly if anything went wrong on startup """
@@ -592,13 +593,14 @@ def _register_payment_status(
             payment_type: PaymentType,
             balance_proof: BalanceProofUnsignedState,
     ):
-        self.targets_to_identifiers_to_statuses[target][identifier] = PaymentStatus(
-            payment_type=payment_type,
-            payment_identifier=identifier,
-            amount=balance_proof.transferred_amount,
-            token_network_identifier=balance_proof.token_network_identifier,
-            payment_done=AsyncResult(),
-        )
+        with self.payment_identifier_lock:
+            self.targets_to_identifiers_to_statuses[target][identifier] = PaymentStatus(
+                payment_type=payment_type,
+                payment_identifier=identifier,
+                amount=balance_proof.transferred_amount,
+                token_network_identifier=balance_proof.token_network_identifier,
+                payment_done=AsyncResult(),
+            )
 
     def _initialize_transactions_queues(self, chain_state: ChainState):
         pending_transactions = views.get_pending_transactions(chain_state)
@@ -787,23 +789,29 @@ def start_mediated_transfer_with_secret(
         if identifier is None:
             identifier = create_default_identifier()
 
-        payment_status = self.targets_to_identifiers_to_statuses[target].get(identifier)
-        if payment_status:
-            if not payment_status.matches(PaymentType.MEDIATED, token_network_identifier, amount):
-                raise PaymentConflict(
-                    'Another payment with the same id is in flight',
+        with self.payment_identifier_lock:
+            payment_status = self.targets_to_identifiers_to_statuses[target].get(identifier)
+            if payment_status:
+                payment_status_matches = payment_status.matches(
+                    PaymentType.MEDIATED,
+                    token_network_identifier,
+                    amount,
                 )
+                if not payment_status_matches:
+                    raise PaymentConflict(
+                        'Another payment with the same id is in flight',
+                    )
 
-            return payment_status.payment_done
+                return payment_status.payment_done
 
-        payment_status = PaymentStatus(
-            payment_type=PaymentType.MEDIATED,
-            payment_identifier=identifier,
-            amount=amount,
-            token_network_identifier=token_network_identifier,
-            payment_done=AsyncResult(),
-        )
-        self.targets_to_identifiers_to_statuses[target][identifier] = payment_status
+            payment_status = PaymentStatus(
+                payment_type=PaymentType.MEDIATED,
+                payment_identifier=identifier,
+                amount=amount,
+                token_network_identifier=token_network_identifier,
+                payment_done=AsyncResult(),
+            )
+            self.targets_to_identifiers_to_statuses[target][identifier] = payment_status
 
         init_initiator_statechange = initiator_init(
             raiden=self,