feat(signer): add ECDSA proxy keys #87

David-Petrov · 2024-08-14T07:26:41Z

Quick summary:

The first two commits are the main content of this PR, so changes are roughly two-fold:

refactor of existing infrastructure to prepare support for different signature schemes (other than BLS) for proxy signatures;
actually implementing ECDSA as a second signature scheme, whilst further refining the multiple signature infrastructure in the process.

The rest of the commits are smaller decisions and easier to follow.

Update 1

From eba97b0 below, the PR takes an alternative approach to implementing ECDSA proxies where we strive to flatten the flow by losing the generics and making the client SDK more type safe.

TODOs:

Take care of local inline TODOs.
Better serialization to request/response structs for a proper REST API
Update Signer API docs (with examples).
Add tests for the ECDSA keys.

fixes #20

Co-authored-by: reo101 <pavel.atanasov2001@gmail.com>

- simplify internals of `EcdsaPublicKey`: no need to store the actual abstract object, just its compressed encoding - we construct it when needed - extract verification logic into a separate trait `Verifier`, not in `SecretKey` - remove some (already) obsolete utility functions from `signature.rs`

- move `GenericProxySigner` from common to manager - simplify inners of `EcdsaSignature`

- define public alias `ConsensusSigner = Signerl<BlsSecretKey>`

…visibility

* also, a small fix with error handling (avoid panicking)

crates/common/src/commit/request.rs

crates/common/src/commit/client.rs

crates/common/src/signer/mod.rs

ltitanb

other changes as discussed

crates/common/src/signature.rs

crates/common/src/signer/schemes/ecdsa.rs

- flatten out the code structure by removing generics - TODO: client SDK still not typed straightforward, but with enums insteand

- thus, conclude an implementation variant where ECDSA and BLS types are separated almost everywhere throughout the signer module (+the client SDK)

crates/signer/src/manager.rs

… key type * also, introduce the `PublicKey` trait * TODO: a custom wrapper type around `BlsPublicKey` was necessary in order to implement `TreeHash` trait for it. Refine the boundaries of usage of alloy's type and our custom wrapper.

crates/signer/src/manager.rs

ltitanb

Let's make sure to add tests for all new keys + signarures

crates/common/src/commit/client.rs

ltitanb · 2024-08-22T18:50:03Z

crates/common/src/signer/schemes/bls.rs

