Fleet sensor refactor #2352

fjarri · 2020-10-13T04:15:36Z

The goal of this PR is to make interactions with FleetSensor consistent and avoid exposing implementation details, or carrying around ad-hoc states (fleet_state_* attributes). More specifically:

FleetState is made into a real class. It encapsulates incremental updates and collection-like interface to iterate over the state's nodes. It is exposed from FleetSensor as current_state.
FleetSensor itself manages the buffers with new/deleted nodes and a list of previous states.
Previous states are kept as simple ArchivedFleetState instances that do not retain references to actual nodes (since we don't use them anyway), only checksum/nickname/population.
The user (meaning the higher levels of the codebase) must call record_fleet_state() in order for the state to get updated. Currently there are multiple cases of accessing the node list without an explicit update.
Fleet states for remote nodes are kept in FleetSensor.remote_states instead of in fleet_state_* attributes.
abridged_* methods are renamed to better represent their purpose. The former abridged_node_details() (now status_info() is now used for both JSON and HTML /status endpoints outputs.

Rough edges and possible improvements:

This PR may include a fix for Use "canonical address" instead of checksum address to key known_nodes #1995, if we decide on the course of actions.
It is possible to avoid calculating the new state's checksum in some cases. Since remote nodes are immutable, if we retain the local node's metadata and remote nodes' addresses in archived states, we can compare those with the current ones, and use the archived state's checksum if there's a match. Not sure if there will be a noticeable benefit.
The usage of the current state can be made more explicit if we don't forward most of the calls from FleetSensor to FleetSensor.current_state.
An alternative to the explicit record_fleet_state() call is to automatically update the state (if there are new nodes) whenever current_state is accessed.
In case of matching fleet states, the teacher sends FLEET_STATES_MATCH along with the fleet state. That seems excessive, but changing that without a proper protocol versioning will be a problem.

Note: if this PR is merged, https://github.com/nucypher/nucypher-monitor/blob/master/monitor/crawler.py will need to be updated.

jMyles · 2020-10-27T10:53:58Z

nucypher/acumen/perception.py

+        # Checking if the node already has a checksum address
+        # (it may be created later during the constructor)
+        # or if it mutated since the last check.
+        if self._this_node_ref is not None and getattr(self._this_node_ref(), 'finished_initializing', False):


While I like the way this logic is evolving, these weakref gymnastics aren't really doing it for me. Is this all just to avoid having to lug around additional_nodes_to_track?

Not really. There are several issues being solved here.

Circular references. The current code with additional_nodes_to_track has them too: Ursula -> known_nodes -> additional_nodes_to_track -> Ursula. Hence the weakrefs.

This code can be called at Ursula construction time, when its full metadata is still not available. A check for the local node to be available (currently a very awkward getattr of finished_initializing) removes the need in an additional mutating call that you have to remember to invoke sometime after you create the Ursula.

The local node can be mutated anytime. So we have to request the new metadata every time the state is updated.

None of these are strong enough to justify the enormous hit in readability, IMO.

weakref is something best saved for when it's truly needed, like when it's the only way out of a hairy performance bottleneck with an object whose representation is too large (in memory) to justify doing some other way.

None of these are strong enough to justify the enormous hit in readability, IMO.

I wouldn't call two dereferences of a weakref an "enormous hit in readability". And the weakref is there only for dealing with the first point; if you get rid of it, the code will remain pretty much the same because there are still points 2 and 3 to worry about. Now if we prohibit node mutation, that will improve the readability.

weakref is something best saved for when it's truly needed, like when it's the only way out of a hairy performance bottleneck with an object whose representation is too large (in memory) to justify doing some other way.

Ursula is the main object being held by the cycle here, and it's pretty large. Of course, it mainly matters for testing, but we've already hit a similar problem not so long ago.

Personally, I believe that it is better to take a slight readability hit and use a weakref than debug a problem caused by a reference cycle half a year later.

P.S. I guess it's close to the difference in our views on immutability: your position is that you only need to use weakrefs when you absolutely must, and mine is that you only need to keep reference cycles when you absolutely must.

I have to admit that this line is pretty tough to read...

if self._this_node_ref is not None and getattr(self._this_node_ref(), 'finished_initializing', False):

Related discusson: https://ptb.discord.com/channels/411401661714792449/411401661714792451/772182575237431306

nucypher/acumen/perception.py

KPrasch

Excellent readability improvements throughout - +1 on ArchivedStates as part of the fleet state lifecycle.

nucypher/network/nodes.py

nucypher/network/server.py

nucypher/acumen/nicknames.py

nucypher/acumen/perception.py

derekpierre · 2021-01-15T17:00:01Z

nucypher/acumen/perception.py

+    def unpack_snapshot(data):
+        return FleetState.unpack_snapshot(data)
+
+    def record_fleet_state(self):


There will be some consequences to the status monitor code which will need to be updated - https://github.com/nucypher/nucypher-monitor/blob/master/monitor/crawler.py#L251. Not a huge deal, just something to note (@KPrasch )

Thanks for noticing that, it'll have to be updated.

KPrasch · 2021-01-15T20:05:32Z

How many reviewers would you like on this PR?

cygnusv

Great work @fjarri, as usual!

nucypher/acumen/perception.py

nucypher/network/nodes.py

tests/integration/learning/test_domains.py

KPrasch

Thanks @fjarri, left a few comments for ya!

nucypher/acumen/perception.py

KPrasch · 2021-02-15T17:10:52Z

nucypher/acumen/perception.py

+        # Checking if the node already has a checksum address
+        # (it may be created later during the constructor)
+        # or if it mutated since the last check.
+        if self._this_node_ref is not None and getattr(self._this_node_ref(), 'finished_initializing', False):


I have to admit that this line is pretty tough to read...

if self._this_node_ref is not None and getattr(self._this_node_ref(), 'finished_initializing', False):

nucypher/acumen/perception.py

KPrasch · 2021-02-15T18:29:33Z

nucypher/network/server.py

-            response = jsonify(payload)
-            return response
+
+        return_json = request.args.get('json') == 'true'


Why check for true explicitly here - to avoid the case of evaluating false as True? I find this to be a bit awkward, perhaps we need to consider using an alternate endpoint?

Isn't that the correct way to do it? As far as I understand, URL arguments are given to the request object as strings, casting them is the user's responsibility.

Yeah, alright, fair enough.

tests/integration/learning/test_fleet_state.py

tests/unit/test_external_ip_utilities.py

KPrasch

Well done @fjarri - Thanks for taking to time to think through the nuances of this PR.

KPrasch · 2021-02-19T01:20:57Z

nucypher/network/server.py

-            response = jsonify(payload)
-            return response
+
+        return_json = request.args.get('json') == 'true'


Yeah, alright, fair enough.

KPrasch · 2021-02-19T01:25:24Z

nucypher/acumen/perception.py

+        if this_node_changed or remote_nodes_updated or remote_nodes_slashed:
+            # TODO: if nodes were kept in a Merkle tree,
+            # we'd have to only recalculate log(N) checksums.
+            # Is it worth it?


This is a very interesting suggestion, perhaps worth moving off this PR for further discussion.

KPrasch · 2021-02-19T01:26:04Z

nucypher/acumen/perception.py

+            msg = f"Rejected node {node} because its domain is '{node.domain}' but we're only tracking '{self._domain}'"
+            self.log.warn(msg)
+
+    def __getitem__(self, item):


Just a note: I've deprecated this method in #2513

KPrasch · 2021-02-19T01:26:41Z

nucypher/acumen/perception.py

+                                  timestamp: maya.MayaDT,
+                                  population: int):
+        nickname = Nickname.from_seed(state_checksum, length=1)
+        self.remote_states[checksum_address] = ArchivedFleetState(checksum=state_checksum,


👍🏻 Great.

fjarri force-pushed the fleet-sensor-refactor branch 8 times, most recently from f3cd6df to c81922e Compare October 18, 2020 16:34

fjarri requested review from KPrasch, jMyles and cygnusv October 20, 2020 02:36

jMyles reviewed Oct 27, 2020

View reviewed changes

KPrasch reviewed Dec 16, 2020

View reviewed changes

nucypher/acumen/perception.py Outdated Show resolved Hide resolved

KPrasch reviewed Dec 16, 2020

View reviewed changes

nucypher/network/nodes.py Outdated Show resolved Hide resolved

nucypher/network/nodes.py Outdated Show resolved Hide resolved

nucypher/network/server.py Outdated Show resolved Hide resolved

nucypher/network/server.py Outdated Show resolved Hide resolved

KPrasch mentioned this pull request Jan 11, 2021

[WIP] Buckets #2513

Closed

7 tasks

fjarri force-pushed the fleet-sensor-refactor branch 2 times, most recently from 4b140d1 to ec25522 Compare January 15, 2021 05:53

fjarri added a commit to fjarri/nucypher that referenced this pull request Jan 15, 2021

Newsfragment for PR nucypher#2352

5425b26

fjarri changed the title ~~[WIP] Fleet sensor refactor~~ Fleet sensor refactor Jan 15, 2021

fjarri marked this pull request as ready for review January 15, 2021 05:58

fjarri requested review from derekpierre and vepkenez January 15, 2021 06:07

fjarri added the Ursula 👩‍🚀 Effects the "Ursula" development area label Jan 15, 2021

derekpierre reviewed Jan 15, 2021

View reviewed changes

cygnusv approved these changes Jan 25, 2021

View reviewed changes

nucypher/acumen/perception.py Outdated Show resolved Hide resolved

nucypher/network/nodes.py Show resolved Hide resolved

nucypher/network/nodes.py Outdated Show resolved Hide resolved

This was referenced Jan 26, 2021

Persistent TLS certificates; Simplify Ursula Initialization #2536

Merged

A name for methods returning JSON-serializable things #2540

Closed

fjarri added a commit to fjarri/nucypher that referenced this pull request Jan 26, 2021

Newsfragment for PR nucypher#2352

4e14463

fjarri force-pushed the fleet-sensor-refactor branch from 4d121f0 to f49b26e Compare January 26, 2021 07:24

fjarri added a commit to fjarri/nucypher that referenced this pull request Jan 28, 2021

Newsfragment for PR nucypher#2352

45ab737

fjarri force-pushed the fleet-sensor-refactor branch from f49b26e to 08d7ce5 Compare January 28, 2021 01:46

KPrasch reviewed Feb 15, 2021

View reviewed changes

tests/integration/learning/test_domains.py Show resolved Hide resolved

KPrasch reviewed Feb 15, 2021

View reviewed changes

fjarri added a commit to fjarri/nucypher that referenced this pull request Feb 16, 2021

Newsfragment for PR nucypher#2352

461e89c

fjarri force-pushed the fleet-sensor-refactor branch 2 times, most recently from 9f90455 to cbc204b Compare February 16, 2021 06:14

fjarri added 3 commits February 16, 2021 22:45

Newsfragment for PR nucypher#2352

3eb3dd3

Refactor FleetSensor

4de9b91

Implement changes from the review

37929b3

fjarri force-pushed the fleet-sensor-refactor branch from 575da0a to 37929b3 Compare February 17, 2021 06:46

KPrasch approved these changes Feb 19, 2021

View reviewed changes

KPrasch merged commit 9c46f5e into nucypher:main Feb 19, 2021

This was referenced Feb 20, 2021

Help nucypher-monitor get the info it needs #2574

Merged

Updates to work with the new FleetSensor API nucypher/nucypher-monitor#94

Merged

fjarri deleted the fleet-sensor-refactor branch March 3, 2021 22:47

fjarri mentioned this pull request Mar 23, 2021

Bring back a few bits of blockchain node status that were accidentally removed #2611

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fleet sensor refactor #2352

Fleet sensor refactor #2352

fjarri commented Oct 13, 2020 •

edited

Loading

jMyles Oct 27, 2020

fjarri Oct 28, 2020

jMyles Oct 29, 2020

fjarri Oct 29, 2020 •

edited

Loading

KPrasch Feb 15, 2021

jMyles Feb 15, 2021

KPrasch left a comment

derekpierre Jan 15, 2021

fjarri Jan 15, 2021

KPrasch commented Jan 15, 2021

cygnusv left a comment

KPrasch left a comment

KPrasch Feb 15, 2021

KPrasch Feb 15, 2021

fjarri Feb 16, 2021

KPrasch Feb 19, 2021

KPrasch left a comment

KPrasch Feb 19, 2021

KPrasch Feb 19, 2021

KPrasch Feb 19, 2021

KPrasch Feb 19, 2021 •

edited

Loading

Fleet sensor refactor #2352

Fleet sensor refactor #2352

Conversation

fjarri commented Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjarri Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch commented Jan 15, 2021

cygnusv left a comment

Choose a reason for hiding this comment

KPrasch 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

KPrasch 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

KPrasch Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

fjarri commented Oct 13, 2020 •

edited

Loading

fjarri Oct 29, 2020 •

edited

Loading

KPrasch Feb 19, 2021 •

edited

Loading