Support secp256k1 consensus key #644

mkaczanowski · 2022-10-13T22:21:52Z

In brief

This change adds support for secp256k1 consensus key.

Currently there are two key types allowed consensus and account. The former is ed25519 curve while the latter is secp256k1, yet consensus key selection defaults (hardcode) to ed25519 curve.

What's the usecase?

While a vast majority of cosmos-sdk networks use ed25519 key type for consensus, there are some exceptional networks that use secp256k1 curve. In current state those networks can't utilize the tmkms.

In particular, Band (https://bandprotocol.com/bandchain) is using the secp256k1 key for consensus.

Test plan

Unittests / integration tests

cargo test --features softsign,yubihsm,yubihsm-mock,yubihsm-server,tx-signer

test result: ok. 16 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.77s

Test live (band testnet)

I've joined the Band Testnet with secp256k1 consensus key:

    "validator_info": {
      "address": "B775452614685F0722377EB76C430C268C07CD3B",
      "pub_key": {
        "type": "tendermint/PubKeySecp256k1",
        "value": "AiBi550KhHiCFh+wI+cq7rTW7rVKqF427R0/w+LgYPKA"
      },
      "voting_power": "3"
    }

Softsign

You can see the signed blocks here:

          {
            "block_id_flag": 2,
            "validator_address": "B775452614685F0722377EB76C430C268C07CD3B",
            "timestamp": "2022-10-09T23:49:23.462338551Z",
            "signature": "qgR32PRHfHBU/OnXapQN6c4YIYtLLX7kPlryLmmm8agWtDsOJ4AE5C50jomwODr5ZX2EZWhn7iBYtniWeLh6yA=="
          }

Config used:

[[chain]]
id = "band-laozi-testnet5"
state_file = "/validators/band/tmkms/state/band-laozi-testnet5-consensus.json"

[chain.key_format]
account_key_prefix = "bandvalconspub"
consensus_key_prefix = "bandvalconspub"
type = "bech32"

[[validator]]
addr = "unix:///validators/band/kms.sock"
chain_id = "band-laozi-testnet5"
protocol_version = "v0.34"
reconnect = true

[providers]
[[providers.softsign]]
chain_ids = [ "band-laozi-testnet5" ]
key_type = "account"
path = "/validators/band/tmkms/band-laozi-testnet5-consensus.key"

YubiHSM

You can see the signed blocks here:

          {
            "block_id_flag": 2,
            "validator_address": "B775452614685F0722377EB76C430C268C07CD3B",
            "timestamp": "2022-10-13T22:12:05.617160344Z",
            "signature": "RAj2RCI17Rgy+USwOXjLZW+DsozSRode592MlE4VpHUS49vSTD/odrxN7i/yVuYJfL4mG1zVrkoaio+LFWwk9w=="
          },

Config used:

[[chain]]
id = "band-laozi-testnet6"
state_file = "/opt/validators/band/tmkms/state/band-laozi-testnet6-consensus.json"

[chain.key_format]
account_key_prefix = "bandvalconspub"
consensus_key_prefix = "bandvalconspub"
type = "bech32"

[[validator]]
addr = "unix:///opt/validators/band/kms.sock"
chain_id = "band-laozi-testnet6"
protocol_version = "v0.34"
reconnect = true

[providers]
[[providers.yubihsm]]

[[providers.yubihsm.keys]]
chain_ids = [ "band-laozi-testnet6" ]
key = <KEY_ID>
type = "account"

[providers.yubihsm.adapter]
addr = "tcp://<YUBIHSM_CONNECTOR_IP>:<PORT>"
type = "http"

[providers.yubihsm.auth]
key = <KEY_ID>
password = "<PASS>"

mkaczanowski · 2022-10-17T14:20:49Z

hey @tony-iqlusion,

do you think you can review this PR? (or propose other reviewer).

thanks

tony-iqlusion · 2022-10-17T15:57:35Z

There is a backlog of other PRs to review first, and I am also busy with other things including security response

activenodes · 2022-11-14T18:09:09Z

@mkaczanowski it's fantastic! 😍

activenodes · 2022-11-21T09:51:03Z

Sorry to ping you @tony-iqlusion, this feature from @mkaczanowski is awesome.
If I can help you, if you need a server configured in secp256k1 consensus key testnet, let me know...

tony-iqlusion · 2022-11-21T20:42:41Z

For starters, this PR needs a rebase. After that it's blocked on code review by myself. This is an extremely security critical project and changes to it must be made very carefully.

As I mentioned earlier, there are a queue of other PRs which were opened without prior planning before this one such as #613 which I've been trying to work through, and I have very little time for this sort of unplanned work.

It's also Thanksgiving week here in the US, and after that the holiday season, which further limits my time. I would say I can work on this in Q1 2023 if you want a rough estimate.

If you need these changes sooner, I would suggest briefly maintaining a fork with them people can use until such a time as they can get upstreamed.

mkaczanowski · 2022-11-21T22:34:38Z

Please take your time, if one wants to use that feature, feel free to clone the branch (beta testers are most welcome).

In a meantime, I'll rebase PR 👍

mkaczanowski · 2023-04-01T19:51:01Z

Hey, @tony-iqlusion I've just rebased the branch. It's been a while and I haven't seen much traction here:
#613

I wonder if maybe it'll be better/easier/faster to review & merge this PR prior. I don't introduce any new dependencies, so that's one item less to worry about.

Anyway, lemme know if you find some time to look at this one :)

activenodes · 2023-05-25T09:21:05Z

+1 @tony-iqlusion

tony-iqlusion · 2023-05-25T17:06:01Z

