Modern Forge (1.13+) support

[danorris709]

Adds a fix for #248

Updates the Forge connection logic for the new Forge client handshake and adds the "reset packet" sending implementation. There is a pending Forge fix for interpreting this packet, however this has to come first (especially given the fix doesn't have to exist in Forge for the solution to work to begin with).

This has all been tested with Vanilla, Spigot, and Forge (and Forge with mods) clients and servers interchangeably.

[Xernium]

While I really welcome this I really don’t want another forge-specific implementation.
I’m not putting in my veto though and I’m far too busy at the moment to oversee this so I’m leaving it in the hands of either @electronicboy or @astei or another paper dev team member to decide.
What I don’t like however is how the brigadier types are handled, in my opinion a capsulation should be negotiated so this can work with the existing CrossStitch code.
And the breaking api change has to go for this to be considered within the next minor version releases, please correct that.
But other than that I certainly didn’t expect this and it’s a welcome surprise.

[astei]

I am actually familiar with this work. There are some show-stopper issues with this, though.

The biggest showstopper of all is if the Forge team is willing to accept this. We are more than willing to accept this and put in the legwork - by popular request, up to the point where I have received offers from people willing to compensate me in cold hard cash for this.

[Rasgnarok]

Pixelmon has been promoting this solution for which we've been discussing with Lex on Forge's role in this. Much like Velocity, they will have their own changes to propose, but Forge is absolutely willing to accept a solution proxies will use @astei .

[LexManos]

Just to be clear on Forge's end. We are not against adding a hook for the proxy community. As long as it's sane/not bespoke. If we have to add a custom thing for every proxy project out there it's not going to happen. So if the proxy community is able to agree on a solution, and the implementation on Forge's end is sane. Then I'll be willing to entertain it.

From my perspective, the main requirement I have is it MUST fully disconnect the user and let him reconnect. Any other solution would just cause more potential headakes.

The rest of it is up for you guys to decide:
Is there data that needs to be synced to the proxy?
Is this allowing reconnecting to a different ip/port?
How to keep it secure for end users, so that random servers cant send them to other random servers?
Anything else you guys need to agree on.

Once then it should be written up as a spec outside of thee implementation so that other proxy {and modloader} services can implement it.

Yes, the Pixelmon people have talked to me about this, and are pushing for it, Same with the guy who offered large sums of cash to fix it. I have not agreed to include any solution because I have no seen a spec that has been agreed upon by your community. But I am willing to entertain it if you guys can actually agree on something.

[astei]

As long as it's sane/not bespoke. If we have to add a custom thing for every proxy project out there it's not going to happen.

Agreed on this one.

Is there data that needs to be synced to the proxy?

As long as Forge is able to re-negotiate the client-server handshake, then we're fine. In fact, it would be best if Velocity was not involved beyond forwarding messages and sending the reset packet, the more to make the support portable.

Is this allowing reconnecting to a different ip/port?

No. From the client side, the player always remains connected to the proxy, which is acting as the server. The only change is that Velocity is sending the Join Game and Respawn packet to reset the client state for the new server. From there, Velocity just forwards packets back and forth.

How to keep it secure for end users, so that random servers cant send them to other random servers?

Server switches are always mediated by the proxy, which will only do the switch in response to a disconnect from the server (in which case Velocity will attempt to fall back to a different server) or someone firing a connection request (be that through the API, or most likely, the BungeeCord plugin messaging channel).

[danorris709]

I believe this solution (and the soon to follow Forge side) matches all the above requirements and doesn't create any vulnerabilities. I am confident it's not specific to one proxy solution and am perfectly happy to write up the specification for others to follow if necessary

[LeStegii]

Sorry for the meme, but is this really just happening?

[astei]

A couple more changes and we can merge this.

[LexManos]

No. From the client side, the player always remains connected to the proxy, which is acting as the server. The only change is that Velocity is sending the Join Game and Respawn packet to reset the client state for the new server. From there, Velocity just forwards packets back and forth.

I understand how proxies currently work yes, that does not work in a modded environment because you can not get back into the handshake mode. And things like registry data is still in use. Which is why I said:

From my perspective, the main requirement I have is it MUST fully disconnect the user and let him reconnect. Any other solution would just cause more potential headakes.

So if you maintain the connection it's a non-starter. I've stated this clearly.

Also, those following questions were not something you should answer to me. They are something other proxy providers have brought up as concerns and you guys have to decide upon themselves. Hence, Is there data that needs to be synced to the proxy? As it is a fresh connection there may be data the proxy needs to know to figure out which backend server you're trying to connect to.

I have tried to explain this as clearly as I can to both you guys, and the Pixelmon folks. If they did not understand that and have started this process with the wrong approach then that is on them. I am leaving it to your side of the community {not JUST Paper, it has to take the other proxy providers into account} to design a spec. On Forge's end it's essentially:

client.disconnect();
ConnnectScreen.startConnecting(currentServer, currentPort)

The entire point is that you get kicked out to essentially the main menu, so that the world, player instance, and everything associated with it gets destroyed. Thus making sure that the rendering thread or anything else are not using registry data. Because registry data needs to be swapped out.

I recommend before you even consider merging this, you do as I instructed the Pixelmon folks, and design a spec that is agreed upon by the other proxy communities, and is documented outside of some random project's implementation.

[danorris709]

No. From the client side, the player always remains connected to the proxy, which is acting as the server. The only change is that Velocity is sending the Join Game and Respawn packet to reset the client state for the new server. From there, Velocity just forwards packets back and forth.

I understand how proxies currently work yes, that does not work in a modded environment because you can not get back into the handshake mode. And things like registry data is still in use. Which is why I said:

From my perspective, the main requirement I have is it MUST fully disconnect the user and let him reconnect. Any other solution would just cause more potential headakes.

So if you maintain the connection it's a non-starter. I've stated this clearly.

Also, those following questions were not something you should answer to me. They are something other proxy providers have brought up as concerns and you guys have to decide upon themselves. Hence, Is there data that needs to be synced to the proxy? As it is a fresh connection there may be data the proxy needs to know to figure out which backend server you're trying to connect to.

I have tried to explain this as clearly as I can to both you guys, and the Pixelmon folks. If they did not understand that and have started this process with the wrong approach then that is on them. I am leaving it to your side of the community {not JUST Paper, it has to take the other proxy providers into account} to design a spec. On Forge's end it's essentially:

client.disconnect();
ConnnectScreen.startConnecting(currentServer, currentPort)

The entire point is that you get kicked out to essentially the main menu, so that the world, player instance, and everything associated with it gets destroyed. Thus making sure that the rendering thread or anything else are not using registry data. Because registry data needs to be swapped out.

I recommend before you even consider merging this, you do as I instructed the Pixelmon folks, and design a spec that is agreed upon by the other proxy communities, and is documented outside of some random project's implementation.

I am sorry, but you are totally and utterly incorrect here @LexManos. I have been in a call with you before where you have outright agreed on statements that directly contradict what you've stated here.

I strongly recommend that you further investigate this issue further, and consider conversations of the past, before making anymore incomprehensible claims.

[danorris709]

I understand how proxies currently work yes, that does not work in a modded environment because you can not get back into the handshake mode. And things like registry data is still in use. Which is why I said:

This statement is incorrect. We have an implementation that does not require a complete disconnect and reconnect packet (using IPs etc.) and that is just not hpw a proxy would work as @astei said.

So if you maintain the connection it's a non-starter. I've stated this clearly.

As I said above, it is a complete starter and it works.

I have tried to explain this as clearly as I can to both you guys, and the Pixelmon folks. If they did not understand that and have started this process with the wrong approach then that is on them. I am leaving it to your side of the community {not JUST Paper, it has to take the other proxy providers into account} to design a spec. On Forge's end it's essentially:

We did not start this process with the wrong approach. We spoke to you two months ago about this, and since we have contacted every single developer possible to attempt to meet your insane requirements. The fact is there is not a single developer in the community who wants to work on this as a result of your attitude towards it.

The proof of that being it has taken two months for us to get anywhere with it, and you've not only ignored our personal messages to you for this but you've chosen to publicly attack the pull request that is on a completely different platform. You could've waited for us to PR Forge and then shut us down rather than going out of your way to shut it down as early as possible.

The entire point is that you get kicked out to essentially the main menu, so that the world, player instance, and everything associated with it gets destroyed. Thus making sure that the rendering thread or anything else are not using registry data. Because registry data needs to be swapped out.

Again, you're completely wrong here and it further proves you really do not know what you're talking about in relation to this. The implementation here is perfectly fine and can be replicated anywhere else. I will have a specification document ready before I make the PR to Forge so please be prepared.

[astei]

We are not going to have mud-slinging in this thread. I truly want this to succeed - users want it, it's a big hole we want filled, and we cannot let petty arguments get in the way.

I would recommend that @danorris709 document how this is going to fully work, on both the proxy and Forge sides before we proceed any further. I believe this is a reasonable request, and it'll also assist in adding modern Forge client support to Waterfall and to any other proxies that wish to support modern Forge.

As for the implementation of this feature, implementing this as a full disconnection and reconnection to the proxy is possible, but not desirable from our view. (I see a path forward with disconnecting the client and reconnecting them with a "cookie", but there are significant problems with this - I cannot elaborate further due to IRL circumstances.) The approach outlined by @danorris709 is consistent with how Forge support has been implemented in BungeeCord and Velocity prior to Forge for Minecraft 1.13. I think the registry sync issues can be worked around - Velocity has been successfully resetting the client state using methods native to the game, and I feel that there has to be a path with native Forge support as well.

[ME1312]

I understand how proxies currently work yes, that does not work in a modded environment because you can not get back into the handshake mode. And things like registry data is still in use. Which is why I said:

From my perspective, the main requirement I have is it MUST fully disconnect the user and let him reconnect. Any other solution would just cause more potential headakes.

So if you maintain the connection it's a non-starter. I've stated this clearly.

...

On Forge's end it's essentially:

client.disconnect();
ConnnectScreen.startConnecting(currentServer, currentPort)

The entire point is that you get kicked out to essentially the main menu, so that the world, player instance, and everything associated with it gets destroyed. Thus making sure that the rendering thread or anything else are not using registry data. Because registry data needs to be swapped out.

I've heard you state these requirements before, but in the end, I'm always left with a single question. If it is important that these objects in forge be thrown away so that they may be rebuilt, why don't you throw them away so they can be rebuilt?

In other words, what is keeping the disconnect from being simulated?

[electronicboy]

I think that the big general issue is the maintainability of it all, it's much easier to just not deal with flow changes and to just restart the connection vs tryna get stuff to line up later and generally replaying a lot of logic in an unexpected state. Theres a good chunk of state in the server and client which is sync'd up during connection, and to keep the existing connection you'd need to caox the client back into a sane state, prime the client to be able to yeet and resync that data, and be hospitable with the current behavioural expectations of mods

This is generally what we discussed years ago as to how it would be best done, as it generally wouldn't require any special consideration from mods, etc; The only caveat I had is if there should be some raw data that should be exchanged back to the proxy for routing purposes to safely identify individual clients across a "switch", i.e. the proxy would send a message to reconnect to the client with some payload with a client ID, or a signed payload akin to {"target": "magicalserver1"}, ofc, the client shouldn't care what that is, IMHO, and should just pass back whatever was sent there

[astei]

@ME1312 Adding a new packet is a hard no from my perspective. All the functionality needed for this can be accomplished through login plugin messages.

[ME1312]

@ME1312 Adding a new packet is a hard no from my perspective. All the functionality needed for this can be accomplished through login plugin messages.

2nd paragraph it is then. OK on everything else?

[ME1312]

Alright, the draft has been started. Let me know what you think! Also, please don't quote the entire post because that would make this thread totally unreadable.

4/25/2022

The following is a draft specification for new plugin messaging channel(s) that proxies could use to reconnect forge clients, using data types and formatting specified by wiki.vg. It is not yet implementable in its current state as the channel identifier constants have yet to be decided upon.

[Clientbound] (Play) Plugin Message: Disconnect

This plugin message is sent by the proxy to initiate the server switch process. Upon receiving this, implementing clients are expected to cache the data, initiate a disconnect from the proxy, and reconnect as soon as possible.

Field	Type	Description
Channel	Identifier	An agreed upon, constant, namespaced identifier for this plugin message channel.
Data	Byte Array	Field Type Description Brand String (32767) A constant value that the proxy uses to identify itself. Data Byte Array Data that supporting proxies will use to complete the transfer. The format is undefined beyond this point, so implementing clients should not attempt to parse the contents.

[Clientbound] Login Plugin Request: Reconnect

This plugin message is sent by the proxy before connecting to a backend server to determine where to send the user. Implementing proxies are expected to send this to any client for which they detect the FML2 header, but there is no harm in sending it to clients of any type should the need arise.

Field	Type	Description
Message ID	VarInt	An ID for which the proxy will await a response.
Channel	Identifier	An agreed upon, constant, namespaced identifier for this plugin message channel.
Data	Byte Array	A zero-length array must be interpereted as a GET request. Otherwise, this is to be interpereted as a RESET request. RESET requests function and contain the same internal data structure as (Play) Plugin Message: Disconnect.

[Serverbound] Login Plugin Response: Connect

This plugin message is sent in response to the above GET request by all clients. It is also sent in the event a RESET request fails to disconnect from the proxy.

Field	Type	Description
Message ID	VarInt	The ID that was received in the previous plugin message.
Success	Boolean	Clients that don't support this specification will output false. Implementing clients that are not in the process of reconnecting must output false. Implementing clients that are in the process of reconnecting must output true.
Data	Byte Array	This value is only present when the above value is true. Field Type Description Brand String (32767) The value the previous proxy identified itself with. It is reasonable for proxies to reject the following transfer data when it comes from an unsupported brand. Data Byte Array Data the previous proxy supplied to complete the transfer.

[Xernium]

Even though I have absolutely no time for this I need to throw this in here; this was my original draft
Packet design-wise your ideas are in the right direction. But till take more than that, like negotiating a capsulation identifier for vanilla/modified features like brigadier

[ME1312]

Hmm... well, the outside (right-most and left-most) paths look like what I specified, but I can't say I understand the logic behind the middle bit at all. Also, isn't that the kind of problem namespaced identifiers were introduced to solve?

[ME1312]

After thinking it over, my main issue with the above chart really is that 2nd disconnect. We should be able to accomplish this functionality with the single disconnect we're given. Although, I have since updated the draft spec to allow for better exception handling. Also, if we really need extra data to move forward, then let me know.

Currently, I believe we should be able to successfully connect forge (and vanilla) users to servers with the following logic:
https://svgshare.com/i/gaa.svg

[ME1312]

What I don’t like however is how the brigadier types are handled, in my opinion a capsulation should be negotiated so this can work with the existing CrossStitch code.

Packet design-wise your ideas are in the right direction. But till take more than that, like negotiating a capsulation identifier for vanilla/modified features like brigadier

Isn't that the kind of problem namespaced identifiers were introduced to solve?

Viewing these together clued me into the fact that you're probably trying to tell me that Forge might be modifying the internal data structure of something under a mojang-standard namespace. If that is indeed the case, then my response would be:

Instead of having Forge negotiate this with the proxy (client - proxy), Forge should negotiate this with itself (client - server). My suggestion for how that should be implemented is:

0. Forge servers should theoretically know ahead-of-time whether or not protocol-breaking mods exist on them, so
1. When protocol-breaking mods are active, move any default vanilla-modified command structures to forge:
2. Forge clients should be able to accept either identifier for the command structure, naturally

[electronicboy]

mods really shouldn't be modifying the built in types, that would just basically be bad practice, namespaces exist for the reason that mods can create their own types without sitting in the vanilla namespace

The proxy needs to modify these data structures in order to do things, i.e. adding commands to the client for things like /server, and thus, we need to be able to decode the structure in some form of manner; The idea of an encapsulation format is basically to wrap said non-standard types so that we don't need to know how to de/encode those specific types, we can just pass them through which will be unwrapped on the other side, akin to what cross stitch does

[ME1312]

I feel like we said the same thing, just with different words. Essentially, what I suggested would make it so anything not minecraft: or brigadier: standard could just be forwarded as-is with no further parsing proxy-side.

[Xernium]

As @electronicboy said this is where it gets interesting.

The proxy may be required to intercept the packets sent from downstream OR upstream:

Such packets may include but arent limited to scoreboards, client settings, resource packs, tablist, respawn ... etc.
There needs to be an encapsulation support inside these packets if the modded server/client interfere or else the proxy cant function correctly.

Secondly, your way of interaction with the server completely breaks both Velocity and Bungeecords event-flow.

In a normal scenario:
Client connect > Login > Login success > Determine server > Send to server
In your scenario:
Client connect > Login > Determine server > Send to server > Login success
Which not only is inconsistent with the flow in future events or server switches but also doesn't account for the reconnections

Addendum:

What I failed to mention in my chart is that once the client disconnects to initiate a reconnect, the connection should be counted as successful and the ChooseInitialServer sequence would run on Velocity in my scenario.

That initial server would then be present already for the reconnection, no events prior to the ChooseInitialServerEvent would be fired and the data from the previous connection would be used + the determined server.

Additionally: You cannot hold the server or client in pre-login-success for long enough that some of the heavier plugins complete blocking events while choosing a server. Vanilla clients in particular have way too restrictive timeouts for that. It would only work for a modded client that expects to wait.

This way you're not breaking the entire architecture of both proxy and possible plugins.
We cannot consider this feature if a complete break in architecture and event-flow is the price

This feature simply isn't important enough for that.

[Xernium]

I feel like we said the same thing, just with different words. Essentially, what I suggested would make it so anything not minecraft: or brigadier: standard could just be forwarded as-is with no further parsing proxy-side.

Ideally yes, but that breaks the API for those on the proxy-side

[ME1312]

If the forge team isn't willing to budge on this...

From my perspective, the main requirement I have is it MUST fully disconnect the user and let him reconnect. Any other solution would just cause more potential headakes.

So if you maintain the connection it's a non-starter. I've stated this clearly.

...and proxies aren't willing to budge on this...

Secondly, your way of interaction with the server completely breaks both Velocity and Bungeecords event-flow.

In a normal scenario: Client connect > Login > Login success > Determine server > Send to server In your scenario: Client connect > Login > Determine server > Send to server > Login success Which not only is inconsistent with the flow in future events or server switches but also doesn't account for the reconnections

Well then, I'm afraid this is once-again the end of the line, unfortunately. We can't just send Login Plugin Requests/Responses in Play state, after all.

[Xernium]

Well then, I'm afraid this is once-again the end of the line, unfortunately. We can't just send Login Plugin Requests/Responses in Play state, after all.

That's not what I was trying to get at. It is both possible, this is the issue I created my sequence chart for. The complexity is just a lot higher.
Essentially: If a verdict on reconnection is reached, the login is marked as "Successful" and an initial server is chosen. The connection is then closed with a reconnect token for the client while its kept as a dummy-connection (waiting) on the proxy.
Upon the reconnection and identification of the client the proxy will re-attach the connection to the waiting connection. From there the connection to the server is established, plugin messages are synced, login is successful, and the server post-connect events are triggered.

For every other client the flow doesnt change.

[ME1312]

That could work, yes, but it still seems overly complicated when compared to just treating the reconnect as a reconnect.

Also, thoughts regarding the following breaking a connection's memory-tracking?

... but I'd like to remind everyone of the extremely rare but non-zero possibility that the proxy instance the user disconnects from might not be the same one they eventually reconnect to (because of load-balancing techniques such as SRV records).

[electronicboy]

that's what the reconnect token is for, hence why I said that if the type of data should be determined by spec, it should probs be something akin to a JWT token

[TepelStreeltjeEoA]

Hi there, no more updates on this?

[Rasgnarok]

For anyone following up on progress here, Pixelmon's added https://github.com/Just-Chaldea/Forge-Client-Reset-Packet/ to the base jar to allow compatibility with this PR, and we will continue to keep the PR up to date with latest Velocity to provide an alternative. We hope that your teams do find a solution for those that have waited since 2018 for it, but until then, Pixelmon will do its best to maintain it for the communities that rely on this beyond 1.13+.

[wildmaster84]

Has there been any more progress (or conversations) about a proper way to add modern forge support or is there still conflict between ideas?

[Rasgnarok]

Has there been any more progress (or conversations) about a proper way to add modern forge support or is there still conflict between ideas?

As far as I've seen, no further discussion has happened. This PR is still pending, and Forge <-> Velocity <-> Paper have not publicly set down on a way to approach this issue without resorting to this PR.

This sadly means you need to rely on community effort to maintain modded proxies until an effort is made across these teams.

[electronicboy]

Forge reached out to several proxy Devs under the basis that they wanted to get people to work together to come up with a solution, afaik nobody other than us have responded to such a thing and so we're in this general joyous position of this being a low priority for us Vs everything else we need to do, and seemingly nobody else caring

[Rasgnarok]

That's simply because you've not community-outreached this. This PR is currently working as expected for several big networks as they've had to go ahead without proxy devs on the matter. You won't see any traction until there is a public, decisive effort from the teams involved to solve it.

As far as everyone knows, this issue has been pending since 2018 with no progress. It's only natural you don't hear about them, when modded networks have been considered dead to proxies, @electronicboy .

I've had a lot of people contact me based on my comment above alone. Perhaps the Paper team could write an FAQ or announcement if they want this interest funneled through its channels.

[electronicboy]

That's simply because you've not community-outreached this. This PR is currently working as expected for several big networks as they've had to go ahead without proxy devs on the matter. You won't see any traction until there is a public, decisive effort from the teams involved to solve it.

Forge has outreached to the relevant parties that they want to work with to come up with a spec which they agree with, out of all the parties involved, we're the only group who has cared, and generally even work a spec up, but it's pretty hard to get a community agreed upon consensus of what to do here when the proxy dev community just doesn't care. public traction is irrelevant here as that's not exactly what we're after, unless somebody can actually get md to respond to his emails, that's moot.

As far as everyone knows, this issue has been pending since 2018 with no progress. It's only natural you don't hear about them, when modded networks have been considered dead to proxies, @electronicboy .

I mean caring on the side of people who can actually get stuff done here, public support doesn't magically make this PR tenable to us. supporting a solution which goes against the interests of actually getting this supported in the core itself would just be backwards, imho.

[Rasgnarok]

While I understand your POV, I think you misunderstand what public views would mean for priority setting and agenda-making. Making this a tenable PR starts by knowing just how needed or wanted it is, and I think that's where differences lie. As long as it's not a priority, it won't be further developed either.

[electronicboy]

public views wouldn't change my own ability to commit to this, this pr as is is NOT tenable, getting the marching band together is very unlikely to change forges hard line on this PR being untenable

[Xernium]

I really don’t care about the politics side of things here.
The velocity team has come to terms with the fact that we likely won’t get a mod-loader independent implementation of this kind of protocol.
That said, some of this PR are parts we won’t/can’t accept or budge on, namely:

1. Alteration of the event flow; It breaks api in a way that would require a major new version of velocity. Possible but out of scope currently
2. Forge specific argument types. We already offer a way to capsulate custom argument types- these have to go.
3. Custom pipeline logic. Same reason as with events
4. Client reliance (too heavily) and the api collisions with existing login api.

If we can work out a solution that gets around all of that, which is hard but not impossible, then the velocity team won’t stand in the way.

[danorris709]

Firstly, it's not just about "getting the marching band together". This PR, alongside the Forge clientside mod, works. You, and the Forge team, are yet to provide any form of solution that works despite your constant barrage of complaints where you claim "we could've done this three years ago". It's not about finding the perfect solution, or finding a solution everyone agrees with because ultimately that will never happen.

Aside from the fact that this needs rebasing why is it that the Velocity team has suddenly changed their mind on accepting this PR? As per last contact it was perfectly fine and the only person standing in the way was Lex. I already went over the breaking API requests and fixed those so I find your point mute? And the same stands for custom Forge arguments.

[electronicboy]

I'm not aware anywhere where the team agreed on this PR in this form being tenably accepted, as individuals we want forge support but we want an agreed upon manner to do this

as it currently stands, we want a generic means which can be implemented by various parties from a proper specification, this needs to be done in a manner that forge and even fabric would be happy to implement, and as it stands, this is currently not it; I'm closing this, as this PR is not tenable to us, and the back and forth is not going to solve anything here unless somebody actually gets the applicable parties invested into a solution which fits at least appeases the most people