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

CIP-0005, CIP-0036 | Added clarity to voting key definitions #427

Merged

Conversation

Ryun1
Copy link
Collaborator

@Ryun1 Ryun1 commented Jan 12, 2023

Motivation

  • Concerns were raised around the definition of CIP-36's voting keys and how these should be represented in supporting tooling (good spot from @janmazak and @gitmachtl).
  • Avoid confusion with current and future protocol governance.

Proposed Changes

CIP-05

CIP-36

  • Added clarity for voting / vote key terminology. Linking to CIP-05 prefixes.
  • Gave examples of potential naming supporting tooling could use.
  • 4f6b879: Added preference for implementations to use term "vote".
  • 825d54d: Added more example keyTypes with descriptions, as per gitmachtl's suggestion.
  • 825d54d: Added explanation for how other projects can add themselves to current keyType descriptions, as per gitmachtl's suggestion.

Rationale

With these changes we are trying to avoid confusion of CIP-36's vote keys with current and future protocol governance keys and processes.

CIP-05

  • The previously added gov_sk and gov_vk which were intended to relate to such keys, are likely to cause confusion, so I have added new prefixes.
  • By leaving gov_sk and gov_vk - we intend to reserves these prefixes for potential use in the future protocol governance.
  • 825d54d: By leaving vote_sk and vote_vk - we intend to reserves these prefixes for potential use in the future protocol governance.

CIP-36

  • By associating both the terms vote and voting, we attempt to avoid future confusion by not allowing other standards to utilize one term.
  • Adding explicit keyType's was the level of clarity that was requested, I believe.
  • 825d54d: By encouraging the use of terms such as "CIP-36 vote keys" we leave the base "vote key" for use by future standards.
  • 825d54d: Adding a fuller list of keyType with descriptions better satisfied the concerns raised in comments on this PR.
  • 825d54d: By allowing future projects to update keyType descriptions we cater for future use of these CIP-36 vote keys. This was suggested by gitmachtl here.

Note: Submitted as one PR so the related changes are kept together, but if its preferable to have 2 PR's please advise.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

Looks like an essential improvement to me, as far as I can understand it.

cc @kevinhammond @zeegomo @KtorZ @mark-stopka

@rphair rphair added the Update Adds content or significantly reworks an existing proposal label Jan 12, 2023
@rphair rphair requested a review from KtorZ January 12, 2023 16:04
### Voting key derivation path
### Voting key

The terms voting keys and vote keys should be used interchangeably to indicate the keys described in this specification. The term governance should not be associated with these keys.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the vote/voting dichotomy worth it? For talking about in natural language it's OK, but for code it's not great if you have two ways of naming the same thing. So in the section "Tooling" I'd propose to only have one thing, or one of them strongly preferred (I'd go for the shorter version if other reasons are absent).

