Tendermint v0.34 compatibility tracking

[greg-szabo]

This issue is a discussion/research issue to track the required items for Tendermint 0.34 compatibility. There are other issues open that relate to this, but at one point all of them need to be implemented to release a compatible KMS. (I don't have the permissions to create a milestone.)

Please add any new identified ideas or issues here.

• Dependency upgrades: signatory v0.21 v0.22 (this depends on an stdtx dependency upgrade which will hopefully take place with the above protobuf upgrade)
• tx-signer: gRPC support for Transaction Sources #97 - gRPC Transaction Sources (also mentioned in tx-signer: post-Stargate roadmap #96 tx-signer: post-Stargate roadmap)
• Update KMS to support gRPC #73 gRPC support (possible overlap with tx-signer: gRPC support for Transaction Sources #97 but the point is there are some gRPC stuff to do)
• Upgrade to Tendermint-rs v0.17 domain types (this adds protobuf encoding)
• Upgrade KMS amino types to protobuf types
• Tendermint v0.34 support for ledger-validator-app backend #212 ledger-validator-app support intent to deprecate, see Deprecating Ledger support #292

[tony-iqlusion]

This WIP PR takes care of the Signatory-related dependency upgrades: https://github.com/iqlusioninc/tmkms/pull/162/files

Edit: merged!

[tomtau]

fyi, I've got this quick 'n dirty port that works with 0.34: tomtau@64b4bc7

A few hurdles I encountered:

1. the textual input in Bech32 in Cosmos SDK still expects Amino prefixes before public key payloads -- the current plan seems to be to remove bech32 representation of public keys (due to some library hardcoded character limits): Remove bech32 PubKey support cosmos/cosmos-sdk#7447

2. current domain type checking in Tendermint-rs is too restrictive for SignProposalRequest and SignVoteRequest sent from Go Tendermint: Make the signature on Proposal optional field? informalsystems/tendermint-rs#626 (Edit: this PR fixes it: Added empty option to signature informalsystems/tendermint-rs#635 -- fyi the quick port patched here tomtau@f4dfa2e )

[tony-iqlusion]

I just landed a PR that makes it possible to use both the Amino and Protobuf RPC messages:

#201

This PR is at least sufficient to allow a stargate-4 gaiad to start up when using priv_validator_laddr. I still haven't tested signing when using protobufs.

I'd like to double check there aren't any Amino regressions, but after that I think we can start working on a tendermint crate upgrade in earnest (possibly after a tendermint-rs v0.17.0-rc2 release)

[tony-iqlusion]

I have a PR open for a tendermint-rs v0.17.0-rc1 upgrade: #206

@tomtau will take a look at what I can use from tomtau@64b4bc7

[tomtau]

that commit was with raw protobuf types as a workaround to that strict tendermint-rs domain type checking which has been fixed -- so it's probably better to use domain types if possible: f4dfa2e as they perform checks that were previously done here in request.validate()

[tomtau]

This PR is at least sufficient to allow a stargate-4 gaiad to start up when using priv_validator_laddr. I still haven't tested signing when using protobufs.

@tony-iqlusion I've just tested it (I used the latest "develop" e0a7390) -- and there's a small issue in creating signing payloads; a quick fix (using the raw proto types) for this is here: tomtau@2fa1678

btw, domain types have to_signable_vec for this (https://github.com/informalsystems/tendermint-rs/blob/master/tendermint/src/proposal.rs#L95)

[tomtau]

hm, even with that quick fix, there's still an issue if pre-commit is <nil>.
on tendermint/cosmos side:

I[2020-11-09|16:22:42.200] Executed block                               module=state height=456 validTxs=0 invalidTxs=0
I[2020-11-09|16:22:42.217] Committed state                              module=state height=456 txs=0 appHash=967A6285C3ED389558719D8FE2F3611EBB2EF6D58CABF48EC347A3D6756FA85B
E[2020-11-09|16:22:48.235] Error with msg                               module=consensus height=457 round=0 peer= err="error adding vote" msg="[Vote Vote{0:55FA80439436 457/00/SIGNED_MSG_TYPE_PRECOMMIT(Precommit) 000000000000 0DE728F409B7 @ 2020-11-09T08:22:48.220803Z}]"

on tmkms side:

Nov 09 16:22:41.956  INFO tmkms::session: [...] signed PreVote:93D7531A32 at h/r/s 456/0/1 (0 ms)
Nov 09 16:22:41.992  INFO tmkms::session: [...] signed PreCommit:93D7531A32 at h/r/s 456/0/2 (0 ms)
Nov 09 16:22:47.172  INFO tmkms::session: [...] signed Proposal:FC1319D355 at h/r/s 457/0/0 (0 ms)
Nov 09 16:22:47.203  INFO tmkms::session: [...] signed PreVote:FC1319D355 at h/r/s 457/0/1 (0 ms)
Nov 09 16:22:48.223  INFO tmkms::session: [...] signed PreCommit:<nil> at h/r/s 457/0/2 (0 ms)

[tomtau]

hm, even with that quick fix, there's still an issue if pre-commit is <nil>.
on tendermint/cosmos side:

this should fix it: tomtau@e834853

[tomtau]

@greg-szabo one more issue discovered for 0.34 compatibility: informalsystems/tendermint-rs#663

[tony-iqlusion]

a quick fix (using the raw proto types) for this is here: tomtau/tmkms@2fa1678

@tomtau cool, I can try integrating that as a quick fix

btw, domain types have to_signable_vec for this (https://github.com/informalsystems/tendermint-rs/blob/master/tendermint/src/proposal.rs#L95)

I'll definitely want to switch to that after landing #206

[tony-iqlusion]

@tomtau PR'd a slightly modified version of your branch: #211

Edit: landed after we tested it and confirmed it working on stargate-4! 🎉