Use secp256k1 for validator keys. #3377
Conversation
deuszx
force-pushed
the
secp256k1-validator-keys
branch
8 times, most recently
from
f34441a to
9d991e5
Compare
| @@ -112,7 +115,7 @@ async fn test_block_size_limit() { | |||
| let mut chain = ChainStateView::new(chain_id).await; | |||
|
|
|||
| // The size of the executed valid block below. | |||
| let maximum_executed_block_size = 685; | |||
| let maximum_executed_block_size = 720; | |||
There was a problem hiding this comment.
TODO: need to investigate this.
There was a problem hiding this comment.
That's exactly as much as the 33 bytes for Secp256k1PublicKey + 2 bytes for length prefix but I don't know where that'd come from.
There was a problem hiding this comment.
Should we create an issue for it and add a TODO here?
There was a problem hiding this comment.
I'll do that if I don't figure this out before merging.
There was a problem hiding this comment.
OK, I think where this might come from - there's a ExecutionOutcome::System(SystemMessage::CreateCommittee(Committee))) and that innermost struct contains ValidatorState which now also gets AccountPublicKey.
deuszx
force-pushed
the
secp256k1-validator-keys
branch
3 times, most recently
from
42cf98a to
74a0942
Compare
| CONTENT: U8 | ||
| SIZE: 33 | ||
| Secp256k1Signature: | ||
| NEWTYPESTRUCT: BYTES |
There was a problem hiding this comment.
TODO: Serialize as array, not vector.
There was a problem hiding this comment.
Switched to serialization with compact representation (which is always 64 bytes). See Serialize secp256k1 signatures in a compact form rather than DER..
| // 1. Record samples for types with custom deserializers. | ||
| // 2. Trace the main entry point(s) + every enum separately. | ||
| { |
There was a problem hiding this comment.
LGTM. This code is meant to be between // 1. and // 2..
There was a problem hiding this comment.
Done in Review part 2..
| @@ -112,7 +115,7 @@ async fn test_block_size_limit() { | |||
| let mut chain = ChainStateView::new(chain_id).await; | |||
|
|
|||
| // The size of the executed valid block below. | |||
| let maximum_executed_block_size = 685; | |||
| let maximum_executed_block_size = 720; | |||
There was a problem hiding this comment.
Should we create an issue for it and add a TODO here?
deuszx
force-pushed
the
secp256k1-validator-keys
branch
2 times, most recently
from
a2a3dab to
ab824fb
Compare
| * `--public-key <PUBLIC_KEY>` — The public key of the validator | ||
| * `--account-key <ACCOUNT_KEY>` — The public key of the account controlled by the validator |
There was a problem hiding this comment.
I would expect --xxx-public-key in both cases for some value xxx.
Couple of naming options: (key = private or public key depending on the context)
- validator key vs user key
- validator key vs owner key
- (validator's) server key vs (validator's) client key <-- seems like the most precise because both keys belong to the validator node.
There was a problem hiding this comment.
OK, but let's do the renaming in a single sweep in a separate PR. This one is already too big and it (rename) is orthogonal.
There was a problem hiding this comment.
Sure, this works.
For the records, ideally, you would start by renaming ValidatorState::public_key into something more specific like ValidatorState::server_public_key in order to "make space". Then this PR would add (say) ValidatorState::client_public_key.
There was a problem hiding this comment.
Initially it was "authority key" which I think made sense (since validator can have many keys but only one is signing blocks).
There was a problem hiding this comment.
"authority key" is nice. Something like: "validator authority key" vs "validator user/owner/client key" then
There was a problem hiding this comment.
No strong opinion about the names, but let's do the renaming in a separate PR, yes. 👍
deuszx
force-pushed
the
secp256k1-validator-keys
branch
from
ab824fb to
75ebeba
Compare
Motivation
We want to make certificates easily verifiable on EVM and there's no precompile for ed25519 on Ethereum.
Proposal
Use
Secp256k1*forValidator*keys.This required adding a secondary key - called
account_key- toValidatorState. That's b/c validators can also act as fallback owners during block proposals and blocks are still signed withAccount*keys -ed25519.Test Plan
CI should catch regressions. New tests have been added to verify signature/public key (de)serialization and conversions to/from gRPC protobuf.
Release Plan
This is a breaking change and as such requires full network restart.
Links