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

Alter PRE-adapted tDec API design for CBD-DKG #166

Closed
piotr-roslaniec opened this issue Feb 1, 2023 · 30 comments
Closed

Alter PRE-adapted tDec API design for CBD-DKG #166

piotr-roslaniec opened this issue Feb 1, 2023 · 30 comments
Assignees

Comments

@piotr-roslaniec
Copy link
Contributor

piotr-roslaniec commented Feb 1, 2023

@piotr-roslaniec piotr-roslaniec changed the title Sktach an API design for the inclusion of ferveo into the CBD design Sketch an API design for the inclusion of ferveo into the CBD design Feb 1, 2023
@theref
Copy link
Contributor

theref commented Feb 23, 2023

Ritual Initiation

We already have Cohorts and Strategies so this will stay unchanged!

import { Cohort, Strategy } from '@nucypher/nucypher-ts';

const config = {
  threshold: 3,
  shares: 5,
  porterUri: 'https://porter-tapir.nucypher.community',
};
const newCohort = await Cohort.create(config);
const newStrategy = Strategy.create(newCohort);

import detectEthereumProvider from '@metamask/detect-provider';
import { providers } from 'ethers';

const MMprovider = await detectEthereumProvider();
const mumbai = providers.getNetwork(80001);

const web3Provider = new providers.Web3Provider(MMprovider, mumbai);
const newDeployed = await newStrategy.deploy('test', web3Provider);

The background work will be very different though. newStrategy.deploy needs to call initiateRitual on the Coordinator contract.

This also means that newStrategy will be unusable until the ritual has completed successfully (this is similar to having to deploy the strategy previously, however the await will now take much longer. Therefore the Strategy class probably needs to have a getStatus function that returns one of:

enum RitualState {
    NON_INITIATED,
    AWAITING_TRANSCRIPTS,
    AWAITING_AGGREGATIONS,
    TIMEOUT,
    INVALID,
    FINALIZED
}

Strategy class will also contain the ritualID, this should probably be exposed.

@theref
Copy link
Contributor

theref commented Feb 23, 2023

Another option for Ritual Initiation...

We don't handle it within the strategy because this could become restrictive. Instead we just give detailed examples of how to achieve the functionality with standard tools (ethersjs, hardhat, truffle, etc):

import { Cohort, CoordinatorABI, CoordinatorAddress } from '@nucypher/nucypher-ts';

const config = {
  threshold: 3,
  shares: 5,
  porterUri: 'https://porter-tapir.nucypher.community',
};
const newCohort = await Cohort.create(config);
const nodes = newCohort.ursulaAddresses;

import { ethers } from "ethers";
const provider = new ethers.providers.Web3Provider(window.ethereum);
const coordinatorContract = new ethers.Contract(CoordinatorAddress, CoordinatorABI, provider);

// currently only connected via provider which is read-only, we need to connect using the signer
const signer = provider.getSigner();
const CoordinatorWithSigner = coordinatorContract.connect(signer);

// initiate ritual
tx = await CoordinatorWithSigner.initiateRitual(nodes);

There are similar questions regarding PSS and how that status is handled.

@theref
Copy link
Contributor

theref commented Feb 27, 2023

Another question... Cohort and Strategy are really just configuration objects, but which should handle the interface to the DKG ritual? In the comments above I outlined an example where it was contained within Strategy, but there's an argument that it should be part of the Cohort.

Thus const newCohort = await Cohort.create(config); would be calling initiateRitual on the Coordinator contract. A Strategy would no longer need to be deployed, so whilst it would be a breaking api change, the overall flow would be simpler. At this point a reasonable question become, what is the point of Strategy then? are there other configuration options that it should contain? (default conditions is a good example).

@piotr-roslaniec
Copy link
Contributor Author

Currently, Cohort represents a set of nodes that may be re-used in multiple CBD strategies. The same remains true for rituals - we could have a bunch of hand-picked nodes from which we draw ritual participants. But in order to make a decision on that design we need to have a better understanding of how node sampling is supposed to work in DKG.

@arjunhassard arjunhassard changed the title Sketch an API design for the inclusion of ferveo into the CBD design Finalize API design for the inclusion of ferveo into CBD Apr 18, 2023
@arjunhassard arjunhassard changed the title Finalize API design for the inclusion of ferveo into CBD Alter PRE-adapted tDec API design for CBD-DKG May 4, 2023
@arjunhassard
Copy link
Member

arjunhassard commented May 4, 2023

@piotr-roslaniec
Copy link
Contributor Author

Marking this issue as a blocker for #197

Need further clarity on the design of CBD API in nucypher-ts:

  • Shared or separate interfaces in Strategy
  • Design of the following user flows: ritual initialization; aggregate verification; fetching DKG public key

@piotr-roslaniec
Copy link
Contributor Author

This issue is updated by changes in #210

Introduces a separate CbdStrategy and PreStrategy with their corresponding sub-types as an initial draft of the new API

@KPrasch
Copy link
Member

KPrasch commented Jun 14, 2023

This issue can now be understood as two separate efforts:

  1. Remove PRE-Adapted-tDec
  2. Design & Implement CBD API

@piotr-roslaniec
Copy link
Contributor Author

Notes from #227

  • Consider hiding the threshold parameter user API since it has been hardcoded to shares // 2 + 1 in nucypher (for the time being)
  • Rename CbdTDecDecrypter etc., to something less descriptive: ThresholdDecrypter, Bob, etc.,
  • Allow usage of existing rituals in Strategy. Support ritualId as a Strategy parameter, or explore an alternative design.

@derekpierre
Copy link
Member

Some of my notes from observations made while working with nucypher-ts:

@piotr-roslaniec
Copy link
Contributor Author

piotr-roslaniec commented Jun 29, 2023

Addressed some comments

There are some outstanding talking points related to this issue. Form discussion on Discord:

  1. Renaming of the nucypher-ts repository
  2. Naming of the NuCypher product suite
  3. Deprecating of PRE API in nucypher-ts (TBD)
  4. Refactoring PRE API into a separate package under nucypher-ts in the monorepo (TBD)
  5. Continued support for PRE post-CBD (TBD)
  6. Making high-level decisions and creating a timeline for 3), 4), 5)
  7. Low-level naming decisions and low-level API design (Strategy vs Policy)

