Networking Protocol

[fervent token]

soon™️

[mental ingot]

@velvet whale Can you approve the blog post?

Or are there still changes you want to make

[velvet whale]

yes. after the neo pr is merged the build number needs to be in the blog post

[mental ingot]

Okey

But the content is fine?

Are you able to review the PR for the documentation repo as well?

I would like to do: NF PR, Docs PR, and then Blog post with updated links and build numbers

[indigo hazel]

My perspective is that a task that either has a reason to force the server to wait for completion or has the potential of causing a disconnect on the client (i.e. like the registry sync which disconnects on mismatch) then a packet should be sent back as ACK. Otherwise the task can be completed immediately

[fervent token]

what is ACK? I don't recognize the acronym and was half assuming orion was just having a stroke/typo from a phone or something

[buoyant pagoda]

usually shorthand for "acknowledge"

so an ACK packet is usually sent in response to a received packet

[indigo hazel]

In this case basically just a return packet that finishes the task on receipt on the server

On another note: #191 and #289 should be invalid now, right?

#92 as well

[buoyant pagoda]

double-check?

unless Orion coughs up the answers

[fervent token]

okay thanks, I didn't think of it being shorthand rather than an acronym

[indigo hazel]

I'm not home so I can't check, I just remembered we had a few network-related issues 😄

[buoyant pagoda]

one of the common ways to hear of it is with TCP -- SYN and SYN-ACK packets (which is pretty descriptive of what ACK is in context )

[mental ingot]

@opal ferry When you get back from new years festivities, could you give me a ping. There is a descreptency between fabrics and neoforges implementation of the new net protocol

Causing Hypixel to think we are fabric

And triggering a disconnect because the protocol negotiation fails

[obsidian nova]

My question is why is Hypixel negotiating modded data

[mental ingot]

It is needed, to figure out what custom packet payloads exist and function

[obsidian nova]

Yeah, but why does Hypixel care 🤔

[steady sonnet]

anticheat

[obsidian nova]

Fair Ig

[steady sonnet]

and/or mod integrations for things like timers in games

[obsidian nova]

Do they have any of those (yet)?

Well you probably don't know more about Hypickel than me lol

[mental ingot]

You think so right

[obsidian nova]

Because who really does

[steady sonnet]

well, they send special badlion:timers packets but I think they do that through vanilla's thing, not the FML protocol

[mental ingot]

But mods are not privy to the new protocol

Only payloads

[steady sonnet]

Yeah, it won't work for them anymore

[opal ferry]

Tomorrow will be better for me, unless its a quick question 😄

[obsidian nova]

Does the reg sync use a common protocol, or is it just the payload sync?

[opal ferry]

My guess is hypixel uses the minecraft:register packets, I even think bukkit/paper has something to do with them.

[mental ingot]

It does

[opal ferry]

A good test should be to connect to a bukkit/paper and fabric server, with mods/plugins registerting on the same channels. Make sure that you can send packets between them as well as making sure each platform knows the othersides channels.

[mental ingot]

But it seems like they send a format that i did not implement

[opal ferry]

What format are you sending?

It should be a null terminator seperated list of strings. eg: modid:packet\0neo:example\0fabric:example

[mental ingot]

It is not

Because we send the version and optionality of the channel information as well

[opal ferry]

Thats all that should be sent in that packet, let me find the "docs" on it. Give me a sec its hidden somewhere

[mental ingot]

How else can we figure out what is compatible and what is not

[opal ferry]

I cannot find it, but this isnt something that we (or anyone else) changed.

[mental ingot]

Okey

So that is an annoying thing then

Because I did not find proper documentation on this

And i just send the relevant channel information

In a list

You can do that tomorrow

The actuall contents of the payloads is not really high prio issue

[opal ferry]

https://web.archive.org/web/20150323064912/http://www.dinnerbone.com/blog/2012/01/13/minecraft-plugin-channels-messaging/

[obsidian nova]

wiki.vg says it

[opal ferry]

Yeah, thats where I found it

[mental ingot]

That page is way outdated

And does not reflect mojangs implemtation one bit

[opal ferry]

Did you implement my spec using the c:version and c:register packets? These expanded upon minecraft:register to include the target phase.

[mental ingot]

No it is still the minecraft:register as specified in your diagram, as well as the old forge protocol used

[opal ferry]

Im struggling to find proper docs on what minecraft:register should be, its just always been a simple \0 list of strings

[mental ingot]

I think we are hitting the problem on the nose

[opal ferry]

I might not have been clear enough to say that mc:r was left untouched.

[mental ingot]

Because I could not find anything on it either

The problem with MC:R is that its specification itself is outdated

[opal ferry]

Just trying to find it in the bukkit source to double check they still do the same

[mental ingot]

Minecraft already itself has payloads which have more then 16 characters

[obsidian nova]

The 16 limit isn't there

The current setup has been a null-separated list of ResourceLocations

Forge, Fabric, and CraftBukkit+ have used that

Though CB's ResourceLocation parser is overly strict for that packet, but other than that, it adheres

[opal ferry]

@mental ingot see #wisdoms-private message (Private for anyone else), I did initally think about changing this but as you have found its easier said than done 😄

[obsidian nova]

I'm curious, which channel is that 🤔

[opal ferry]

An other option maybe to just not support minecraft:register? and do it all via the c: packets?

Im not sure I want to change things again though.

[buoyant pagoda]

this makes me think of a 'carrier' channel idea

like, a channel that's sent in via minecraft:register, that says "hey, we support c:register"

[opal ferry]

That's what we do now, use minecraft:register to "bootstrap" ourselves into sending the common packets. And we still support the minecraft:register meaning that it should somewhat work on platforms that dont use it.

Anyway, I'll be around more next year (tomorrow). Ill be happy to help test mixed neo/fabric setups.

[buoyant pagoda]

neat

[obsidian nova]

Should I reopen https://github.com/neoforged/NeoForge/issues/348 ?

[buoyant pagoda]

so, @mental ingot to be clear (so we can write up a comment on that issue to explain), the cause is that Hypixel detects the protocol and assumes the client to be Fabric (since that's the primary implementor of that protocol so far), but the implementation of the protocol between Fabric and NeoForge is incompatible?

[mental ingot]

No actually it simply uses the very old protocol for mc:reg

I am not sure where the error is

[buoyant pagoda]

i believe that's technically a different issue...? since the index doesn't exist anymore

[mental ingot]

There is an argument to be had that we are wrong

Because we have now a different format for mc:reg

[buoyant pagoda]

so our implementation of minecraft:register is incompatible with the implementation of Fabric (and vanilla)?

[opal ferry]

What error specifically is hypixel sending back? Does it say anything?

[buoyant pagoda]

I got a similar error response when trying to connect to MCC: Island, with different numbers

...waits for Discord to send the image.

to copy the screenshot from the issue https://github.com/neoforged/NeoForge/issues/450

(that's from a Hypixel connection, according to the issue)

[opal ferry]

Humm, yeah that could be anything. 🤔

Would help to have the server log, it could possibly be caused by a plugin or proxy.

[buoyant pagoda]

did we ever test a Fabric server to NeoForge client connection

[opal ferry]

I'd suggest more testing on top of just being able to connect. Should be able to send packets between the two, make sure the sendable/receivable channels are correct. And maybe even custom configuration tasks.

[obsidian nova]

Just have to wait for Arch API to update

Then there will be at least a few testable mods

[opal ferry]

Id just make a basic test mod for both loaders, its shouldnt be that complex.

[fervent token]

#maintenance-talk message I blame you for making me do this orion

[obsidian nova]

Note: this will probably take a bit, please don't actually wait

Oh you already said that

[mental ingot]

The problem is that next to the negotiation protocol there are a handfull of other protocols that would need to line up for this to be usefull

The registry sync and the config mainly

However I would be completely open to find common groudn here

And adapt to something we can all agree on

[obsidian nova]

It would be so cool if a server, having only multiloader content mods, was able to have clients from both loaders able to join

[opal ferry]

Registry sync is possibly something we can do later, but I think there is value in it for mods that dont add registry entries. Its not uncommon for mods to also have a plugin for bukkit/paper that talks to the client plugin.

I get its going to be less common that you would use a neo/fabric server in this way, but it seems like a neat thing to support 🙂 And does lead to other opportunities down the line.

I'm hoping that one day a reg sync packet wont be needed, but that might be wishful thinking.

[obsidian nova]

Wdym? Like official Mojang support?

[mental ingot]

Well they really only need to sync their network ids over

That is the only thing that we need to do these days to make it work

It is one tiny packet that they would need to send

.....

And that would solve it all

[obsidian nova]

Well they seem to be working on datadriven blocks...

[opal ferry]

If they continue to make everything into a datapack then it might well just fix its self.

[obsidian nova]

What do they use for dynamic registries?

[mental ingot]

Agreed

[mental ingot]

A completely different system

Allthough they should just indeed extend that and all it a day

[wary solar]

for dynamic registries, the entire registry is sent from the server to the client

[obsidian nova]

Oh yeah fair

[wary solar]

it doesn't exist on the client at all until the client gets the registry from the server

so the client registry is guaranteed to be 100% identical to the server registry

(until mods fuck with it at least)

[obsidian nova]

Wouldn't datadriven blocks have the same issue then?

[wary solar]

no, it actually solves the problem

so the way it looks like mojang is setting it up,

they'll split blocks into two registries, a static codec registry and a dynamic block registry

where they have one codec for each existing block class

and each existing block instance gets a block json in the dynamic registry

so the registry of block instances will be 100% synced from server to client

now, that probably won't fix the blockstate id map, but it will make it a problem that suddenly affects mojang

so they'll either need to A) somehow make blocks instantiate in the same order on both sides (both when blocks load on the server and get synced on the client), or B) just yeet the global blockstate id map and make blockstate ids per-chunk only

[obsidian nova]

Blockstate IDs are only used over the network

[wary solar]

correct

but the numbers do currently need to be the same on both sides, in order to be used over the network

[obsidian nova]

So why wouldn't they just use their existing dynamic registry system for that?

[wary solar]

it's not currently a problem for mojang, because blocks init the same way on both sides

but when they move blocks to the dynamic registry... they'd have to make sure the blocks construct in the same order

I don't remember what the registry looks like when serialized

[obsidian nova]

It's ordered

[wary solar]

another potential problem is I'm not sure if dynamic elements construct in the same order they're registered

well, whatever, it's mojang's problem to solve, not ours

[mental ingot]

We will figure something out

[wary solar]

I mean mojang can't make blocks data-driven until block id desync stops being a problem

one of these three things has to happen before they can finish implementing that
A) make sure dynamic reg stuff registers in the same order it gets constructed (it might even already work this way, mooting the point entirely)
B) separate out the creation of blockstate ids so they're not a side-effect of block construction anymore
C) stop using global blockstate ids

[dim wagon]

yeah it's been an interesting one to think about

like they kinda have some fundamental issues they have to rectify before they can proceed with this

I'm just hoping when they do rectify it, they do it properly and not just bandaid it

a proper rectification should allow for item properties to be data driven too

and entitie stats too I think

[steady sonnet]

The blog post says that the event name is RegisterPacketHandlerEvent but it actually appears to be RegisterPayloadHandlerEvent

[static kelp]

the eye bleed is real.. jfc, got enough warnings on here? also, center-aligned and not left aligned like the other blocks..

[buoyant pagoda]

that looks like a very real case of causing warning fatigue

[static kelp]

I can't even look at it. It actually causes me eye pain.

But my stance from before stands: it's a porting and major breaking change, you DO NOT need everything in a warning tag. Shit is expected to break

READ

[buoyant pagoda]

i'm quite surprised https://github.com/neoforged/NeoForge/issues/454 got past us

I'd thought prod was tested

[fair sandal]

Might be a last minute chanhe

In any case we should get rid of these useless factory methods

[errant summit]

is anyone working on this?

[fair sandal]

I don't think so

[mental ingot]

No

But there is also the issue of the map item

With its packet

Which needs a wrapper call

[fair sandal]

toVanillaPacket should maybe be added ye

[errant summit]

where is the issue for that?

[buoyant pagoda]

i don't remember a filed issue for that

so one of youse elaborate so someone can fix it

[mental ingot]

The concept does not really exist anymore

[mental ingot]

There is not. It was a discussion we had in fc

[errant summit]

well I can't fix what I can't see

so either make an issue or post the tl;dr here

[mental ingot]

I know

I will make an issue one I unfuck my pc

[brisk lynx]

Hey, I proposed a tiny documentation fix: https://github.com/neoforged/Documentation/pull/43

[dim wagon]

uh am I missing something or does neither the news page nor the documentation actually say how to send a packet lol

and this is wrong

[brisk lynx]

Just made a PR for that

The wring class name, not the docs on sending a packet

[mental ingot]

Yeah it does not

PacketDistributor is your friend

Or basically any listener like mojang uses can now also send payloads

[dim wagon]

ok that's cool but it's pretty critical info missing from the docs lol

[mental ingot]

It is

[dim wagon]

yay this finally got in lol

[mental ingot]

If you notice something that is missing. Let me know. We can easily add stuff to the context

[dim wagon]

yeah just moving over mods atm

got like 40 packets to do

so we'll soon find out I guess lol

it would be nice to include a note about whether the reply handler is safe to execute in the network thread or whether a completablefuture is required

[mental ingot]

It is safe. But that is a good poiny

[dim wagon]

yeah fairly safe to assume so

but for clarity sake on a public documentation

[mental ingot]

Yeah

[dim wagon]

a lot of modders coming into modding barely understand the basics

[mental ingot]

It is difficult to know what people are looking for documentation wise

[dim wagon]

the less they have to 'know' the better

[buoyant pagoda]

orion, seeing that you're not in a position right now to file an issue report for that "map item with its packet" issue above, would you agree to asking Schurli to retarget https://github.com/neoforged/NeoForge/pull/455 to be solely that major regression fix?

@mental ingot

[mental ingot]

Sure

PC is still fucked

[buoyant pagoda]

@errant summit fyi on the above, please

i'll rename the PR in an hour or so, if you haven't replied yet

[buoyant pagoda]

that was raised in the past by pup

[hollow canopy]

oriononline

[Reference to](#1170668658813575230 message) #1170668658813575230 [➤ ](#1170668658813575230 message)They are async executions from the perspective of the calling thread

sciwhiz12 referenced · Dec 28, 2023, 7:44 PM

[dim wagon]

yeah I'm just posting stuff as I see it while I implement my networking

I disagree with the logic of that response though

when you have a non 'async' marked method

it implies that they run on different threads

but they do not

from the from the perspective of the user (the person writing and interpreting the names of your methods)

it's wrong

[fair sandal]

async means it doesn't happen immediately

doesn't mean anything about which thread executes the task though

[buoyant pagoda]

at the very least, that javadoc should've been clarified that the asynchronicity is from the perspective of the calling thread

[dim wagon]

^

I disagree with calling it async

but if you can't do that

then at least update the javadoc

[buoyant pagoda]

since at first glance, you would assume a contradiction between "async" in the method name and "sync" in the method javadoc

[dim wagon]

the fact that this has been brought up now, and in the past

indicates that that is in fact, true

[fair sandal]

JavaScript is async but single threaded 😉

[dim wagon]

sure

[buoyant pagoda]

doesn't make it any less confusing, though

[dim wagon]

but I'm approaching this from the perspective of someone using neoforge

which I figure might be the intended audience

[obsidian nova]

Sure, but that's because functions are themselves async, not blocking

So I/O can simply let the runtime do other stuff on the same thread

[buoyant pagoda]

and again, that doesn't counter that one likely would assume a contradiction exists between the method name and javadoc, due to the use of opposing terms

[dim wagon]

NetworkHooks gone?

openscreen was handy =/

[velvet whale]

https://neoforged.net/news/20.4networking-rework/#:~:text=Opening menus with custom data

[gaunt lark]

(Seeing as that doesn't actually link for me)

Opening menus with custom data

In the past, NeoForge supported opening UIs from the server side with additional data, via NetworkHooks.openScreen(...). This system has been moved and is now part of the server ServerPlayer extension. You can call the method openMenu with the same parameters.

[buoyant pagoda]

I believe the thingy at the end works for Chromium-based browsers, not Firefox

[gaunt lark]

Smh, he should of used https://neoforged.net/news/20.4networking-rework/#opening-menus-with-custom-data instead, that should be cross browser

[mild spruce]

So with LexForge, Fabric, and previous NeoForge networking.
It is optional to check if the other side has the packet registered.
This is important if we would like to communicate with say Paper/Bukkit plugins. Looking through the code, I see it as a hard check, not an optional check.

[mental ingot]

If you make your payload optional, then it becomes optional

See IPayloadRegistrar.optional() and all payloads forge implements

[mild spruce]

I must have missed that

Let me look, thanks

From the javadoc it looks like it just makes it side optional.
It looks like it will still block sending packets to a bukkit server from the client.

[mental ingot]

It is on the server to tell the client what payloads it supports

[mild spruce]

So bukkit plugin devs need to send over what they can listen to? ugh. Seems a bit overly complex, especially with how bukkit networking works.
It would be nice to allow devs a way to be able to send packets without having that verification check.

[mental ingot]

The literal other way around

The server interogates the client

It asks, what do you need, and what are your optionals. And what versions do these payloads have

The client answers with that information in a list of supported payloads

The server then determines, okey, I have payloads A, the client has payloads B. They are compatible or not

And then either sends the client the list of agreed upon payloads

Or disconnects the client with a set of reasons why

[mild spruce]

Has this been tested with communication with bukkit servers yet?

[mental ingot]

The basic communication protocol is cloned from Fabric

[velvet whale]

has a neo -> fabric client / server setup been tested

[mental ingot]

Allthough it turns out that due to a missing standard for minecraft:register (yes there exists one from dinnerbone, but that is sadly outdated) the implementation forge uses is not directly compatible

[mental ingot]

No

We have no directive for that

And given that the payloads internal structures where not defined, or outdated, I am willing to bet that we used different data structures internally

Making the payloads incompatible

[buoyant pagoda]

I mean, the recent now-resolved issue report shows that testing wasn't done for Neo<->Neo connections in production

[mental ingot]

Even though the basic protocol with the payload order is retained

[buoyant pagoda]

I'd say that paints a pretty bleak picture on the state of testing
we probably ought to institute more rigorous standards of testing for PRs of this magnitude in the future

[velvet whale]

they are defined https://github.com/FabricMC/fabric/pull/3244

[buoyant pagoda]

eh. I'd think that, if we were to implement a shared protocol, we would best look at its reference implementation (in Fabric) and check our compatibility with it

particularly evident for specifications which apparently are not as detailed as they should be

[velvet whale]

does our impl have none of the c packets which are defined by fabric's spec after the discussion in #wisdoms-private?

[mental ingot]

No

We really on minecraft:register solely

[velvet whale]

...that doesn't sound like compliance with the discussed specification

[buoyant pagoda]

then what, pray tell, do we implement from the common channel registration packets idea that's shared with Fabric?

[velvet whale]

sounds like only the loosely defined minecraft:register which the c versioned packets were meant to move the reliance from

[pliant stream]

we probably still need to support register to interoperate with plugins that use it, even if it's not used for mods or for interop with fabric mods

[mental ingot]

The problem is that the c: packets are all way to late for us

And I asked modmuss if we could move them

And the answer was unclear to no

[mild spruce]

NeoForge client -> Fabric server

[14:09:26] [Render thread/WARN] [minecraft/ClientCommonPacketListenerImpl]: Client disconnected with reason: Internal Exception: io.netty.handler.codec.DecoderException: net.minecraft.ResourceLocationException: Non [a-z0-9_.-] character in namespace of location: version\u0000fabric:custom_ingredient_sync\u0000c:register\u0000fabric:re

[velvet whale]

why

[mental ingot]

Because we need them BEFORE registry sync!

[velvet whale]

mhh, mc:reg historically using null chars to separate

[mental ingot]

Yeah

[delicate dawn]

ew (the null char thing)

[mental ingot]

Yeah

Which we currently don't follow

We expect a set of payload information entries in that packet

Or it being empty when the server sends it to the client

[buoyant pagoda]

then we should change to follow

and maybe introduce our own payload sent directly afterwards minecraft:register for our own data (given that both sides discard unknown payloads)

[mental ingot]

No

We discussed that many times over

I even asked if we could get a common protocol, and the answer was no

So I did the best I could

I adapted as much of modmusses protocol that I could

All the communication channel are there

I am willing to create a set of custom "forge:*" payloads

Or work towards making an actual common standard with modmuss that addresses this

But the last time I asked

I got a flat no

[marsh loom]

Why do we require the version before registry sync but Fabric doesn't? i don't think our registries are that incompatible any more..

[mental ingot]

They are!

We still send custom payloads with registry data

[buoyant pagoda]

I despise Discord for this kind of communication, insofar as it's quite difficult to look back on any kind of historical discussion

[marsh loom]

be glad we're not in irc :p

everything would literally be lost to the wilds of time

[velvet whale]

I'm surprised this didn't happen in fc harold

[velvet whale]

this isn't a matter of payload contents it's a matter of why does our payload need to happen before the c: packets

[velvet whale]

also to what is this a no

[buoyant pagoda]

in any case, our next course of action is to correct our implementation of minecraft:register to the format that every server and client expects since its introduction

[mental ingot]

See the protocol from modmuss

[buoyant pagoda]

it is not our place to unilaterally change the format used by minecraft:register (and even then, changing its format would need coordination amongst every one to follow based on some shared signal, perhaps the MC network procotol version)

[mental ingot]

I agree, I missed the mark there

But I made a judgement call, either change the MC:Reg packet to a modern format

Or not follow the fabric protocol at all

Given that we agreed upon following the fabric protocol as much as humanly possible

That is what I did

And there are massive problems with the "use a custom payload" solution

Because nobody and I mean nobody, no server and no client

Speaks the forge protocol

[buoyant pagoda]

I do not see how the implementation of Fabric's common configuration payload protocol affects the format of minecraft:register

[mental ingot]

I had a choice:

[buoyant pagoda]

define "speaks the Forge protocol"

[velvet whale]

neither would they "speak" our modifications to a 10 year old standard

[mental ingot]

Either follow the payload order that fabric uses and adapt the mc:reg packlet to what we need. Or don't follow the protocol and follow mc:reg

[mental ingot]

The payload order would at least be the same

Now

I would propose we figure out a way together with modmuss as to how we can make this work

For all of us

[buoyant pagoda]

additionally, what source do you have for your implementation of the minecraft:network channel?

[mental ingot]

Nothing

I needed an answer packet

There exists none at that time

[buoyant pagoda]

why did you put it under minecraft?

[mental ingot]

Why not?

It is the companion payload to minecraft:register

I see no reason to put it anywhere else

Look

If fabric can agree to pull c:version and c:register

To before the registry sync

Like I already asked before, then it should be doable

I have the code for that somewhere in a local stash

[velvet whale]

well first of all it's a packet specific to us and not a 10 year old standard but I also still fail to see why reg sync has to be before c: packets

what causes this major difference between systems that achieve the same goal

[mental ingot]

Fabric as far as I know does not send any of its own payloads related to the registry sync

Yet we do

So we need the ability to figure out if those payloads are supported

Before the registry sync happens

Additionally: In the red box is when configuration tasks (both modded and vanilla are supposed to run)

Given that modders explicitly should be able to send payloads

All of the setup and negotiation needs to be completed by then

[buoyant pagoda]

it seems that the c:register packet contains the phase at which the channels within it applies for

[mental ingot]

Well that makes some sense

But the c:register spec does not

Because it can't be for the "login" phase

AS this runs after the login phase

[buoyant pagoda]

when exactly does the configuration phase begin

[velvet whale]

fabric has registry sync under its own namespace (as there's no standard for that and doesn't really need to be) https://github.com/FabricMC/fabric/blob/1.20.4/fabric-registry-sync-v0/src/main/java/net/fabricmc/fabric/impl/registry/sync/packet/DirectRegistryPacketHandler.java

[mental ingot]

Interesting

[buoyant pagoda]

perhaps the "registry sync" referred to in the diagram is ambiguous, since it can refer to a mod registry sync (Fabric registries, Forge registries), or the vanilla datapack registry sync (ClientboundRegistryDataPacket)

[mental ingot]

Maybe, but that does not explain the red box

And we asked modmuss about that

You can scroll back both here or in #wisdoms-private

But he was not really reciptive to our comments

I treat registry sync, as one example of a configuration task (as it is one, one we extend with an additional task, but it is one)

[buoyant pagoda]

The red box would be "within this space runs all of the vanilla and modded configuration tasks"

[mental ingot]

Yeah

That is what his diagram shows is it not?

[buoyant pagoda]

well technically vanilla's datapack registry sync isn't a configuration task, since it is just Minecraft sending the packet to the other side

[velvet whale]

Registry sync doesn't refer to vanilla, but to actual modded registry id sync

[buoyant pagoda]

iirc, configuration tasks work on an acknowledgement basis

it is likely that the diagram isn't all encompassing anyway, given that it omits other details like the brand packet, the other vanilla packets sent after that (enabled features, registry data, tags, etc.

[velvet whale]

yes, negotiating the contents of the configuration tasks sounds redundant

[mental ingot]

Where are you two going with these arguments?

[buoyant pagoda]

and isn't meant so much as a specification, but a guide on generally where the packets are sent over

[mental ingot]

?!?

This was literally what we where pointed at as the "Specification"?

And after repeatedly asking

Was confirmed

Even when we pointed out problems with it

[buoyant pagoda]

shrugs

in any case, we probably ought to make a more comprehensive spec, since that diagram lacks some more info I think

[mental ingot]

I completely agree

Do we agree that we want to support the archaic minecraft:register?

[buoyant pagoda]

yes, definitely

[mental ingot]

Okey

I think I can have something tomorrow morning

I have a fully working PC again

[buoyant pagoda]

it has to stay; if not for our sake, then the sake of every other party that understands its well known format

[mental ingot]

Agreed, in hindsight

I think

Allthough I don't like it

We need to create a set of process and communiation diagrams

[buoyant pagoda]

i'm going to try do a dive on the networking and perhaps write a spec, but nobody wait on me because I'm also going to be quite busy

[mental ingot]

As well as object specs that represent the protocol

This can at least be trivially fixed

Since it is just an internal fix

[buoyant pagoda]

as time passes on, my ambition with this idea grows

[mental ingot]

XD

[mild spruce]

I did some testing with NeoForge and LexForge.
They can connect to each other. But NeoForge discards LexForge packets.
Received a modded custom payload packet commonnetworking:example_packet_one with an unknown or not accepted channel. Not parsing packet.
NeoForge Client -> LexForge Server
Many mods have made the effort to make themselves loader side agnostic when playing multiplayer. It is now broken, but I think supporting minecraft:register should fix it.

[opal ferry]

Oh, I was told you were using the c: packets, by the radio silence I assumed everything was working find and dandy. Clearly not...

You should very much be able to send packets between mod loaders and plugin servers. Stuff such as reg sync doesnt need to happen.

[velvet whale]

we will always discard lexforge packets because simplechannel uses an extra int discriminator

[mild spruce]

I wrote this library specifically for Multi Loader template and side agnostic so Fabric and Forge can communicate.
https://github.com/mysticdrew/common-networking

[velvet whale]

it's reimplementing custom payloads on top of custom payloads

[velvet whale]

seems like orion wants the spec to negotiate configuration packets

was your design ever supposed to negotiate those and isn't that unnecessary anyways

[opal ferry]

That should be tottally possible, you can include the intended phase in the c:register packet. We use it during configuration to send the play packets, and minecraft:register to send the configuration packets during configuration.

[velvet whale]

if modded config packets are received before c:register how is that possible

[opal ferry]

You would need to do the two way minecraft:register containing the c packets first.

[velvet whale]

wait so what's the format of mc:registee

[opal ferry]

The minecraft:register packets are fine when used for packets in the current phase, the issue comes when you want to send the packets of another phase

[opal ferry]

A null terminator separated string list of packet names. This has gone unchanged for many years.

[mental ingot]

See this is my problem with minecraft:register

It simply does not work if we split apart the phases

if we keep the old spec

Then we need to assume that the payloads are sendable at all times

Which vanilla does not guarantee

[velvet whale]

which includes the configuration packets, correct? ie neoforge:regsync

[buoyant pagoda]

well, packet in the sense of custom payloads, iirc

[velvet whale]

there's no "if"s here. we are not touching mc:reg. the cost of touching it is huge

[opal ferry]

Yes, we use minecraft:register to send all of the configuration packets, including the reg sync ones and any custom ones. We then use c:register to send the names for the PLAY phase, so they are ready. Previously this was done in login.

[buoyant pagoda]

yeah -- minecraft:register, like with all the vanilla packets and their structures, are off-limits to modifying their format

[mental ingot]

That would mean that no buckit or plugin server could use play packets though

[buoyant pagoda]

when is it appropriate to send c:register for the other two phases?

[opal ferry]

Yes, changing mc:reg breaks compatibility with everything that exists, and its none of our business to do anything to anything in minecraft:

[mental ingot]

Would it be possible to drop: minecraft:register entirely

Or only have it contain the c:* payloads needed for the negotiation

?

[buoyant pagoda]

I would heavily hesitate to not sending minecraft:register

[opal ferry]

That was the goal of c:register, to allow them to do so before PLAY started. Previously we had a proprietary packet set during login, bukkit can still send/receive minecraft:register as before.

[mental ingot]

Yeah but any payload id negotiatied via miencraft:register is only valid in config

Not in play

For that you would need to also handle c:register

[opal ferry]

Whenever really, we send the ones for the play phase during configuration.

[velvet whale]

so mc:register is for the config packets and play packets are c:register

[buoyant pagoda]

...for some reason, I'm getting a severe case of deja vu

[mental ingot]

Yeah

This feels like we are back to where we were 2 months ago

When we were disecting this protocol

And pointing out the flaws and problems

[opal ferry]

mc:register is for the current phase, c:register contains the intended phase (generally not the current phase, but can be)

[mild spruce]

That's a non issue, it can be handled on the buffer before sending and on receiving.
I do it with fabric packets already so my mods can communicate no matter what loader the other side is.
The issue here is the missing negotiation.
When Neo connects to a LexForge server, it thinks it is on a Vanilla server.

[buoyant pagoda]

that sounds like what I've been thinking, yeah
though it implies that we can send minecraft:register again for the play phase, which is an assumption that certain servers might not like

[mental ingot]

Hmm

The original spec would allow it yeah

[opal ferry]

Yes, you can send it again.

[buoyant pagoda]

When Neo connects to a LexForge server, it thinks it is on a Vanilla server.
I don't see the problem here? if NeoForge doesn't receive any info that the other side is also NeoForge, then it assumes it's vanilla

[opal ferry]

We also support a minecraft:unregister does what it says on the tin, but I dont see that being terribly useful tbh.

[buoyant pagoda]

in which case proper handling applies: NeoForge servers with mods (that add to built-in registries, or have required network channels) would reject connections from 'vanilla' clients

it would be up to the LexForge server to determine if the other side is either LexForge or vanilla, and reject connections from 'vanilla' clients (which may be NeoForge, Fabric, vanilla, etc.) based on its own conditions (registry, network, the works)

[mental ingot]

Could we not properly use this?

[opal ferry]

I don't think I've seen anyone actually use it, unless someone has a use case its likely fine to totally ignore? I don't know if you previously supported it?

Being able to reconfigure the client makes it redundant imo.

[buoyant pagoda]

I believe Forge did implement handling for minecraft:unregister, though I don't recall how it implements it exactly

iirc, there was a certain NetworkEvent that gets fired for unregistration of a channel

hmmm, is minecraft:register additive or replacing

[mental ingot]

That is what I was wondering yeah

[opal ferry]

Thinking about it, the previous use case would have likely invoked a proxy between two differently modded servers. Allowing the server to unregister the packets not present on the new server.

[opal ferry]

additive

[mild spruce]

lexforge is also additive.

[buoyant pagoda]

so, minecraft:register can be used at any time to tell the other side that it's accepting payloads for those channels, while minecraft:unregister can be used at any time to tell the other side that it no longer accepts payloads for those channels

perhaps we can make it so both payloads are fired at the start (or middle) and the end of phases, respectively

[mental ingot]

Started working on the flow: https://hackmd.io/Vk6ARdvHSQmj_tdDKS2Bag?view

[buoyant pagoda]

correct me if I'm wrong: is it the ServerboundPongPacket (iirc) that initiates the bulk of the configuration phase (the server sending the vanilla brand/registry/tag data packets?

[indigo hazel]

On a Neo server yes, on a vanilla server no

[velvet whale]

are you rewriting the entire thing

[indigo hazel]

To be specific: on a vanilla server ServerConfigurationPacketListenerImpl#startConfiguration() immediately throws brand, feature, reg data and tags packets at the client. On a Neo server, that method sends minecraft:register and a ping packet and the pong packet then runs the patched-in runConfiguration() method which does what the start method does in vanilla

[buoyant pagoda]

...huh, TIL that the /debugconfig command exists (though only when SharedConstants.IS_RUNNING_IN_IDE is active)

[mental ingot]

Okey here is my first attempt:

https://hackmd.io/Vk6ARdvHSQmj_tdDKS2Bag?both

With just the mc:reg

And the common packets

This is the successfull full flow

I am going to draw up the rest

[velvet whale]

c:versions can be a map and it's actually c:register, and the required stuff doesn't really make sense, that's on the client (in the receiver sense) to check

[mental ingot]

No

The whole idea is that the client does no check what so ever

The server interogates the client

And then sets the channels that are supported

[velvet whale]

that sounds flawed, it should be the receiver that rejects unsupported versions

[mental ingot]

Exactly

That is what the server is here

[velvet whale]

what

versions unsupported by the client shouldn't be rejected by the server

[mental ingot]

The server is the critical receiver

[velvet whale]

that doesn't matter, the server shouldn't be able to override the configuration the client has

[mental ingot]

If the client needs to figure that out, then we need an additional roundtrip

It is more efficient if we only send the versions from one side to the other

And have that side then perform the negotiation

And answer

With the negotiated payloads and versions

[velvet whale]

this isn't about efficiency, it's about trusting the client. if the client says it can only support version X of channel Y it shouldn't be up to the server to decide to disregard it

[mental ingot]

I am not saying he should disregard it

I never said that

Obviously if the client sends X for Y

[velvet whale]

if a server side mod can easily yeet the entire concept of versions something failed

[mental ingot]

Then the server has to respect this

[mental ingot]

That is a massively flawed way of thinking here

[indigo hazel]

And it doesn't. The server asks what the client supports, then compares that to what it supports and only accepts the connection if both sides agree

[mental ingot]

Yes

That is correct xfact

That is already how the negotiation is currently implemetned

[velvet whale]

yes. and the server can easily disregard what the client supports

[mental ingot]

If that is your issue, then we can stop right here.

The underlying channel is exposed, on a binary level. There is no real way for us to prevent any mod, from any side, from using that connection to send a mal constructed payload

[velvet whale]

if the client isn't responsible for validating the versions on its side

[mental ingot]

Regardless of whether the server and client previously agreed

[indigo hazel]

If it does, then the server implementation is broken, simple as that. Neo's implementation does not disregard it

[mental ingot]

Correct

[buoyant pagoda]

when you say "disregard what the client supports", does that mean the server picking a version which the client doesn't support?

[mental ingot]

I think he does

But that makes 0 sense to me

[velvet whale]

I am specifically focused on mods titled "no version checks" or shit like that as a hacky way for servers to allow different versions of mods to work on the client while the mod isn't supposed to work

[mental ingot]

Then you will need to start with removing mixins, and every single coremod access

Because as I said the underlying netty connection is accessible

And incase a mod wants to do that

There is literally nothing we can do to prevent taht

[velvet whale]

you can't pretend to have all possible supported versions of a channel on the server side

[indigo hazel]

Those mods specifically targeted the server list version check display, which doesn't exist anymore. If those now start fucking around with network version negotiation, then all hope is lost

[mental ingot]

Yes I can, because there can only be 1

the c:versions packet and the new neoforge api only support 1 version per channel

So yes

The server knows exactly what version the client supports

Because there is one, or none

[indigo hazel]

To be fair, most people implemented their simplechannel version check the same way: exact equality, so it's not like this is a major change from how it worked previously

[mental ingot]

It is not

We also discussed this at large in here

Up to the point where I suggested even per payload instance versioning as an option

All of that was rejected

For this much simpler API version

[indigo hazel]

I mean, you can still do that by just calling IPayloadRegistrar#versioned() again

[mental ingot]

Practically yes

But it is a bit more static then per payload instance

[indigo hazel]

True

[velvet whale]

not if the server is the first to send the versions. but whatever, it's not me you'll have to convince to change the system once again

[mental ingot]

The server does not send the versions first though

Please see the protocols proposed and approved in fabric

Never mind

Yes the server sends it first in Fabric

But that is a bad idea

[velvet whale]

and why is that

[mental ingot]

Because the client is the untrusty party in the protocol

We do literally everything to validate whatever the client says

And to reduce the influence the client has on decisions the server makes

To move a critical part of the protocol negotiation to it is simply a bad idea in my opinion

[velvet whale]

yes, same argument can be applied if a server ignores the versions requested by the client and decides for the client to proceed, and then send a malicious payload

[mental ingot]

The server ignoring is a break of the agreed upon protocol

[velvet whale]

and arguably clients don't check payloads received from the server because the server is the trusted part

[mental ingot]

And the client gets notified of all selected versions

The client is free to revalidate the work performed by the server if it does not trust it

But having the client dictate this

Is in my opinion not the way to go

[velvet whale]

and an "untrusted" client is also breaking the protocol if it's decided random things based on the versions the server provides it. here it's a matter of what side is more vulnerable. and that's the client because the server is meant to always validate the packets received from the client. clients sometimes can't validate what the server sends to begin with

[mental ingot]

No?

The server is the more vulnarable part

It always has been

It always will be

We have had this discussion now many times over

For example with configs

Or with codec based networking

Or with this

And we always agreed

The server is the leading part

It decides what goes and what not

[steady sonnet]

I thought the whole reason for those 'no version checks' mods existing was because the previous network negotation system(s) had bugs, e.g. with too many mods in 1.16

if the system is correct, such a mod should not be required

[mental ingot]

Correct

[velvet whale]

youre incorrectly assuming that my generalized naming refers to what the current mods do 😛

[indigo hazel]

The bugs were part of the reason IIRC, but the main reason was one-sided mods incorrectly or not at all setting their display test, which understandably confused users

[indigo hazel]

If people start making mods to bypass network version checks, then we have significantly larger issues on a non-technical level IMO. At that point any and all bets are off and even additional client-side validation will be likely be pointless

Again, this type of network version check is not new, most people set their simplechannel version check to exact equality as well

[mental ingot]

That is not what he actually is arguing

He is arguing against the server determining the version to use, or better said whether or not it is compatible

[indigo hazel]

Which makes zero sense to me

[mental ingot]

He is arguing that it needs to be the client to make that decision

Because in his eyes as stated above, the client is the more vulnerable party

[indigo hazel]

One side must be authoritative and that's usually the server for good reason

[fervent token]

I mean server should validate but it doesn’t really hurt for the client to confirm if the versions are ones it is willing to use. Though somewhat pointless

[indigo hazel]

Sure, nothing against having the client check again

[steady sonnet]

I thought we were negotiating payload versions now anyway, not mod versions

[indigo hazel]

Correct

[fervent token]

Eh server can be authoritative but client should be allowed to bail is I think what Maty is talking about

[steady sonnet]

So client-side mods (e.g. Sodium) shouldn't need to do anything special

[indigo hazel]

Which is trivial to add onto what we have now

[mental ingot]

The client is always allwoed to bail

It is already there

[indigo hazel]

Correct

[mental ingot]

Simply disconnect the connection

[fervent token]

That’s what I thought, that if version is wrong client just bails

[mental ingot]

Yeah the client can just disconnect at any time

For any reason

There is obviously no way for the server to prevent this

I thought this was obvious

[mild spruce]

So a couple thoughts with the upcoming changes to network.
Provide a way for modders to send packets without checking if the remote side has it, both forge and fabric have these options. I added the check for forge a while back which does not force the check unless the modder wants to check. Some times we don't want to check, mainly for bukkit and possibly for sponge, I forget how their networking works.

Second, ClientConfigurationPacketListenerImpl#handleCustomPayload and ServerConfigurationPacketListenerImpl#handleCustomPayload
Does an instanceof on the incoming packets to decide if it is a modded or vanilla packet.
The instanceof's here will always mean a vanilla connection for the session if the configuration packet is not a neoforge packet. This in turn means any mod packets will be ignored/dropped and not handled.

This should be changed so we can make our mods cross loader compatible again.

[mental ingot]

Some comments on this:

1. No we thought this through: All payloads will be filtered, the negotiation protocol, even in its slimmer pure minecraft:register based variant, requires the server to announce all its channels. So we know exactly which channels are available and which not. And can prevent weird disconnects trivially this way, making for a more robust and user-friendly environment without swallowing failures or errors.

2. I am not sure what you mean with the instance-of check in handleCustomPayload with respects to modded or not..... The payloads are completely de-serialized, and it is completely irrelevant what their class type was on the sending side. As such as long as the payloads writer-reader implementations line up between the server and client, these instance-of checks are not a problem.

It would be nice if you guys could take a look at this variant of the protocol: https://hackmd.io/Vk6ARdvHSQmj_tdDKS2Bag?view

Especially @opal ferry

The full flow does the negotiation twice

Which we maybe could reduce

By sending one more packet "c:flow" around between client and server

That holds that information

And we could technically put them all into one packet

But the fabric protocol also kind of splits the version and the remaining information of

So I followed that design here

@velvet whale This would be the section you would be interested in: https://hackmd.io/Vk6ARdvHSQmj_tdDKS2Bag?view#Negotiation-failure-full-packet-flow it discusses first the case where the server determines that they are not compatible, but it goes later on into the situation where the clietn determines that the server screwed up

I still need to write out the structure of the individual payload types

But that is doable

And I only want to do that once modmuss has given some input

If it is possible to join the different payloads together into one

Or if he wants to keep it seperate

[opal ferry]

The whole required thing isn't something we have, if you send a packet to the other side and it cannot accept it so be it, this even applies to vanilla. It should never disconnect.

Adding that seems to over complicate the whole thing imo. The goal I had with c:register was to be able to send all the of play packets during configuration, so you have them ready at the start of play. Previously this was done during login.

[delicate dawn]

forge has used for a long time the presence / absence of network channels as a test for client/server compatibility

I probably don't understand the situation well enough so excuse if I say something stupid

I feel that mandatory "channels" (payload types, as it may be) should be something opt-in -- if the other end doesn't declare any, it should be assumed that all of the payloads are optional and ok to ignore

[opal ferry]

You could possibly send the required channels in your own neo specific packet? Im not aware of any other platform having this concept?

[mild spruce]

Fabric has ClientPlayNetworking.canSend(message.id()) and ServerPlayNetworking.canSend(player, message.id())
Forge has channel.isRemotePresent(connection)
These are checks done before sending the packet that modders do. I see no reason why neoforge cannot offer something similar to opt out of checking if the remote has the channel registered.

Bukkit plugins cannot send on the "minecraft:register" channel, bukkit blocks it, no idea why that was decided. So a plugin cannot tell the client what channels it is listening to that way.

[mental ingot]

We do have this endpoint: "listener.isConnect(ResourceLocation payloadId)"

We simply do not transfer the payloads if that methods returns false

So you are free to call the "send(CustomPacketPayload payload)" method with your payload instance all you want, it won't send them

[mental ingot]

Then the question becomes: Does Fabric treat all payloads as required?

Because from the original mc:reg spec, it seems to indicate that all listed channel are required

[mild spruce]

Yes, but we want to send them if we are connected to a bukkit server.

[mental ingot]

As long as the bucket server indicates in his mc:reg packet that he has that channel, that is not a problem

[mild spruce]

you cannot. Bukkit prevents plugins from using that channel.

[opal ferry]

It doesn't really have that concept, as Mysticdrew pointed out the modder can choose not to send it by first checking canSend, but you can still send if it even if that returns false.

[mental ingot]

Yeah, but it needs to list all other channels in a packet that it sends on that channel

[mental ingot]

The modder is not relevant for the discussion

Let me rephrase

[opal ferry]

I was under the assumption that bukkit handles that packet for you. I would expect a bukkit API for plugins to say that they can recieve on a channel. Fabric also blocks modders sending their own packets under the minecraft namespace.

[mental ingot]

Given a channel mod:something in fabric, what happens if the client has this and the server does not, and vice versa? Is connection still allowed, or is it rejected?

[mental ingot]

Yeah I think drew is not understanding how the protocol works, and just thinking of his own case. Which is not really productive here. His concerns are covered since we have the exact same endpoint. We just allow the modder to call send regardless, and do the "canSend" check for him. Not sending the packet if he can't send it

[opal ferry]

Its allowed.

[iron ivy]

I think drew is saying that the packet is still sent, its only not sent if they do that check explicitly prior to sending.

[mental ingot]

I understand, but that is a weird assumption, you have no idea as a modder what happens with that packet on the receiving side. It might crash, log, swallow, disregard the payload entire, parse it partially or process it properly

[iron ivy]

'they' here being the modder, not the api

[mental ingot]

Okey...... Is there a way in Fabric that I can express: Hey I need my companion mod on the server. If it is not present prevent log in?

[opal ferry]

An unknown incoming packet should be swallowed, the same way that vanilla does atm. Im not even sure if it logs a message.

[mild spruce]

Yes exactly.

[mental ingot]

Sadly due to the way that is implemented that might not always be the case

Payloads only of certain sizes are discarded

Additionally with proxies involved, as well as other mods being able to register in different namespaces, there is literally 0 guarantee that this works 100% of the time

I understand the general premis though

[opal ferry]

Yes (and no), the modder can use canSend during configuration and opt to disconnect the client if it knows it cannot be handled. You can see a working example of how I do this here: https://github.com/TechReborn/TechReborn/blob/972f0a5e6fb261ba6b7960698d4284d536ef1fa1/src/main/java/techreborn/events/OreDepthSyncHandler.java#L57-L64 One day we might add a higher level API, but thats off topic here.

[mental ingot]

Okey interesting

So yeah that is the missing piece then

Because we offer such an API

And that is where the "Required" concept comes in

[opal ferry]

That would be specific to loader IMO, and can then do more rigorous checks maybe involving the mod versions or whatever.

[mental ingot]

NF Implements that on Payload ID level

But not on mod level

Okey

This clears up a decent amount

Now may I take a couple of minutes of your time

And ask you some questions with regards to the contents of c:register and c:version?

There are some things that are unclear to me

Lets start simple:

In c:register there seems to be a "login" phase

At least that is what I interpret the second field as

Why is that there?

[opal ferry]

Uhh, good question. I dont think that should be there, likely a bad example.

We dont handle that

[mental ingot]

Okey

Next: What is the order of the versions in c:version. The diagram seems to make it just contain an array?

But what do the individual entries in that payloads array then represent?

[opal ferry]

Those are the versions that the sending side can support. Going by my diagram the server is saying it supports versions 1, 2 and 3.

The client then responds back saying it only supports v1. If the client also understood it would respond back with 1, 2. And then v2 would be used

[mental ingot]

but versions of what?

[velvet whale]

c:register spec

[mental ingot]

What?

Do channels then not have versions what so ever?

[opal ferry]

c:register has the version the version in it, but we only use that to validate that it matches the previously negotiated version

[mental ingot]

Yeah that was clear

But what is the context of c:version then?

[velvet whale]

it avoids a "mc:register, take 2" situation

where we need to work around limitations of c:register because the spec has been set in stone for years

[mental ingot]

Maty, please, don't take this the wrong way. I would like some answers from modmuss. You are literally just guessing

If you are not

A link to were you got all this knowledge from and a reason why you did not hand that link out weeks ago when me and schurli asked for it would be nice

[velvet whale]

I was part of the discussion in #wisdoms-private, bold of you to assume I was guessing but sure

[mental ingot]

Where?

I read that channel like 5 times now

And I can't find it

Or i might be misreading it

[opal ferry]

Yeah Maty helped design this. The two way versioning is needed to ensure that the client/server both use the highest commonly supported version. I think this idea started here: #wisdoms-private message

[mental ingot]

Okey

Then this whole diagram makes even less sense

Why is all of this done AFTER the configuration tasks?

[opal ferry]

It can be done before within spec, and maybe it should be. It works as we only use it to send the channels for the play phase.

[mental ingot]

Okey......

I am starting to see where this is going

[opal ferry]

We actually do this as a configuration task.

[mental ingot]

Yeah

But it means that untill that task has ran, no other mod configuration tasks should really use the network then

Because "canSend" can't work

[opal ferry]

It will work for configuration, as that has already been handled by minecraft:register

[mental ingot]

?!?

Okey....

Seems pointless to do this in 2 ways

[opal ferry]

Thats needed anyway as you need to know if you can send the c:version packet.

[mental ingot]

And without versioning in the configuration phase a particular bit more dangerious

Yeah

I was thinking of doing JUST c:version, c:register and neoforge:network_* as the payloads in the MC:Reg

And then do a proper negotiation run on both phases

Alternatively

NF could include all channels it has regardless of phase in the MC:reg payload

And then run a versioning negotiation afterwards

[opal ferry]

That could work, you could then use c:register with the configuration phase, and then do modded configuration.

[mental ingot]

Yeah

But as far as I see

[mild spruce]

For regards to #2 here.
If it is not a neoforge packet, it leaves isModdedConnection to false and sets isVanillaConnection to true.
That creates an emptyVanilla NetworkPayloadSetup.
So when a modded packet comes through, the payloadSetup is null in NetworkingRegistry line189 for play packets. Spits out a warning log and disregards the packet.
I am testing this with a Forge Server and NeoForge client currently.
This is happening before any discriminator stuff which can be handled in the packet encode/decoder itself.

[mental ingot]

There are two things majorly missing from this protocol, 1 thing missing from my current impl, and 1 thing mis implemented in my current impl

Missing from Protocol:

1. Support for versioned payloads (it seems like only the c:register is versioned as far as I understand)
2. Support for optional and/or required payloads

Missing in my current impl:

1. Versioning in the negotiation packets

Wrongly implemented in my current impl:

1. Not following the mc:reg spec

[mental ingot]

That instance of check can not check what the CLASS TYPE of the sender is/was. It checks the class type of the received de-serialized instance! Which this code is perfectly fine for!

Even if fabric sends its payload and that was serialized from a completely different class, as long as we speak the same "language" (which we currently do not!) this works 100% of the time

[mental ingot]

@opal ferry Would you be open to discussing an extension to the protocol in some form of "c"-packets, so that we can cover the two missing points noted here?

I know it is not used by fabric now, but it might be a worthwhile investment for future compat?

[opal ferry]

I dont understand point 1)? Are you wanting for every payload (e.g from mods) to have a version?

[mental ingot]

Yes

That is something forge always supported

And it is supported in the new API

[opal ferry]

For the required payloads we could just mark everything as not required.

[mental ingot]

I mean we could make it optional

But it would be something we would then need to define a convention for

[opal ferry]

Humm, thats something we leave up-to the modder, when we have done in this fabric API for other packers we handled this via registering two channels e.g you would have modid:my-packet-v1 and modid:my-packet-v2 you can then use the canSend method to decide what to send.

[mental ingot]

Yeah

We don't do that

[opal ferry]

Otherwise the server will always try to send the latest version, not knowing if the client can handle it?

[mental ingot]

The main idea of the version is to have the client not connect to a server with an incompatible mod version installed

Forge basically prevents the connection if say version A is on the client and version B is on the server

[opal ferry]

Yeah, we have no concept of that atm (Its something we want to have, just very low prio). I have a feeling that would be better handled separately as each of us sees fit.

[mental ingot]

Okey....

[opal ferry]

If you strongly disagree let me know

[mental ingot]

Not really

Cause if the aim is to connect a forge client to a fabric server or vice versa

Then we will at least need to define some concept of this is going to work

Should forge consider the channels then all versionless?

And prevent connection if the modder ever sets a version on the forge side?

What happens if you guys ever introduce such a mechanic

You kind of need the same logic

And those should be compatible

Else you risk breaking stuff weirdly

[steady sonnet]

couldn't the protocol be changed again in something like 1.21 since the vanilla versions aren't even cross-compatible at that point

[mental ingot]

Hence me trying to figure this out

[mental ingot]

I am not asking to do it now

All I am asking is to make a spec to gether

When ever the other side wants to implement that

Is up to them

As long as we agree on something sanely, with a fallback

It should not matter if it is implemented now, tomorrow, or in year, or with 1.21

[opal ferry]

You raise a good point. However getting to a point where we can have a common protocol here is a lot more work, and I'm not sure its going to happen. What I imagine is:

You have a forge client mod that says its required on the server, this will prevent connecting to a Fabric/Vanilla/Bukkit server.

If you want your client mod to be able to connect to one of these servers, your mod has to say its optional on the server. It can still send packets to the server, and use canSend or whatever to know if the server can understand.

[mental ingot]

Hmm

[opal ferry]

I hope that makes sense

[mental ingot]

So to connect with fabric channels need to be optional and versionless?

[mental ingot]

Yes it does

Just not sure I am happy with that solution. It puts a lot of drawbacks on nf modders and the use of the fabric protocol then seems not really there to me personally

[opal ferry]

Im not convinced that versioned payloads is a good idea, as the sender has no way of knowing the version to send.

[mental ingot]

They have to line up

Basically it sets the version of the "code" that the deserializer uses to create a payload instance

And version A on the client means that there also needs to be a version A on the server, else it won't work

[steady sonnet]

I think what modmuss is saying is that if you had a mod (let's say a minimap mod) and it had packets mapmod:chunk_info_v1 and mapmod:chunk_info_v2, you could theoretically design it so that an older mapmod client can still work with a newer mapmod server

[mental ingot]

I understand what he says

[opal ferry]

Ignoring the versioned payloads, what drawbacks do you get?

[mental ingot]

No control as a modder over what needs to exist on the other side of the connection (regardless of server or client)

[mental ingot]

That is a way of thinking, but simply not what the Forge API and the new NF API ever did

[steady sonnet]

I think with the current API you could still do it but it would require making a payload with a fixed version and then implementing negotiation yourself

which is one way to do it, I guess

('you' refers to the mod author)

[opal ferry]

Your either required on both sides (current status-quo if I understand correctly). Or you are optional on the other side, but the mod can still choose to send packets.

[mental ingot]

It would actually require a payload without version and marked as optional, cause that is what fabric is

[steady sonnet]

hm

fabric has no concept of required?

[mental ingot]

Not sure what you mean with that first sentence here.

I currently interpret it from the NF side. Then yes that is true, by default a payload is required and versionless. If a payload is versioned then it needs the exact same version on the server and client. If it is optional then the payload is allowed to be missing on the other end to connect. If a payload is optional, but has a version set, then if it is present on the other side the version needs to line up.

[mental ingot]

No, and it has no concept of versioning

It only has versionless optional payloads

So I am completely unsure what to do at the moment

[opal ferry]

The fabric mod could implement the version handling on their own, as they will get the entire unmodified payload. I believe mods already had to do similar for the packet ids on old forge.

[mental ingot]

Not really

Because forge always had APIs for this

And even has a custom UI in case where it does not line up

It will show a very nice interface that indicates what the problem is

[opal ferry]

My goal is to at-least be able to send packets between the two, at a low level (for advanced modders), these higher level ideas regarding versions and required packets is great but beyond what we need to be talking about.

[opal ferry]

Yes, I think thats great and you should keep that while also having full control over it.

[obsidian nova]

Snippet from my own code (for reference of what has been in (Neo)Forge):

final String protocolVersion = Integer.toString(BingoNetwork.PROTOCOL_VERSION);
final Set<String> allowedVersions = Set.of(
    protocolVersion, NetworkRegistry.ACCEPTVANILLA, NetworkRegistry.ABSENT.version()
);
NetworkRegistry.newEventChannel(
    BingoNetwork.PROTOCOL_VERSION_PACKET,
    () -> protocolVersion, allowedVersions::contains, allowedVersions::contains
);

(this code exists with Neo 20.4.5-beta)

[mental ingot]

The problem is that your current protocol simply does not suit our needs. We would need to do a whole lot of extra round trips with extra loose data to implement all of this, and if a single mod is in the stack that does what forge recommends, and that is to set a version and mark the channel as required, then all of this out of the window

Lets rephrase that

If I could come up with something nice and generic

[mild spruce]

Let mods handle version mismatches on their end.

[mental ingot]

That is not really what 99% of the modders want

WOuld fabric be open to me implementing it for them in the fabric api?

At least most parts of the protocol?

[opal ferry]

Im not totally against the required flag, that seems easy enough even if we say they are all optional (I'd have to think it though fully first). The version in each payload isn't something I'm too keen on, but a fabric mod should be able to implement that on their side without too much fuss.

If one mod (even if its the default) does say its required on both sides and disconnects when joining a none neo server, then its working fine.

The goal is to allow specific mods that know they can handle being optional on the server, to still connect and send packets. I expect 99% of mods wont fit into this bracket.

[obsidian nova]

A version compatibility API on Fabric would be nice. But I don't think the idea was prefixing a version in every packet

[mental ingot]

I am not so sure about that, from talking to a lot of people, they would be extremely happy to support cross loader mods, and I personally for example would

There are for sure a lot of modders, that would like to stay on "their" platform

That is for sure

[opal ferry]

Making it "just work" can be the end goal, there is a lot involved in doing that. I think we need to take one step at a time. At least getting it back to a point where it was previously seems to be the main issue atm.

[mental ingot]

But I think there are a lot of modders that would easily want to support it

[mental ingot]

Like the compromise I am willing to make is the folliwing:

I would like to take your existing work and protocol, expand it with common payloads for those cases, and discuss them with maintainers of both loaders and the general public. Then I would implement them in both. Maybe not completely, so the fancy UI from forge might not exist in fabric for example. And some APIs might be initially missing but populated with default values. But I would love to work on the core concept, so we can get to it. I don't think we are far away from it. Yes that would mean that fabric would get a similar version per payload id API, and the use of that API would be completely optional (like it is here on forge).

But it would solve this issue

[opal ferry]

Getting some input from others is a good idea 👍

Im still not 100% sold on the versioning, but that can always change

[mental ingot]

Okey

I will write something upo

Do you mind if I pull the negotiation to the complete start of the configuration phase

Instead of doing it split?

[opal ferry]

Yeah, thats tottaly fine, and even within spec. Just a bad diagram 😄

[mental ingot]

Okey

Perfect

I will see what I can do then in the next few days

You did already most of the heavy lifting

I just need to figure out a way to get this to behave with what we need

I assume you want to keep the current version of the payloads as is?

Or are you fine with a bump already?

I am now specifically talking about the version for the register payload

[opal ferry]

Yes, it would need to go to v2. We have had this released for a while now.

Thankfully it was designed to be versioned 🙂

[mental ingot]

Yes

Which is what I am very gratefull for

We can just break

Because we are still in beta

So I don't mind doing this right now 😄

I am going to walk the dog

Clean my head

And figure this out

I like the concept of mc:reg contains all channels

So I might use this

But I would like to trigger it from the server.....

And have the client send all its channels to the server

To do the negotiation there

Hmm hmm

Problems and ideas

[mental ingot]

Okey back from a walk

I have something in my head

[mental ingot]

Okey I started writing stuff down: https://hackmd.io/SYCaVQyMQZaaBAp6oGqfSg

@buoyant pagoda you said you might be interested in helping?

[velvet whale]

@mental ingot can you hotfix the mcreg packet

[mental ingot]

No

It requires an internal protocol rewrite

But I can make something like a mini version

If you help me with the protocol?

What would you pot in the MC reg?

All channels?

Then again

Even if i fix the content of the mc ten

The nf cliënt would not connect to anything that does not send a network structure payload

But i think i can work with that somewhat

I need to think this through a day

Aan wie something up

[mental ingot]

@opal ferry One more question:

With respect to the minecraft:(un)register payloads, they only indicate listening abilities correct? Not sending abilities? (From Dinnerbone: "When you have a mod that requests to listen on a specific Plugin Channel, you should send a Plugin Message with the channel "REGISTER", followed by the name (or names, separated by \0) of the channels it's now listening on." -> https://web.archive.org/web/20220711204310/https://dinnerbone.com/blog/2012/01/13/minecraft-plugin-channels-messaging/)

So in practice if the server sends his list, then that means those are the channels he is able to listen to. Any other channel not in that list is simply not available correct?

[opal ferry]

Yes, I think that matches my understadning of it. You announce what channels you can now (or cannot) receive on .

[mental ingot]

Okey that makes my life a lot easier

I just need to figure out how to "trigger" the sync somewhat sanely

[mental ingot]

Right now the server sends the first "minecraft:register" with his list as an indicator and trigger.

I personally feel like that might not be right anymore for a modern protocol

[valid idol]

I mean I don't think there is anything stopping the client from sending their listener list first, right?

And since you can add/remove listeners at any time, why not just delay it?

[mental ingot]

Yes

But right now not

Because I misread the protocol

And misimplemented the minecraft:register

I am working however on a fix for that

But I wanted all my ducks in a row this time

To prevent miscommunication

[errant summit]

how is this going orion?

[mental ingot]

https://hackmd.io/SYCaVQyMQZaaBAp6oGqfSg?both

Slow

But I am collecting everything into one document

And I have a design in mind that will work

And provide flexible extension points for the loader features

Like registry sync and config sync 😄

So that fabric can eventually implement that in the future as well 😄

Or not

But for example have forge then load the default server config values

Going to do some grocery shopping now

And by tonight I hope to have completle documentation working

@errant summit Does that update you enough?

Or do you need more details

[obsidian nova]

Fabric already has registry sync

[mental ingot]

Forge does to

They just use different protocols

[mild spruce]

I don't think Forge uses the simplenetworking for the minecraft:register payload as currently it works with fabric. If it did, it would have the discriminator and then not work with fabric.

[buoyant pagoda]

iirc, there's special handling for minecraft:register and minecraft:unregister in Forge

[mild spruce]

All yea looking at the code it does not use SimpleNetworking to send that packet.

so the bit about that in the hackmd doc is not accurate.

https://github.com/MinecraftForge/MinecraftForge/blob/1.20.x/src/main/java/net/minecraftforge/network/ChannelListManager.java

[mental ingot]

That is an updated protocol

The older version, at least on 1.20.2 neoforge had index in there

It might not be simple channel

That I should indeed update

But it contained a payload index

[velvet whale]

https://github.com/MinecraftForge/MinecraftForge/blob/de13ba8ab89669b53d10cb62f850ce270bb150c1/src/main/java/net/minecraftforge/network/MCRegisterPacketHandler.java#L52-L63 forge didn't in 1.20.1

[mental ingot]

Well then I fucked up in the 1.20.2 port

Somewhere

Probably when I implemented SimplePayload

[velvet whale]

neither did neo in 1.20.2 https://github.com/neoforged/NeoForge/blob/4a4486b400e506870c1fd87be3ed8ecefbb1714f/src/main/java/net/neoforged/neoforge/network/MCRegisterPacketHandler.java#L47-L59

oh

ew i see

[mental ingot]

Yes it deos: https://github.com/neoforged/NeoForge/blob/8e5e38fb3410ea515ad21611d54da92a0a6a575a/src/main/java/net/neoforged/neoforge/network/MCRegisterPacketHandler.java#L118

[mild spruce]

I have had forge and fabric communicating with each other since 1.17 without issue of the mc:reg packet having a discriminator

[mental ingot]

I will update the documentation

Thanks for pointing that out

Still

For the intends of the document

I did not care for its use of the discriminator or not

I cared for the fact that it used a side-channel

[errant summit]

I fixed some grammar and spelling mistakes in the hackmd, looks good (I'm assuming you are working on the fabric part of the doc atm)

[mental ingot]

Yeah

I am adding protocol flows and the like now

Then the last section on neoforges current impl

And then a kind of tldr / good things bad things section

[errant summit]

and done is the thesis /s

[fair sandal]

Needs a modding journal 😛

[mental ingot]

Can somebody read this through in case I forgot something: https://hackmd.io/SYCaVQyMQZaaBAp6oGqfSg?view#Fabric-configuration-phase-protocol ?

[opal ferry]

All looks correct to me so far (I didn't look at the legacy forge section in detail)

[mental ingot]

And now we have the anaylsis and requirements added: https://hackmd.io/SYCaVQyMQZaaBAp6oGqfSg?view#Analysis

[opal ferry]

In the comparisons, for the fabric one, what do you mean by Partially versioned? It should be fully versioned, allowing a mix of new/old clients & servers to commuicate.

[mental ingot]

Well in the protocol documentation you do the versioning negotiation after config phases have run. I know that you said that it can be done before. But I can not really find any direct evidence of that, so I have to go of the existing documentation. That means that any payloads send before that are either completely unversioned or the system uses the minecraft:register process, which is by its nature completely unversioned anyway.

In other words, from the current existing documentation there is not other conclusion to draw that only parts of the protocol are versioned, especially when it comes to the use of the payload channels during configuration

[opal ferry]

Ah I see what you mean, thats fair. The common packets themselves are versioned, but everything else isn't (at least within the spec)

A quick explanation of "side-channel" might be handy, I think I have an idea of what you mean but its not super clear.

[mental ingot]

Correct

Will do

Currently making a nice cup of tea before I continue

[opal ferry]

I also think "simple" should be a requirement, but thats likely already gone out the window 😛

[mental ingot]

I think you are right

But "simple" is pretty difficult to do

Also it can have many different facets

Simple can be a protocol with very few payloads

Or very few round trips

Or easy to understand

Or small and simple payload types

It is a bit of a weird compoennt

[errant summit]

I found another issue in our current implementation, if the client is fabric and the server is neo the registry sync would fail because the isVanillaConnection check would detect it as modded and expect the registry sync to happen

so the new protocol needs a c:loader_brand packet so something like that can happen only if the other side is a certain loader

[mental ingot]

I have a much better idea for that

You will see 😄

[buoyant pagoda]

[opal ferry]

It should only disconnect if the registries are modified and the client cannot handle the reg sync packet. I believe this is how it would work with a fabric server and a neo client.

[mental ingot]

Do you have a link to the spec of your reg sync packet?

[opal ferry]

I don't think there is a written spec, without looking I have no idea about the details. I know it gets split into multiple packets when needed.

https://github.com/FabricMC/fabric/pull/416 somewhat explains it along with the PR that was merged https://github.com/FabricMC/fabric/pull/1853 It does quite a bit to reduce the size.

[errant summit]

nope it is a configuration task that only completes if it gets the correct response

[opal ferry]

Ours is a configuration task, but it only starts if the client can handle it.

Later down the road we can possibly look into using a shared packet for it, but I'm honestly not sure its going to be worth the effort. Might be best a 3rd party mod to start with? idk.

[errant summit]

I just noticed... we are sending translatable components as disconnect reason for incompatible vanilla clients, but vanilla clients don't have the translations for those

[fair sandal]

do they not have the fallback translation set?

[errant summit]

nope

[buoyant pagoda]

i recall having noted this some while ago, though I don't remember if I said it

[ebon thunder]

Seems like the DecoderException when connecting to hypixel is thrown on the client while it is reading the minecraft:register packet and is due to it trying to use vanilla string reading instead of null separated strings

[errant summit]

Feature arm

[misty talon]

hey got a question. Is there any particular reason why the registration method only includes a read handler? I know the write is done via the interface CustomPacketPayload ... but in my case this means I have to create another object just to do the write operation (something I rather want to avoid).

[mental ingot]

Yes you just described the reason

We used a specific interface for this

Because it is what minecraft it self uses internally

For simplicity of the API, and the maintainability of the system

We decided not to provide any wrapper what so ever anymore

[misty talon]

ok, thanks! A bit sad, but I guess I have to work with it

[dim wagon]

another object?

all my packets have read/write/handle in the same thing

[misty talon]

I have automated the read and write operation of my packets, that's why they are not included inside the packet itself anymore

[dim wagon]

automated in what way?

[misty talon]

once a packet type is registered it will got through all fields and tries to find handlers for each type. So basically all you need to do is to define fields inside your packet class and it will be automatically send over (unless if they are marked as transient)

[dim wagon]

well that sounds terrible

but alright lol

you could still do that with an intermediate class though

[misty talon]

why is that terrible?

[dim wagon]

that passes out the encode/decode as needed

[misty talon]

and yes I can solve it with another class in between, I just tried to avoid it

[dim wagon]

yeah

ahwell

[iron ivy]

It's just a safer variant of java serialization. Seems resonable to me tbh. I wouldn't do it, but each to their own

[misty talon]

yeah ... and it helps to minimate issues where you write or read things in wrong other and overall it's quite tediou to do that for each packet

[iron ivy]

I actually had a project at one point that did exactly this, but it ASM generated encode/decode functions for a class. Totally overkill..

[misty talon]

yeah that's sounds a bit overkill. Mine is quite basic: https://github.com/CreativeMD/CreativeCore/blob/1.20/src/main/java/team/creative/creativecore/common/network/type/NetworkFieldTypes.java#L182
You have to register all types you want to use

[iron ivy]

neat

[mild spruce]

that is pretty cool

[errant summit]

orion how is the new protocol coming along?

[obsidian nova]

Will the old register protocol be dropped at some point?

It would be neat if we could get Spigot and wiki.vg to migrate

But then again, that would be completely dropping a 13-year old well-established protocol

[buoyant pagoda]

that's the problem -- you'd have to get everyone who currently recognizes the well-established protocol to change away, if we are to drop it

[ebon thunder]

Some servers also send invalid characters in the register packet

[mental ingot]

@opal ferry Just double checking my notes: For the minecraft:register and minecraft:unregister it is not needed to list all payloads in the packet right. It just indicates that you are now able to listen to that payload. Not to what payloads you can listen to. So receiving one only effects the state of the listed payload ids, not the unlisted ones correct?

[opal ferry]

Yes, thats correct

[mental ingot]

perfect thanks

[errant summit]

orion just confirming you are doing the design in uml diagrams first right?

[mental ingot]

Yes

Fully

Working on it now