@mkaczanowski can you rebase again? The branch is out-of-date

mkaczanowski · 2023-05-25T21:34:02Z

@tony-iqlusion yup, rebased 👍

tony-iqlusion · 2023-05-26T19:28:57Z

Cargo.toml

@@ -65,6 +65,7 @@ zeroize = "1"
 abscissa_core = { version = "0.7", features = ["testing"] }
 byteorder = "1"
 rand = "0.7"
+parameterized = "1.0.1"


I'd prefer not to add additional dependencies, even if they're dev-dependencies

Ok, I removed it. Sacrificed a bit "neatness" of the test code, but I think it's fine this way too: 5cb01cf

tony-iqlusion · 2023-05-26T19:31:31Z

src/keyring/signature.rs

+    ///  ED25519 signature
+    ED25519(ed25519::Signature),
+
+    /// ECDSA signagure (e.g secp256k1)
+    ECDSA(ecdsa::Signature),


Please follow RFC 430 naming conventions

Suggested change

/// ED25519 signature

ED25519(ed25519::Signature),

/// ECDSA signagure (e.g secp256k1)

ECDSA(ecdsa::Signature),

/// ED25519 signature

Ed25519(ed25519::Signature),

/// ECDSA signagure (e.g secp256k1)

Ecdsa(ecdsa::Signature),

Although it's debatable if an enum here is really helpful. It could just be a (newtype for) Vec<u8> unless the algorithm information is actually needed to be retained

updated the naming to match RFC:
7b8aaf0

Although it's debatable if an enum here is really helpful. It could just be a (newtype for) Vec unless the algorithm information is actually needed to be retained

I don't have strong opinion for either. Let me know if you think I should change it

mkaczanowski · 2023-05-26T21:07:29Z

runnig tests again:

$ cargo test --features softsign,yubihsm,yubihsm-mock,yubihsm-server,tx-signer
test result: ok. 16 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.59s

mkaczanowski · 2023-05-30T08:48:21Z

@tony-iqlusion does it look good to you now?

tony-iqlusion

Security audit failure is unrelated. I'll follow up on that.

tony-iqlusion · 2023-05-30T16:15:15Z

Thanks!

mkaczanowski · 2023-05-30T17:35:24Z

@tony-iqlusion thanks! Could you cut a new release, say v0.12.3?

tony-iqlusion · 2023-05-30T17:55:03Z

There's a lot more work I'd like to get into the next release

mkaczanowski added 6 commits May 25, 2023 23:30

add support for secp256k1 conensus key

3613a8e

apply missing documentation

e2fe8db

rename ed25519.rs to pubkey.rs

b055a30

specialize error messages

626c973

apply rustfmt

b8db325

apply clippy suggestions

0d37275

tony-iqlusion reviewed May 26, 2023

View reviewed changes

mkaczanowski added 2 commits May 26, 2023 22:21

conform with RFC 430

7b8aaf0

remove parametrized crate

5cb01cf

tony-iqlusion approved these changes May 30, 2023

View reviewed changes

tony-iqlusion merged commit b6ce617 into iqlusioninc:main May 30, 2023

activenodes mentioned this pull request Sep 18, 2023

Enable support for secp256k1 consensus key #761

Closed

tony-iqlusion mentioned this pull request Oct 24, 2023

v0.13.0 #796

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support secp256k1 consensus key #644

Support secp256k1 consensus key #644

mkaczanowski commented Oct 13, 2022

mkaczanowski commented Oct 17, 2022

tony-iqlusion commented Oct 17, 2022

activenodes commented Nov 14, 2022

activenodes commented Nov 21, 2022

tony-iqlusion commented Nov 21, 2022

mkaczanowski commented Nov 21, 2022

mkaczanowski commented Apr 1, 2023

activenodes commented May 25, 2023

tony-iqlusion commented May 25, 2023

mkaczanowski commented May 25, 2023

tony-iqlusion May 26, 2023

mkaczanowski May 26, 2023

tony-iqlusion May 26, 2023

mkaczanowski May 26, 2023

mkaczanowski commented May 26, 2023

mkaczanowski commented May 30, 2023

tony-iqlusion left a comment

tony-iqlusion commented May 30, 2023

mkaczanowski commented May 30, 2023

tony-iqlusion commented May 30, 2023

Support secp256k1 consensus key #644

Support secp256k1 consensus key #644

Conversation

mkaczanowski commented Oct 13, 2022

In brief

What's the usecase?

Test plan

Unittests / integration tests

Test live (band testnet)

Softsign

YubiHSM

mkaczanowski commented Oct 17, 2022

tony-iqlusion commented Oct 17, 2022

activenodes commented Nov 14, 2022

activenodes commented Nov 21, 2022

tony-iqlusion commented Nov 21, 2022

mkaczanowski commented Nov 21, 2022

mkaczanowski commented Apr 1, 2023

activenodes commented May 25, 2023

tony-iqlusion commented May 25, 2023

mkaczanowski commented May 25, 2023

tony-iqlusion May 26, 2023

Choose a reason for hiding this comment

mkaczanowski May 26, 2023

Choose a reason for hiding this comment

tony-iqlusion May 26, 2023

Choose a reason for hiding this comment

mkaczanowski May 26, 2023

Choose a reason for hiding this comment

mkaczanowski commented May 26, 2023

mkaczanowski commented May 30, 2023

tony-iqlusion left a comment

Choose a reason for hiding this comment

tony-iqlusion commented May 30, 2023

mkaczanowski commented May 30, 2023

tony-iqlusion commented May 30, 2023