Otherwise, anyone working with two tools that use different names, will have to choose one of the ways, and then his code will be inconsistent with the other tool (and everyone dealing with that code will have to wonder "why are we using vote while the tool uses voting ". And people will needlessly get stupid bugs when integrating tools/libraries and mistakenly use one name instead of the other.

[A mess of this kind was created by reward account / reward address / stake address / staking address for that particular type of Cardano address. Every time someone new comes to the ecosystem, he has to think about whether these different names refer to the same thing or not, and ask around for clarification.]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@janmazak

Thanks for your suggestion - you raise a good point, I agree. I will change the wording to prefer the term "vote" for tooling.

@gitmachtl
Copy link
Contributor

As talked in the Discord-Chat, we should also add VoteExtendedSigningKey_ed25519 as a possible key-type for the json key-format. Because its an extended key if derived from path 1694H/1815H/0H/0/0 BIP32 with BIP39 mnemonic, if not a simple ed25519 keypair is used.

So currently we would have the following key types for the json format:

type description
VoteVerificationKey_ed25519 Vote Verification Key
VoteSigningKey_ed25519 Vote Signing Key
VoteExtendedSigningKey_ed25519 Vote Signing Key

The hardware related key/files via cardano-hw-cli would than be:

type description
VoteVerificationKey_ed25519 Hardware Vote Verification Key
VoteHWSigningFile_ed25519 Hardware Vote Signing File

@disassembler
Copy link
Contributor

I agree with the thought we want to have types/description to identify these key types so you can use them across multiple tools and don't shoot yourself in the foot, however; I think we should be very specific in the purpose for the keys rather than using a generic "Vote" context. Ideally, keys used by Catalyst would have a Catalyst Prefix and keys used for on-chain would have a separate prefix (e.g. DRep), and the the derivation paths would not be the same for security reasons. It's a bad idea in my opinion to use the same keys for multiple purposes. This is primarily why we created the type system in the first place to prevent people from accidentally using a payment key as a cold key for their stake pool. While it is possible at a cryptographic level, we wanted people to not accidentally do it.

@gitmachtl
Copy link
Contributor

gitmachtl commented Jan 14, 2023

Going with the suggestion of @disassembler, we could do the following than for catalyst voting specific:

Catalyst Voting Keys should be derived from path 1694H/1815H/0H/0/0 to be "real" voting keys according to CIP-36.

CLI based .vkey/.skey json format:

type description bech-prefix
CatalystVoteVerificationKey_ed25519 Catalyst Vote Verification Key cvote_vk
CatalystVoteExtendedVerificationKey_ed255191 Catalyst Vote Verification Key cvote_vk
CatalystVoteSigningKey_ed25519 Catalyst Vote Signing Key cvote_sk
CatalystVoteExtendedSigningKey_ed25519 Catalyst Vote Signing Key cvote_sk

1: Not sure if we need an extended version here, tools can strip the last bytes of an extended one and the result would be a normal verification key. But maybe just specify it to have a complete set.

The hardware related .vkey/.hwsfile via cardano-hw-cli in json format:

type description bech-prefix
CatalystVoteVerificationKey_ed25519 Hardware Catalyst Vote Verification Key cvote_vk
CatalystVoteHWSigningFile_ed25519 Hardware Catalyst Vote Signing File -

@janmazak
Copy link
Contributor

@gitmachtl @disassembler
HW wallets can't distinguish keys. There's the same API and the same code to process general voting keys and catalyst keys, and the keys are given from the outside, a HW wallet has no idea if it is catalyst or not. So it cannot choose the appropriate prefix (vote_vk/cvote_vk). The prefix cannot be given together with the key, that would be just a security hazard (a HW wallet must only display info that it understands / can verify, everything else comes with a big warning and is displayed as-is in raw form, and under no circumstances wrong info should be displayed, e.g. a key with a false prefix).

On the other hand, it's possible to make the distinction on the level of cardano-hw-cli, but the cli tool must know what is catalyst and what is not --- so far the key derivation path is the same AFAIK, so it would have to just be told when writing keys obtained from a HW wallet to a file.

@gitmachtl
Copy link
Contributor

gitmachtl commented Jan 15, 2023

we can for now change the gov_vk in the cardano-app into cvote_vk right? we have the voting_purpose indicator, that is 0 for catalyst. would it be possible to just do a simple kind of

case DELEGATION_KEY: {
		STATIC_ASSERT(SIZEOF(subctx->stateData.delegation.votingPubKey) == GOVERNANCE_VOTING_PUBLIC_KEY_LENGTH, "wrong voting public key size");

 		switch (subctx->stateData.votingPurpose) {

 		     case 0:  
        		ui_displayBech32Screen(
        		        "Voting public key",
        		        "cvote_vk",
        		        subctx->stateData.delegation.votingPubKey, GOVERNANCE_VOTING_PUBLIC_KEY_LENGTH,
        		        callback
        		);
 		        break;
 		     
 		     default: 
        		ui_displayBech32Screen(
        		        "Voting public key",
        		        "ed25519_vk",
        		        subctx->stateData.delegation.votingPubKey, GOVERNANCE_VOTING_PUBLIC_KEY_LENGTH,
        		        callback
        		);
 		        break;
 		     
 		}

	break;
	}

in the cardano-app ?

as we have only catalyst for now, i think we can code it for that atm right?

@janmazak
Copy link
Contributor

@gitmachtl No. The voting purpose comes in the last APDU, so it is not known at the moment delegations are processed. (This could be changed.)

@Ryun1
Copy link
Collaborator Author

Ryun1 commented Jan 16, 2023

@disassembler @gitmachtl @janmazak

Thanks everyone for your insights on this, I really appreciate the input.

I see two options here:

  1. Follow Catalyst naming.
  2. Follow generic CIP-36 naming.

I believe intent of CIP-36's design was to create a generic governance standard which could be used for different projects. For this, a derivation path was chosen for keys so that these keys could be used across projects. So although Catalyst is currently the only user of 1694H/1815H/0H/0/0 this does not stop other projects from using 1694H/1815H/0H/0/0 in their use of CIP-36. Remember to do this other projects would have to register this intent in this CIP and their delegations/registrations transactions would be distinguished from Catalysts' by the voting_purpose field. I believe one of the benefits of voting_purpose is that it allows for the reuse of the same "vote" keys across projects.

With this in mind and reading everyone's thoughts I see two options; 1. We name these keys after Catalyst, which does make sense as Catalyst is the only current implementer. This does not stop other projects from using these same keys in the future as per CIP-36's design. 2. We name these keys after the CIP itself, making these keys not tied to Catalyst which may make this standard more attractive to other projects at the risk of making things confusing.

@gitmachtl
Copy link
Contributor

gitmachtl commented Jan 16, 2023

@Ryun1 @disassembler @janmazak

ok, so if we go with your type suggestion @Ryun1 but keep the purpose also in the description, we could do it like:

CLI based .vkey/.skey json format:

type description bech-prefix
CIP36VoteVerificationKey_ed25519 Catalyst Vote Verification Key cvote_vk
CIP36VoteExtendedVerificationKey_ed25519 Catalyst Vote Verification Key cvote_vk
CIP36VoteSigningKey_ed25519 Catalyst Vote Signing Key cvote_sk
CIP36VoteExtendedSigningKey_ed25519 Catalyst Vote Signing Key cvote_sk

The hardware related .vkey/.hwsfile via cardano-hw-cli in json format:

type description bech-prefix
CIP36VoteVerificationKey_ed25519 Hardware Catalyst Vote Verification Key cvote_vk
CIP36VoteHWSigningFile_ed25519 Hardware Catalyst Vote Signing File -

So that it is clear by the type how the keys are derived (via CIP36 specification), but the usecase for a specific purpose is reflected in the description. And the bech prefix is a generic (cardano) cvote_vk/sk. So if other vote-purposes come alone in the future, we simply add another description.

Tools like hw-cli, or the spo scripts, cntools, can write out the files with the right type & description depending on the purpose/path. HW-Wallets would only show the generic cvote_ prefixed bech string for the voting keys.

@JaredCorduan what do you think?

@rphair rphair changed the title CIP-0005, CIP-0036 | Added clarity to voting key defintions CIP-0005, CIP-0036 | Added clarity to voting key definitions Jan 16, 2023
@JaredCorduan
Copy link
Contributor

JaredCorduan commented Jan 16, 2023

@JaredCorduan what do you think?

I think what you have proposed sounds great! I appreciate the effort to reduce any confusion that could arise from the voting keys used in Catalyst and those related to CIP-1694.

@rphair
Copy link
Collaborator

rphair commented Jan 16, 2023

FYI @JaredCorduan @gitmachtl @Ryun1 @janmazak this is on the CIP Meeting agenda for tomorrow & when @KtorZ is also there we can assess whether there is a consensus about #427 (comment), and if so then we could be ready to approve & merge this (cc @disassembler).

@gitmachtl
Copy link
Contributor

FYI @JaredCorduan @gitmachtl @Ryun1 @janmazak this is on the CIP Meeting agenda for tomorrow & when @KtorZ is also there we can assess whether there is a consensus about #427 (comment), and if so then we could be ready to approve & merge this (cc @disassembler).

sounds pretty good to me :-)

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

With unilateral approval after the last discussion & changes it looks like this is ready to merge at today's CIP meeting.

Ryun1 pushed a commit to Ryun1/CIPs that referenced this pull request Feb 9, 2023
Ryun1 pushed a commit to Ryun1/CIPs that referenced this pull request Feb 9, 2023
Ryun1 added a commit to Ryun1/CIPs that referenced this pull request Nov 17, 2023
…-foundation#427)

* added cip-36 style vote key bech32 prefixes to cip-05

* Clarified CIP-36 style voting keys

* Added preference for the term vote keys in implmentations

* Aligned with gitmachtl's suggestions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Update Adds content or significantly reworks an existing proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants