bugfix: lock expiration must be considered

A lock expired message must be ignored if the current block number is
smaller than the lock's expiration. Additionally, the receiver must wait
until the block in which the lock expired is confirmed, this is necesary
to handle the corner case where:

- The secret registration ocurred near the lock expiration
- A reorg happened
- The user's node was following the blocks which didn't have he secret
  registered in time

fix raiden-network#2720

docs/changelog.rst

@@ -4,6 +4,7 @@ Changelog
 
 * :feature:`2713` Added the protocol version in the Ping message.
 * :feature:`2708` Add `--showconfig` CLI flag which dumps all configuration values that will control Raiden behavior.
+* :bug:`2720` A lock expired message must be considered invalid if the block in which the lock expired has not been confirmed.
 
 * :release:`0.12.0 <2018-10-05>`
 * :feature:`2699` Add ``/channels/<token_address>`` REST-API endpoint to query all node's channels for a specific token.

raiden/tests/integration/test_integration_events.py

@@ -490,7 +490,7 @@ def test_secret_revealed(raiden_chain, deposit, settle_timeout, token_addresses)
     secret = sha3(target)
     secrethash = sha3(secret)
 
-    hold_event_handler.hold_secret_for(secret)
+    hold_event_handler.hold_secretrequest_for(secret)
 
     app0.raiden.start_mediated_transfer_with_secret(
         token_network_identifier,

raiden/tests/integration/test_settlement.py

@@ -15,7 +15,6 @@
 from raiden.tests.utils.protocol import HoldOffChainSecretRequest
 from raiden.tests.utils.transfer import (
     assert_synced_channel_state,
-    claim_lock,
     direct_transfer,
     get_channelstate,
 )
@@ -160,7 +159,7 @@ def test_lock_expiry(raiden_network, token_addresses, deposit):
     transfer_1_secret = sha3(target + b'1')
     transfer_1_secrethash = sha3(transfer_1_secret)
 
-    hold_event_handler.hold_secret_for(secrethash=transfer_1_secrethash)
+    hold_event_handler.hold_secretrequest_for(secrethash=transfer_1_secrethash)
 
     alice_app.raiden.start_mediated_transfer_with_secret(
         token_network_identifier,
@@ -221,7 +220,7 @@ def test_lock_expiry(raiden_network, token_addresses, deposit):
     transfer_2_secret = sha3(target + b'2')
     transfer_2_secrethash = sha3(transfer_2_secret)
 
-    hold_event_handler.hold_secret_for(secrethash=transfer_2_secrethash)
+    hold_event_handler.hold_secretrequest_for(secrethash=transfer_2_secrethash)
 
     alice_app.raiden.start_mediated_transfer_with_secret(
         token_network_identifier,
@@ -283,7 +282,7 @@ def test_batch_unlock(raiden_network, token_addresses, secret_registry_address,
     secret = sha3(target)
     secrethash = sha3(secret)
 
-    hold_event_handler.hold_secret_for(secrethash=secrethash)
+    hold_event_handler.hold_secretrequest_for(secrethash=secrethash)
 
     alice_app.raiden.start_mediated_transfer_with_secret(
         token_network_identifier,
@@ -419,7 +418,7 @@ def test_settled_lock(token_addresses, raiden_network, deposit):
     secret = sha3(target)
     secrethash = sha3(secret)
 
-    hold_event_handler.hold_secret_for(secrethash=secrethash)
+    secret_available = hold_event_handler.hold_secretrequest_for(secrethash=secrethash)
 
     app0.raiden.start_mediated_transfer_with_secret(
         token_network_identifier,
@@ -429,17 +428,26 @@ def test_settled_lock(token_addresses, raiden_network, deposit):
         secret,
     )
 
-    gevent.sleep(1)  # wait for the messages to be exchanged
+    secret_available.wait()  # wait for the messages to be exchanged
 
     # Save the merkle tree leaves from the pending transfer, used to test the unlock
     channelstate_0_1 = get_channelstate(app0, app1, token_network_identifier)
     batch_unlock = channel.get_batch_unlock(channelstate_0_1.our_state)
     assert batch_unlock
 
-    claim_lock(raiden_network, identifier, token_network_identifier, secret)
+    hold_event_handler.release_secretrequest_for(
+        app1.raiden,
+        secrethash,
+    )
+
+    direct_transfer(
+        app0,
+        app1,
+        token_network_identifier,
+        amount,
+        identifier=2,
+    )
 
-    # Make a new transfer
-    direct_transfer(app0, app1, token_network_identifier, amount, identifier=2)
     RaidenAPI(app1.raiden).channel_close(
         registry_address,
         token_address,
@@ -495,7 +503,7 @@ def test_automatic_secret_registration(raiden_chain, token_addresses):
     secret = sha3(target)
     secrethash = sha3(secret)
 
-    hold_event_handler.hold_secret_for(secrethash=secrethash)
+    hold_event_handler.hold_secretrequest_for(secrethash=secrethash)
 
     app0.raiden.start_mediated_transfer_with_secret(
         token_network_identifier,
@@ -560,7 +568,7 @@ def test_start_end_attack(token_addresses, raiden_chain, deposit):
     secret = sha3(target)
     secrethash = sha3(secret)
 
-    hold_event_handler.hold_secret_for(secrethash=secrethash)
+    hold_event_handler.hold_secretrequest_for(secrethash=secrethash)
 
     app0.raiden.start_mediated_transfer_with_secret(
         token_network_identifier,

raiden/tests/unit/test_channelstate.py

@@ -1236,17 +1236,19 @@ def test_channel_must_never_expire_locks_with_onchain_secret():
     )
 
     is_valid, _, _ = channel.is_valid_lock_expired(
-        lock_expired,
-        channel_state,
-        channel_state.partner_state,
-        channel_state.our_state,
+        state_change=lock_expired,
+        channel_state=channel_state,
+        sender_state=channel_state.partner_state,
+        receiver_state=channel_state.our_state,
+        block_number=block_number,
     )
 
     assert not is_valid
 
     channel.handle_receive_lock_expired(
-        channel_state,
-        lock_expired,
+        channel_state=channel_state,
+        state_change=lock_expired,
+        block_number=block_number,
     )
 
     assert lock.secrethash in channel_state.partner_state.secrethashes_to_lockedlocks

raiden/tests/unit/transfer/mediated_transfer/test_mediatorstate.py

@@ -1874,14 +1874,24 @@ def test_mediator_lock_expired_with_receive_lock_expired():
         1,
     )
 
+    block_before_confirmed_expiration = expiration + DEFAULT_NUMBER_OF_CONFIRMATIONS_BLOCK - 1
     iteration = mediator.state_transition(
         iteration.new_state,
         lock_expired_state_change,
         channel_map,
         pseudo_random_generator,
-        10,
+        block_before_confirmed_expiration,
     )
+    assert not must_contain_entry(iteration.events, SendProcessed, {})
 
+    block_lock_expired = block_before_confirmed_expiration + 1
+    iteration = mediator.state_transition(
+        iteration.new_state,
+        lock_expired_state_change,
+        channel_map,
+        pseudo_random_generator,
+        block_lock_expired,
+    )
     assert must_contain_entry(iteration.events, SendProcessed, {})
 
     assert iteration.new_state

raiden/tests/unit/transfer/mediated_transfer/test_targetstate.py

@@ -4,6 +4,7 @@
 import pytest
 
 from raiden.constants import UINT64_MAX
+from raiden.settings import DEFAULT_NUMBER_OF_CONFIRMATIONS_BLOCK
 from raiden.tests.utils import factories
 from raiden.tests.utils.events import must_contain_entry
 from raiden.tests.utils.factories import (
@@ -456,12 +457,12 @@ def test_target_receive_lock_expired():
     expiration = block_number + from_channel.settle_timeout - from_channel.reveal_timeout
 
     from_transfer = factories.make_signed_transfer_for(
-        from_channel,
-        lock_amount,
-        initiator,
-        our_address,
-        expiration,
-        UNIT_SECRET,
+        channel_state=from_channel,
+        amount=lock_amount,
+        initiator=initiator,
+        target=our_address,
+        expiration=expiration,
+        secret=UNIT_SECRET,
     )
 
     init = ActionInitTarget(
@@ -499,16 +500,86 @@ def test_target_receive_lock_expired():
         1,
     )
 
+    block_before_confirmed_expiration = expiration + DEFAULT_NUMBER_OF_CONFIRMATIONS_BLOCK - 1
     iteration = target.state_transition(
         init_transition.new_state,
         lock_expired_state_change,
         from_channel,
         pseudo_random_generator,
-        10,
+        block_before_confirmed_expiration,
     )
+    assert not must_contain_entry(iteration.events, SendProcessed, {})
 
+    block_lock_expired = block_before_confirmed_expiration + 1
+    iteration = target.state_transition(
+        init_transition.new_state,
+        lock_expired_state_change,
+        from_channel,
+        pseudo_random_generator,
+        block_lock_expired,
+    )
     assert must_contain_entry(iteration.events, SendProcessed, {})
-    assert iteration.new_state is None
+
+
+def test_target_lock_is_expired_if_secret_is_not_registered_onchain():
+    lock_amount = 7
+    block_number = 1
+    initiator = factories.HOP6
+    pseudo_random_generator = random.Random()
+
+    our_balance = 100
+    our_address = factories.make_address()
+    partner_balance = 130
+
+    from_channel = factories.make_channel(
+        our_address=our_address,
+        our_balance=our_balance,
+        partner_address=UNIT_TRANSFER_SENDER,
+        partner_balance=partner_balance,
+    )
+    from_route = factories.route_from_channel(from_channel)
+    expiration = block_number + from_channel.settle_timeout - from_channel.reveal_timeout
+
+    from_transfer = factories.make_signed_transfer_for(
+        from_channel,
+        lock_amount,
+        initiator,
+        our_address,
+        expiration,
+        UNIT_SECRET,
+    )
+
+    init = ActionInitTarget(
+        from_route,
+        from_transfer,
+    )
+
+    init_transition = target.state_transition(
+        None,
+        init,
+        from_channel,
+        pseudo_random_generator,
+        block_number,
+    )
+    assert init_transition.new_state is not None
+
+    secret_reveal_iteration = target.state_transition(
+        target_state=init_transition.new_state,
+        state_change=ReceiveSecretReveal(UNIT_SECRET, from_channel.partner_state.address),
+        channel_state=from_channel,
+        pseudo_random_generator=pseudo_random_generator,
+        block_number=block_number,
+    )
+
+    expired_block_number = from_transfer.lock.expiration + DEFAULT_NUMBER_OF_CONFIRMATIONS_BLOCK
+    iteration = target.state_transition(
+        target_state=secret_reveal_iteration.new_state,
+        state_change=Block(expired_block_number, None, None),
+        channel_state=from_channel,
+        pseudo_random_generator=pseudo_random_generator,
+        block_number=expired_block_number,
+    )
+    assert must_contain_entry(iteration.events, EventUnlockClaimFailed, {})
 
 
 @pytest.mark.xfail(reason='Not implemented #522')

raiden/tests/utils/protocol.py

@@ -1,11 +1,18 @@
-import logging
+import structlog
+from gevent.event import Event
 
 from raiden.raiden_event_handler import RaidenEventHandler
 from raiden.raiden_service import RaidenService
 from raiden.transfer.mediated_transfer.events import SendSecretRequest
-from raiden.utils import pex
+from raiden.utils import pex, typing
 
-log = logging.getLogger(__name__)
+log = structlog.get_logger(__name__)
+
+
+class SecretRequestState(typing.NamedTuple):
+    secrethash: bytes
+    secret_request_available: Event
+    secret_request_event: SendSecretRequest
 
 
 class HoldOffChainSecretRequest(RaidenEventHandler):
@@ -17,18 +24,48 @@ class HoldOffChainSecretRequest(RaidenEventHandler):
     """
 
     def __init__(self):
-        self.secrethashes_to_hold = list()
+        self.secrethashes_to_hold = dict()
 
-    def hold_secret_for(self, secrethash):
+    def hold_secretrequest_for(self, secrethash: typing.SecretHash):
         if secrethash not in self.secrethashes_to_hold:
-            self.secrethashes_to_hold.append(secrethash)
+            waiting_event = Event()
+            self.secrethashes_to_hold[secrethash] = SecretRequestState(
+                secrethash=secrethash,
+                secret_request_available=waiting_event,
+                secret_request_event=None,
+            )
+        else:
+            waiting_event = self.secrethashes_to_hold[secrethash].secret_request_available
+
+        return waiting_event
+
+    def release_secretrequest_for(self, raiden: RaidenService, secrethash: typing.SecretHash):
+        hold_state = self.secrethashes_to_hold.get(secrethash)
+
+        if hold_state and hold_state.secret_request_available.is_set():
+            secret_request_event = hold_state.secret_request_event
+            assert secret_request_event
+            del self.secrethashes_to_hold[secrethash]
+
+            super().handle_send_secretrequest(raiden, secret_request_event)
+            log.info(
+                f'SecretRequest for {pex(secret_request_event.secrethash)} released.',
+                node=pex(raiden.address),
+            )
 
     def handle_send_secretrequest(
             self,
             raiden: RaidenService,
             secret_request_event: SendSecretRequest,
     ):
-        if secret_request_event.secrethash not in self.secrethashes_to_hold:
+        hold_state = self.secrethashes_to_hold.get(secret_request_event.secrethash)
+        if hold_state is None:
             super().handle_send_secretrequest(raiden, secret_request_event)
         else:
-            log.info(f'SecretRequest for {pex(secret_request_event.secrethash)} held.')
+            new_hold_state = hold_state._replace(secret_request_event=secret_request_event)
+            new_hold_state.secret_request_available.set()
+            self.secrethashes_to_hold[secret_request_event.secrethash] = new_hold_state
+            log.info(
+                f'SecretRequest for {pex(secret_request_event.secrethash)} held.',
+                node=pex(raiden.address),
+            )