Running federated node via CLI #447

KPrasch · 2018-09-23T01:02:39Z

No description provided.

…ableNode certificate deterministic certificate filename generation via common name

…on filepath

…s; Integrate with local filesystem paths in dev and non-dev modes.

…he essentials, pushing preinit logic into UrsulaConfiguration.produce

…ion of multi inheretance

jMyles

I'm uncomfortable with this pattern:

instance_of_foo = Bar(...)

That's what seems to be emerging here, and I'm not sure why. Perhaps it's part of the logic trying to move toward factory style?

Also... missleware?

jMyles · 2018-09-23T19:00:52Z

nucypher/crypto/api.py

-                          full_filepath,
-                          force: bool = True,
+                          full_filepath: str,
+                          force: bool = True,  # TODO


What's this TODO?

To set Force to False by default, but, I changed my mind for now.

jMyles · 2018-09-23T19:02:22Z

nucypher/network/nodes.py

        self._interface_signature_object = interface_signature

+        # TODO: This gets messy when it is None


Did you mean to move this comment down here? Haven't you obsoleted it above? Maybe just comment it out for now?

Yep - it's obsolete - needs cleanup

jMyles · 2018-09-23T19:02:44Z

nucypher/network/nodes.py

@@ -34,6 +43,12 @@ class WrongMode(TypeError):
        Raise when a Character tries to use another Character as decentralized when the latter is federated_only.
        """

+    @classmethod
+    def from_tls_hosting_power(cls, tls_hosting_power: TLSHostingPower, *args, **kwargs):


That's fucking interesting, man.

Yeah - I'm not using this anywhere yet, but if we continue to init VerifiableNode in the same fashion we do now, this makes sense.

I really like this.

jMyles · 2018-09-23T19:04:29Z

nucypher/network/nodes.py

-    def certificate(self):
-        return self._crypto_power.power_ups(TLSHostingPower).keypair.certificate
+    @property
+    def common_name(self):


Wow. Not kidding around.

So, common name is always from the cert? Surely it makes sense to check that it matches the checksum address before returning it?

Or are we in pretending-this-is-not-a-mixin pattern here?

Perhaps as I read on, it will become clearer. :-)

Yes the thinking here is that common name is always from the certificate, which needs to better better validated. with regard to mixins, I've had a similar question while working with Learner and VerifiableNode. Is this class a mixin? If not, then I'm inclined to localize the attributes.

I don't presently have a vision of it other than as a mixin, although there's no inherent reason that it can't be an inventory item.

jMyles · 2018-09-23T19:07:08Z

nucypher/characters/base.py

@@ -46,21 +47,31 @@ class NotEnoughTeachers(RuntimeError):
        pass

    def __init__(self,
-                 always_be_learning: bool = False,
-                 start_learning_on_same_thread: bool = False,
+                 start_learning_now: bool = False,


Umm, yeah - good change.

jMyles · 2018-09-23T19:14:39Z

examples/finnegans-wake-federated.py

-              always_be_learning=True,
-              known_certificates_dir=CERTIFICATE_DIR,
-              )  # TODO: 289
+ALICE = AliceConfiguration(network_middleware=RestMiddleware(),


What am I dealing with here? This thing returns an Alice? Is this to be the canonical way to get an Alice? That seems like an anti-pattern to me. Why can't the user init the class they want?

If you look at the bottom of this call, there is a chained method call to .produce() which outputs an Alice. You can still init any Character directly, of course, but while debugging demo-code I wanted to use temporary directories as a means of handling the nodes runtime files, which is cleaner / easier to do with config.

FWIW I actually intend to reset this module before taking this PR off WIP

jMyles · 2018-09-23T19:16:48Z

nucypher/config/node.py

@@ -14,17 +14,21 @@
 class NodeConfiguration:


Yeah, config has grown to be something of a mirror of Character, as you expressed concern it might. I still think I like it, but I'm joining you in your trepidation.

Same - I like like the benefits of having a layer between the filesystem and the character but also dislike the additional complexity. Overall Im +1 on NodeConfiguration classes.

I think the factory-style pattern relieves some tension regarding Character initialization.

jMyles · 2018-09-23T19:18:11Z

cli/demos/finnegans-wake-demo.py

-                    rest_port=rest_port,
-                    db_name='ursula-{}.db'.format(rest_port),
-                    federated_only=federated_only)
+    URSULA = UrsulaConfiguration(temp=True,


Similar concern to the one I raised above - is this to be the canonical way to get a Character?

deprecated ✔️

jMyles · 2018-09-23T19:21:27Z

nucypher/network/middleware.py

@@ -7,10 +7,10 @@

 class RestMiddleware:

-    def consider_arrangement(self, arrangement):
+    def consider_arrangement(self, arrangement, certificate_filepath):


I don't understand this change. Why do we ever want to pass in a cert other than the one that belongs to the Ursula in the arrangement?

✔️ - removed.

jMyles · 2018-09-23T19:23:33Z

nucypher/characters/lawful.py

@@ -242,7 +242,7 @@ def get_treasure_map(self, alice_verifying_key, label):
        if not self.known_nodes and not self._learning_task.running:
            # Quick sanity check - if we don't know of *any* Ursulas, and we have no
            # plans to learn about any more, than this function will surely fail.
-            raise self.NotEnoughUrsulas
+            raise Ursula.NotEnoughUrsulas


Not sure I understand this change either. Do we really want this exception only to live on Ursula (and thus require Ursula to be imported everywhere we raise it?).

...and if we do, then is NotEnoughUrsulas still a good name?

No - I do no think we want to require the import of Ursula in this case.

KPrasch · 2018-09-24T04:58:04Z

I'm uncomfortable with this pattern:
instance_of_foo = Bar(...)

Not sure If I understand. Is there a specific place in the pull request you are referring to?

…, everywhere

…recruits Ursulas from the blockchain

… preperation for futher integration.

KPrasch · 2018-09-25T17:38:21Z

I'm unsure why CI is failing all builds with ImportError: cannot import name 'Route' - I cannot reproduce this type of failure locally.

KPrasch · 2018-09-25T18:19:30Z

The Issue was that a new version of APIStar has been released: https://github.com/encode/apistar/releases. Pinned version 0.5.42 for now.

tuxxy · 2018-09-26T17:03:07Z

examples/run_federated_ursula.py

@@ -34,7 +34,6 @@
 def spin_up_ursula(rest_port, db_name, teachers=(), certificate_dir=None):
    metadata_file = "examples-runtime-cruft/node-metadata-{}".format(rest_port)

-    asyncio.set_event_loop(asyncio.new_event_loop())  # Ugh.  Awful.  But needed until we shed the DHT.


Are we planning to remove asyncio completely? I think we might be able to use it for task scheduling (ie: kfrag expiration)

tuxxy · 2018-09-26T17:04:00Z

nucypher/characters/base.py

-                 always_be_learning: bool = False,
-                 start_learning_on_same_thread: bool = False,
+                 start_learning_now: bool = False,
+                 learn_on_same_thread: bool = False,


What would be the need to move it to a different thread than the one you start it on?

tuxxy · 2018-09-26T17:04:31Z

nucypher/characters/base.py

+        if save_metadata and known_metadata_dir is None:
+            raise ValueError("Cannot save nodes without a known_metadata_dir")
+
+        known_nodes = known_nodes or tuple()


This is much better than before.

tuxxy · 2018-09-26T17:05:27Z

nucypher/characters/lawful.py

@@ -385,20 +378,26 @@ class Ursula(Character, VerifiableNode, Miner):
    # TLSHostingPower still can enjoy default status, but on a different class
    _default_crypto_powerups = [SigningPower, EncryptingPower]

+    class NotEnoughUrsulas(Learner.NotEnoughTeachers, MinerAgent.NotEnoughMiners):


Why move this here? Mostly because Ursula is here?

tuxxy · 2018-09-26T17:06:49Z

nucypher/network/nodes.py

@@ -34,6 +43,12 @@ class WrongMode(TypeError):
        Raise when a Character tries to use another Character as decentralized when the latter is federated_only.
        """

+    @classmethod
+    def from_tls_hosting_power(cls, tls_hosting_power: TLSHostingPower, *args, **kwargs):


I really like this.

tuxxy

🐧

jMyles

Looks like my concerns are addressed now, with the exception that I think that exceptions stemming from a dearth of nodes (Learners or Ursulas) rightly belong on Character (or a similarly universally positioned class).

KPrasch added 12 commits September 22, 2018 15:34

Update templae configuration file

9061da4

localize certificate references on VerifiableNode

b255416

Push node metadata writing logic up in Learner; Integrate with Verifi…

d1daf87

…ableNode certificate deterministic certificate filename generation via common name

Allow HostingKeypair to handle certi generation without a configurati…

f7b99df

…on filepath

Reflect character initialization logic in NodeConfiguration subclasse…

1376451

…s; Integrate with local filesystem paths in dev and non-dev modes.

Update tests to new learner names, help with cleanup a little.

aaba332

Let the CLI lean on configuration defaults; slim down run_ursula to t…

2393050

…he essentials, pushing preinit logic into UrsulaConfiguration.produce

Use strager certificate to consider arrangement

4c89845

Use configuration in simulate demno

51a3d0b

Resolve discrepancy with NotEnoughNodes exceptions; preserve discript…

5e10e48

…ion of multi inheretance

