tendermint-rs: Fixes, cleanups, and updates #228

tarcieri · 2019-04-15T15:33:56Z

These comments apply to the tendermint-rs crate only. I've tried to avoid touching the KMS code as much as possible in this PR.

Fix --no-default-features build (and test in CI)
Eliminate most usages of (deprecated) extern crate syntax
Use the derive cargo feature of serde feature instead of serde_derive. Ditto for failure.
Rename serializers cargo feature of tendermint-rs to serde
Use 2018 edition module name convention (modname.rs instead of modname/mod.rs)
Update x25519-dalek to 0.5.2
Update zeroize to 0.6
Moves ValidatorAddr => tendermint::Address
Moves tendermint::secret_connection::PeerId => tendermint::node::Id and adds a corresponding tendermint::account::Id
Impl Serialize and Deserialize for: tendermint::block::Height, tendermint::Hash, tendermint::PublicKey
Adds a tendermint::Moniker type

- Fix --no-default-features build (and test in CI) - Eliminate most usages of (deprecated) `extern crate` - Use the `derive` cargo feature of `serde` feature instead of `serde_derive` - Rename `serializers` cargo feature of tendermint-rs to `serde` - Use 2018 edition module names (`modname.rs` instead of `modname/mod.rs`) - Update `x25519-dalek` to 0.5.2 - Update `zeroize` to 0.6

tarcieri · 2019-04-15T15:37:17Z

I've been working on a tendermint-rpc crate (which I could eventually upstream into the tendermint crate proper, under e.g. an rpc cargo feature) and ran into a few snags trying to use types from the tendermint crate.

This PR contains fixes for the issues I encountered. Ideally I'd like to use the crate without secret-connection, so fixing the build without that feature is helpful.

Moves the ValidatorAddr type into the Tendermint crate, as it's generally useful for any remote address. Also moves the `PeerId` type from `secret_connection` into a `node::Id` crate which isn't gated on the rest of the Secret Connection code.

...and a `FromStr` impl. This enables parsing block heights from JSON. Right now these impls are specialized to strings (as they internally use e.g. `String::deserialize` and `String::serialize`) but in the future with the use of a custom visitor they could be extended to support both strings and numbers. Strings are immediately useful for JSONRPC.

Adds a type for representing monikers

Support for serializing and deserializing hash values from hex strings

Adds serde serializers/deserializers for PublicKey types which support the JSON serialization used by the JSONRPC interface.

Adds a type for representing IDs of Tendermint accounts, which are computed as a truncated hash of a secp256k1 public key.

tarcieri · 2019-04-15T18:19:58Z

This is ready for review (although I'm going to integrate this branch into my Tendermint RPC crate and make sure everything checks out there).

I don't think any of these changes will impact the KMS as they all touch features of the tendermint crate the KMS wasn't using aside from factoring the KMS's config::validator::ValidatorAddr type => tendermint::address::Address, which is hopefully just an innocuous name change.

tarcieri · 2019-04-15T18:24:55Z

tendermint-rs/src/account.rs

+    }
+}
+
+impl From<secp256k1::PublicKey> for Id {


This implementation is copy/paste from node::Id, but I assume account IDs/addresses work the same way? Are there test vectors I can use?

Should this be called account::Address instead?

but I assume account IDs/addresses work the same way?

Yes, the only difference is the curve (for now). secp vs ed2219

Are there test vectors I can use?

Does this work?
https://github.com/tendermint/tendermint/blob/5b8888b01b7542cd6dd856e7a322f3d4e5ccba16/crypto/secp256k1/secp256k1_test.go#L24-L30

Should this be called account::Address instead?

Good question. Tendermint doesn't have these semantics (Address is the truncated hash of whatever pubkey given, ID is only for nodes). Gaia has account addresses, but it is the bechified version: https://github.com/cosmos/cosmos-sdk/blob/9cdd1d3e9ebf5fdb72203acbaf6231c5dcde82b9/types/address.go#L73

I think it is consistent to stick with ID here.

Will add that test vector to this PR, thanks!

Looking at the test vectors in the address code you linked, the addr is:

1CKZ9Nx4zgds8tU7nJHotKSDr4a9bYJCa3

I can't tell what format that is. It's mixed case so possibly Base64?

The format I'm looking at (i.e. the validator_info object's address field in the /status RPC endpoint) appears to be hex, and looks like:

C73833E9BD86D34EDAD4AFD571FB5D0926294CD5

The code in this PR is fine with that format, which is what I'm immediately interested in using this for. So I guess I'll leave it as-is for now.

I can't tell what format that is. It's mixed case so possibly Base64?

