Prepare publishing of blinded balance proofs #3452

konradkonrad · 2019-02-11T12:03:06Z

This is progress towards #3273

The PR adds the necessary pieces to publish blinded balance proofs to the global matrix room monitoring.

In particular:

add a new event EventNewBalanceProofReceived
add a state diffing mechanism, that detects new balance proofs from the global StateManager.dispatch method, and injects the new events
add transport.send_global calls in the RaidenMonitoringEventHandler
add a matrix integration test for the global messages to be sent
add EventNewBalanceProofReceived to test_mediated_transfer_events happy case

It also changes the matrix transport behavior for send_global: previously send_global would raise AssertionError, if a room target was not set up beforehand. Now the target room will be joined automatically. This is meant to avoid the overhead of checking for the room existence in sending code, or catching the Assertion. If this last change is contentious, we could add a non-asserting is_global_room_available(room_name) method instead.

konradkonrad · 2019-02-11T12:48:25Z

This is still missing recovery at startup -- see #3454

konradkonrad · 2019-02-11T12:48:30Z

One more comment: ideally publishing to the monitoring service would block further processing of the channel until the message was sent(received). The idea behind that is, that monitoring is needed for safety, and therefore the channel's protocol should not proceed until monitoring was successfully requested (note: current MS design does not give any feedback on a request).

ulope · 2019-02-11T12:58:34Z

The question is what form of feedback would make sense since there should ideally be many monitoring services watching the broadcast channel.

codecov · 2019-02-11T13:28:15Z

Codecov Report

Merging #3452 into master will increase coverage by 0.14%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #3452      +/-   ##
==========================================
+ Coverage    76.3%   76.44%   +0.14%     
==========================================
  Files         102      102              
  Lines       13207    13257      +50     
  Branches     1855     1865      +10     
==========================================
+ Hits        10077    10134      +57     
+ Misses       2471     2465       -6     
+ Partials      659      658       -1

Impacted Files	Coverage Δ
raiden/network/transport/matrix/transport.py	`74.23% <0%> (-0.85%)`	⬇️
raiden/transfer/architecture.py	`86.15% <100%> (+6.84%)`	⬆️
raiden/raiden_event_handler.py	`89.44% <90%> (+1.75%)`	⬆️
raiden/transfer/events.py	`74% <0%> (-0.41%)`	⬇️
raiden/transfer/state.py	`69.11% <0%> (+0.15%)`	⬆️
raiden/network/transport/matrix/client.py	`69.84% <0%> (+0.38%)`	⬆️
raiden/network/transport/udp/udp_transport.py	`90.76% <0%> (+0.8%)`	⬆️
raiden/network/proxies/token_network.py	`81.39% <0%> (+1.47%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71957e1...73358b6. Read the comment docs.

codecov · 2019-02-11T13:28:16Z

Codecov Report

Merging #3452 into master will increase coverage by 0.04%.
The diff coverage is 87.71%.

@@            Coverage Diff             @@
##           master    #3452      +/-   ##
==========================================
+ Coverage   76.49%   76.53%   +0.04%     
==========================================
  Files         103      103              
  Lines       13318    13366      +48     
  Branches     1881     1891      +10     
==========================================
+ Hits        10187    10230      +43     
- Misses       2472     2474       +2     
- Partials      659      662       +3

Impacted Files	Coverage Δ
raiden/utils/typing.py	`100% <ø> (ø)`	⬆️
raiden/network/transport/matrix/transport.py	`73.95% <0%> (-0.52%)`	⬇️
raiden/raiden_service.py	`84.11% <100%> (+0.18%)`	⬆️
raiden/messages.py	`88.99% <100%> (ø)`	⬆️
raiden/raiden_event_handler.py	`89.44% <88.88%> (+1.75%)`	⬆️
raiden/transfer/mediated_transfer/events.py	`83.33% <88.88%> (-0.78%)`	⬇️
raiden/transfer/views.py	`88.03% <93.33%> (+0.81%)`	⬆️
raiden/utils/__init__.py	`67.44% <0%> (-4.66%)`	⬇️
raiden/network/proxies/secret_registry.py	`82.71% <0%> (-1.24%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4043886...47ab2d1. Read the comment docs.

konradkonrad · 2019-02-11T15:01:47Z

The question is what form of feedback would make sense since there should ideally be many monitoring services watching the broadcast channel.

This PR is probably the wrong place to discuss this.

Nevertheless: In a long term view, I could see MS's publishing some batched "acknowledged balance_hashes" to the same room every once in a while (tbd.). Since MS's will compete in QoS, clients would whitelist some MS addresses and wait for N ack's before proceeding.

raiden/raiden_event_handler.py

raiden/transfer/architecture.py

hackaugusto · 2019-02-12T13:21:12Z

raiden/transfer/architecture.py

+        # Assuming, there is no transition None->new balance proof:
+        if not hasattr(old_state, 'identifiers_to_paymentnetworks'):
+            return
+        for old_payment_network_identifier, current_payment_network_identifier in zip(


I don't think this works as you expect, identifiers_to_paymentnetworks are dictionaries, and iterating over them returns the dict keys in order of insertion (python 3.6+):

>>> d = {1:1, 2:2} >>> d2 = {2:2, 1:1} >>> list(zip(d, d2)) [(1, 2), (2, 1)]

Which for all purposes is undefined at this point.

Since this is getting the new balance proofs, it just needs to get the keys from current_state.identifiers_to_paymentnetworks, if there is no corresponding key in old_state.identifiers_to_paymentnetworks then we know all BPs are new.

hackaugusto · 2019-02-12T13:42:50Z

raiden/transfer/architecture.py

+        if old_state == current_state:
+            return
+        # Assuming, there is no transition None->new balance proof:
+        if not hasattr(old_state, 'identifiers_to_paymentnetworks'):


Checking if the attribute exists is not sufficient, you should check if it's not None, getattr(.., .., None) is probably best here.

hackaugusto · 2019-02-12T13:46:07Z

raiden/transfer/architecture.py

@@ -272,6 +295,66 @@ def dispatch(self, state_change: StateChange) -> List[Event]:

        return events

+    def detect_balance_proof_change(self, old_state, current_state):


please, move this to the views module and add types

hackaugusto · 2019-02-12T13:47:48Z

raiden/transfer/architecture.py

+                self.current_state,
+                iteration.new_state,
+        ):
+            iteration.events.append(EventNewBalanceProofReceived(balance_proof))


This is no place for this logic, the state manager does all it needs to do already. You can just call detect_balance_proof_change after a state change is processed.

rakanalh

The PR looks good to me (only made minor comments). But mostly, it's lacking a test which makes sure that the chain-state diff works. Could you please add a test for this?

rakanalh · 2019-02-13T15:05:33Z

raiden/transfer/mediated_transfer/events.py

+    def __init__(self, balance_proof):
+        self.balance_proof = balance_proof
+
+    def to_dict(self):


Please add return type.

rakanalh · 2019-02-13T15:05:51Z

raiden/transfer/mediated_transfer/events.py

+        }
+
+    @classmethod
+    def from_dict(cls, data):