@theref
Copy link
Contributor

theref commented Jul 8, 2023

Some more thought after chats with @arjunhassard @derekpierre @cygnusv @manumonti

How do we feel about removing some of the objects and taking a more functional approach? Those objects were required before because we carried around state, but that's no longer the case.

We're thinking something crazy simple along the lines of:

import taco

public_key = # read from Coordinator contract
conditions = # build some conditions
ciphertext = taco.encrypt("plaintext", public_key, conditions, porter_uri)
evidence = # generate signature, zk proof, whatever
plaintext = taco.decrypt(ciphertext, ritual_id, evidence, porter_uri)

I'm aware that this looks like python not ts, but the idea is the same

@piotr-roslaniec
Copy link
Contributor Author

piotr-roslaniec commented Jul 10, 2023

How do we feel about removing some of the objects and taking a more functional approach?

Sounds good to me. Are we still interested in making some of the protocol objects compatible with PRE?

Additional thoughts: Do we want to rely on configuration objects, like Cohort? Or do we hide them behind ritual initiation and refer to them using ritual_id? This question probably extends to at least a couple of other places.

# How do we smuggle cohort config here?
conditions = # Do we plug it into a condition?
ciphertext = taco.encrypt("plaintext", public_key, conditions, porter_uri, ...) # Or as a parameter here?

@theref
Copy link
Contributor

theref commented Jul 18, 2023

Do you need to know the cohort here? or can you just encrypt with the public_key?

But yeah, there's definitely still some state around...

@cygnusv
Copy link
Member

cygnusv commented Jul 20, 2023

