Stateless API #258

fjarri · 2020-06-03T06:02:33Z

Plan:

get rid of Capsule.set_correctness_keys()
- CHANGED: now Capulse objects have with_correctness_keys method that returns a PreparedCapsule object, which can be used for reencryption (with pre.decrypt_reencrypted()), cfrag verification (PreparedCapsule.verify_cfrag()) and kfrag verification (PreparedCapsule.verify_kfrag())
- CHANGED: set_correctness_keys() and get_correctness_keys() have been removed.
get rid of Capsule.attach_cfrag()
- CHANGED: instead of pre.decrypt() there's pre.decrypt_original() that takes a Capsule, and pre.decrypt_reencrypted() that takes a PreparedCapsule and a sequence of CapsuleFrag.
- CHANGED: attach_cfrag() and first_cfrag() were removed.
get rid of CapsuleFrag.attach_proof(). The proof is now attached on cfrag creation.
address issue Why there is no correctness_keys in result=capsule.to_bytes() #257
check type annotations
anything else?

To finalize:

Is PreparedCapsule the best name for a Capsule with correctness keys?
Should PreparedCapsule be derived from Capsule? (it is not at the moment)
Check that the number of provided cfrags is above threshold before trying to decrypt? Check that they are distinct as well?
There is some verification going on deep in the hierarchy that may be redundant. If we verify in high level functions, can we skip e.g. the capsule verification in CorrectnessProof constructor?

tuxxy · 2020-06-04T16:03:55Z

@fjarri This is a great start! Some thoughts on the present draft:

The thing that sticks out to me the most is the lack of a unified API for decryption. It's nice being able to keep a single encrypt and decrypt method for usability and API simplicity.

fjarri · 2020-06-04T17:24:47Z

Yes, I wasn't certain about this change. The rationale was the following:

Originally, decrypt() took a Capsule object, and depending on its contents (whether it had cfrags or not), the decryption method was chosen.

Now, we basically have one function that decrypts a capsule, and another that decrypts a list of cfrags (and the capsule is only used for verification). Besides having different parameters, they are used in different situations, so the user never has to write something along the lines of

if condition:
    decrypt_original()
else:
    decrypt_reencrypted()

It's always either one or the other. So I thought that giving them different names instead of having dynamic dispatch inside decrypt() (and a more complicated type signature with Union and Optional) would make the API more logical.

Since decrypt_reencrypted() is being used much more often, it could inherit the name decrypt(), but that's details.

cygnusv

First pass at the PR, looking good! I have some comments regarding some design decisions but in principle the logic remains unchanged.

Pipfile

cygnusv · 2020-07-30T17:29:25Z

umbral/pre.py

    def __repr__(self):
        return "{}:{}".format(self.__class__.__name__, hex(int(self.bn_sig))[2:17])


+class PreparedCapsule:


Have you considered subclassing Capsule here?

I have, it's mentioned in the PR description :) Don't know how justified it is. They are used in different contexts, after all.

Why subclass? Do they actually share any logic?

And if the answer is "no" - and I think it is - then is thing construction even a "Capsule" at all? And is it even necessary?

"Why subclass? Do they actually share any logic?" - I agree, that's what I thought too. Essentially, it's a CapsuleThatCanVerifyFrags. Perhaps it will be better to have a Capsule and VerificationMetadata, with the latter including delegating, receiving, and signing keys?

cygnusv · 2020-07-31T11:09:14Z

umbral/pre.py

-        cfrag.prove_correctness(capsule, kfrag, metadata)
-
-    return cfrag
+    return CapsuleFrag.from_kfrag(prepared_capsule.capsule, kfrag, metadata)


I'm not very convinced of this change. Re-encryption, the current cryptographic core of the NuCypher network gets demoted behind CapsuleFrag.from_kfrag(). On second thoughts, maybe I'm just being dramatic here, but I'd like others' input here too.

Also not a big fan of this change. If the core of the API is central to Proxy Re-Encryption, then hiding where it happens in a method like this makes it a bit difficult to understand and use.

The user still only sees reencrypt(). Would it be better to have KFrag.reencrypt() -> CFrag instead or CapsuleFrag.from_kfrag()?

Or even Capsule.reencrypt(kfrag) -> CFrag

What's wrong with the original place?

The rationale for putting this code in from_kfrag() is that it is the only way to create a CapsuleFrag (besides deserialization), so it is logical to make it a (classmethod) constructor. Both to ensure that a CapsuleFrag is always in a valid state, and that all the ways to create it are easily discoverable.

A somewhat weaker argument is that it is not common in Python to have a free-standing function accessing internal state of objects of several types. If I were looking for a way to create a CapsuleFrag, I'd look into methods of CapsuleFrag, KFrag and Capsule first before searching for a separate function.

That said, this change is not essential to the main purpose of the PR, and I can roll it back if you're opposed to it.

All those are valid arguments. I'd like the opinion of @jMyles and @KPrasch too. At some moment, we decided to leave the core logic in the pre.py module as standalone functions, instead of encapsulating them in any class. But this was 2.5 years ago and it's rained a lot since then.

pre.reencrypt() isn't really going anywhere, it just becomes a thin wrapper over from_kfrag() or wherever the logic ends up.

umbral/pre.py

cygnusv · 2020-07-31T11:20:31Z

umbral/pre.py

+    except InvalidTag as e:
+        raise UmbralDecryptionError() from e
+
+    return cleartext


Now that I reread this part, I recall why there was a single decrypt method. It was not only to provide one single entry point for decryption (which is a design choice that you contest in this PR, and I can be fine with the change), but also because the internal logic is identical in both methods, except for the intermediate step (decapsulation). Maybe there's a solution that satisfies both points of view?

For this part, why not replace InvalidTag with UmbralDecryptionError straight in UmbralDEM.decrypt()?

@cygnusv: Can you help me understand why your design preference has changed regarding a single entry point vs multiple?

@fjarri: In either scenario, raising UmbralDecryptionError in the lower frame seems acceptable to me, sure.

There is a certain potential barrier for moving UmbralDecryptionError to dem.py: GenericUmbralError will have to be moved somewhere as well, and the applications, which could just import from .pre before, will need to import from .dem as well. So at the moment I just extracted the DEM operations into a private function in pre.py.

Although the right way would be to move UmbralDecryptionError to dem.py, since it is a DEM error, and export all the main API from __init__.py.

cygnusv · 2020-07-31T11:30:57Z

vectors/vectors_cfrags.json