Looks like it is base58 (I should have provided the following lines for context too):
https://github.com/tendermint/tendermint/blob/5b8888b01b7542cd6dd856e7a322f3d4e5ccba16/crypto/secp256k1/secp256k1_test.go#L36

tarcieri · 2019-04-15T18:25:21Z

tendermint-rs/src/amino_types/vote.rs

@@ -198,7 +198,7 @@ impl ConsensusMessage for Vote {

        self.block_id
            .as_ref()
-            .map_or(Ok(()), |bid| bid.validate_basic())
+            .map_or(Ok(()), ConsensusMessage::validate_basic)


clippy prefers this (no closure, even though syntactically it's still just as cumbersome)

tarcieri · 2019-04-15T18:26:09Z

tendermint-rs/src/error.rs

@@ -1,11 +1,9 @@
 //! Error types

+use failure::*;


This pulls in Fail and the corresponding custom derive macro for it

tarcieri · 2019-04-15T18:27:18Z

tendermint-rs/src/public_keys.rs

 use subtle_encoding::{bech32, hex};

 /// Public keys allowed in Tendermint protocols
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "serde", serde(tag = "type", content = "value"))]


These are configured to parse JSON in the form:

{ "type": "tendermint/PubKeyEd25519", "value": "RblzMO4is5L1hZz6wo4kPbptzOyue6LTk4+lPhD1FRk=" }

tarcieri · 2019-04-15T18:28:52Z

tendermint-rs/src/public_keys.rs