data param type and return type.

rakanalh · 2019-02-13T17:32:56Z

raiden/utils/typing.py

+T_EventNewBalanceProofReceived = TypeVar(
+    'T_EventNewBalanceProofReceived',
+    bound='raiden.transfer.mediated_transfer.events.EventNewBalanceProofReceived',
+)


Please remove

please follow the conversation at #3374 (comment) and 69831a8

rakanalh · 2019-02-13T17:33:47Z

raiden/transfer/mediated_transfer/events.py

+    def from_dict(
+            cls: Type[T_EventNewBalanceProofReceived],
+            data: Dict,
+    ) -> T_EventNewBalanceProofReceived:


I am not sure why you had to declare a type for this. The type would be EventNewBalanceProofReceived, no need to declare yet another one in typing.

please follow the conversation at #3374 (comment) and 69831a8
tldr; class is not available inside class definition

None of our other events / state changes / states declare this. Otherwise we would have lots of types. Please follow the same convention used in other events in the same file.

rakanalh · 2019-02-13T17:34:13Z

raiden/transfer/mediated_transfer/events.py

+    def __init__(self, balance_proof: BalanceProofSignedState) -> None:
+        self.balance_proof = balance_proof
+
+    def to_dict(self) -> Dict:


Dict[str, Any]

rakanalh · 2019-02-13T17:34:27Z

raiden/transfer/mediated_transfer/events.py

+class EventNewBalanceProofReceived(Event):
+    """ Event for newly received balance proofs. Useful for notifying monitoring services. """
+
+    def __init__(self, balance_proof: BalanceProofSignedState) -> None:


No need for -> None

#3374 (comment)

rakanalh · 2019-02-13T17:34:59Z

raiden/transfer/mediated_transfer/events.py

