Implement feedback from integrating tDec flow #227
Conversation
piotr-roslaniec
force-pushed
the
tdec-poc
branch
from
8231c49
to
a8aaf8e
Compare
piotr-roslaniec
force-pushed
the
tdec-poc
branch
from
a8aaf8e
to
c2acc61
Compare
|
Bundled size for the package is listed below: build/module/types/ethers-contracts/factories: 82.03 KB |
Codecov Report
@@ Coverage Diff @@
## tdec-epic #227 +/- ##
=============================================
- Coverage 83.89% 83.45% -0.44%
=============================================
Files 37 37
Lines 956 973 +17
Branches 121 122 +1
=============================================
+ Hits 802 812 +10
- Misses 147 155 +8
+ Partials 7 6 -1
|
piotr-roslaniec
requested review from
KPrasch,
theref,
derekpierre and
manumonti
| provider: participant.provider, | ||
| aggregated: participant.aggregated, | ||
| decryptionRequestStaticKey: SessionStaticKey.fromBytes( | ||
| fromHexString(participant.decryptionRequestStaticKey) |
There was a problem hiding this comment.
This data is currently relayed from porter?
There was a problem hiding this comment.
It's fetched from Coordinator contract
| ciphertext: Ciphertext, | ||
| aad: Uint8Array | ||
| ): Promise<readonly Uint8Array[]> { | ||
| variant: FerveoVariant, |
src/characters/cbd-recipient.ts
Outdated
| @@ -90,21 +82,25 @@ export class CbdTDecDecrypter { | |||
| provider, | |||
| this.ritualId | |||
| ); | |||
| // We only need the `threshold` participants | |||
| const sufficientDkgParticipants = dkgParticipants.slice(0, this.threshold); | |||
There was a problem hiding this comment.
This can be hardcoded as shares // 2 + 1 to match nucypher/nucypher (can be in another PR)
There was a problem hiding this comment.
So we don't want to support tunable threshold any time soon? Taking notes for API design.
| const sharedSecret = ephemeralSessionKey.deriveSharedSecret( | ||
| decryptionRequestStaticKey |
| DkgPublicParameters.fromBytes(dkgPublicParams), | ||
| threshold | ||
| ); | ||
| return new CbdTDecDecrypter(new Porter(porterUri), ritualId, threshold); |
There was a problem hiding this comment.
(in another PR) CbdTDecDecrypter -> ThresholdDecrypter || Bob || something else?
There was a problem hiding this comment.
Yes, suggestions for naming are welcome
| if (!withConditions) { | ||
| throw new Error('Conditions are required for CBD encryption.'); | ||
| } |
There was a problem hiding this comment.
Agree! This is a major differentiating factor between the PRE-adapted-Tdec and the real one.
| } | ||
| // TODO: Create a new DKG ritual here | ||
| throw new Error('Not implemented'); | ||
| // TODO: Update API: Replace with getExistingRitual and support ritualId in Strategy |
| decryption_results: { | ||
| encrypted_decryption_responses: Record< | ||
| ChecksumAddress, | ||
| Base64EncodedBytes | ||
| >; |
There was a problem hiding this comment.
@derekpierre - (wrt porter) what's the reason behind the nesting here? What other data can be inside results other than the values of decryption_results?
There was a problem hiding this comment.
To provide some consistency for Porter responses. All Porter requests PRE / CBD return as follows:
{
"result": ...,
"version": "x.y.z"
}Where result could be anything. For example:
- https://docs.nucypher.com/en/latest/application_development/web_development.html#example-response
- https://docs.nucypher.com/en/latest/application_development/web_development.html#id7
- (the above for CBD)
We can definitely rethink it if it feels too redundant.
|
👨🏻🚀 Approved with non-blocking comments. Well done! |
src/characters/cbd-recipient.ts
Outdated
|
|
||
| // TODO: How many errors are acceptable? Less than (threshold - shares)? | ||
| // TODO: If Porter accepts only `threshold` decryption requests, then we may not have any errors | ||
| if (Object.keys(errors).length > 0) { |
There was a problem hiding this comment.
Porter can return both successes and errors in the response from /cbd_decrypt, so
- success is threshold number of responses returned (ignore any errors)
- If less than threshold number of responses then fail with any errors provided.
There was a problem hiding this comment.
I only need to check for presence of any errors then
There was a problem hiding this comment.
No. Errors can be reported even if the overall operation was successful i.e. some Ursula's may fail while waiting for sufficient responses.
There was a problem hiding this comment.
So we'd only want to throw here if we didn't receive enough responses:
if (Object.keys(encryptedResponses).length < this.threshold) {
throw new Error(
`CBD decryption failed with errors: ${JSON.stringify(errors)}`
);
}
There was a problem hiding this comment.
👍 - Correct.
With a message something like..."Threshold of responses not met; CBD decryption failed with errors..."
src/characters/cbd-recipient.ts
Outdated
| const encryptedResponse = encryptedResponses[ursula]; | ||
| if (!encryptedResponse) { | ||
| throw new Error(`Missing encrypted response from ${ursula}`); | ||
| } |
There was a problem hiding this comment.
Would this fail if any ursula did not return an encrypted response...? i.e.assuming a threshold number of responses have been returned, we only care about the ursulas that responded.
There was a problem hiding this comment.
Yes. I agree that we only need threshold responses and then we can call it a day, but I wanted to throw this error here to highlight some possible but difficult-to-debug edge-case. It's possible that encryptedResponses[ursula] is undefined and may result in some weirdness.
There was a problem hiding this comment.
The issue here is that Porter only waits for a threshold number of responses, so some Ursulas may not have responded by the time Porter returns because the threhold was already met. So you likely will have Ursulas without responses, but that not be a problem.
| decryption_results: { | ||
| encrypted_decryption_responses: Record< | ||
| ChecksumAddress, | ||
| Base64EncodedBytes | ||
| >; |
There was a problem hiding this comment.
To provide some consistency for Porter responses. All Porter requests PRE / CBD return as follows:
{
"result": ...,
"version": "x.y.z"
}Where result could be anything. For example:
- https://docs.nucypher.com/en/latest/application_development/web_development.html#example-response
- https://docs.nucypher.com/en/latest/application_development/web_development.html#id7
- (the above for CBD)
We can definitely rethink it if it feels too redundant.
src/characters/cbd-recipient.ts
Outdated
| const contextStr = await conditionExpr.buildContext(provider).toJson(); | ||
| const { sharedSecrets, encryptedRequests } = this.makeDecryptionRequests( | ||
| this.ritualId, | ||
| variant, | ||
| ciphertext, | ||
| conditionExpr, | ||
| contextStr, | ||
| dkgParticipants | ||
| sufficientDkgParticipants |
There was a problem hiding this comment.
Is this limiting the number of Ursulas that we send the decryption request to, to be only a threshold number?If so, we need a threshold to respond, but we should still send requests to more than the threshold, or else we are expecting no non-responsive Ursulas. Porter already takes care of ensuring a threshold number of responses.
There was a problem hiding this comment.
Agreed - let's call that a robustness enhancement and track it in an issue.
There was a problem hiding this comment.
I see, I think I was thrown off by how Porter handles this. I can remove this reduction of participants in this PR.
|
Documented comments on API design here: #166 (comment) |
|
Bundled size for the package is listed below: build/module/types/ethers-contracts/factories: 82.03 KB |
piotr-roslaniec
force-pushed
the
tdec-poc
branch
from
c837fb1
to
78b3303
Compare
|
Bundled size for the package is listed below: build/module/types/ethers-contracts/factories: 82.03 KB |
Type of PR:
Required reviews:
What this does:
lynxwith: Implement end-to-end tDec flow nucypher-ts-demo-tdec#14nucypher-coreto0.10.0lynxIssues fixed/closed:
Why it's needed:
nucypher-tsup to speed withdkg-dev-7Notes for reviewers:
src/characters/cbd-recipient.ts,src/agents/coordinator.ts, and insrc/characters/porter.ts