fix(privval): always sign extension

[melekes]

IFF a vote is a precommit for a non-nil block.

Refs #837, cometbft/cometbft#2357
Fixes #831 (comment)

[tony-iqlusion]

Don't we still want to avoid signing extensions for chains that aren't using them?

Does it need a per-chain config option?

[melekes]

What if a chain later decides to enable extensions? It stops tmkms, changes config, starts tmkms?

[tony-iqlusion]

Yes, that's how other config changes are managed today.

I'm also curious if it would be problematic for TMKMS to return extension signatures on an empty extension on a chain that wasn't expecting it.

YubiHSM2s also aren't the fastest signers, so it would be desirable to not sign the extensions on chains that don't need them even if it's tolerated by CometBFT.

[cxyzhang0]

I ran the code per 08c06c4 and am getting
protocol error: can't add signature to empty extension

[cxyzhang0]

I ran the code per 08c06c4 and am getting protocol error: can't add signature to empty extension

Ok, I have to remove the check for empty vote extension in add_extension_signature to bypass the error. I am adding another feature to TMKMS to support PKCS11-based HSMs. Those HSMs are network devices and it would be desirable to avoid unnecessary signatures.

[melekes]

@tony-iqlusion I've opened cometbft/cometbft#2439 to see if we can remove this behavior from Comet.

[BrendanChou]

Appreciate the change here!

Would you require the cometbft issue to be solved before this PR gets merged into a release?

I think there are validators that would appreciate a tmkms release so that they can start using it on testnet environments (and very soon mainnet environments), so trying to give them an estimate of when they should update versions

[tony-iqlusion]

I'll go ahead and merge this, but prior to release we need to add a configuration option to tmkms.toml which allows enabling/disabling extension signatures on a chain-by-chain basis.

That's going to be a breaking change from v0.13.1 which would sign extensions in present, since it will (for now) default to false and require people using chains with extensions explicitly enable it, so it will need to go out in a v0.14 release.

It would be great if people running testnets for the chains which require this can test it out prior to release.

[tqin7]

I'll go ahead and merge this, but prior to release we need to add a configuration option to tmkms.toml which allows enabling/disabling extension signatures on a chain-by-chain basis.

That's going to be a breaking change from v0.13.1 which would sign extensions in present, since it will (for now) default to false and require people using chains with extensions explicitly enable it, so it will need to go out in a v0.14 release.

It would be great if people running testnets for the chains which require this can test it out prior to release.

May I ask how we can test this out now on a testnet that has vote extensions? For example, would we need to build from source ourselves? Thanks!

[tony-iqlusion]

I can cut a prerelease

[melekes]

cometbft/cometbft#2439 (comment) we've decided to alter the interface and protos to send a flag (sign_extension), which will instruct whenever tmkms (and other signers) should sign extension or not. The change will come in CometBFT v1.0.

[tony-iqlusion]

Nice! Thank you!

I can either add a manual flag for now, or we can revert this until the new flag is available.

[tqin7]

I can cut a prerelease

Thanks! May I ask when we should expect a pre-release? dYdX testnet can help test it out.

[tony-iqlusion]

I released v0.14.0-pre.0 in #866

Note that I do still plan on at least temporarily adding a flag to the config to toggle on extension signing, so if you do attempt to use this prerelease you'll need to add the configuration at some point in the future.

[cxyzhang0]

In pub fn add_extension_signature, I have to do the following commenting to make it work. Is that expected?
`
// if vote.extension.is_empty() {
// return Err(Error::protocol(
// "can't add signature to empty extension".into(),
// ));
// }

`

[tony-iqlusion]

@cxyzhang0 can you open a PR which removes those lines?

[melekes]

opened #867

[tqin7]

Thanks for the pre-release! We have verified that it works on dYdX testnet. May I ask when we can expect an official release?

[tony-iqlusion]

I still need to implement feature gating for signing the extension. There are also Tendermint/CosmRS upgrades which would be good too.

I've also largely been waiting on confirmation that the latest prerelease actually works, so I'm glad to get a report that it does!

[tony-iqlusion]

I've released v0.14.0-pre.2. This notably includes a new configuration option you'll need to set in order to indicate extensions should be signed:

https://github.com/iqlusioninc/tmkms/blob/621bd41/tmkms.toml.example#L19

In the future it will be able to detect this automatically from CometBFT, but for now it will require setting it manually.