From my ETHBCN notes (sorry I didn't share this earlier, I thought I did 🙈 ):

Encrypt

Either the creator is blockchainy (which gives them access to the public key from the ritualID by querying the Coordinator):

    import { taco } from "@thresold-network/taco";
    messageKit = taco.encrypt(message, ritualId, web3Provider, conditions)

or not (i.e., the creator must know the public key):

    import { taco } from "@thresold-network/taco";
    messageKit = taco.encrypt(message, publicKey, conditions)

Decrypt

    import { taco } from "@thresold-network/taco";
    taco.decrypt(messageKit, porterURI, web3Provider)

## Notes

  • MessageKit = CTXT + ACP
  • user-defined context variables are not common
  • ritualID is part of the ACP
  • variant is hidden, there's a default to simple

@cygnusv
Copy link
Member

cygnusv commented Jul 25, 2023

Potential iteration based on comments from discussion with @ghardin1314 @derekpierre @jMyles. Mayor changes in debate here:

  • Increasing Porter's role: The discussion here is whether it's ok delegating a bit more work to Porter, both for encryption and decryption, at the expense of increasing the centralizing force of Porter (although everyone can use their own Porter, of course). Depending on Porter's set up, a browser user trading the web3 provider for Porter can have a better experience (less network roundtrips if Porter has in turn a local web3 provider) or worse (if Porter incurs in external calls)
  • Provider vs Signer: Separation between RPC read calls (the "provider") and signing operations (the "signer"), in line with the nucypher codebase (with the BlockchainInterface / Signer separation) and some JS libraries like Viem. This could even allow in the future for non-wallet signing approaches (e.g. signing with keypairs that are not mapped to blockchain accounts).

Encrypt

3 possible approaches:

  • Blockchainy creator: Access to the public key from the ritualID by querying the Coordinator directly via a web3 provider. Optional signer in case it's necessary for authenticating the creator.
    import { taco } from "@thresold-network/taco";
    messageKit = taco.encrypt(message, conditions, ritualId, web3Provider, [signer])
  • Ported creator: Access to the public key from the ritualID by querying the Coordinator via Porter. Optional signer in case it's necessary for authenticating the creator.
    import { taco } from "@thresold-network/taco";
    messageKit = taco.encrypt(message, conditions, ritualId, porterURI, [signer])
  • Lightweight creator: If creator know the public key, we can make it completely independent from on-chain information (assuming the adapt the signing message for that, e.g., not relying on block time but local time). Optional signer in case it's necessary for authenticating the creator.
    import { taco } from "@thresold-network/taco";
    messageKit = taco.encrypt(message, conditions, publicKey, [signer])

Decrypt

    import { taco } from "@thresold-network/taco";
    message = taco.decrypt(messageKit, porterURI, [signer])
  • For the moment, let's think only on Porter-based solutions, since we can't directly connect to nodes with self-signed certificates from the browser.
  • Following the approach discussed earlier, if we require Porter, we may as well use it as the web3 endpoint as well.
  • Optional signer in case it's necessary for authenticating the consumer (which is almost always the case, except for conditions not based in accounts like timelocks)

Thoughts?

@derekpierre
Copy link
Member

derekpierre commented Jul 25, 2023

Thanks for posting this @cygnusv 🎸 !

3 possible approaches:

The approaches do not have to be exclusive, and providing more than one encrypt function is not a bad thing. 1) and 3) are pretty natural based on our API thus far.

Porter creator: Access to the public key from the ritualID by querying the Coordinator via Porter.

Just of note: at this current moment, Porter only connects to the Ethereum network and does not connect to the Polygon network at all, since it never had to previously. Of course, we control the code, and that can be changed if desired.

@derekpierre
Copy link
Member

I guess, if we are providing 3) which is the lowest common function (and provides the most flexibility), then we would need to provide an API for getting the public key from a ritual Id...?

If we are, is that functional or Object-oriented, and does that remove the need for 1)? 1) would just be a convenience method at that point.

If functional, something like:

publicKey = taco.getPublicKey(ritualId, web3Provider)

Or, if object-oriented then something like:

coordinator = new Coordinator(web3Provider)
publicKey = coordinator.getPublicKey(ritualId)
...

There may be other methods on the object, like getRitual(ritualId), perhaps getThreshold(ritualId)...?

Thinking out loud here.

@ghardin1314
Copy link
Contributor

Just of note: at this current moment, Porter only connects to the Ethereum network and does not connect to the Polygon network at all, since it never had to previously. Of course, we control the code, and that can be changed if desired.

Not sure exactly the full logic of the signature validation on the Ursula side (do they make an RPC call to verify the block number/hash used?) but does the signature really need to be chain specific? Can all :userAddress verification signatures be against eth mainnet?

@derekpierre
Copy link
Member

derekpierre commented Jul 25, 2023

Not sure exactly the full logic of the signature validation on the Ursula side (do they make an RPC call to verify the block number/hash used?) but does the signature really need to be chain specific? Can all :userAddress verification signatures be against eth mainnet?

Here's the verification function on the Ursula side - https://github.com/nucypher/nucypher/blob/development/nucypher/policy/conditions/context.py#L30. It's basically an eth_account.recover_message(...) call.

For signing the chain id is included in the domain section as part of the EIP712 spec (as it pertains to replay attack). See Definition of domainSeparator section - https://eips.ethereum.org/EIPS/eip-712.

@ghardin1314
Copy link
Contributor

For signing the chain id is included in the domain section as part of the EIP712 spec (as it pertains to replay attack). See Definition of domainSeparator section - https://eips.ethereum.org/EIPS/eip-712.

True, but the chainId is mostly used when verifying the signature onchain to avoid cross chain replays. Since this is purely offchain signature verification, it does not really matter what the chainId is (or if there even is one)

@derekpierre
Copy link
Member

True, but the chainId is mostly used when verifying the signature onchain to avoid cross chain replays. Since this is purely offchain signature verification, it does not really matter what the chainId is (or if there even is one)

