node::Id changes values to capital letters in hex encoding

[tzemanovic]

Steps to reproduce

The node::Id type (used in e.g. TendermintConfig via net::Address) rewrites values to capital letters when parsed with FromStr and serialized with Display. When used with e.g. the persistent_peers field in the p2p config, the capitalized node ID then do not match on connection authentication and hence cause failures.

What's the definition of "done" for this issue?

The node::Id shouldn't modify the values parsed from raw strings.

[tony-iqlusion]

I kind of disagree with this. I think there should be a canonical capitalization which is always output consistently, and inputs that are non-canonical can be accepted, but I don't think it's expected they should round trip to the input.

As an example edge case here, what about this sort of capitalization?

• aAaaaaAAaaaaAAAAAAAaaaaaaAAAAAAAAAAAAAaaaAAA...

If anything, I think it'd be much more confusing if that were echoed back verbatim.

[tzemanovic]

I kind of disagree with this. I think there should be a canonical capitalization which is always output consistently, and inputs that are non-canonical can be accepted, but I don't think it's expected they should round trip to the input.

As an example edge case here, what about this sort of capitalization?

* `aAaaaaAAaaaaAAAAAAAaaaaaaAAAAAAAAAAAAAaaaAAA...`

If anything, I think it'd be much more confusing if that were echoed back verbatim.

The node ID as defined in TM Go uses Address from the crypto package, according to which the edge case example (or any mixed case string for that matter) would be invalid. They use raw bytes comparison, so it's case-sensitive. I'm not sure what's the best way to solve this, but unless this is changed in TM Go, I don't think this canonicalization in tendermint-rs is correct.

[thanethomson]

@tzemanovic could you please provide more detail as to how to reproduce this issue? With error logs, config, etc.

It'll provide clarity on exactly where the problem is and who should be responsible for fixing it.

[tzemanovic]

@tzemanovic could you please provide more detail as to how to reproduce this issue? With error logs, config, etc.

It'll provide clarity on exactly where the problem is and who should be responsible for fixing it.

Sure, in anoma, we're using tendermint-rs to update the configuration for tendermint. Because its Display instance formats node IDs in capital hex, it causes mismatch in tendermint's p2p authentication, as it doesn't match the expected node ID, which is used in tendermint in lowercase hex with case sensitive comparison, e.g.:

I[2021-11-02|13:23:47.445] Dialing peer                                 module=p2p address=3C4C144078FBCDE78FBA38330C94C81C678D00E9@127.0.0.1:26661
E[2021-11-02|13:23:47.456] Error dialing peer                           module=p2p err="auth failure: conn.ID (3c4c144078fbcde78fba38330c94c81c678d00e9) dialed ID (3C4C144078FBCDE78FBA38330C94C81C678D00E9) mismatch"

[thanethomson]

Can Tendermint validly have two different peers on the same network with IDs 3c4c144078fbcde78fba38330c94c81c678d00e9 and 3C4C144078FBCDE78FBA38330C94C81C678D00E9? That doesn't seem right to me (i.e. it seems like something that should be fixed in Tendermint).

[thanethomson]

I suppose it states in the code that node IDs must be lowercase: https://github.com/tendermint/tendermint/blob/d32913c889b3e7a94c6fc73ad2669523d4ba23e4/types/node_id.go#L22

So we should conform to that.