+
+    @classmethod
+    def from_dict(
+            cls: Type[T_EventNewBalanceProofReceived],


We don't give a type for cls and self.

This event will be emitted when there are any newly received (and processed) balance proofs.

This adds an integration test that ensures that messages for EventNewBalanceProofReceived will create global messages from RaidenMonitoringEventHandler. Note: the special event handler needs to be enabled for this to work.

The previous implementation was short, but hard to read. This now uses a more explicit state traversal.

`make_room_alias` moved in the meantime.

rakanalh

LGTM

@rakanalh

changes were addressed and @rakanalh approved

konradkonrad added the dev: Please Review label Feb 11, 2019

konradkonrad force-pushed the publish_blinded_balance_proofs branch 2 times, most recently from 12bd0f5 to f6aff08 Compare February 11, 2019 12:40

hackaugusto reviewed Feb 11, 2019

View reviewed changes

raiden/raiden_event_handler.py Show resolved Hide resolved

raiden/transfer/architecture.py Outdated Show resolved Hide resolved

raiden/transfer/architecture.py Outdated Show resolved Hide resolved

konradkonrad requested a review from hackaugusto February 11, 2019 16:42

konradkonrad force-pushed the publish_blinded_balance_proofs branch 4 times, most recently from 10de2b2 to c7d901b Compare February 12, 2019 10:02

konradkonrad mentioned this pull request Feb 12, 2019

Publish Monitoring Requests on Recovery #3454

Closed

hackaugusto previously requested changes Feb 12, 2019

View reviewed changes

konradkonrad force-pushed the publish_blinded_balance_proofs branch 7 times, most recently from e8c6ea1 to 417b4ba Compare February 13, 2019 10:23

rakanalh suggested changes Feb 13, 2019

View reviewed changes

konradkonrad force-pushed the publish_blinded_balance_proofs branch 2 times, most recently from d31d30d to 7dd2691 Compare February 13, 2019 17:06

rakanalh suggested changes Feb 13, 2019

View reviewed changes

konradkonrad force-pushed the publish_blinded_balance_proofs branch from 7dd2691 to 1430580 Compare February 14, 2019 00:59

Add state diff method to detect changed balance_proofs

6f7005d

konradkonrad added 16 commits February 14, 2019 02:00

Add new Event: EventNewBalanceProofReceived

e052944

This event will be emitted when there are any newly received (and processed) balance proofs.

Emit events for received balance proofs

929f229

Send monitoring messages from RaidenMonitoringEventHandler

186b360

Add test for matrix send global on EventNewBalanceProofReceived

87791b9

This adds an integration test that ensures that messages for EventNewBalanceProofReceived will create global messages from RaidenMonitoringEventHandler. Note: the special event handler needs to be enabled for this to work.

Auto join global room on send_global

93748af

Use constant for monitoring matrix room

2569828

Ensure EventNewBalanceProofReceived is created

1c32e85

Use a more explicit state traversal

1a4db9b

The previous implementation was short, but hard to read. This now uses a more explicit state traversal.

Move state diff into views module

7e1c1cf

Compare states in raiden_service

3c24f50

Move EventNewBalanceProofReceived to mediated_transfer/events.py

73c6556

EventNewBalanceProofReceived will no longer be in the log

ddbfec2

Fix transport test

9bdaddc

`make_room_alias` moved in the meantime.

Add unit test for state diff method

43ce102

Add type hints to EventNewBalanceProofReceived

1430580

Remove return types for EventNewBalanceProofReceived

47ab2d1

rakanalh approved these changes Feb 14, 2019

View reviewed changes

konradkonrad merged commit 7437712 into raiden-network:master Feb 14, 2019

konradkonrad deleted the publish_blinded_balance_proofs branch February 14, 2019 16:38

rakanalh mentioned this pull request Feb 15, 2019

Monitor Requests (blinded balance proofs) must be published into a global matrix room #3273

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare publishing of blinded balance proofs #3452

Prepare publishing of blinded balance proofs #3452

konradkonrad commented Feb 11, 2019 •

edited

Loading

konradkonrad commented Feb 11, 2019 •

edited

Loading

konradkonrad commented Feb 11, 2019

ulope commented Feb 11, 2019

codecov bot commented Feb 11, 2019

codecov bot commented Feb 11, 2019 •

edited

Loading

konradkonrad commented Feb 11, 2019

hackaugusto Feb 12, 2019

hackaugusto Feb 12, 2019

hackaugusto Feb 12, 2019

hackaugusto Feb 12, 2019

hackaugusto Feb 12, 2019

rakanalh left a comment

rakanalh Feb 13, 2019

konradkonrad Feb 13, 2019

rakanalh Feb 13, 2019

konradkonrad Feb 13, 2019

rakanalh Feb 13, 2019

konradkonrad Feb 14, 2019

rakanalh Feb 13, 2019

konradkonrad Feb 14, 2019

rakanalh Feb 14, 2019 •

edited

Loading

rakanalh Feb 13, 2019

rakanalh Feb 13, 2019

konradkonrad Feb 14, 2019

rakanalh Feb 13, 2019

rakanalh left a comment

		@@ -272,6 +295,66 @@ def dispatch(self, state_change: StateChange) -> List[Event]:

		return events

		def detect_balance_proof_change(self, old_state, current_state):

Prepare publishing of blinded balance proofs #3452

Prepare publishing of blinded balance proofs #3452

Conversation

konradkonrad commented Feb 11, 2019 • edited Loading

konradkonrad commented Feb 11, 2019 • edited Loading

konradkonrad commented Feb 11, 2019

ulope commented Feb 11, 2019

codecov bot commented Feb 11, 2019

Codecov Report

codecov bot commented Feb 11, 2019 • edited Loading

Codecov Report

konradkonrad commented Feb 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakanalh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakanalh Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakanalh left a comment

Choose a reason for hiding this comment

konradkonrad commented Feb 11, 2019 •

edited

Loading

konradkonrad commented Feb 11, 2019 •

edited

Loading

codecov bot commented Feb 11, 2019 •

edited

Loading

rakanalh Feb 14, 2019 •

edited

Loading