pex: update pex messages

[cmwaters]

This PR proposes a p2p breaking change, cleaning up the PEX messages by removing the v1 version for the legacy p2p system and having a single pex receive and response message type.

[tac0turtle]

this will be treated as a stop the node and restart type of upgrade?

[cmwaters]

Yup. It would require a coordinated restart. But regardless of this change I think 0.36 will be coordinated upgrade anyway

[creachadair]

I recommend marking the removed field tags as reserved so we don't try to reuse them later.

Suggested change

	string url = 1 [(gogoproto.customname) = "URL"];
	string url = 1 [(gogoproto.customname) = "URL"];
	reserved 2, 3; // See https://github.com/tendermint/spec/pull/352

[creachadair]

Suggested change

	PexResponse pex_response = 2;
	PexResponse pex_response = 2;
	reserved 3, 4; // See https://github.com/tendermint/spec/pull/352

[cmwaters]

Thanks, I wonder if we should reserve 1, 2, 3, and 4 and have 5 and 6 for the new pex_request/pex_response?

[cmwaters]

Is something proto breaking if you rename it?

[creachadair]

Is something proto breaking if you rename it?

It is a breaking change to the generated code, but as long as the structural type doesn't change it's wire-compatible. The main reason to reserve old field tags is for wire-compatibility.

[creachadair]

Thanks, I wonder if we should reserve 1, 2, 3, and 4 and have 5 and 6 for the new pex_request/pex_response?

I didn't actually look at the history prior to this diff. I think it's generally a good idea to never reuse a field tag. A common trick is to keep a // next id: 25 comment in each message type so that you can tell what to use next quickly.

The temptation to try to keep the numbering compact and visually ordered is understandable, but probably not worthwhile: The intent of explicit field tags is so that newer messages don't blow up older software (they will simply ignore the unknown tags) and older messages can still be read by newer software. The "reserved" syntax just helps the proto compiler prevent us from re-using them.

[tac0turtle]

what are your thoughts on versioning the proto files? seems like this could be a good first use case for this?

[creachadair]

what are your thoughts on versioning the proto files? seems like this could be a good first use case for this?

I'm ambivalent about versioning the file. I don't really see what problems adding another version marker would solve.

If we break the wire format, we'll have to update the software version anyway, and if we don't break the wire format, then the existing software should continue to work without changes. That's the original reason for the schema language having explicit field tags.

Of course, we might add new fields and need to update the software to take advantage of them, but at that point the version the client cares about is the version of the service it's talking to, not the schema file.

IMO wire-format changes are the only ones that matter. Even if the generated code changes, old versions of the library should continue to work as well as they ever did. If we want to change how the service interprets old messages (e.g., we cut out some feature and start reporting errors) then the client has to update because the service version changed.

[cmwaters]

Ok I made some updates to this file. It seems like you can't use reserved within a oneof sum so I've just left a comment

[creachadair]

The field tags inside a sum are in the same namespace as the enclosing message, so you can simply put the reserved clause outside the oneof declaration. (I wrote it wrong in my example)

[cmwaters]

ok thanks!