Skip to content
This repository has been archived by the owner on Feb 23, 2022. It is now read-only.

pex: update pex messages #352

Merged
merged 4 commits into from
Oct 13, 2021
Merged

pex: update pex messages #352

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cecd0fe
update pex messages
cmwaters Oct 8, 2021
d88d71d
add a quick note about URL parsing
cmwaters Oct 8, 2021
6064a4f
add reserved fields
cmwaters Oct 13, 2021
07cbc8e
add reserved tag to pex message
cmwaters Oct 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 1 addition & 15 deletions proto/tendermint/p2p/pex.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ option go_package = "github.com/tendermint/tendermint/proto/tendermint/p2p";
import "gogoproto/gogo.proto";

message PexAddress {
string id = 1 [(gogoproto.customname) = "ID"];
string ip = 2 [(gogoproto.customname) = "IP"];
uint32 port = 3;
string url = 1 [(gogoproto.customname) = "URL"];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

message PexRequest {}
Expand All @@ -17,21 +15,9 @@ message PexResponse {
repeated PexAddress addresses = 1 [(gogoproto.nullable) = false];
}

message PexAddressV2 {
string url = 1 [(gogoproto.customname) = "URL"];
}

message PexRequestV2 {}

message PexResponseV2 {
repeated PexAddressV2 addresses = 1 [(gogoproto.nullable) = false];
}

message PexMessage {
oneof sum {
PexRequest pex_request = 1;
PexResponse pex_response = 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is something proto breaking if you rename it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok thanks!

PexRequestV2 pex_request_v2 = 3;
PexResponseV2 pex_response_v2 = 4;
}
}
33 changes: 2 additions & 31 deletions spec/p2p/messages/pex.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@ Pex has one channel. The channel identifier is listed below.

## Message Types

The current PEX service has two versions. The first uses IP/port pair but since the p2p stack is moving towards a transport agnostic approach,
node endpoints require a `Protocol` and `Path` hence the V2 version uses a [url](https://golang.org/pkg/net/url/#URL) instead.

### PexRequest

PexRequest is an empty message requesting a list of peers.
Expand All @@ -33,32 +30,8 @@ PexResponse is an list of net addresses provided to a peer to dial.

### PexAddress

PexAddress provides needed information for a node to dial a peer.

| Name | Type | Description | Field Number |
|------|--------|------------------|--------------|
| id | string | NodeID of a peer | 1 |
| ip | string | The IP of a node | 2 |
| port | port | Port of a peer | 3 |


### PexRequestV2

PexRequest is an empty message requesting a list of peers.

> EmptyRequest

### PexResponseV2

PexResponse is an list of net addresses provided to a peer to dial.

| Name | Type | Description | Field Number |
|-------|------------------------------------|------------------------------------------|--------------|
| addresses | repeated [PexAddressV2](#PexAddressV2) | List of peer addresses available to dial | 1 |

### PexAddressV2

PexAddress provides needed information for a node to dial a peer.
PexAddress provides needed information for a node to dial a peer. This is in the form of a `URL` that gets parsed
into a `NodeAddress`. See [ParseNodeAddress](https://github.com/tendermint/tendermint/blob/f2a8f5e054cf99ebe246818bb6d71f41f9a30faa/internal/p2p/address.go#L43) for more details.

| Name | Type | Description | Field Number |
|------|--------|------------------|--------------|
Expand All @@ -72,5 +45,3 @@ Message is a [`oneof` protobuf type](https://developers.google.com/protocol-buff
|--------------|---------------------------|------------------------------------------------------|--------------|
| pex_request | [PexRequest](#PexRequest) | Empty request asking for a list of addresses to dial | 1 |
| pex_response | [PexResponse](#PexResponse) | List of addresses to dial | 2 |
| pex_request_v2 | [PexRequestV2](#PexRequestV2) | Empty request asking for a list of addresses to dial | 3 |
| pex_response_v2 | [PexRespinseV2](#PexResponseV2) | List of addresses to dial | 4 |