Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement feedback from integrating tDec flow #227

Merged
merged 7 commits into from
Jun 27, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,163 changes: 565 additions & 598 deletions abi/Coordinator.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
"prebuild": "yarn typechain"
},
"dependencies": {
"@nucypher/nucypher-core": "^0.9.0",
"@nucypher/nucypher-core": "^0.10.0",
"axios": "^0.21.1",
"deep-equal": "^2.2.1",
"ethers": "^5.4.1",
Expand Down
2 changes: 1 addition & 1 deletion src/agents/contracts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const POLYGON: Contracts = {

const MUMBAI: Contracts = {
SUBSCRIPTION_MANAGER: '0xb9015d7b35ce7c81dde38ef7136baa3b1044f313',
COORDINATOR: undefined,
COORDINATOR: '0x0f019Ade1D34399D946CF2f161386362655Dd1A4',
};

const GOERLI: Contracts = {
Expand Down
25 changes: 19 additions & 6 deletions src/agents/coordinator.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { SessionStaticKey } from '@nucypher/nucypher-core';
import { ethers } from 'ethers';

import {
Coordinator,
Coordinator__factory,
} from '../../types/ethers-contracts';
import { BLS12381 } from '../../types/ethers-contracts/Coordinator';
import { fromHexString } from '../utils';

import { getContract } from './contracts';

Expand All @@ -19,26 +21,37 @@ export interface CoordinatorRitual {
aggregatedTranscript: string;
}

export type DkgParticipant = Coordinator.ParticipantStructOutput;
export type DkgParticipant = {
provider: string;
aggregated: boolean;
decryptionRequestStaticKey: SessionStaticKey;
};

export class DkgCoordinatorAgent {
public static async getParticipants(
provider: ethers.providers.Provider,
ritualId: number
): Promise<DkgParticipant[]> {
const Coordinator = await this.connectReadOnly(provider);
// TODO: Remove `as unknown` cast after regenerating the contract types: https://github.com/nucypher/nucypher-contracts/pull/77
return (await Coordinator.getParticipants(
ritualId
)) as unknown as DkgParticipant[];
const participants = await Coordinator.getParticipants(ritualId);

return participants.map((participant) => {
return {
provider: participant.provider,
aggregated: participant.aggregated,
decryptionRequestStaticKey: SessionStaticKey.fromBytes(
fromHexString(participant.decryptionRequestStaticKey)
derekpierre marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +40 to +43
Copy link
Member

Choose a reason for hiding this comment

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

This data is currently relayed from porter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fetched from Coordinator contract

),
};
});
}

public static async getRitual(
provider: ethers.providers.Provider,
ritualId: number
): Promise<CoordinatorRitual> {
const Coordinator = await this.connectReadOnly(provider);
return await Coordinator.rituals(ritualId);
return Coordinator.rituals(ritualId);
}

private static async connectReadOnly(provider: ethers.providers.Provider) {
Expand Down
57 changes: 25 additions & 32 deletions src/characters/cbd-recipient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,9 @@ import {
DecryptionSharePrecomputed,
DecryptionShareSimple,
decryptWithSharedSecret,
DkgPublicParameters,
EncryptedThresholdDecryptionRequest,
EncryptedThresholdDecryptionResponse,
SessionSharedSecret,
SessionStaticKey,
SessionStaticSecret,
ThresholdDecryptionRequest,
} from '@nucypher/nucypher-core';
Expand All @@ -18,17 +16,17 @@ import { DkgCoordinatorAgent, DkgParticipant } from '../agents/coordinator';
import { ConditionExpression } from '../conditions';
import {
DkgRitual,
FerveoVariant,
getCombineDecryptionSharesFunction,
getVariantClass,
} from '../dkg';
import { fromHexString, fromJSON, toJSON } from '../utils';
import { fromJSON, toJSON } from '../utils';

import { Porter } from './porter';

export type CbdTDecDecrypterJSON = {
porterUri: string;
ritualId: number;
dkgPublicParams: Uint8Array;
threshold: number;
};

Expand All @@ -38,27 +36,24 @@ export class CbdTDecDecrypter {
private constructor(
private readonly porter: Porter,
private readonly ritualId: number,
private readonly dkgPublicParams: DkgPublicParameters,
private readonly threshold: number
) {}

public static create(porterUri: string, dkgRitual: DkgRitual) {
return new CbdTDecDecrypter(
new Porter(porterUri),
dkgRitual.id,
dkgRitual.dkgPublicParams,
dkgRitual.threshold
);
}

// Retrieve and decrypt ciphertext using provider and condition set
// Retrieve and decrypt ciphertext using provider and condition expression
public async retrieveAndDecrypt(
provider: ethers.providers.Web3Provider,
conditionExpr: ConditionExpression,
variant: number,
ciphertext: Ciphertext,
aad: Uint8Array
): Promise<readonly Uint8Array[]> {
variant: FerveoVariant,
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻

ciphertext: Ciphertext
): Promise<Uint8Array> {
const decryptionShares = await this.retrieve(
provider,
conditionExpr,
Expand All @@ -69,14 +64,11 @@ export class CbdTDecDecrypter {
const combineDecryptionSharesFn =
getCombineDecryptionSharesFunction(variant);
const sharedSecret = combineDecryptionSharesFn(decryptionShares);

const plaintext = decryptWithSharedSecret(
return decryptWithSharedSecret(
ciphertext,
aad,
sharedSecret,
this.dkgPublicParams
conditionExpr.asAad(),
sharedSecret
);
return [plaintext];
}

// Retrieve decryption shares
Expand All @@ -90,21 +82,25 @@ export class CbdTDecDecrypter {
provider,
this.ritualId
);
// We only need the `threshold` participants
const sufficientDkgParticipants = dkgParticipants.slice(0, this.threshold);
Copy link
Member

Choose a reason for hiding this comment

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

This can be hardcoded as shares // 2 + 1 to match nucypher/nucypher (can be in another PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we don't want to support tunable threshold any time soon? Taking notes for API design.

const contextStr = await conditionExpr.buildContext(provider).toJson();
const { sharedSecrets, encryptedRequests } = this.makeDecryptionRequests(
this.ritualId,
variant,
ciphertext,
conditionExpr,
contextStr,
dkgParticipants
sufficientDkgParticipants
Copy link
Member

@derekpierre derekpierre Jun 27, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed - let's call that a robustness enhancement and track it in an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, I think I was thrown off by how Porter handles this. I can remove this reduction of participants in this PR.

);

const { encryptedResponses, errors } = await this.porter.cbdDecrypt(
encryptedRequests,
this.threshold
);

// 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) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only need to check for presence of any errors then

Copy link
Member

Choose a reason for hiding this comment

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

No. Errors can be reported even if the overall operation was successful i.e. some Ursula's may fail while waiting for sufficient responses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)}`
      );
    }

Copy link
Member

Choose a reason for hiding this comment

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

👍 - Correct.

With a message something like..."Threshold of responses not met; CBD decryption failed with errors..."

throw new Error(
`CBD decryption failed with errors: ${JSON.stringify(errors)}`
Expand All @@ -124,9 +120,14 @@ export class CbdTDecDecrypter {
variant: number,
expectedRitualId: number
) {
const decryptedResponses = Object.entries(encryptedResponses).map(
([ursula, encryptedResponse]) =>
encryptedResponse.decrypt(sessionSharedSecret[ursula])
const decryptedResponses = Object.entries(sessionSharedSecret).map(
([ursula, sharedSecret]) => {
const encryptedResponse = encryptedResponses[ursula];
if (!encryptedResponse) {
throw new Error(`Missing encrypted response from ${ursula}`);
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

return encryptedResponse.decrypt(sharedSecret);
}
);

const ritualIds = decryptedResponses.map(({ ritualId }) => ritualId);
Expand Down Expand Up @@ -171,10 +172,9 @@ export class CbdTDecDecrypter {
const sharedSecrets: Record<string, SessionSharedSecret> =
Object.fromEntries(
dkgParticipants.map(({ provider, decryptionRequestStaticKey }) => {
const decKey = SessionStaticKey.fromBytes(
fromHexString(decryptionRequestStaticKey)
const sharedSecret = ephemeralSessionKey.deriveSharedSecret(
decryptionRequestStaticKey
Comment on lines +167 to +168
Copy link
Member

Choose a reason for hiding this comment

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

very nice

);
const sharedSecret = ephemeralSessionKey.deriveSharedSecret(decKey);
return [provider, sharedSecret];
})
);
Expand Down Expand Up @@ -205,7 +205,6 @@ export class CbdTDecDecrypter {
return {
porterUri: this.porter.porterUrl.toString(),
ritualId: this.ritualId,
dkgPublicParams: this.dkgPublicParams.toBytes(),
threshold: this.threshold,
};
}
Expand All @@ -217,15 +216,9 @@ export class CbdTDecDecrypter {
public static fromObj({
porterUri,
ritualId,
dkgPublicParams,
threshold,
}: CbdTDecDecrypterJSON) {
return new CbdTDecDecrypter(
new Porter(porterUri),
ritualId,
DkgPublicParameters.fromBytes(dkgPublicParams),
threshold
);
return new CbdTDecDecrypter(new Porter(porterUri), ritualId, threshold);
Copy link
Member

Choose a reason for hiding this comment

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

(in another PR) CbdTDecDecrypter -> ThresholdDecrypter || Bob || something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, suggestions for naming are welcome

}

public static fromJSON(json: string) {
Expand Down
6 changes: 5 additions & 1 deletion src/characters/enrico.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,15 @@ export class Enrico {
withConditions = this.conditions;
}

if (!withConditions) {
throw new Error('Conditions are required for CBD encryption.');
}
Comment on lines +60 to +62
Copy link
Member

Choose a reason for hiding this comment

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

Agree! This is a major differentiating factor between the PRE-adapted-Tdec and the real one.


if (!(this.encryptingKey instanceof DkgPublicKey)) {
throw new Error('Wrong key type. Use encryptMessagePre instead.');
}

const aad = toBytes(withConditions?.toJson() ?? '');
const aad = withConditions.asAad();
const ciphertext = ferveoEncrypt(
plaintext instanceof Uint8Array ? plaintext : toBytes(plaintext),
aad,
Expand Down
21 changes: 17 additions & 4 deletions src/characters/porter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { Base64EncodedBytes, ChecksumAddress, HexEncodedBytes } from '../types';
import { fromBase64, fromHexString, toBase64, toHexString } from '../utils';

// /get_ursulas

export type Ursula = {
readonly checksumAddress: ChecksumAddress;
readonly uri: string;
Expand All @@ -40,6 +41,7 @@ export type GetUrsulasResult = {
};

// /retrieve_cfrags

type PostRetrieveCFragsRequest = {
readonly treasure_map: Base64EncodedBytes;
readonly retrieval_kits: readonly Base64EncodedBytes[];
Expand Down Expand Up @@ -79,8 +81,15 @@ type PostCbdDecryptRequest = {
};

type PostCbdDecryptResponse = {
encrypted_decryption_responses: Record<ChecksumAddress, Base64EncodedBytes>;
errors: Record<ChecksumAddress, string>;
result: {
decryption_results: {
encrypted_decryption_responses: Record<
ChecksumAddress,
Base64EncodedBytes
>;
Comment on lines +85 to +89
Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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:

We can definitely rethink it if it feels too redundant.

errors: Record<ChecksumAddress, string>;
};
};
};

export type CbdDecryptResult = {
Expand Down Expand Up @@ -174,8 +183,12 @@ export class Porter {
new URL('/cbd_decrypt', this.porterUrl).toString(),
data
);

const { encrypted_decryption_responses, errors } =
resp.data.result.decryption_results;

const decryptionResponses = Object.entries(
resp.data.encrypted_decryption_responses
encrypted_decryption_responses
).map(([address, encryptedResponseBase64]) => {
const encryptedResponse = EncryptedThresholdDecryptionResponse.fromBytes(
fromBase64(encryptedResponseBase64)
Expand All @@ -186,6 +199,6 @@ export class Porter {
string,
EncryptedThresholdDecryptionResponse
> = Object.fromEntries(decryptionResponses);
return { encryptedResponses, errors: resp.data.errors };
return { encryptedResponses, errors };
}
}
2 changes: 1 addition & 1 deletion src/conditions/compound-condition.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import Joi from 'joi';

import { Condition } from './base/condition';
import { Condition } from './base';
import { contractConditionSchema } from './base/contract';
import { rpcConditionSchema } from './base/rpc';
import { timeConditionSchema } from './base/time';
Expand Down
6 changes: 5 additions & 1 deletion src/conditions/condition-expr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Conditions as WASMConditions } from '@nucypher/nucypher-core';
import { ethers } from 'ethers';
import { SemVer } from 'semver';

import { objectEquals, toJSON } from '../utils';
import { objectEquals, toBytes, toJSON } from '../utils';

import {
Condition,
Expand Down Expand Up @@ -90,6 +90,10 @@ export class ConditionExpression {
return new ConditionContext([this.condition], provider);
}

public asAad(): Uint8Array {
return toBytes(this.toJson());
}

public equals(other: ConditionExpression): boolean {
return (
this.version === other.version &&
Expand Down
2 changes: 1 addition & 1 deletion src/conditions/context/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Conditions as WASMConditions } from '@nucypher/nucypher-core';
import { ethers } from 'ethers';

import { fromJSON, toJSON } from '../../utils';
import { Condition } from '../base/condition';
import { Condition } from '../base';
import { USER_ADDRESS_PARAM } from '../const';

import { TypedSignature, WalletAuthenticationProvider } from './providers';
Expand Down
Loading
Loading