@@ -1,51 +1,51 @@
 {
  "name": "Test vectors for CFrags",
-  "description": "This is a collection of CFrags, originated from the enclosed Capsule, under the enclosed delegating, verifying and receiving keys. Each CFrag must deserialize correctly and can be replicated with a call to `pre.reencrypt(kfrag, capsule, provide_proof=False)`",
+  "description": "This is a collection of CFrags, originated from the enclosed Capsule, under the enclosed delegating, verifying and receiving keys. Each CFrag must deserialize correctly and can be replicated with a call to `pre.reencrypt(kfrag, capsule)`",


The original goal of this test vector was to provide something completely deterministic and reproducible. Re-encryption is deterministic, but proving correctness is not, so that's why this test vector used the option provide_proof=False.

Having said that, our test vectors are incomplete, we don't have anything for correctness proofs, so I guess these ones can stay in this form or another.

Hm, the way I understand it, generating a proof involves randomness, but once it is created and put in a vector, the rest is deterministic?

Yes, you're right. But the point of this test vector is to be able to replicate re-encryption (and hence, production of CFrags). There's however value in having a test vector of valid correctness proofs, so maybe we can also keep these.

jMyles · 2020-09-10T11:36:27Z

README.rst

-  capsule.set_correctness_keys(delegating=alices_public_key,
-                               receiving=bobs_public_key,
-                               verifying=alices_verifying_key)
+  prepared_capsule = capsule.with_correctness_keys(delegating=alices_public_key,


This reads like a layer of indirection to me. We had a "capsule", but if we add something to it, we get not the same "capsule", but a different "capsule"? Of a different class?

See above about having a VerifyingMetadata (or something along these lines) instead that would wrap these three keys. What do you think about this approach?

And then compose that on this capsule?

I'm interested to hear about the why in addition to the what. Why is this change (and the rest of the capsule fissure) helpful in your mind?

Because the verifying metadata is secondary to the capsule. A capsule can be used without it. You can keep it as a separate object, or as a part of another object combining both the capsule and the keys, that's details. The change I'm introducing here is to remove the mutability. I do not understand why you are calling it a fissure.

jMyles

This PR feels clever to me.

It masks the mutation of Capsule by instantiating a separate manifestation of the same knowledge, PreparedCapsule. It then masks the state of a PreparedCapsule by directing the user to two different methods for decryption, which repeat some logic.

A Capsule is a stateful concept; rewriting the code to mask that creates, rather than removes, a layer of indirection.

This seems to be a reopening of the essence of #38 by another name. I think #38 was properly merged, and that this PR is properly closed wontfix.

That's not to discount the innovation and bold thinking, @fjarri. I surmise that you may not even have read #38 before crafting this PR, and yet you successfully reverse engineered those changes. I think that deserves praise and admiration. :-)

fjarri · 2020-09-11T22:31:34Z

This PR feels clever to me.

Somehow I feel it's not a compliment :)

It masks the mutation of Capsule by instantiating a separate manifestation of the same knowledge, PreparedCapsule.

No, I would say that it's the same knowledge plus the information to verify it. The capsule does not change. I think there's too much verification going on in the code as it is, and making it explicit will help eliminate some of excessive checks.

It then masks the state of a PreparedCapsule by directing the user to two different methods for decryption, which repeat some logic.

See the comment about the logic above. The "repeating logic" is just type checks. The actual logic is different (_open_capsule vs _decapsulate_original), as is the set of parameters.

A Capsule is a stateful concept; rewriting the code to mask that creates, rather than removes, a layer of indirection.

I would disagree here. A capsule is two curve points and one scalar. Nothing changes inside it. It can be used with or without cfrags (involving different algorithms), with or without verification keys.

This seems to be a reopening of the essence of #38 by another name. I think #38 was properly merged, and that this PR is properly closed wontfix.

I would disagree with that as well. PR #38 was essentially rolled back/fixed later, because now the reconstruct() logic is contained in _decapsulate_reencrypted(), and, as it turns out, no additional ReconstructedCapsule class is necessary — its data is ephemeral and doesn't need to be saved.

…tead (PreparedCapsule)

…` instead Fix a type hint

jMyles · 2020-10-01T19:54:35Z

No, I would say that it's the same knowledge plus the information to verify it

Well yeah. The question is how to semantically represent this in a way that tells a true story about this knowledge plus the information to verify it.

I would disagree here. A capsule is two curve points and one scalar. Nothing changes inside it. It can be used with or without cfrags (involving different algorithms), with or without verification keys.

The metaphor I imagine is that, indeed, nothing changes inside it, but that it has slots for Bob to plug in CFrags to the outside of it, and it eventually glows with activation, in when enough have been plugged. The plugging-in of the CFrags, the activation, and eventually the accessibility of the information contained therein (whether we picture it as Bob grasping it, or the "chimney" notion of plaintext being emitted as an industrial product) are all part of the lifecycle of this object - all mutations.

I would disagree with that as well. PR #38 was essentially rolled back/fixed later, because now the reconstruct() logic is contained in _decapsulate_reencrypted(), and, as it turns out, no additional ReconstructedCapsule class is necessary — its data is ephemeral and doesn't need to be saved.

Be that as it may, it still divides the Capsule into two, as if it disappears in the middle of Bob's custody and a new one appearing in its place. And it splits PRE.decrypt in almost the same fashion as it was split prior to #38

This PR seems to cost richness in the metaphor. I find it more difficult to reason about, and more difficult to remember the lifecycle of the objects in question. It makes it perhaps easier for a computer to understand, but harder for a human to. Presently, I can picture everything about what Bob does and why; it aligns logically with the primitives in use at each stage.

fjarri · 2020-10-03T22:54:26Z

The metaphor I imagine is that, indeed, nothing changes inside it, but that it has slots for Bob to plug in CFrags to the outside of it, and it eventually glows with activation, in when enough have been plugged. The plugging-in of the CFrags, the activation, and eventually the accessibility of the information contained therein (whether we picture it as Bob grasping it, or the "chimney" notion of plaintext being emitted as an industrial product) are all part of the lifecycle of this object - all mutations.

This PR seems to cost richness in the metaphor.

Yes, that's exactly where we disagree. My position is that our code has way too many metaphors. There's no need to call 1 + 2 + 3 "a journey of a plus operator, and a sum becoming alive", or write it as s = Sum(); s.add(1); s.add(2); s.add(3); s.calculate(). Sometimes a banana is just a banana. It was not my initial intention to remove any metaphors, but, as a matter of fact, it is a welcome side effect.

Be that as it may, it still divides the Capsule into two, as if it disappears in the middle of Bob's custody and a new one appearing in its place.

Nope, the capsule is immutable and therefore exactly the same (and thus doesn't need to "reappear" anywhere - you can just reuse it if you wish). It's in the mutable case where you have a new capsule appearing, but you can't tell that it's a new capsule because it's the same object, yet with different contents.

I find it more difficult to reason about, and more difficult to remember the lifecycle of the objects in question.

If the objects are immutable, they have no lifecycle, and you don't need to think about it. It makes things simpler.

Anyway, I understand that you're a firm -1 on immutability, and I'm out of arguments. Anyone else wants to chime in?

cygnusv · 2021-03-15T10:56:18Z

@fjarri let's try to merge the stateless API PR as is, up to the new changes to accommodate umbral v2, and include these latter changes in a new PR.

fjarri · 2021-03-16T00:48:32Z

@cygnusv Done, created #263

Not sure what the CI fails are about. Tests run fine locally.

KPrasch · 2021-03-16T00:58:18Z

The failing CI tests are showing python 3.5 compatibility, and benchmarking in python 3.6. Perhaps you do not reproduce these failures locally because you are using a different version of python, and not executing the benchmarks?

fjarri · 2021-03-16T01:15:50Z

The failing CI in 3.5 is actually because of BytestringSplitter which uses f-strings, so that should perhaps be updated.

The other one is pysha3 failing to build, and also it's kind of strange with the versions:

Creating a virtualenv for this project…
Pipfile: /home/circleci/pyUmbral-36/Pipfile
Using /usr/bin/python3 (3.7.3) to create virtualenv…
created virtual environment CPython3.7.3.final.0-64 in 917ms

So it's really 3.7 and not 3.6...

fjarri · 2021-03-16T01:25:57Z

I'm actually not sure this PR is even that important now, seeing as how #263 will be a big refactor even compared to it, so it might as well be a refactor compared to the main branch.

fjarri · 2021-03-18T18:56:34Z

Deprecated in favor of #263. RIP.

fjarri added the enhancement label Jun 3, 2020

fjarri requested review from cygnusv, jMyles, KPrasch and tuxxy June 4, 2020 05:00

fjarri marked this pull request as ready for review July 10, 2020 16:27

fjarri changed the title ~~[WIP] Stateless API~~ Stateless API Jul 10, 2020

cygnusv reviewed Jul 31, 2020

View reviewed changes

fjarri mentioned this pull request Aug 21, 2020

[WIP] Parallel Bob.retreive() nucypher/nucypher#2063

Closed

jMyles reviewed Sep 10, 2020

View reviewed changes

jMyles suggested changes Sep 10, 2020

View reviewed changes

fjarri added 6 commits September 11, 2020 21:19

Test suite fixes

5d20b43

Remove mutation in set_correctness_keys() and create a new object ins…

76e95c9

…tead (PreparedCapsule)

Get rid of attach_cfrag() and pass cfrags to `decrypt_reencrypted()…

42f1b9d

…` instead Fix a type hint

Attach a proof to a cfrag on creation unconditionally

ca86304

Remove code duplication from decrypt* functions

3e95809

Verify Capsules once on creation

6696ebb

fjarri force-pushed the stateless branch from 9be361a to 6696ebb Compare September 12, 2020 05:17

fjarri mentioned this pull request Mar 13, 2021

Interfacing with Python nucypher/rust-umbral#41

Merged

fjarri force-pushed the stateless branch from b474f0a to 6696ebb Compare March 16, 2021 00:32

fjarri force-pushed the stateless branch from ab5f50d to 6696ebb Compare March 17, 2021 05:22

fjarri closed this Mar 18, 2021

fjarri deleted the stateless branch May 27, 2021 06:07

Stateless API #258

Stateless API #258

Conversation

fjarri commented Jun 3, 2020 • edited Loading

tuxxy commented Jun 4, 2020

fjarri commented Jun 4, 2020 • edited Loading

cygnusv 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

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

fjarri Sep 12, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jMyles left a comment

Choose a reason for hiding this comment

fjarri commented Sep 11, 2020 • edited Loading

jMyles commented Oct 1, 2020

fjarri commented Oct 3, 2020

cygnusv commented Mar 15, 2021

fjarri commented Mar 16, 2021

KPrasch commented Mar 16, 2021

fjarri commented Mar 16, 2021

fjarri commented Mar 16, 2021

fjarri commented Mar 18, 2021

fjarri commented Jun 3, 2020 •

edited

Loading

fjarri commented Jun 4, 2020 •

edited

Loading

fjarri Sep 12, 2020 •

edited

Loading

fjarri commented Sep 11, 2020 •

edited

Loading