+        serde(
+            rename = "tendermint/PubKeySecp256k1",
+            serialize_with = "serialize_secp256k1_base64",
+            deserialize_with = "deserialize_secp256k1_base64"


Haven't tested any of this but the names appear to match what's in Tendermint

But #228 (comment) means you tested this manually?

Yes, also there's a unit test for parsing/serializing Ed25519 keys from JSON here:

https://github.com/tendermint/kms/pull/228/files/611692b6455408c5f01ad50eedde9a13ec47dabc#diff-e73395ccad975224b12459de892a0722R217

I haven't tested anything secp256k1-related, however I'll add the account key test vectors at least. They won't exercise this, but it seems like if Ed25519 works then barring a spelling mistake above secp256k1 should work too since serde is doing the parsing and the differences are all declarative.

tarcieri · 2019-04-15T18:54:30Z

Using the latest commit with my Tendermint RPC crate, it's able to parse the following from the /status endpoint using serde:

ResponseWrapper { jsonrpc: Version("2.0"), id: Id(""), result: StatusResponse { node_info: NodeInfo { protocol_version: ProtocolVersionInfo { p2p: ProtocolVersion(7), block: ProtocolVersion(10), app: ProtocolVersion(0) }, id: Id([107, 144, 211, 118, 249, 191, 221, 131, 198, 217, 53, 27, 247, 178, 244, 88, 183, 77, 234, 204]), listen_addr: Tcp { peer_id: None, host: "0.0.0.0", port: 26656 }, network: chain::Id(cosmoshub-1), version: Version { major: 0, minor: 30, patch: 1, pre: [], build: [] }, channels: Channels("4020212223303800"), moniker: Moniker("technodrome"), other: OtherInfo { tx_index: On, rpc_address: Tcp { peer_id: None, host: "0.0.0.0", port: 26657 } } }, sync_info: SyncInfo { latest_block_hash: Sha256([212, 177, 17, 67, 176, 201, 203, 19, 48, 186, 237, 130, 92, 159, 239, 19, 151, 156, 145, 225, 55, 223, 147, 195, 151, 74, 23, 201, 190, 214, 99, 237]), latest_app_hash: Sha256([56, 254, 63, 6, 227, 235, 147, 108, 46, 225, 77, 166, 190, 161, 95, 151, 254, 248, 129, 72, 36, 240, 34, 238, 6, 99, 93, 123, 44, 57, 160, 186]), latest_block_height: block::Height(410744), latest_block_time: Timestamp(2019-04-15T13:16:17.316509229Z), catching_up: false }, validator_info: ValidatorInfo { address: Id([199, 56, 51, 233, 189, 134, 211, 78, 218, 212, 175, 213, 113, 251, 93, 9, 38, 41, 76, 213]), pub_key: Ed25519(signatory::ed25519::PublicKey(45:b9:73:30:ee:22:b3:92:f5:85:9c:fa:c2:8e:24:3d:ba:6d:cc:ec:ae:7b:a2:d3:93:8f:a5:3e:10:f5:15:19)), voting_power: VotingPower(0) } } }

Looks like we could probably use some custom Debug impls for some of the types above, but the good news is they all appear to be parsing correctly

liamsi

The changes look good to me! I would like to do more manual testing but this doesn't block this PR.

Thanks @tarcieri!

tarcieri requested a review from liamsi April 15, 2019 15:33

tarcieri force-pushed the tendermint-rs/updates-and-cleanups branch from 31f1be0 to 53c9931 Compare April 15, 2019 15:34

tarcieri force-pushed the tendermint-rs/updates-and-cleanups branch from 53c9931 to f9953ea Compare April 15, 2019 15:41

tarcieri force-pushed the tendermint-rs/updates-and-cleanups branch 2 times, most recently from 122b5e0 to e9ca43e Compare April 15, 2019 17:19

tendermint-rs: Initial Moniker type

934f7d8

Adds a type for representing monikers

tarcieri force-pushed the tendermint-rs/updates-and-cleanups branch from e9ca43e to 934f7d8 Compare April 15, 2019 17:19

tony-iqlusion added 2 commits April 15, 2019 10:20

tendermint-rs: Impl FromStr, Deserialize, and Serialize for Hash

58e41e9

Support for serializing and deserializing hash values from hex strings

tendermint-rs: Impl Serialize and Deserialize for PublicKey

4e2af2c

Adds serde serializers/deserializers for PublicKey types which support the JSON serialization used by the JSONRPC interface.

tarcieri force-pushed the tendermint-rs/updates-and-cleanups branch from adea22b to 4e2af2c Compare April 15, 2019 17:45

tendermint-rs: account::Id type

611692b

Adds a type for representing IDs of Tendermint accounts, which are computed as a truncated hash of a secp256k1 public key.

tarcieri changed the title ~~[WIP] tendermint-rs: Fixes, cleanups, and updates~~ tendermint-rs: Fixes, cleanups, and updates Apr 15, 2019

tarcieri commented Apr 15, 2019

View reviewed changes

tony-iqlusion added 2 commits April 15, 2019 11:35

tendermint-rs/Cargo.toml: Document all features on docs.rs

248c39d

tendermint-rs: impl Serialize for account::Id and node::Id

39cfe2d

liamsi approved these changes Apr 16, 2019

View reviewed changes

tarcieri merged commit 689f48a into master Apr 16, 2019

tarcieri deleted the tendermint-rs/updates-and-cleanups branch April 16, 2019 16:13

This was referenced Apr 16, 2019

tendermint-rs v0.6.0 #230

Merged

tendermint-rs v0.7.0-alpha1 #240

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tendermint-rs: Fixes, cleanups, and updates #228

tendermint-rs: Fixes, cleanups, and updates #228

tarcieri commented Apr 15, 2019 •

edited

Loading

tarcieri commented Apr 15, 2019

tarcieri commented Apr 15, 2019

tarcieri Apr 15, 2019

liamsi Apr 16, 2019 •

edited

Loading

tarcieri Apr 16, 2019

tarcieri Apr 16, 2019 •

edited

Loading

liamsi Apr 16, 2019

tarcieri Apr 15, 2019 •

edited

Loading

tarcieri Apr 15, 2019

tarcieri Apr 15, 2019

tarcieri Apr 15, 2019

liamsi Apr 16, 2019

tarcieri Apr 16, 2019 •

edited

Loading

tarcieri commented Apr 15, 2019

liamsi left a comment

tendermint-rs: Fixes, cleanups, and updates #228

tendermint-rs: Fixes, cleanups, and updates #228

Conversation

tarcieri commented Apr 15, 2019 • edited Loading

tarcieri commented Apr 15, 2019

tarcieri commented Apr 15, 2019

tarcieri Apr 15, 2019

Choose a reason for hiding this comment

liamsi Apr 16, 2019 • edited Loading

Choose a reason for hiding this comment

tarcieri Apr 16, 2019

Choose a reason for hiding this comment

tarcieri Apr 16, 2019 • edited Loading

Choose a reason for hiding this comment

liamsi Apr 16, 2019

Choose a reason for hiding this comment

tarcieri Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

tarcieri Apr 15, 2019

Choose a reason for hiding this comment

tarcieri Apr 15, 2019

Choose a reason for hiding this comment

tarcieri Apr 15, 2019

Choose a reason for hiding this comment

liamsi Apr 16, 2019

Choose a reason for hiding this comment

tarcieri Apr 16, 2019 • edited Loading

Choose a reason for hiding this comment

tarcieri commented Apr 15, 2019

liamsi left a comment

Choose a reason for hiding this comment

tarcieri commented Apr 15, 2019 •

edited

Loading

liamsi Apr 16, 2019 •

edited

Loading

tarcieri Apr 16, 2019 •

edited

Loading

tarcieri Apr 15, 2019 •

edited

Loading

tarcieri Apr 16, 2019 •

edited

Loading