Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split handshake vs orders protocol versions #16531

Merged
merged 7 commits into from Jun 20, 2019

Conversation

@pchote
Copy link
Member

commented May 11, 2019

This PR takes into account the recent discussion related to the immediate order changes, and updates (and documents) the network protocol to support them without breaking runtime mod switching.

Fixes #16459.
Supersedes #16467.

Key points:

  • The ServerOrder class has been removed, and immediate orders (except for the handshake) are serialized via the 0xFF order type.
  • The 0xFE order changes from #15615 are reverted, and this order type now only used for the handshake request and response.
  • The old server protocol version is redefined to apply only to the handshake, and kept at version 7
  • A new orders protocol version is defined and incremented to version 8
  • Clients tell the server the orders protocol that they are using in their handshake response, which custom servers can use to account for protocol differences and support multiple in parallel
  • A new OrderType enum is introduced to replace hardcoded magic numbers
  • The network packet structure is documented in ProtocolVersion.cs

I expect a followup PR after this one will bump the orders protocol to 9, which will enable the rest of the target fields for immediate orders and change the lobby commands to use these instead of manually packing and unpacking order data into strings.

I recommend enabling "Hide whitespace changes" in the Diff settings when reviewing.

@pchote pchote requested a review from jrb0001 May 11, 2019

@pchote pchote force-pushed the pchote:order-protocol branch from 45c3f15 to d5fd2b9 May 11, 2019

@pchote pchote added this to the Next Release milestone May 11, 2019

@pchote pchote force-pushed the pchote:order-protocol branch from d5fd2b9 to 56e8cb8 May 11, 2019

OpenRA.Game/Network/Order.cs Outdated Show resolved Hide resolved

@pchote pchote force-pushed the pchote:order-protocol branch from 56e8cb8 to a991d4c May 11, 2019

@pchote pchote force-pushed the pchote:order-protocol branch from a991d4c to de66f5d May 11, 2019

@pchote pchote requested a review from abcdefg30 May 11, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented May 11, 2019

Updated. This now also removes ServerOrder as discussed previously in IRC.

@pchote pchote force-pushed the pchote:order-protocol branch from de66f5d to 29bf97c May 11, 2019

public static readonly int Version = 7;
// OpenRA's network protocol defines a packet structure:
// - Int32 specifying the length of the packet, ignoring this length field.
// The connection will be terminated if a packet with length > 131072 is received by the server

This comment has been minimized.

Copy link
@Phrohdoh

Phrohdoh May 17, 2019

Member

Why 131072?

This comment has been minimized.

Copy link
@pchote

pchote May 18, 2019

Author Member

"128kB is enough for anyone"

@jrb0001
Copy link
Contributor

left a comment

I still think that stacked handshakes is a really bad approach unless there is simply no other way. In this case, the initial handshake could be changed in a fully backwards compatible and also much more future-proof way and combined with dedicated compatibility code, mod switching would still work with all client-server combinations. Also it would allow dropping the compatibility code when it is no longer needed.

You need to test both version 7 client --> version 8 server as well as version 8 client --> version 7 server if you want to make sure that the switching works in all cases.

OpenRA.Game/Server/ProtocolVersion.cs Show resolved Hide resolved
OpenRA.Game/Server/ProtocolVersion.cs Outdated Show resolved Hide resolved
OpenRA.Game/Server/ProtocolVersion.cs Show resolved Hide resolved
OpenRA.Game/Server/ProtocolVersion.cs Outdated Show resolved Hide resolved
// - [optional] Fingerprint: String used to query the players authentication public key
// - [optional] AuthSignature: AuthToken signature generated using the client's authentication private key
// - [optional] OrdersProtocol: ProtocolVersion.Orders that the client will use (assumed 7 if omitted)
// - Server disconnects client if Mod or Version do not match or it does not accept the requested OrderProtocol

This comment has been minimized.

Copy link
@jrb0001

jrb0001 May 18, 2019

Contributor

That won't work for version 8 client --> version 7 server.

This comment has been minimized.

Copy link
@pchote

pchote May 18, 2019

Author Member

This isn't a problem in practice because the client will have detected the mod/version mismatch and prompted the player to switch to the version 7 client.

OpenRA.Game/Server/Server.cs Outdated Show resolved Hide resolved
OpenRA.Game/Network/Order.cs Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member Author

commented May 18, 2019

I still think that stacked handshakes is a really bad approach unless there is simply no other way.

So far nobody has proposed a concrete solution that doesn't break the mod switching, so there is currently no other way.

You need to test both version 7 client --> version 8 server as well as version 8 client --> version 7 server if you want to make sure that the switching works in all cases.

I have already tested this, but note that the orders protocol doesn't come into this at all - compatibility is provided by the long established mod/version checks. The OrdersProtocol addition isn't needed for this, and was added mainly to cover your server needs.

@pchote pchote force-pushed the pchote:order-protocol branch 3 times, most recently from d3d1fe4 to 143bbfa May 18, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented May 18, 2019

Updated.

OpenRA.Game/Server/Server.cs Outdated Show resolved Hide resolved

@pchote pchote force-pushed the pchote:order-protocol branch from 143bbfa to 2cacbfc May 31, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

Rebased and updated.

@pchote

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2019

Just a reminder that this is still blocking the Mod SDK - fixing that before we merge this risks modders shipping the broken bleed orders code, causing a split in online compatibility.

I've also been recently talking with someone who is very interested in adapting our netcode to build a live-joinable sole-survivor type mod/game. Their progress is likewise blocked on this.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

@jrb0001 could you confirm if this works for you and fixes the issue if you find time for it?

@jrb0001

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

@matjaeck #16459 is obsoleted but not fixed by this PR. There is still the issue with thirdparty mods following bleed but I can't see any good way to deal with that incompatibility.

I am not sure what exactly I should test. pchote told me that he did interop tests iirc, but I have no idea what exactly they included and what they didn't.

@teinarss
Copy link
Contributor

left a comment

Otherwise its looks good!

OpenRA.Game/Network/Order.cs Outdated Show resolved Hide resolved
pchote added 7 commits May 11, 2019
Remove unused field from HandshakeRequest.
This field was not serialised, so compatibility
is not impacted.
Restore 0xFE order for handshakes.
This restores handshake compatibility with protocol 7 servers.
Remove unused PauseGame handling.
Pause is not an immediate order.
Split Protocol version into Handshake vs Orders.
Handshake is kept at 7.
Orders is incremented to 8 to reflect immediate order changes.

@pchote pchote force-pushed the pchote:order-protocol branch from 2cacbfc to 2189dfa Jun 20, 2019

@pchote pchote requested a review from abcdefg30 Jun 20, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

Replaced NotImplementedException with InvalidDataException and rebased. Ping @abcdefg30 and @teinarss for re-review.

@teinarss
Copy link
Contributor

left a comment

Looks good now!

@pchote pchote added the PR: Needs +2 label Jun 20, 2019

@abcdefg30 abcdefg30 merged commit a260b50 into OpenRA:bleed Jun 20, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

@pchote pchote deleted the pchote:order-protocol branch Aug 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.