Yep it doesn't really matter, but it does prevent reuse of the signature for other chains - forcing a new signing; unless that's the thing you are looking to avoid...

@ghardin1314
Copy link
Contributor

Yep it doesn't really matter, but it does prevent reuse of the signature for other chains - forcing a new signing; unless that's the thing you are looking to avoid...

Are you actually checking that anywhere though? Like if the Condition is an EvmCondition involving polygon, will Ursula throw an error if my signed message is against Eth mainnnet?

Also how does this work with cross/multi chain conditions (which is think are supported or planned to be?) Does Ursula need to verify a signature against each chain with a corresponding Condition?

@derekpierre
Copy link
Member

Are you actually checking that anywhere though? Like if the Condition is an EvmCondition involving polygon, will Ursula throw an error if my signed message is against Eth mainnnet?

I haven't dug into the eth_account recover_message call but you're probably right that it may not be checked. I guess from an eth signing perspective, the chain doesn't matter with respect to wallet ownership - it's all the same.

@ghardin1314
Copy link
Contributor

I haven't dug into the eth_account recover_message call but you're probably right that it may not be checked. I guess from an eth signing perspective, the chain doesn't matter with respect to wallet ownership - it's all the same.

From the link you sent, it looks like the Ursula only checks to see if the signature itself is valid and has no validation about the other parameters passed in. In reality, this is no better than just using a random salt to sign and verify against.

Although I'm not sure the extra step of validating the block number/hash is really work the RPC call in this case. Either way, it doesnt seem to me that the signature validation really needs to match the chain in which the conditions refer to

@piotr-roslaniec
Copy link
Contributor Author

3 possible approaches

We can expose a simplified API using existing primitives:

export interface TacoMessageKit {
  ciphertext: Ciphertext;
  aad: Uint8Array;
  decrypter: ThresholdDecrypter;
  conditions: ConditionExpression;
}

export const encrypt = async (
  message: string,
  conditions: ConditionExpression,
  ritualId: number,
  web3Provider: ethers.providers.Web3Provider
): Promise<TacoMessageKit> => {
  const cohort = await makeCohort([]);
  const strategy = CbdStrategy.create(cohort);
  const deployedStrategy = await strategy.deploy(web3Provider, ritualId);
  const { ciphertext, aad } = deployedStrategy
    .makeEncrypter(conditions)
    .encryptMessageCbd(message);

  return {
    ciphertext,
    aad,
    decrypter: deployedStrategy.decrypter,
    conditions,
  };
};

export const decrypt = async (
  messageKit: TacoMessageKit,
  web3Provider: ethers.providers.Web3Provider
): Promise<Uint8Array> => {
  return await messageKit.decrypter.retrieveAndDecrypt(
    web3Provider,
    messageKit.conditions,
    FerveoVariant.simple,
    messageKit.ciphertext,
    false,
  );
};

export const taco = {
  encrypt,
  decrypt,
};

Then, we can selectively expose them if needed:

import { Strategy } from "@thresold-network/core"; // common? shared?

Alternatively, we can eliminate those primitives either during refactoring to a more straightforward taco API or later.

@ghardin1314
Copy link
Contributor

That looks pretty good to me generally. Its been a week or so since I looked at this, do you need to deploy a new strategy every time you encrypt something? What exactly are you deploying here? And how would you encrypt using an existing strategy?

@piotr-roslaniec
Copy link
Contributor Author

piotr-roslaniec commented Aug 7, 2023

Strategy is "just a wrapper" on a ritual that produces an encrypter and a decrypter. When we run Strategy.deploy without the ritualId parameter, we start a new ritual. When we use a ritualId, we re-use an existing ritual.

do you need to deploy a new strategy every time you encrypt something?

We don't have to deploy a new strategy every time we encrypt something, we can re-use our ritual.

What exactly are you deploying here?

What we're deploying is a new DKG ritual. In case when we're re-using the existing ritual, we still want to run some checks in balances to validate its correctness. Perhaps the notion of "deploying" is not the best description. Or we could use another verb in case of re-using a ritual.

And how would you encrypt using an existing strategy?

To encrypt using an existing strategy, re-use ritualId.

The code example I shared is just for illustratory purposes, i.e. to show how we could implement that today without changes to the structure of the existing API. I want to iterate on it before we settle on the final design.

@ghardin1314
Copy link
Contributor

Ah gotcha, I do think deploy might be a bit misleading here as I associate it with writing something to the blockchain. Other than that I think it looks great

@piotr-roslaniec
Copy link
Contributor Author

Closed by #263

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Completed
Development

No branches or pull requests

7 participants