+// derive_more
+#[derive(Deref, From, Into)]
+pub struct BlsPublicKey {
+    inner: BlsPublicKeyInner,


if this is just to re-implement TreeHash maybe let's also open an issue upstream?

I had a quick thought about what we can do regarding opening an issue upstream, but I decided for a wrapper type as a common solution. Now that you propose it, I will have a deeper look into how easy this would be (or maybe if the trait is intentionally not implemented); and eventually open up an issue.

Opened up the issue here.

Implementing the required changes in tree_hash locally wasn't a problem, but then I had a lot of versioning issues, which tie into this task, so I can't say I've got a working PoC of this approach, thus I've not opened a PR in tree_hash's repo.

Leaving the wrapper type as a temporary solution for now.

Added a TODO in dea0b0c; minimal reexport (as relevant for the wrapper type) in f103c8d

…ic key type thus remove `SignProxyBlsRequest` and `SignProxyEcdsaRequest` as duplicated structs

Cargo.toml

…parte methods

makes deserialization of signatures properly fallible * also, add both proxy examples in `da_commit`

* replace derived Serialize/Deserialize with sane API ser/de * hex encode fields properly * simplify the inner types of `EcdsaPublicKey` and `EcdsaSignature` to native rust arrays

for the current scale of our needs, we can do without a common (unifying) type for both public key types (BLS and ECDSA), so we basically separate into two separate flows which have only the `PublicKey` trait in common, and that's with a really narrow scope mainly for client-side convenience.

crates/common/src/commit/client.rs

crates/common/src/commit/request.rs

crates/common/src/signer/schemes/bls.rs

crates/common/src/signer/schemes/ecdsa.rs

not immediately necessary

they're reexported from the `k256` crate; were previously explicitly imported to enable `serde` feature flag, that's not necessary any longer after type simplifications around inner ECDSA pubkey/signature representation.

…onsistency

David-Petrov and others added 5 commits August 9, 2024 17:19

refactor(signer)!: generalize for other signature schemes than BLS

02acf38

Co-authored-by: reo101 <pavel.atanasov2001@gmail.com>

feat(signer)!: add support for ECDSA proxy keys

a7c76df

Co-authored-by: reo101 <pavel.atanasov2001@gmail.com>

Merge branch 'main' into feat/ecdsa-proxy-keys

5530567

chore(clippy): infallible instead of fallible conversions

a5450b6

David-Petrov force-pushed the feat/ecdsa-proxy-keys branch from c88ef02 to a9ff828 Compare August 14, 2024 11:01

David-Petrov added 5 commits August 14, 2024 16:25

refactor(signer)!: move around

4e96742

- move `GenericProxySigner` from common to manager - simplify inners of `EcdsaSignature`

chore(signer)!: rename associated type and type alias

4b283e2

choer(signer): rename client Signature type

9d70712

refactor(signer)!: remove default type parameter in Signer

287da33

- define public alias `ConsensusSigner = Signerl<BlsSecretKey>`

chore(clippy): remove unnecessary nested reference

431f189

David-Petrov marked this pull request as ready for review August 14, 2024 13:55

David-Petrov requested review from ltitanb and fbrv August 14, 2024 13:55

David-Petrov added 4 commits August 15, 2024 11:49

chore: remove commented code

8041e0f

refactor(signer/ecdsa): derive unnecessary trait impls

23437a2

style: remove unnecessary constraint duplication

9078e25

refactor(signer)!: rename module signers to signer and constrain …

377aa78

…visibility

David-Petrov force-pushed the feat/ecdsa-proxy-keys branch from 76b1fda to 377aa78 Compare August 15, 2024 11:00

David-Petrov changed the title ~~Add ECDSA proxy keys~~ feat(signer): add ECDSA proxy keys Aug 15, 2024

refactor(loader): unify ConsensusSigner type alias usage

b83841e

* also, a small fix with error handling (avoid panicking)

David-Petrov commented Aug 19, 2024

View reviewed changes

crates/common/src/commit/request.rs Outdated Show resolved Hide resolved

David-Petrov commented Aug 19, 2024

View reviewed changes

crates/common/src/commit/client.rs Outdated Show resolved Hide resolved

David-Petrov commented Aug 19, 2024

View reviewed changes

crates/common/src/signer/mod.rs Outdated Show resolved Hide resolved

David-Petrov commented Aug 19, 2024

View reviewed changes

crates/common/src/signer/mod.rs Outdated Show resolved Hide resolved

ltitanb requested changes Aug 19, 2024

View reviewed changes

crates/common/src/signature.rs Outdated Show resolved Hide resolved

crates/common/src/signer/schemes/ecdsa.rs Show resolved Hide resolved

David-Petrov added 4 commits August 20, 2024 13:32

chore!: remove forgotten debug

7e1bf44

refactor(signer)!: [WIP] simplify proxy keys

eba97b0

- flatten out the code structure by removing generics - TODO: client SDK still not typed straightforward, but with enums insteand

refactor(signer)!: separate BLS and ECDSA types in client SDK

ad6d373

- thus, conclude an implementation variant where ECDSA and BLS types are separated almost everywhere throughout the signer module (+the client SDK)

chore(signer)!: reformat & clean up

fede09c

David-Petrov force-pushed the feat/ecdsa-proxy-keys branch from 9ac30c9 to fede09c Compare August 21, 2024 16:11

David-Petrov commented Aug 21, 2024

View reviewed changes

crates/signer/src/manager.rs Outdated Show resolved Hide resolved

mempirate mentioned this pull request Aug 22, 2024

Commit-Boost integration chainbound/bolt#201

Closed

David-Petrov added 2 commits August 22, 2024 11:58

refactor(signer/manager): remove enum GenericProxySigner

68fce43

David-Petrov commented Aug 22, 2024

View reviewed changes

crates/signer/src/manager.rs Outdated Show resolved Hide resolved

ltitanb reviewed Aug 22, 2024

View reviewed changes

refactor(signer/request): make SignProxyRequest generic in the publ…

780299e

…ic key type thus remove `SignProxyBlsRequest` and `SignProxyEcdsaRequest` as duplicated structs

David-Petrov self-assigned this Aug 23, 2024

David-Petrov commented Aug 23, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

David-Petrov added 7 commits August 23, 2024 11:27

refactor(signer/manager)!: split generic get_delegation into two se…

7ddcd3a

…parte methods

refactor(signer/client): remove unnecessary Vec<u8> deserialization

b8f228b

makes deserialization of signatures properly fallible * also, add both proxy examples in `da_commit`

feat(signer): add tests for ECDSA proxies

635f620

refactor(signer/api)!: proper struct serde

dd864ec

* replace derived Serialize/Deserialize with sane API ser/de * hex encode fields properly * simplify the inner types of `EcdsaPublicKey` and `EcdsaSignature` to native rust arrays

refactor(signer): remove unnecessary module

b3286d5

refactor(signer/request): move GetPubkeysResponse

f075db9

fbrv reviewed Aug 28, 2024

View reviewed changes

David-Petrov added 9 commits August 28, 2024 15:36

refactor(signer): reduce code duplication around SignRequest

9c4f412

style/refactor(signer): reduce method size

4e652d9

refactor(deps)!: revert version updates

9907059

not immediately necessary

chore(deps)!: remove unused deps

0ff2091

they're reexported from the `k256` crate; were previously explicitly imported to enable `serde` feature flag, that's not necessary any longer after type simplifications around inner ECDSA pubkey/signature representation.

chore(signer): add TODO for BlsPublicKey wrapper

dea0b0c

refactor(signer)!: re-export request types properly from prelude

5fc50ae

feat(signer/api): update api docs

0bcd2d7

refactor(signer/client): re-export bls types and rename methods for c…

f103c8d

…onsistency

chore(docs): update md files in book

da550d3

fbrv approved these changes Aug 30, 2024

View reviewed changes

David-Petrov merged commit f1e6ff1 into main Aug 30, 2024
4 checks passed

David-Petrov deleted the feat/ecdsa-proxy-keys branch August 30, 2024 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(signer): add ECDSA proxy keys #87

feat(signer): add ECDSA proxy keys #87

David-Petrov commented Aug 14, 2024 •

edited

Loading

ltitanb left a comment

ltitanb left a comment

ltitanb Aug 22, 2024

David-Petrov Aug 23, 2024

David-Petrov Aug 27, 2024 •

edited

Loading

feat(signer): add ECDSA proxy keys #87

feat(signer): add ECDSA proxy keys #87

Conversation

David-Petrov commented Aug 14, 2024 • edited Loading

Quick summary:

Update 1

TODOs:

ltitanb left a comment

Choose a reason for hiding this comment

ltitanb left a comment

Choose a reason for hiding this comment

ltitanb Aug 22, 2024

Choose a reason for hiding this comment

David-Petrov Aug 23, 2024

Choose a reason for hiding this comment

David-Petrov Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

David-Petrov commented Aug 14, 2024 •

edited

Loading

David-Petrov Aug 27, 2024 •

edited

Loading