Collapse Configuration classes, adjust middleware certificate validation

6b0d7c9

Update tests with updates to configuration and missleware

9b04aee

KPrasch requested review from jMyles, tuxxy and michwill September 23, 2018 04:09

Once again, allow mypy failures.. for now

d615587

KPrasch force-pushed the federated_node branch from b37ab09 to d615587 Compare September 23, 2018 04:24

jMyles requested changes Sep 23, 2018

View reviewed changes

KPrasch changed the base branch from kokonusswasser to master September 24, 2018 19:55

KPrasch changed the base branch from master to kokonusswasser September 24, 2018 19:55

KPrasch force-pushed the federated_node branch from 9858388 to e4252ec Compare September 24, 2018 21:40

KPrasch added 7 commits September 24, 2018 18:42

Deprecate CLI demo module; Respond to RFC & cleanup

293f9c1

Push from_config up one level into character

c3923e2

split Alice and Bob fixtures by operating mode; Cleanup cleanup logic…

d02a8bc

…, everywhere

Pass-through caught MinerAgent.sample's exception message when Alice …

922eb9a

…recruits Ursulas from the blockchain

Fix circleCI stale filepath for eth-contracts build.

540e25a

split Alice.grant test by operating mode; Cleanup BlockchainPolicy in…

4b7fcd3

… preperation for futher integration.

cleanup stale consider_arrangement certificate filepaths

17d35a2

KPrasch force-pushed the federated_node branch from 2cbb4b1 to 17d35a2 Compare September 25, 2018 01:43

KPrasch changed the title ~~[WIP] Running federated node via CLI~~ Running federated node via CLI Sep 25, 2018

KPrasch added 4 commits September 25, 2018 09:28

remove extra time travel (moved to ursula fixture)

d6fc416

Resolve CI ImportErrors

ef31fb9

Quiet down ursula init logger

67df78f

Cast constant_sorrow constant to int

d84014a

KPrasch force-pushed the federated_node branch from fb75a89 to 9631804 Compare September 25, 2018 18:09

Pin version of apistar; post update

76b87da

KPrasch force-pushed the federated_node branch from 9631804 to 76b87da Compare September 25, 2018 18:10

tuxxy reviewed Sep 26, 2018

View reviewed changes

tuxxy approved these changes Sep 27, 2018

View reviewed changes

jMyles approved these changes Sep 27, 2018

View reviewed changes

KPrasch merged commit b700ae9 into nucypher:kokonusswasser Sep 27, 2018

		self._interface_signature_object = interface_signature

		# TODO: This gets messy when it is None

Running federated node via CLI #447

Running federated node via CLI #447

Conversation

KPrasch commented Sep 23, 2018

jMyles 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch Sep 24, 2018 • edited

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

KPrasch Sep 24, 2018 • edited

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

KPrasch commented Sep 24, 2018

KPrasch commented Sep 25, 2018 • edited

KPrasch commented Sep 25, 2018

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

tuxxy left a comment

Choose a reason for hiding this comment

jMyles left a comment

Choose a reason for hiding this comment

KPrasch Sep 24, 2018 •

edited

KPrasch Sep 24, 2018 •

edited

KPrasch commented Sep 25, 2018 •

edited