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

Add a public key field to the Participant struct #77

Merged

Conversation

piotr-roslaniec
Copy link
Contributor

@piotr-roslaniec piotr-roslaniec commented May 4, 2023

@KPrasch
Copy link
Member

KPrasch commented May 6, 2023

I'm curious what's compelling us to put ferveo public keys onchain specifically? As of nucypher@dkg-dev-2 these keys are known to participants by using P2P/node discovery which can be interpreted as a duplicated effort?

@piotr-roslaniec
Copy link
Contributor Author

piotr-roslaniec commented May 8, 2023

are known to participants by using P2P/node discovery

We also need to make this information ([Transcript, PublicKey] pairs) available to anyone who wants to verify the aggregate (Bob).

It feels somewhat redundant, but it should have the same availability guarantees as Transcript itself.

I'm interested to see whether an alternative design is possible.

@derekpierre
Copy link
Member

We also need to make this information ([Transcript, PublicKey] pairs) available to anyone who wants to verify the aggregate (Bob).

The ability to verify without having to learn about the network is advantageous. Learning is a very involved process. A web-based Bob would be able to verify without needing to learn.

@KPrasch it would also save us (on the Python-side) from needing Ursula objects for this code: https://github.com/nucypher/nucypher/blob/development/nucypher/characters/lawful.py#L696

@piotr-roslaniec
Copy link
Contributor Author

Notes from a discussion on Discord voice:

  • It's not possible to counterfeit (Transcript, PublicKey) pair. And so, we only need to provide DA for the Transcript. PublicKey can provide separately without compromising the security of aggregate verification.
  • Putting PublicKey on-chain is a naive design
  • An alternative design: Make it available through Porter, and when Porter is deprecated, use the same method od PublicKey discovery to communicate directly with nodes

@KPrasch @derekpierre

@KPrasch
Copy link
Member

KPrasch commented Jun 7, 2023

Another consideration here is if we want to derive new keys for each participant+ritual. Otherwise, if nodes only have one long-term ferveo keypair, there's no sense it writing it to the Participant struct.

@piotr-roslaniec
Copy link
Contributor Author

Otherwise, if nodes only have one long-term ferveo keypair, there's no sense it writing it to the Participant struct.

In that case, mapping could be better (updated).

@piotr-roslaniec
Copy link
Contributor Author

I just need 2 approvals so I'm going to ahead and merge if that sounds ok

@piotr-roslaniec piotr-roslaniec force-pushed the participant-pk branch 2 times, most recently from f4ab924 to d38ed40 Compare July 6, 2023 13:17
@@ -30,6 +30,8 @@ contract Coordinator is AccessControlDefaultAdminRules {
event TimeoutChanged(uint32 oldTimeout, uint32 newTimeout);
event MaxDkgSizeChanged(uint16 oldSize, uint16 newSize);

event ParticipantPublicKeySet(address indexed participant, BLS12381.G1Point publicKey);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a G2 point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

@@ -61,10 +63,17 @@ contract Coordinator is AccessControlDefaultAdminRules {
Participant[] participant;
}

struct ParticipantKey {
uint32 ritualId;
Copy link
Member

Choose a reason for hiding this comment

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

Is this more like fromRitualId or ... something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

ParticipantKey memory newRecord = ParticipantKey(lastRitualId, _publicKey);
keysHistory[provider].push(newRecord);

emit ParticipantPublicKeySet(provider, _publicKey);
Copy link
Member

Choose a reason for hiding this comment

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

Is it beneficial to include the ("startiing from") ritual id in the event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could simplify filtering
✔️

emit ParticipantPublicKeySet(provider, _publicKey);
}

function getProviderPublicKey(address _address, uint _ritualId) public view returns (BLS12381.G1Point memory) {
Copy link
Member

Choose a reason for hiding this comment

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

If this function is not called by the other functions in the contract i.e. only called from an external entitiy, I believe using external instead saves a copy step of arguments.

Also, let's call the parameter _provider.

Suggested change
function getProviderPublicKey(address _address, uint _ritualId) public view returns (BLS12381.G1Point memory) {
function getProviderPublicKey(address _provider, uint _ritualId) external view returns (BLS12381.G1Point memory) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

@@ -130,10 +143,22 @@ def test_initiate_ritual(coordinator, nodes, initiator, erc20, flat_rate_fee_mod
assert coordinator.getRitualState(0) == RitualState.AWAITING_TRANSCRIPTS


def test_test_provider_public_key(coordinator, nodes):
Copy link
Member

Choose a reason for hiding this comment

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

Typo?:

Suggested change
def test_test_provider_public_key(coordinator, nodes):
def test_provider_public_key(coordinator, nodes):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

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.

👨🏻‍🚀

Copy link
Member

@cygnusv cygnusv 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 @piotr-roslaniec. An additional idea from a discussion with @KPrasch: what a about adding the ritual public key as part of the post transcript TX?

  • Piggy-backing from the post transcript TX simplifies things, maybe we can make it an optional parameter in case the the node needs to set the key. Less TXs means simpler client code, less problems.
  • Prepares API for when/if Ferveo keys are generated per-ritual.
  • Code definitely would look uglier, but I think this is bearable considering Coordinator is for the private beta release.

If this sounds good, we can do this in a separate PR, no need to do it here.

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.

🎸

@piotr-roslaniec
Copy link
Contributor Author

Documented possible extension in: #95

@piotr-roslaniec piotr-roslaniec merged commit 3eebc89 into nucypher:development Jul 8, 2023
2 checks passed
@piotr-roslaniec piotr-roslaniec deleted the participant-pk branch July 8, 2023 06:58
@jdbertron
Copy link

Another consideration here is if we want to derive new keys for each participant+ritual. Otherwise, if nodes only have one long-term ferveo keypair, there's no sense it writing it to the Participant struct.

I don't follow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants