Enable support for secp256k1 consensus key
activenodes opened this issue · 24 comments
it's merged. Actually it would be nice if we could cut minor release (that would include):
iqlusioninc/yubihsm.rs@33b1f99
we are running a forked version. @activenodes our band runs with the patch for couple of months and no issues so far
Sorry @mkaczanowski trying to build (from main too) the result is always the 0.12.2 without the support for secp256k1
I suppose I can cut a minor release, although it technically breaks some of the contracts we set out for in the README.md, like preserving MSRV
It would probably make more sense for me finish slogging through some of the other upgrades and cutting a major release instead
Sorry @mkaczanowski @tony-iqlusion
In the meantime is there a way to compile and test the main version with the support?
@activenodes cargo install --git https://github.com/iqlusioninc/tmkms.git --features [e.g. yubihsm]
Testing with a VM, same result... mmm...
cargo install --git https://github.com/iqlusioninc/tmkms.git --features=softsign
[..]
Compiling hkdf v0.12.3
Compiling tmkms v0.12.2 (/root/.cargo/git/checkouts/tmkms-1899b2a624fe7801/6459935)
Finished release [optimized] target(s) in 1m 31s
Installing /root/.cargo/bin/tmkms
Installed package `tmkms v0.12.2 (https://github.com/iqlusioninc/tmkms.git#64599354)` (executable `tmkms`)
# tmkms softsign import x.json x.key
error: couldn't load x.json:
0: serde json error
1: unknown variant `tendermint/PrivKeySecp256k1`, expected `tendermint/PrivKeyEd25519` at line 8 column 41
tmkms version
0.12.2
# whereis tmkms
tmkms: /root/.cargo/bin/tmkms
Unfortunately I don't think softsign is currently supported. It would need to be added to tendermint-rs, which currently only supports Ed25519 private keys:
https://docs.rs/tendermint/0.33.2/tendermint/private_key/enum.PrivateKey.html
It's used here in the first example #644
Well... I'll try YubiHSM2 tomorrow
I would suggest asking @mkaczanowski as I have no idea how it would be possible to import such a key given the current code
ah, we haven't used the softsign, but yubihsm. I felt this was too easy PR :). Lemme see what I can work out here
@activenodes the softsign import may not work now as you noticed, but for our tests I did test it for:
- softsign
- yubihsm
you can see it in the original PR:
#644
and here you have a configuration snippet that works:
[[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"
simply don't use the import atm because in fact it's not working properly
To solve this issue properly I would have to at least touch two repos:
https://docs.rs/tendermint-config/0.33.1/src/tendermint_config/priv_validator_key.rs.html#16-25 (tendermint_config)
https://docs.rs/tendermint/0.33.1/src/tendermint/private_key.rs.html#22-33 (tendermint)
so that won't be a quick fix. I also wonder if it is even worth doing it that way, band is likely the only network nowadays that uses secp256k1 as consenus key.
You can sign with tmkms, as described above. I don't think the import is needed as the secp256k1 base64 private key doesn't have to be split into two parts like ed25519 (iirc).
I am leaning towards #wontfix
The tendermint-rs crates are all in a monorepo, so you can change both in a single commit: https://github.com/informalsystems/tendermint-rs
They're under the config and tendermint subdirectories respectively.
okay, lemme see what I can do here then
I'm doing some tests and if copying .priv_key.value is actually sufficient I don't think the import is necessary.
Almost midnight, tomorrow I will do some tests in testnet
Thanks
Working!
2023-09-22T09:18:12.426524Z INFO tmkms::session: [band-laozi-testnet6@tcp://*.*.*.*:*****] connected to validator successfully
2023-09-22T09:18:14.784821Z INFO tmkms::session: [band-laozi-testnet6@tcp://*.*.*.*:*****] signed PreCommit:<nil> at h/r/s 11217014/0/2 (0 ms)
2023-09-22T09:18:14.976293Z INFO tmkms::session: [band-laozi-testnet6@tcp://*.*.*.*:*****] signed PreVote:AD5CCC69C4 at h/r/s 11217015/0/1 (0 ms)
2023-09-22T09:18:15.088960Z INFO tmkms::session: [band-laozi-testnet6@tcp://*.*.*.*:*****] signed PreCommit:AD5CCC69C4 at h/r/s 11217015/0/2 (0 ms)
2023-09-22T09:18:17.821481Z INFO tmkms::session: [band-laozi-testnet6@tcp://*.*.*.*:*****] signed PreVote:BADF232A18 at h/r/s 11217016/0/1 (0 ms)
2023-09-22T09:18:18.115865Z INFO tmkms::session: [band-laozi-testnet6@tcp://*.*.*.*:*****] signed PreCommit:BADF232A18 at h/r/s 11217016/0/2 (0 ms)
2023-09-22T09:18:20.107639Z INFO tmkms::session: [band-laozi-testnet6@tcp://*.*.*.*:*****] signed PreVote:52CFC18940 at h/r/s 11217017/0/1 (0 ms)
2023-09-22T09:18:20.317600Z INFO tmkms::session: [band-laozi-testnet6@tcp://*.*.*.*:*****] signed PreCommit:52CFC18940 at h/r/s 11217017/0/2 (0 ms)
Thanks guys 💯
I've made the PR in tendermint-rs repo, but to get this working with tmkms one would have to upgrade tmkms deps and that requires some work.
I slightly hacked the current tmkms main branch to show that the tendermint-rs PR fixes the issue:
mkaczanowski@fca89a7
./target/debug/tmkms softsign import ~/.band/config/priv_validator_key.json out.key
2023-09-22T19:42:23.198244Z INFO tmkms::commands::softsign::import: Imported Ed25519 private key to out.key
^^ ignore ed25519 message, it is secp256k1 key.
@tony-iqlusion do you have any plans to upgrade the tendermint-rs version soon?
Yes, I have a local PR to do it, but it's a mess because I started working on it before merging the secp256k1 consensus PR and a lot has changed upstream in tendermint-rs.
I need to finish rebasing it.
After changed my key with previous key json .priv_key.value, Working well Thanks!
and then, What about just extracting the priv_key_value in the json file without changing tendermint-rs version.
It's too danger?
9월 25 14:31:28 testnet tmkms-support-secp256k[1693459]: 2023-09-25T05:31:28.846675Z INFO tmkms::session: [band-laozi-testnet6@tcp://***.***:*] signed PreCommit:4A40F36CD2 at h/r/s 11307757/0/2 (0 ms)
9월 25 14:31:31 testnet tmkms-support-secp256k[1693459]: 2023-09-25T05:31:31.549128Z INFO tmkms::session: [band-laozi-testnet6@tcp://***.***:*] signed PreVote:FFABFA2933 at h/r/s 11307758/0/1 (0 ms)
9월 25 14:31:31 testnet tmkms-support-secp256k[1693459]: 2023-09-25T05:31:31.767682Z INFO tmkms::session: [band-laozi-testnet6@tcp://***.***:*] signed PreCommit:FFABFA2933 at h/r/s 11307758/0/2 (0 ms)
9월 25 14:31:34 testnet tmkms-support-secp256k[1693459]: 2023-09-25T05:31:34.697925Z INFO tmkms::session: [band-laozi-testnet6@tcp://***.***:*] signed PreVote:C7C3FCE4DE at h/r/s 11307759/0/1 (0 ms)
9월 25 14:31:34 testnet tmkms-support-secp256k[1693459]: 2023-09-25T05:31:34.789234Z INFO tmkms::session: [band-laozi-testnet6@tcp://***.***:*] signed PreCommit:C7C3FCE4DE at h/r/s 11307759/0/2 (0 ms)
informalsystems/tendermint-rs#1358 is merged, now I think I'll wait for @tony-iqlusion to push his upstream rebase PR (#761 (comment))
This is released as v0.13.0-pre.1 in #779
@mkaczanowski want to integrate private key import support now?
yeah, lemme prepare a PR