Skip to content

Retrieval protocol#2730

Merged
KPrasch merged 25 commits intonucypher:mainfrom
fjarri:retrieval-protocol
Sep 14, 2021
Merged

Retrieval protocol#2730
KPrasch merged 25 commits intonucypher:mainfrom
fjarri:retrieval-protocol

Conversation

@fjarri
Copy link
Contributor

@fjarri fjarri commented Jun 25, 2021

Type of PR:

  • Bugfix
  • Code quality

Required reviews:

  • 3

What this does:

  • removes the unused bob parameter of TreasureMap.construct_by_publisher() (introduced in Immutable treasure maps #2773)
  • removes Bob.matching_nodes_among()
  • removes EncryptedTreasureMap datastore model
  • removes the UmbralMessageKit alias of PolicyMessageKit
  • moves TLSHostingPower to powers.py from server.py (a proper place for it, also helps avoid some import cycles)
  • renames PolicyMessageKit to MessageKit. This is the externally exposed message-for-reencryption, so I think it makes sense to avoid unnecessary qualifiers - previously MessageKit wasn't even instantiated anywhere.
  • moves kits.py to policies/ (seems like the proper place for it, the objects there are all policy-related)
  • makes encrypt_and_sign() a classmethod constructor of MessageKit (renamed to author())
  • author() from above only returns MessageKit and not a tuple of the kit and the signature - the latter is already a part of the kit
  • fixes Define format for re-encryption metadata #259 by removing excess parts of the data Bob sends to Ursula
  • WorkOrder/PRETask/MessageKit streamlining (the existing behavior of externally distributed message kits is preserved; more changes may be made in other PRs depending on the discussion in Message kit contents #2743)
  • Bob supports an external caching of cfrags (through PolicyMessageKit objects)
  • New protocol objects: ReencryptionRequest and ReencryptionResponse
  • encrypted_treasure_map is now required in retrieve(); label and publisher_verifying_key are not needed
  • enrico parameter is removed from retrieve; policy_encrypting_key is mandatory
  • reworked the availability check before retrieval (former Bob.follow_treasure_map). Fixes Make Bob.follow_treasure_map() less obscure #2791
  • (TODO) support serialization of PolicyMessageKit objects? For Bob to store them somewhere.
  • (TODO) naming stuff. retrieve(), retrieve_cfrags(), PolicyMessageKit, RetrievalResult are the main offenders.

The whole retrieval procedure has been significantly simplified. Bob creates a RetrievalPlan plan based on Ursulas from the treasure map, the capsules in the kits, and possibly the attached cfrags in the kits. From it work orders for Ursulas are spawned and the results saved back into the plan.

You can see the new usage in test_bob_handles_frags.py. A lot of tests there were removed (because they tested the internal workings of retrieve()) and replaced with new ones.

For reviewers:

  • is retrieve() is a good name? Maybe retrieve_and_decrypt()?
  • should RetrievalClient be a class? It is pretty much stateless and could probably be rewritten as a function.

fjarri added a commit to fjarri/nucypher that referenced this pull request Jun 26, 2021
@fjarri fjarri force-pushed the retrieval-protocol branch from 6d206f4 to 7b60dac Compare June 26, 2021 04:17
fjarri added a commit to fjarri/nucypher that referenced this pull request Jun 26, 2021
@fjarri fjarri force-pushed the retrieval-protocol branch 5 times, most recently from 84bbcc0 to 5f9b6d1 Compare July 1, 2021 05:22
fjarri added a commit to fjarri/nucypher that referenced this pull request Jul 10, 2021
@fjarri fjarri force-pushed the retrieval-protocol branch 10 times, most recently from 515ea0b to e1fcf4c Compare July 11, 2021 00:03
@fjarri fjarri force-pushed the retrieval-protocol branch 3 times, most recently from cf8cf61 to 132f62a Compare July 11, 2021 21:45
fjarri added a commit to fjarri/nucypher that referenced this pull request Jul 12, 2021
@fjarri fjarri force-pushed the retrieval-protocol branch from 132f62a to d93dfe9 Compare July 12, 2021 23:25
fjarri added a commit to fjarri/nucypher that referenced this pull request Aug 6, 2021
Copy link
Member

@derekpierre derekpierre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎸 - great work @fjarri !

capsule_splitter = BytestringSplitter((Capsule, Capsule.serialized_size()))
cfrag_splitter = BytestringSplitter((CapsuleFrag, CapsuleFrag.serialized_size()))
kfrag_splitter = BytestringSplitter((KeyFrag, KeyFrag.serialized_size()))
checksum_address_splitter = BytestringSplitter((bytes, 20)) # TODO: is there a pre-defined constant?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is one, not even in eth_utils or eth_typings

Copy link
Contributor

@piotr-roslaniec piotr-roslaniec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉 Great work!

Copy link
Member

@KPrasch KPrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the walkthrough in-person. This PR represents the introduction of many new internal objects and altered system metaphors, it will take me a while to get used to this version of the codebase. Overall I think we're introducing some complexity to gain immutability and reduce the number of invalid instance states (as far as I understand this is precisely your goal). 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bob 👨‍💼 Effects the "Bob" development area Code Quality 🔧 Pertaining to code quality improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make Bob.follow_treasure_map() less obscure Define format for re-encryption metadata

5 participants