informalsystems / tendermint-rs

Client libraries for Tendermint/CometBFT in Rust!
Apache License 2.0
605 stars 224 forks source link

Can't get status when validator key is blank #1417

Open penso opened 5 months ago

penso commented 5 months ago

What went wrong?

There is an issue with secp256k1 parsing blank secp256k1 public keys.

❯ export TENDERMINT_RPC_URL=https://dydx-rpc.publicnode.com
❯ ./target/release/tendermint-rpc status
2024-04-27T16:52:01.302601Z  INFO tendermint_rpc: Using HTTP client to submit request to: https://dydx-rpc.publicnode.com/
2024-04-27T16:52:01.647400Z ERROR tendermint_rpc: Failed: serde parse error

Caused by:
    invalid secp256k1 key at line 1 column 1113

Location:
    /Users/penso/.cargo/registry/src/index.crates.io-6f17d22bba15001f/flex-error-0.4.4/src/tracer_impl/eyre.rs:10:9

The status json includes:

{
    "validator_info": {
      "address": "0000000000000000000000000000000000000000",
      "pub_key": {
        "type": "tendermint/PubKeySecp256k1",
        "value": "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
      },
      "voting_power": "0"
    }
}

I don't think an invalid key should block using tendermint-rs for RPC calls, but not sure how this should be fixed. Do we expect public keys to be verified by default?

tony-iqlusion commented 5 months ago

Ugh, it's rather unfortunate whatever decided to do that used a garbage public key, especially as the value 0 is an actually valid SEC1 encoding of the identity point which would've made more sense for this application.

What decided to encode the key this way? Was it CometBFT itself?

penso commented 5 months ago

Ugh, it's rather unfortunate whatever decided to do that used a garbage public key, especially as the value 0 is an actually valid SEC1 encoding of the identity point which would've made more sense for this application.

What decided to encode the key this way? Was it CometBFT itself?

I'm not sure how it got there since I'm not involved in CometBFT neither DYDX. I feel like it's related to the node itself, maybe in its configuration?

tony-iqlusion commented 5 months ago

There is a way to decode these points where k256 will accept this value, namely 33-bytes of all zero: using <k256::AffinePoint as GroupEncoding>::from_repr, which always accepts a 33-byte array as input, and will decode 33-bytes of all zero as the identity point:

https://docs.rs/k256/latest/k256/struct.AffinePoint.html#impl-GroupEncoding-for-AffinePoint

Note however this is not a standard SEC1 encoding, just an eccentricity of how that particular API works because it takes an array as input and can't handle a shorter message, but if it matches what I hope is a CometBFT behavior and not some chain-specific one-off behavior it would fix this particular problem.

The current tendermint::PublicKey::Secp256k1 variant expressly disallows the identity point though, as it's not typically a valid public key, so switching to that would entail changing that variant to represent all secp256k1 public keys as k256::AffinePoint rather than k256::ecdsa::VerifyingKey, or handling this case with e.g. Option, which might be a bit onerous from an API perspective.

penso commented 4 months ago

As a follow-up, the way I "fixed" it is using my own code with reqwest (and not tendermint-rs) to fetch status information from nodes.