Networking Protocol

[mental ingot]

if you want to chat about it

I can hop into a call?

[errant summit]

software development as it is meant to be

yea sure

[mental ingot]

Sitrting in dev-coive

#718181706867146812 😄

[indigo hazel]

Gonna join in on the fun while I scream some more at the light stuff if you don't mind

[mental ingot]

Sure

Join the channel

The more the merrier

https://hackmd.io/SYCaVQyMQZaaBAp6oGqfSg?view

[red aspen]

reading up on this. got to the end and found this interesting

[errant summit]

yea we are actively editing it

[red aspen]

I realized that shortly after I posted. cool feature.

[errant summit]

orion I did some formatting and corrected some typos in the uml

[mental ingot]

Yeah

It is not finished

So you might need to do it again 😄

[velvet whale]

uh the tier sorting registry is a configuration task but the tier sorting registry is a reload listener

which means the value isn't updated on the client after a reload

same is the case for config sync

they shouldn't be config tasks but normal packets

also we no longer check stuff like the modded synced custom registries and denying connections, which will lead to vanilla packet errors

[mental ingot]

TSR Has a sync all, but i might have misconfigured the payload, thought i set it to all

[mental ingot]

True

[mental ingot]

They should be config tasks, but the also should be common payloads

[mental ingot]

We still do. Not sure where you derive that information from. The normal registry sync happens, if an unknown registry is found the client will disconnect, if an unknown entry is send from the server the client will disconnect. Not sure what other cases there are

[velvet whale]

meant datapack registries

[mental ingot]

Hmm

IU would need t6o check

What is special about DP based registries?

[velvet whale]

https://github.com/MinecraftForge/MinecraftForge/blob/45b00b000411d9c7b2e090359b051dc60a73628a/src/main/java/net/minecraftforge/network/ForgePacketHandler.java#L150-L158

[mental ingot]

No I mean with respect to the sync?

Are they send over to deal with id sync or not?

[velvet whale]

vanilla syncs them

but if the client isn't aware of a registry, it will lead to a very undescriptive error

[mental ingot]

What does vanilla do if it gets send a dp reg that it does not have?

That is a really undescriptive error

Okey I will add it to the list of things to check

But fixing that is going to be nearly impossible

Because mojang is the root dependency

It will likely always go first

[hasty yew]

Hmm, I feel like there was a PR a while back to deal with those somehow that I saw...

I feel like maybe it had different logic for vanilla/modded clients or something?

It's been too long since I remember last seeing something about that though, and I'm sure it's mostly irrelevant for the new system given all the changes

[velvet whale]

I mean, we can hardcode a task between ClientboundUpdateEnabledFeaturesPacket and ClientboundRegistryDataPacket

[mental ingot]

We will likely need to do that

Can you make a note on the HackMD document at the bottom?

So that I add it to the new protocol?

[indigo hazel]

Can't be a real config task though because Mojang in their infinite wisdom didn't make these two packets and the tag update packet a config task

[fair sandal]

is there a reason not to use packets like mojang does?

I'd expect the tier sorting to follow the same pattern as other data pack stuff (i.e. using the datapack sync event)

[mental ingot]

We really should not do that

The point of the tasks is that the server and the client work together to sync themselves

Yeah we can send the raw payloads

But then waiting for the response is a pain

[fair sandal]

we don't need a client response for tier sorting, do we?

[mental ingot]

No but we should

Like with other registry operations

We really should wait and have the client tell us, yeah everything is fine

[fair sandal]

I think that reloadable resources should follow the standard established by mojang

[mental ingot]

That depends on what you define as standard...

Because resource packs are reloadable

[fair sandal]

registries are a special case because they influence the ability of the client to connect at all as well as the subsequent packets (raw ids)

[steady sonnet]

mojang's standard historically was to slam the client with massive amounts of data and time it out

[mental ingot]

^

[steady sonnet]

so I don't think we should follow it too religiously

[mental ingot]

The point of the tasks is that they work together

And we can improve this

[fair sandal]

well, configuration tasks are not available when the /reload comment is successful, are they?

so let's use packets for everything instead of having two ways of doing the same thing

the tier sorting registry should use the same system that we want modders to use for their own datapack stuff as well, and it should therefore be reasonably simple

[mental ingot]

That is not really relevant

The TSR already had a second code path

All of the systems you mentioned always had two code paths

The first code path ran when a player joined the world or on login -> That should now be a configuration task

[fair sandal]

they have two places where they send the same packet

[mental ingot]

The second code path ran on reload -> That should still be on reload and syncs to everybody

[mental ingot]

We do to....

[velvet whale]

or just, use the dpsync event and you don't need two places

one place, done

[fair sandal]

just use the data pack sync event yes...

[mental ingot]

That is not fired at the right time

So no

That DPSync event is not going to work

[fair sandal]

then how do you expect modders to sync their own dp stuff if the event does not fire at the right time??

[velvet whale]

what, it's fired when the resources are loaded, when you're supposed to sync datapack contents

that's literally what the javadocs of the event say

[mental ingot]

Yeah no

It is fired too late for a player joining the world

[velvet whale]

https://github.com/neoforged/NeoForge/blob/f8c5835181ac8280cd345fc4d57367699a48e61e/src/main/java/net/neoforged/neoforge/event/OnDatapackSyncEvent.java#L15

"before tags and recipes". for tier sorting it's the event you're supposed to use

[mental ingot]

I don't agree

Because that is outside of the new configuration phase

It is fired when the data for the player entity is synced

And is wholely not related to anything datapack wise

If at all

Then that even should be fired from a configuration phase task

Or scrapped in its entirety

Well mabye not scrapped

But replaced with two things

One configuration phase tasks

[velvet whale]

why do we need tiers to be available on the client before recipes or even tags are?

[fair sandal]

I don't give a shit about moving this to the configuration phase

the configuration phase is a MEANS not an END

[mental ingot]

No actually tech it is not

[fair sandal]

there is no reason to use it here

[mental ingot]

It is a specific phase in the protocol where the player is connected and authenticated, but not joined with the world

Where the data points for updates should be exchanged

[velvet whale]

why are tags not sent during the config phase then

[mental ingot]

Nope

On join it is after tags and recipes!

[indigo hazel]

They are

[mental ingot]

They are!

That event is fired after they are send

It is literally fired after the config phase ends

[mental ingot]

And the player has been spawned

[fair sandal]

this is the correct place to send dp stuff

[mental ingot]

Nope

Not based on the MC code in the configuration listener

[indigo hazel]

Tags are in fact sent before any config task whatsoever runs

[fair sandal]

then why does MC send recipes there?

[mental ingot]

I can not tell you why mojang sends the recipe update packet there

No clue

But it is literally the only thing that does

[fair sandal]

we can just keep using that event as-is

if mojang changes where they send recipes I'm happy to move the tier sorting stuff somewhere else

[velvet whale]

https://github.com/neoforged/NeoForge/blob/f8c5835181ac8280cd345fc4d57367699a48e61e/patches/net/minecraft/server/players/PlayerList.java.patch#L112
https://github.com/neoforged/NeoForge/blob/f8c5835181ac8280cd345fc4d57367699a48e61e/patches/net/minecraft/server/players/PlayerList.java.patch#L16

it's before

in both cases

[mental ingot]

No it is not maty

Check the listener

It already sends a tag update packet

[fair sandal]

(it has to be before recipes, this is important for JEI and firends)

[mental ingot]

See

This is all that is send before that event

Which already includes the update Tags packet!

[indigo hazel]

It's before tags and recipes on reload but only before recipes on join

[mental ingot]

Correct xfact

In practice

The recipes packet can be send 100 times earlier

Hell it should have been send in the configphase

And then receive an ack packet back

Because that thing can be massive

And processing it is maybe not easy

Actually never mind

I figured it out

The packet is the only "data" packet that is a game phase one

So it can not be processed by the configuration phase listener

Only by the game/play phase one

For fucks sake mojang

[fair sandal]

they're probably gonna clean this up at some point

I disagree with needing to receive an ack though

[mental ingot]

100%

Tech the point of the ack is so that it does not dump everything on the client at once

And then time out

But so that it does piecemeal task for task

The ack packet is a means to an end

And indicates to the server that the client finished processing that previous task

And the next one can start

So yeah the ack might be a bit overkill

But it help a lot with weaker connections

What do you guys want to do here?

I mean that event is clearly not fit for purpose anymore

And even behaves significantly different

@fair sandal You mentioned that the current position of that event is important for JEI/NEI and consorts

Could you elaborate on that

So that I can understand it a bit better

[fair sandal]

the only thing that matters is that the event fires before recipes are synced

[mental ingot]

?

[mental ingot]

Why?

[fair sandal]

because JEI will refresh the recipes when it receives the recipe packet

[mental ingot]

Okey

but could it trivally not listen and trigger of a different piece of information

[fair sandal]

however some modded entries might require more than just the recipe packet

[mental ingot]

Like for example a "sync completed" event

[fair sandal]

if such an event existed, it could, yes

[mental ingot]

So the actual position of the event does not matter that much

[fair sandal]

as long as a vanilla server doesn't send it, it's not really practical however

[mental ingot]

It is just currently the convention that they trigger of the recipe packet

[mental ingot]

Understood

[fair sandal]

I think it's reasonable to sync custom stuff before the recipe packet, however it doesn't need to be immediately before

could also be earlier

[mental ingot]

However not really something I can consider. JEI/NEI can easily check if it is running against a vanilla server and behave differently, because in that case no modded data will be received either

[fair sandal]

I don't see us moving the hook past recipes (with the current vanilla code)

[mental ingot]

I mean after is also possible, but yeah I agree that all modded data should be either timestamp less (in a sense that it can be always send in a side-channel) or done in a configuration task if it is needed when a player joins

[mental ingot]

Well

There is an argument to be had, for at the END of the sync phase, aka after recipes

But that argument is weak

[indigo hazel]

I would disagree with that. The premise of that event is perfectly fine in my opinion, it might just be worth considering to move it to a config phase task for the initial one during connection

[mental ingot]

No if it is in a config task, then the modder should use a config task and a reload listener

[fair sandal]

reload listener is a big no

[mental ingot]

Well no

You are right rellist

Is too early

You would need something that runs after say the recipe book has been updated or something like that

At least that is what I think is the "last" moment you could probably send stuff

[indigo hazel]

Practically anything that's sent in that event comes from a reload listener anyway, that wouldn't be any different

[mental ingot]

True

I need to think about this

I think the idea of ODSE is still valid

But I think the architecture and the structure are not really future proof

So it might need some tweaking in my eyes

[fair sandal]

I mean, why change it now instead of waiting for the future to happen

[mental ingot]

I am not sure it needs to change

Maybe it does

Maybe it does not

[fair sandal]

I want the tier sorting to use ODSE though (or whatever the recommended way of syncing reloadable dp contents is)

[mental ingot]

There might be an argument to shrink its scope to at most the reload

And leave the rest to config tasks

Which would retain the current timings

[fair sandal]

note that tags aren't really a config task either

they're just a fire and forget packet

[indigo hazel]

Within the config phase, yes. It should realistically be a config task but Mojank be Mojank

[fair sandal]

• the timeout argument doesn't really make sense since the resources will be sent with regular packets on /reload

[fair sandal]

again, why?

[mental ingot]

Because that is what it is intended for!

Just because you do not like it

[fair sandal]

so the client can say "I already received tags, don't time me out"?

[mental ingot]

Yes

That is the literal whole point

[fair sandal]

how will that work for /reload thuogh

[mental ingot]

Right that is the problem

I don't know where MC is going with this

IMHO they should kick all players back to the config phase on the /reload

But they don't

So maybe an oversight

Maybe not

I don't know

The point is

We need to fix the TSR

By simply allowing the payload to be send during play as well

[velvet whale]

servers with minigames may reload data mid-game (i.e. lovetropics), and sending everyone to yet another round of acknowledgments and kicking them and then having them rejoin is a lot of spent time and can easily screw the minigames, data tracking and all of that. it doesn't make sense, since /reload is a smooth process in vanilla, and on modded servers it usually lags out for like 2 seconds

[fair sandal]

does TSR not use the datapack sync event?

[mental ingot]

No

[velvet whale]

the amount of data during login is much higher than during reload

[fair sandal]

my only requirement is that TSR uses the event...

as the standard way of implementing reloadable datapack contents

so that modders can look at TSR as an example of standard datapack-based custom content

[mental ingot]

It is literally just triggered from the reload listener

I don't see a reason why we need to change that behaviour?

Why does it need to hook into the other parts?

[fair sandal]

because the reload might fail and you don't want to sync in that case

[mental ingot]

?!?

[fair sandal]

in the apply phase of the reload you have no idea that the reload will actually complete, and you are then syncing way earlier than vanilla usually does

[mental ingot]

What?

[indigo hazel]

Reload listeners are done long before the config phase starts, so that doesn't make sense

[fair sandal]

consider /reload

[mental ingot]

TSR Is completely independent of other systems

It requires soly the item registry and a datapack

[indigo hazel]

That's not config phase though

[fair sandal]

yet if a single reload listener fails to apply no update should be sent to clients

I am talking about /reload

[mental ingot]

Yet that desyncs the client and the server

Because there is no way to "undo" the n-1 listeners if the n'th listener fails

So wether we wait or not

Results always in problems if the listener fails

[fair sandal]

no?

the server doesn't update its internal state before the reload is successful

if the reload fails nothing happens

see MinecraftServer#reloadResources

[mental ingot]

That is only partially true

Hmm

[fair sandal]

all of this only runs if the reload succeeds

hence the hook in this.getPlayerList().reloadResources();

[mental ingot]

That makes reload listeners for anything modded seriously impossible to do right

Yeah

I understand

But that is the architectural issue then

Because mojang kind of collects the results from the apply

[fair sandal]

it is an architectural issue with our resloader hook being limited, yes

[mental ingot]

And then "activates" them, for the lack of a better word, when all succeed

[fair sandal]

you'd really need another onReloadSuccessful method, to update global state

[mental ingot]

Yes, yes you do

Oef

This opens up whole can of worms

I hoped we never needed to touch again

This explains that event

[fair sandal]

so yeah I guess that the TSR is incorrect if it updates global state in its apply method

[mental ingot]

Which realistically every single modded listener does

[fair sandal]

the fix I had in mind for a WIP resloader overhaul in fabric was to allow modders to add data to ReloadableServerResources

[mental ingot]

Yeah

I was thinking the exact same thing

[velvet whale]

or TagsUpdatedEvent

[fair sandal]

in modern dynamics I used another event that fires after the reload is complete to actually update the global state

[indigo hazel]

Yeah, which is why in vanilla all data managers are also the reload listener instead of being separate things and recreated on every reload

[fair sandal]

that was my original idea... just store the resource reload listeners in the ReloadableServerResources

[mental ingot]

Yeah but this is literally not documented anywhere, nor is it in any way obvious from the rel listener code that you are supposed to do this

But that brings up a good point

[fair sandal]

yes, it is a major oversight in the APIs provided by both loaders

[velvet whale]

https://github.com/neoforged/Kits/blob/037e4afbd0388b69751867b9b4a2ea5e0dc87a6d/projects/neoforge/src/main/java/net/minecraft/server/ReloadableServerResources.java#L104 but right now the tags updated event is when you're meant to apply data collected by reload listeners

[fair sandal]

and how do you even access the reload listener from the event?

[mental ingot]

I will sleep on this a night or two

I think I have an idea

easier for forge to patch

But it should be doable with a mixin as well

[velvet whale]

static listeners harold

[fair sandal]

yikes

that was my shitty workaround in MD, with private static final Map<ResourceManager, LoadedUpgrades> LOADED_UPGRADES = new WeakHashMap<>();

[fair sandal]

a big advantage of having a static variable to store the results of the reload listeners is that it can be accessed without any context

for TSR, it is probably more appropriate to add onReloadComplete or similar and use it to update the static tier list

unfortunately such a method wouldn't have the PlayerList or even the MinecraftServer available if we also want it to work for client-side reloads

[mental ingot]

Yeah

I need to think this really through

Because there are really 3 or four things to consider

[valid idol]

i sorta skimmed this, but i think the reason why recipes are so late is because the client doesn't know all the recipes, just unlocked ones, and what ones it knows changes during gameplay

[red aspen]

they can also be relocked/unlearned on the client.

[indigo hazel]

No, the client gets all recipes immediately on connect and on reload, the recipe book just hides them from you and gets an update packet of visible recipe names every time that changes, recipes themselves are never sent during normal gameplay

[red aspen]

TIL

[indigo hazel]

I'm working on improving network error logging in the vanilla network stack (i.e. effectively integrating Mrbysco's Spit It Out mod into Neo as discussed with him: #neoforge-github message) and while testing I noticed that packets which are part of a bundle don't get the packet splitter treatment. We should probably change the point in the pipeline where the splitter is inserted in order to capture these packets as well.

[errant summit]

orion do you have time on thursday to finish the design together?

[mental ingot]

Yes probably

[errant summit]

orion ping me when you know when you'll have time

[mental ingot]

I won't be able to make it today

We have an emergency at work

I hope tomorrow will work

[errant summit]

I have time till 18:00 so if you have time today just ping me

[mental ingot]

I should be done around 3 this afternoon

[mental ingot]

@errant summit I need to grab something to frink

And then I am around

[errant summit]

https://hackmd.io/SYCaVQyMQZaaBAp6oGqfSg?view

[mental ingot]

@opal ferry We finally completely finalized the protocol: https://hackmd.io/SYCaVQyMQZaaBAp6oGqfSg#Common-networking-Protocol-V2

Note this add some new things as required to the protocol, primarily the changes are related to support for generic packet splitting particularly for large mod packs this is very usefull. As well as support for synchronizing state id maps in the registry sync, and a better compatibility with the existing fabric protocol.

[obsidian nova]

Not directly related to the protocol, but it would be neat if the packet splitting had an API for streaming packet data as it arrives/sending it streamed

Would be useful for a mod that does file transfers, for instance

Rather than putting everything in memory and having Neo stream it from a byte array

[fair sandal]

is standardizing registry sync really that useful?

[obsidian nova]

Yes. It means that mods can be fully multiloader

[fair sandal]

fabric doesn't have generic packet splitting, as such its registry syncing works differently

[obsidian nova]

Well that's the point of standardizing it, no?

[opal ferry]

Ill take a look shortly, I am in the middle of totally rewriting our networking again 😅

[fair sandal]

nice hackmd though

I like the flowcharts 😄

not convinced by standardizing the config files especially

how we write them is an implementation detail that is only relevant to us

[mental ingot]

Yes it is actually 100% needed. Because vanilla actually uses the registry id in the tag sync packet

So it has to happen

And if the end goal is to support neo client and fabric server (or vice versa) at least on a basic level, then this is actually needed

And not optional

[mild spruce]

please allow connecting to LexForge too, modders can handle the discriminator themselves, that is not on you.

[mental ingot]

No sorry that is not possible because LexForge does this all in the login phase, in ways that is simply not retainable or maintainable

[marsh loom]

It's on Lex whether he implements this handshake protocol

[mild spruce]

Fabric and LexForge can connect to each other right now without issue, so I do not see what the issue is if you support fabric

[mental ingot]

This new protocol is designed to be flexible and extendable, moving towards a common protocol that can be used by everybody

[mental ingot]

You can, but they won't detect each other as "modded"

You can use this version to connect to LexForge alright

[marsh loom]

With mod content? ie, new blocks / items?

[obsidian nova]

I think if you talk to Paper about this they will also implement this

[mental ingot]

But it will consider it as a "vanilla" counterpart

[obsidian nova]

Idk about Spigot or LexForge

[marsh loom]

This was designed with Paper and Sponge in the loop (originally)

[mild spruce]

I have no idea about that as I do not add blocks or items

[marsh loom]

It's not exactly a private matter

[mild spruce]

As long as we can send packets to and from, that is fine. Current version we cannot send or recieve packets because neo blocks them.

[fair sandal]

Ah we need to get paper and sponge in the loop about this stuff too

[mental ingot]

Yes that will be possible, because it will then consider them "AdHoc"

But you can't add any items, block, custom registries nothing

If you touch anything else

[mild spruce]

perfect

[mental ingot]

It will disconnect you

Not even menus

[mild spruce]

That doesn't matter to me.

[mental ingot]

Because they are registries these days

[fair sandal]

I don't think that it's worth trying to have different loaders once you start messing with the registries

[mental ingot]

And they are synced

[mental ingot]

That is not really possible tech

Because vanilla expects the IDs to line up

So either you treat the other loader as modded, and sync the ids

Or you treat him as vanilla

And this whole protocol is pointless

Because the minecraft:register protocol then achieves the same thing

[fair sandal]

Not really. You can get useful info from a modded server without touching registries

(packet flow etc)

[mental ingot]

Meh not really

The point is the old dinnerbone protocol already does what you want tech

But everybody involved agreed that it was outdated and not sufficient

And that we want these loaders to work nicely together

Now that requires some work yes

Because vanilla makes some assumptions we simply have to live with

[fair sandal]

I think that trying to sync registries and especially configs is pointless

By "work together" people mean the most basic stuff (like having waila on a forge server and connecting with a fabric client)

[mental ingot]

Configs is optional

Especially because it is actually not needed

[fair sandal]

For registries imo you just disconnect if there is a mismatch

[mental ingot]

again then the dinnerbone protocol suffices, which people clearly indicated that it was not sufficient. I am sorry if this was all discussed before you arrived

[fair sandal]

The dinnerbone protocol was problematic for other reasons

[hybrid prism]

I'd imagine one cross loader example might be the server component of a minimap communicating with a client that's using another loader

[mental ingot]

Not really, consider this:
Loader a has registration order 1234, loader b registers stuff in 4321. Then the ID lookup for vanillas network packet is broken.

[fair sandal]

Registry sync is gonna fail and disconnect the client in that case

[mild spruce]

currently lexforge and fabric work together enough via the dinnerbone protocol for some mods.
Not sure for JEI or WALIA, works for JourneyMap and some other mods.

[mental ingot]

But why? The user and every average modder says: My mod has the same version with the same shit. Loads be broke

In my opinion, a modder should reasonably be able to create a simple mod for both platforms and have them work together. Does that mean fabric and neoforge need to implement the entire protocol. No. There are optional parts. But at least the basic underlying systems should work the same. We are working to the same goal with the tags etc. There is no real difference for me here.

[velvet whale]

uh i really think configs should be under our own namespace

cause you're never getting that standardized as each does it different (mods included). some may not sync the file but the values

[mental ingot]

That might be a good argument. We discussed this internally, and the reason why we added the optional config sync is simple. There are basically 2 things an average modder needs to have to ensure that the client and server can properly decode the same payloads. Registry ids and configs. We understood that configs are difficult at the moment, because this literally spans the world. And hence the protocol being very lax in that area, including having a note that the contents of that payload are still up for debate. But we strongly believe, and I was not alone in this, that both the registry ids and the configs are needed to provide mods with a stable environment so that they can read, validate and process their payloads

[opal ferry]

I don't think that reg sync should be part of the common packets

[mental ingot]

Why not?

It is required for vanilla as soon as anybody touches any registry at all

[opal ferry]

My idea for this was to just provide the low level mechanisms to send packets between the two platforms, usually when the client is lightly modded

I dont expect people to do this if they have content mods.

[mental ingot]

Okey, that is a bit sad, but from reading #wisdoms-private we interpretted this severaly differently

Specifically because the dinnerbone minecraft:register protocol covers everything that is needed in the case that you do not want to support this

[opal ferry]

I have said multiple times that is possibly something to look at later

[obsidian nova]

I would like to have fully multiloader content mods personally

[mental ingot]

We can make it optional

Would that work?

[opal ferry]

Packet splitting is also not something we are going to start doing for people, that should be on the mod to decide how to do that.

[mental ingot]

Why?

I mean in practice we can trivally make it optional

[obsidian nova]

Packet splitting is immensely useful

[opal ferry]

Config sync, we don't have configs 😄 Likely very opinionated if we did

[mental ingot]

Hence it being optional

But why do you draw the line at splitting vanilla packets

When your exact same code can split every packet

Regardless of what kind it is?

Or do you not split vanilla packets

[obsidian nova]

Ideally Vanilla's own packets should not be split

[mental ingot]

And just let it misteriously fail if the pack gets too big?

[obsidian nova]

As that's inherently incompatible with Vanilla

[opal ferry]

Our reg sync packet is highly optimised for size, I would want a very different format to start. I think its best to be left to the loader

[mental ingot]

We kind of have too, because large mod packs exceed the protocol limit with recipes, tags, etc

[opal ferry]

in 99% of cases if your packet is too big something else has gone badly wrong.

[mental ingot]

That is not really the case especially on large modpacks the recipe packet alone can blow it out of proportion

[opal ferry]

That 1% that knows that the packet can get too big, can trivially implement it.

Fabric doesnt change that, people use 3rd party mods to fix that

[mental ingot]

Okey

[indigo hazel]

The splitter is not injected into the pipeline if the connection is vanilla, so that's a non-issue

[mental ingot]

Can we agree then to make this optional, and point people in the direction of the new protocol

To have that work

At least in somewhat of a harmony

Now to the registry packet:

Link please?

Because I am not sure how you would make this packet any smaller

[opal ferry]

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#L46-L49 Its not fully documented but it shouldnt be too hard to read the details from the code

[mental ingot]

Hmm okey

That seems like a reasonable idea actually for a dense packet there

Cause you are right

You basically are repeating the namespace at least

Not sure what that second secontion means

You are basicallly saying: because the mods use the same namespace, their ids are sequential

And as such you can just send the start and end

And call it a day

Hmm

That is actually not a bad idea

How do you deal with the blockstate and fluidstate id maps?

[opal ferry]

Yes, I forget the details on how this works, its been ages since I looked at it. Its also a good example of where we do have packet splitting.

[mental ingot]

Do you recalculate them on the fly?

[indigo hazel]

The second part as I understand it is basically difference encoding, it only writes the difference to the prior ID instead of the full ID

[opal ferry]

Blockstates are an issue atm, after syncing they are sorted and regenerated. It works fine if the client/server both have the same blockstates

[mental ingot]

Yeah we came to the same problem situation earlier today

Because this eventually reduces to a variant of a pigeon hole principal, to fix this, the only way we knew how, was to send the entire state over

It is not elegant by a long shot

because it requires large packets to do this

The problem we faced, as a thought experiment, is that in reality mod version A van have a blockstate with 2 boolean properties, and int the next it could be 1 boolean and an enum for example

Connecting those mod versions would cause havoc

If sending only the block ids

Because the client would still calculate the indexes with his blockstate definition

While the server had a completely different one

Causing a massive offset

[opal ferry]

Yes, its something that does happen, but easily solved by the user and a slight pain to fix, hence why we havent done it yet

I dont think you need the version selected packet. As you can just use the highest supported version the client responds back with? (it already knows the highest supported by the server)

[mental ingot]

It actually does not

It knows nothing about the server

That is the whole point

The server interogates the client

The client only answers

It is a core concept of secure client to server connections

[opal ferry]

The server can send its supported versions to the client on the first packet?

[mental ingot]

No, because that would allow the client to adapt its response of supported protocols to something that might contain a security issue or an exploitable protocol version

The point of the back and forth is

That the client makes no decision what so ever on the underlying protocol structure

The server decides everything

And the client can only accept or deny the version/channels decided

[opal ferry]

Im not sure I follow tbh,

The server first sends the versions its supports, e.g: [1,2,3]
The client responds with the versions it & the server supports, e.g: [2,3]
The server takes the highest supported version and goes with that. In this case version 3.

If the server knew that version 2 had an problem, it wouldnt tell the client it supported it.

[mental ingot]

It is a simple concept..... The server decides, not the client. If I send the client in the initial packet the list of versions that the server supports, the client can adapt its response, and potentially malicously pick a range of versions it know are compromiseable on the server

But if it never knows what versions the server actually supports

Then it can not request that exploitable version in the first place

It is a general concept of secure protocol negotiations

[opal ferry]

If the server said it only supported v3 inthe first packet, but the client comes back and says to use v2, then the server can disconnect the client?

[mental ingot]

No that is not the point.

Say the server supports 1, 2, 3, where 1 is compromiseable, but needed for backwards compatibility reasons. If the server sends this information to the client initially then the client can craft a response: 1 to force the server into the compromise-able mode, the server can not go back on its word an now say I don't support this afterall.

However if I don't send that information the client has no other option then to send 1. The server can now accept this, or refuse the connection because it is invalid. Or allow it if the operator of the server wants the compatibility mode.

In practice not telling the client what the server supports is the safer option.

It does not matter too much here, yet. But it can matter in the future.

You could probably strip a packet

If the client first sends its channels

And the server just responds with the selected option

Then the client also never learns of the capabilities of the server

[opal ferry]

Right, so the server would technically still support the vunerable version 1, but only when the client doesnt support newer?

[mental ingot]

Correct

[opal ferry]

Got it, that makes sense. Seems unlikely, but thats fine

Thanks for explaining

[mental ingot]

A good malicious client, can still exploit it

But it is a reasonable first step against script kiddies

A professional will get around security by obscurity

[opal ferry]

Oh thats true, seems kinda pointless then. 😄 At least for this usecase

[mental ingot]

But somebody that is just probing will be deterred, which is the initial goal

I mean we make our protocol opensource

So probing it is for a determined enough attacker trivial

He opens the git repo, looks up the PR

and the protocol charts

And starts constructing

But there is no defense against that

What we can do is at least follow basic principals of good protocol engineering

And not open the door completely from the get go

You can probably shave a roundtrip

If you make the client initiate all of this

But that gets tricky with vanilla clients connecting

Allthough in practice both the client and server should know the state of the connection at that point

But then you can get architecturally in hot waters with backwards compatibility with existing protocols, because your server now relies on a very specific packet flow for the connection to continue

So we found this as a compromise

Where the server always sends a "trigger" packet

That indicates the start of the next phase

And then the client and server hash it out

[opal ferry]

I should prob find some resources online for this kind of thing 😄 In this case it makes sense for the server to take command of it.

[indigo hazel]

Isn't the whole idea to prevent trivial versions of something like a TLS downgrade attack?

[mental ingot]

Correct that is indeed the basic principle

As seen with TLS it can be circumvented

But the principal to never let the untrusted party decide something is sound

I will update the protocol and mark some stuff as optional

[fair sandal]

I think that we should leave all of the truly optional (regsync, configs) stuff outside of the protocol

I don't like how complicated this new protocol is, feels like the system we have is overengineered and is now leaking into the protocol :/

I think we should take a step back and reduce complexity

[mental ingot]

I don't agree

The point is that we define stuff to work together on

We can iterate on what we have

We have looked over this with many people now

And your objections to it

Have caused us to re-evaluate what is required and what should be optional

[fair sandal]

IMO we don't need really need more than the fabric protocol already specified

You insisted on having a per-payload version which IMO most people don't have a use for and which apparently makes us unable to support the protocol suggested by modmuss

[mild spruce]

I agree with this. There is no need to make it complex. Vanilla's networking is pretty simple, this feels like an overly complex layer on top of it.

[mental ingot]

Then why do we need a new protocol at all

All you are asking for is what the Minecraft:register protocol already offers

There is no need for any update then

[mild spruce]

I don't know, why is there a need to replace the networking that was already in place with SimpleNetworking?

[mental ingot]

Because it was shit

It was not really designed for the systems mojang has today

[mild spruce]

So it hasn't kept up with the times, that is a fair point.

[mental ingot]

The same can be said for minecraft:register

But in reality both of you are requesting functionality that can be covered with it

If you do not need anything else

Other then agreed upon channels

Then that is it

It is all you need

[mild spruce]

It's not all about what we need. The more complex things are the harder it is to maintain and keep compatible.
While I agree me mc:reg is old, everyone is still using it. It allows for loader compatibility.
I am not saying you're wrong, I was not part of the discussion on why a replacement was needed and I was curious.

[fair sandal]

I think the version selection stuff is a bit pointless

The main goal of the common protocol is to use the vanilla payload format (id + payload), and query whether the other side supports a specific channel

I don't think there was any other requirement coming from the fabric side, or from anyone really

We also need a way to disconnect on mod network version mismatch but that could also be a mods.toml network version or similar, and it doesn't need standardization

[mental ingot]

Sorry tech, but I don't agree

In #wisdoms-private way before you joined this was discussed

With many many many people

This is what fabric proposed

We identified deficiencies with respect to what we need from this protocol

And proposed an upgrade so that it works for both parties

[marsh loom]

(for others watching, that's an internal channel called "wisdoms")

[fair sandal]

It's supposed to be a common protocol though, we can't just make decisions like that

[mental ingot]

The problem is that without this protocol, we can go home

[fair sandal]

The fabric protocol was agreed upon, and fulfils its requirement list imo

[mental ingot]

Because there is no protocl

No it does not tech

That is the point!

That is the whole damn point

It does not fullfill the needs of forge

It did not when it was discussed

It does not now

[dim wagon]

arguing that we should just be fabric is

absolutely bizarre

[mental ingot]

Exactly

[fair sandal]

It clearly did because it was agreed upon ??

[mental ingot]

No

[fair sandal]

That's what I don't understand

[mental ingot]

It was not agreed upon

It was intially proposed that we would look at it when both loaders implement it

Now fabric went ahead with its implementation that is fine

But we only got to it with 20.4.70-beta, and yes a lot is wrong with it

[fair sandal]

I would like to discuss the needs of forge because it seems like we're overdoing it (and fabric might be underdoing it)

[mental ingot]

But it also showed that the intially proposed protocol had severe deficiencies

[mental ingot]

I don't

The needs of forge are clear

We need versioned channels

We need flow based indications of what to invoke and filter

We need packet splitters

[fair sandal]

Why do we need versioned channels? I disagree

[mental ingot]

And we need registry and config sync

[mental ingot]

We had them for ever

Many many many many mods use them

Just because you do not

Does not mean they are not needed as an optional property of a channel

[fair sandal]

People just use them to disconnect when the version is mismatch

[mental ingot]

If you can not see beyond your own plate

Then this discussion is pointless

[fair sandal]

I really don't want to be stuck with an over designed system ... :/

[mental ingot]

Because you will keep on stateting that something others use, is, as you just stated "not needed"

[mental ingot]

It is not over designed

Yes some features need to be made optional

And can be hashed out later

Like reg sync, or configs

But it is needed as part of the protocol

It might not be needed to be implemented by the fabric loader

[fair sandal]

All the optional stuff like regsync has no business being in that basic common protocol...

[mental ingot]

But the definition of the protocol as forge would see it, is needed

[mental ingot]

It has

Because it is needed for the vanilla tag packet to work

[fair sandal]

Sure, maybe the forge protocol but not in the common protocol

[mental ingot]

I do not want, and no body needs, a half assed protocol, which is not better then minecraft:register in any way, and that causes half assed errors and undetermined behaviour or chunk corruption

Think of that what you want

But I don't see the advantage of a half-assed protocol which is not better then anything that was designed nearly 15 years ago

[fair sandal]

Can you link me to some mod that has versioned payloads? I want to see how they were using them in the old system

(as in: multiple handlers depending on the protocol version)

[mental ingot]

I don't have them at the ready, but the API exists

And I am not in the business of removing APIs

Just because people feel like it

[fair sandal]

I need to see a use case

[mental ingot]

Forge

[fair sandal]

The default is to not have the API

[mental ingot]

Forge it self uses it

[fair sandal]

Where is that?

Or was I suppose

[mental ingot]

It still does!

It uses this to allow or disallow what Forge versions are allowed to connect

For example today, it uses the 20.4 text string

To allow all 20.4 client versions to connect with each other

It did this in the past

[errant summit]

have you ever looked at the usb spec? half of that thing is optional!

[mental ingot]

The point of a spec is to have one

Not for it to be hobbled

And completely useless

[errant summit]

optional parts of a spec are there for some to implement that need it (neo) and maybe a fabric mod that wants to have that part of the spec in fabric can just implement it themselves, the point is for it to be common and documented

[mental ingot]

I can really see us provide a fabric mod that implements the registry sync in the same way

And the config sync as well

Put it up on CF
And collect the points?

How about that?

But they should be part of common

So that everybody can implement this

If they want to

But they do not need to

The same with the common tags we all want

Or any of the common infra everybody is looking at

[fair sandal]

There's isn't much common stuff in the works, tags are the main thing

You aren't really using version-specific handlers in forge, you just have a version string that needs to match

That can easily be done by adding a handshake packet after ping and pong

We also don't need to version every single packet channel, we could provide a mod-wide network version instead

[mental ingot]

What do you want from me

[errant summit]

currently you define your own matching logic tho and not just equals

[mental ingot]

I needed to design a compromise

Between an API that allowed virtually anything

And used code callbacks

To something that is at least halfway structured and under control

[fair sandal]

I'm sure we can design something with the current API that works with modmuss' protocol

[mental ingot]

No we can't

Because it literally 100% does not support it

And I have enough of this discussion for today

And frankly for the entire fucking month

[errant summit]

what do you mean by current?

[mental ingot]

Me and schurli have been working our asses of

For fucking months

This was open to you, and everybody else to read

We asked for fucking feedback for months

Including from you

We talked about all of this in this thread for ever

And if you now, now that we ended our work, decide to throw it al overboard, because you like fabric, and completely ignore the needs of any forge mod

Then I am done

I am completely done with this work, this api

All of it

And frankly I would just rather delete it

Then continue working on it

But I won't

Because schurli, xfact and several others also spend countless hours on this

If you now feel offended

[fair sandal]

We have a ton of legacy code that is over designed and needs cleanup, I don't want more to be added to that pile

[mental ingot]

That DOES NOT MATTER A SINGLE FLYING FUCK TO ME

AS I SAID

If you had an issue with the scope of the features we require

Then you should have mentioned that BEFORE the original networking PR

And not now

You are weeks to late

[fair sandal]

I mentioned it, the concerns were brushed off. Now it's leaking into the common protocol and that made me realize just how complicated we made the whole thing

[errant summit]

this is the exact opposite, it is removing the over complicated legacy system (happened in the first pass) and replaces it with a well desigend protocol taht has all needs covered

[mental ingot]

Maybe so, but the arguments to brush them away still stand

So reopening them now gets us no where

Please provide objective feedback on the protocol, not subjective opinions on its scope

[fair sandal]

Yet the current code is overcomplicated. Why do have per-channel version? Why do we have optional packets? (Or should I say why are they possibly non-optional?) What's the use of the packet flow? Etc

[errant summit]

the protocol is not complicated at all (except maybe for the negotiation stuff but that is necessary for security)

[mental ingot]

Because people requested it

Again just because you can not look further then your own plate does not mean it is not needed

Scroll back up in this thread several weeks

When we had this discussion

And you will find enough people asking for them

Maybe the groups asking for those features do not overlap

But that does not mean that we do not need those

[fair sandal]

All of these aspects are solved by having one mod-wide network version, which can be handshaked in another packet after ping pong

[mental ingot]

But that is not what people wanted!

Again

You are arguing for the removal of features people actually requested

[errant summit]

optional packets were even an idea that came from fabrics side afaik

[mental ingot]

Yes it did

[fair sandal]

Yes because all packets are optional on fabric

As such there is no distinction

[mental ingot]

But they are not on forge!

[indigo hazel]

I don't see the significant difference between that and per-payload versions. In one case you compare versions, in the other case you compare versions

[fair sandal]

The difference is that we don't bloat the protocol with packet versions, and that we can do the mod handshake in a separate protocol step

(e.g. beginning of config phase)

And then we get easy compat with the fabric protocol with no real loss of features

[mental ingot]

We can't

We went over this

Thrice now

The fabric protocol is several deficient in the way it determines the versions and channels

Because it runs them after

Even if you pull them to the front

[errant summit]

ok here is my idea: we make a community poll about the channel versioning and see how many people want it

[mental ingot]

I don't see why

If at least a handfull ask for it

then that is suffiecent to not remove it

Again

[fair sandal]

People just want a global mod network version I'm pretty sure

[mental ingot]

These features are agreed upon in a previous PR

We are not in the business of undoing shit

Cause in reality, I would then ask the same thing of the cap attachment split

[errant summit]

so that technitian can see that there actually are people that want them and it's not just us claiming that

[mental ingot]

I hate that it is there

[mental ingot]

It a vail thing to do

[fair sandal]

Then tell me how we would have done block caps

[mental ingot]

We agreed to this API

[mental ingot]

We don't we remove that functionality

It is the same argument

The split adds complexity

So let's just remove it

[fair sandal]

No it's not. Players want their pipes to connect to composters

[mental ingot]

It makes the API and the protocols easier

[fair sandal]

But nobody gives a shit about PER-CHANNEL versioning, I bet

[mental ingot]

I do

[fair sandal]

People just want a disconnect on version mismatch

[mental ingot]

And others which wanted it here in this channel

So if I am nobody then I am sorry

But my work is then done

[fair sandal]

Then give me the damn use case, and why you can't use a global mod version

[mental ingot]

I want it, because I have optional plugins

Those optional plugins have their own versions

And one can connect independently of the version of the main mod

Hence

Payload specific per channel versions

Tada

There that is my usecase

It is a very valid one

[fair sandal]

Ok that is a fair enough use case

I still think a better way to do it is to have a separate handshake aspect to it however

[mental ingot]

That introduces just overhead

Because I do not want the client to connect to the server if the channels are mismatched

[fair sandal]

registerHandshake("myplugin", clientRequired, serverRequired, version) or something

[mental ingot]

So sending the information all at once

[errant summit]

@marsh loom can you make a poll about networking versions here (in an announcement channel) and on github with 3 options:

1. don't need versioning
2. one version per mod
3. one version per channel

[mental ingot]

That is what the API looks like

See the registrar

It is literally that

The payload is also literally that

[fair sandal]

Well it's a bit different cause it wouldn't influence the core networking setup

It's just an extra packet exchange

It's just a disconnect check, no version negotiation for the rest of the connection

[mental ingot]

Well

Maybe an extra packet is possible

It would be an extra round trip

If fabric is fine with it

I used one packet

because that is what fabric does

I just adapted it

To include more optional information

[fair sandal]

It would be an optional set of packets sent after ping pong I would think

[mental ingot]

No after c:protocol_version_suggested at least

Well basically after the supported channels block

Or in there somewhere

I would need to figure out how to indicate the payload intend

And what it means if the payload is missing

[fair sandal]

Well that's why I want to separate it from the core channel system

So that the channels can be sent before that mod(&plugin) version handshake check is done

[mental ingot]

Sure but it has to happen in the same sequence

The negotiation has to complete before the first payload is send

[fair sandal]

Before mod config payloads you mean?

[mental ingot]

Before registry sync

[fair sandal]

Yeah

[mental ingot]

So it would basically happen at the same time it did now

With extra round trips

Is that worth the split?

Because it would be extra overhead

In the form of extra payloads back and forth

[fair sandal]

What I imagine is

<Fabric protocol (4 packets>
<Network version handshake (client and server have the channel info from previous protocol to know if the other side supports a handshake)>
<disconnect or proceed>
<regsync>

[errant summit]

ugh tech I think you are making the protocol more complicated for a bit less code complexity which imo is not a good thing

[fair sandal]

I am separating the handshake from the core channel set exchange

[mental ingot]

It would go where it says "Before Vanilla Configuration Phase"

All the other stuff needs to stay

Because of other reasons

Unrelated to the features

protocol version negotiate is needed

supported channel negotiate is needed

As you stated above

So you are in reality asking me to expand the protocol

Is that really what you want?

[fair sandal]

Isn't a protocol version in the fabric protocol already?

[errant summit]

yea this is the same thing but handled by the server for security

[mental ingot]

Yes and no.

There exists one at the end of the config phase, which says nothing about the stuff in the config phase.

On top the way it is implemented opens the server up to protocol version downgrade enforcement

Neither of which I am willing to support

[fair sandal]

Fabric protocol lets the client choose the protocol version?

[mental ingot]

And has already been discussed with modmuss above that it needs to be changed

[mental ingot]

Yep

See the hackmd

It is all in there

[fair sandal]

Hmmm

[mental ingot]

https://hackmd.io/SYCaVQyMQZaaBAp6oGqfSg#Fabric-configuration-phase-protocol

See the "Negotiation for Play"

[fair sandal]

As we said the client can just pretend to only support the lowest version though

[mental ingot]

The second "c:version" with the selected version comes from the client

[mental ingot]

Exactly

[errant summit]

tech did you actually read the analysis of the old systems?
it includes a conclusion section stating why we do stuff in the new protocol

[mental ingot]

Yeah

This feels like we need to basically re-explain all the work we did over the last 3 monthsd

Even though it is all there

[fair sandal]

Well in reality the client can still do a downgrade attack even if it's not choosing the versions

[mental ingot]

But it is up to the server to reject it properly

The client does not know ahead of time what the server supports

See the discussion above

[fair sandal]

The server can also not support older protocol versions in the fabric protocol

[mental ingot]

https://discordapp.com/channels/313125603924639766/1170668658813575230/1197985866086367352

[mental ingot]

See that and down

[indigo hazel]

It technically can, the important difference is that in the current implementation it can be sure that its attempt will work because the server was too honest while in the suggested implementation it can only attempt the attack but has to expect to be disconnected for trying the wrong thing

[fair sandal]

"1 is compromiseable but needed for backwards compat" there, we have a security flaw already. No matter what we do it's insecure as long as the server allows 1

[fair sandal]

Disconnect then try again with the next version?

[indigo hazel]

The point is to make it harder, you literally cannot make it impossible. See TLS, downgrade attacks on that are also still possible to this day

[fair sandal]

You're not really making it harder for anyone who's competent enough to be a security threat

[mental ingot]

That is not the point

[fair sandal]

Yes it's nice to pretend that it's more secure but the reality is that it's still not secure

[mental ingot]

The point is to not make it trivial

[fair sandal]

It's still trivial. Just try each version from 1 to N until the server allows it

[mental ingot]

Which does not guarantee you an exploitable version

[fair sandal]

I mean just try each exploitable version in order then

If you know which versions are exploitable you can test them 1 by 1

[mental ingot]

Yes but that can be done with every single protocol in existence

That does not however justify us making the protocol explicitly weaker

[fair sandal]

Correct, the only protection is for the server to reject old protocol versions

It's not any weaker in practice

If someone can write a for loop then they can work around your little protection

[errant summit]

ok I think we have discussed this enough for today, it is getting late, lets sleep a night on it and continue tomorrow

[mental ingot]

Modmuss already agreed to this, why are you arguing against it?

[fair sandal]

(to summarize, what I am trying to prove is that c:protocol_version_negociate is not better than the fabric protocol)

[mental ingot]

YOu are literally the only one that thinks that

[fair sandal]

Because it's pretending to be secure without actually being secure?

[mental ingot]

We never said it is secure

We said it is more secure then fabric

[fair sandal]

And it adds an extra set of packets + a round-trip to the protocol for honestly no benefit

[mental ingot]

Because it prevents probative direct lookups

[fair sandal]

Attackers who can craft packets know how to write for loops...

[mental ingot]

And you can't argue that it is not

[errant summit]

we only ever said it is more secure than not having it this way

[mental ingot]

Literally every single secure transport protocol in the fucking world, does not allow the client to determine the version

Or start a protocol negotiation phase for that matter

[errant summit]

(maybe it was a bad idea to start this discussion in the evening our time)

[mental ingot]

It really was

really really was

[marsh loom]

I think throwing out "good" in pursuit of "perfect" is a poor strategy overall.

[fair sandal]

I want to throw out "vaguely useful but not really" for "simplicity" which I think is good philosophy

[mental ingot]

It is not what we are doing

[errant summit]

please let's let it sit for the night (maybe let people form other timezones give feedback) and continue tomorrow

[marsh loom]

Was referring to tech.

[mental ingot]

Ah okey

That could have been interpretted either way

My mistake

[marsh loom]

Server enforced versioning is good. Totally secure protocol is perfect. One is far easier than the other

[fair sandal]

Changing protocol for unclear security benefits isn't all too great I would say

[indigo hazel]

Throwing out all good practices isn't either

[fair sandal]

Tradeoffs

There's no real integrity to protect here, it's not like we're sending banking data to the client

[marsh loom]

It's not totally unclear. It's proven that this strategy is effective in a statistically significant way through many other projects

[fair sandal]

Yet it is also trivial to work around it in this case

[marsh loom]

How relevant that is for us is up to debate

[marsh loom]

As in many other cases. Trivial to work around and not effective are very different things.

You seem to be under the impression that because there's a solution, everyone will take it by default

[errant summit]

guys please I'm tired (and I think most of you are too) lets let this rest and come back to it later

[marsh loom]

In the real world that's not the case

These measures are effective even with a "trivial workaround"

[fair sandal]

These generic statements aren't gonna stop serious attacks

[marsh loom]

Nothing can stop a genuinely serious attack.

Literally nothing

Doesn't mean we can't try to do something

[fair sandal]

Cool then let's keep the protocol stupid simple

It's pointless

[errant summit]

nothing will so that point goes out the window

[mental ingot]

Guys can we stop

[marsh loom]

So your suggestion is to ignore any and all security, because nothing can stop a serious attack. 🤔

[fair sandal]

No, my argument is to not have useless "security measures" in place

[marsh loom]

They're verifiably not useless.

[fair sandal]

They are clearly useless since you can just try multiple protocol versions and see if the server allows them

[marsh loom]

Again, your mental connection between "trivial workaround" and "ineffective" is a fallacy

That's not true in the real world

[fair sandal]

If you can exploit a vulnerability in a protocol you can also write a for loop

[marsh loom]

Yes. You can

[fair sandal]

You're just deluding yourself

[marsh loom]

Right. Deluding myself with the decades of academic research I've performed in this subject. 🤔

Apologies for having prior knowledge in this topic

I'll be sure to come less prepared next time

[errant summit]

you know that curle is a professor right?

[fair sandal]

That's not a reason to overengineer our protocol

[mental ingot]

Seriously is this still going on?!?!?!?

[dim wagon]

I mean I feel like the argument is mostly pointless if the end result still works perfectly cleanly for devs & doesn't conflict with other existing protocols we need to work with

[mental ingot]

I am out

[fair sandal]

Good night

[errant summit]

I asked at least 3 times to stop but got ignored

[marsh loom]

I wanted to understand tech's train of thought, hence the line of questioning

[indigo hazel]

I'm sorry, but a single additional packet being sent is not overengineering, it's a completely trivial measure

[fair sandal]

1. It doesn't improve security at all 2) it fundamentally conflicts with the dinnerbone protocol

[errant summit]

ok everyone in timezones GMT+2 to GMT-2 now stop and sleep on this discussion and come back tomorrow with a fresh mind

[marsh loom]

1 is, yet again, false. I'll give you 2 though

[fair sandal]

The fundamental conflict is a lot worse than the 0.1% security improvement you might get

[marsh loom]

(it's somewhere around 37%)

[dim wagon]

why are we ok with a conflict?

thought the point was to avoid that wherever possible

[marsh loom]

We're not, that's why I'll give it to him :p

[dim wagon]

so if we're not any less secure than vanilla or fabric at present

why should we make a conflict

[mental ingot]

What?!?!

You ahve not read a single line in the hackmd

[indigo hazel]

2. Not having the single additional packet doesn't suddenly make it more compatible with the Dinnerbone protocol. The protocol version negotiation is part of the protocol suggested and implemented by Fabric, the suggested implementation only adds one more packet to that negotiation

[mental ingot]

We are fully compatible with dinnerbones protocol!

[fair sandal]

We are unable to send the real list of supported packets because of all the version negociation

[mental ingot]

We do!

Right at the start!

Look at the hackmd

[fair sandal]

Then why do have all the other shit...

[mental ingot]

Because FORGE AND FABRIC both have other features that they need

And we are back again to 2 hours ago

WTF tech

[errant summit]

I'm going too and I hope to not have more than 10 new messages per person in here tomorrow, so please stop going in circles and continue the discussion tomorrow with a fresh mind

[mental ingot]

Yeah I am out

I am closing discord

Because I am starting to realise tech is not serious

He is just sterring the pot

To get drama

And I am done

[fair sandal]

The protocol is clearly bloated from my pov, you just don't understand how you can fulfill the same set of requirements with less complication IMO.

(either that or I missed a key part, but so far nothing irrecoverable)

[errant summit]

please stop going in circles and continue the discussion tomorrow with a fresh mind

[dim wagon]

if you're leaving, leave

[fair sandal]

Go to sleep then. I will write a summary of my thoughts

[fair sandal]

A NeoForge channel has:

• an identifier (obviously needed)
• a version (imo better split to a different version handshake, see below)
• required and ad hoc packets, the main use case is to make a mod required on one side but having it per-channel is IMO rather pointless. If we separate this to a separate mod network version handshake then we could have all packets be optional (I can see some use cases with required packets to signal new mod versions but that could be handled at the network version level).
• isConnected: probably needs another name but good. In principle indicates that the other side has a handler for this type of packet.
• packet flow: for this I still don't understand the use case, you can use whatever packet flow you want and that shouldn't affect the rest of the protocol.

Based on this analysis, I propose the following two modifications to the fabric protocol:

• Adding an optional mod&plugin version negotiation phase right after Client type determination. This is where you check what is required on the other side and if it matches, and disconnect if it doesn't. In its most basic form this could be a network version in the mods.toml, with an API for mods to register additional components for the plugins (or similar other independent parts) that they have.
• We can do the play version negotiation with a server-oriented design if you prefer, that doesn't actually affect the rest of the protocol.

[valid idol]

but having it per-channel is IMO rather pointless
biggest issue i see with with is multiple mods providing the same protocol on different sides etc
so you might want something that can handle this, dont care what exactly, just as long as it can provide you with the right info etc

[valid idol]

having the version negotiation be exact match seems wrong, if it prevents either side from doing backcompat

also since you cant expect the server and the client to have the same impl (esp in the bukkit/cross loader world),
the mod and plugin version cant actually be used to version the mod / plugin versions, since they cant be changed in lockstep:
it would actually be the shared version of the network protocol they support,
which i think makes more sense to be linked to the specific channels they use (which can be changed together, as they can support x protocol version, or even multiple if its not exact match).

[languid hearth]

i dont think what the literature says is a "downgrade attack" is what is happening in this scenario

yeah i see no connection between this and TLS

what's the use case for supporting multiple protocol versions?

[hasty yew]

Mod changes their network protocol, but still wants to be able to accept client connections using the old protocol, using slightly different logic

Also, any sort of channel version stuff should definitely be per channel not per mod - figuring out which mod something is being sent by isn't trivial without unreliable stack fuckery or reliance on mod bus context stuff or whatever, which is all very "magic"

[languid hearth]

so a server wants to support a more vulnerable protocol version

the server can either choose to accept it => the server has patched the vulerabilities

or the server doesn't

either way if a client chooses to connect with a bad version it'll only affect the client, no?

the only one being hurt by this downgrade "attack" is the client

i'm still not sure on what this is for so correct me if i'm wrong

[valid idol]

yeah i think i agree
server supports old version, patched: fine, intended, backwards compat
server doesnt support old version: fine, not exploitable
server supports old version, in a way that is exploitable: the server's fault for using an exploitable version: the mod should be updated to fix the expolit or remove the version

if the client supports a newer version of the protocol it will use that (esp with server-authoritative version selection)
if it doesnt, itl request the versions it does support, and might be denied if there isnt a secure/compatable version
older protocol version != more vulnerable tho
i too don't see the harm in this negotiation

[pliant stream]

the point of registry syncing is to sync the numeric ids, which is dependent on mod load order.

if we're not going to sync then we need to make registration order fully deterministic

(ideally in a way that can be replicated cross-loader, though cross loader content mods are a pretty big reach in general - but certainly in a way that can be replicated with different minor versions of mods on each side)

[obsidian nova]

There are a few multiloader content mods (e.g. Create and many addons)

It would be neat if they... just worked cross-loader

[brave rose]

Do you mean Fabric client and Neo server or vice versa?

While more standardization with networking would make that easier, I'm pretty sure there are other things like Neo vs. Porting Lib NBT keys and formats that would make only networking standardization insufficient

[fair sandal]

Nobody wants to deal with the horrible bug reports that this would imply

[brave rose]

I agree it's not worth supporting such a configuration for Create

However I think that it may be feasible for other mods, so standardized registry sync could still be helpful

[pliant stream]

there's certainly no reason "mcreator mod that adds a new ore that's better than netherite" couldn't work cross loader, for example

but regardless, even ignoring the cross-loader scenario, we need either registry sync or fully deterministic registration

and i believe historically it has not been a problem for a client to have registered blocks or items that do not exist on the server.

[hasty yew]

Fully deterministic registration, that doesn't depend on the presence of server only or client only mods of any sort or on mod versions, seems like a nightmare to get consistently with the current approach to mod ordering. Registry sync seems cleaner

Like imagine. Some mod declares an ordering constraint in version X that wasn't present in version Y. Thus, the mods with version X vs Y would sort differently, even if registries haven't changed at all - so you'd have to, like, sort the registries by resource location or something? I guess you could do that?

But even then the checks that both sides have the same registry entries are an important bit of verification I definitely wouldn't remove

[buoyant pagoda]

IMO, we can work on registry sync as a common protocol in the future (since it could be technically optional, and signalled by the presence of the channel in negotiations in minecraft:register or equivalent)
no use in trying to implement in NF a 'common' registry sync protocol that only we at NF have agreed to use (since modmuss seemed to indicate that Fabric would keep to their own optimized registry sync protocol)

at this point, the main focus should be refactoring networking to fixup the minecraft:register issue

either looking at Orion's proposed spec for the common protocol, or making our own protocol for now (fixing/replacing our broken use of minecraft:register) and looking to implement a common protocol in the future

[pliant stream]

I do think Luke hit on something important, in that it's only the order of objects in registries, and not the order of execution of the registry events - that would need to be deterministic. That's at least somewhat more tractable.

[hasty yew]

Well, sorta. Verifying that both sides have the same stuff in their registries (resource locations) matters too. And the block state fuckery makes it all even more complicated. As sci said, luckily at least partially a problem for another day

[pliant stream]

Well, yeah, I meant for going to the "verify and require that they're the same rather than syncing" approach

In principle we could use a hash.

[hasty yew]

Lots of options

Does mean that if they're not the same you have to request more information from the server to display a sensible error, but... still, something to think over in the future. Currently the major priority is probably just getting stuff working

[brave rose]

Some mods intentionally only register blocks server-side

[pliant stream]

I don't see how that can work. I could see it working for entity types, but not blocks or items

[fair sandal]

It works with a lot of hacks

[fair sandal]

Yes, I think that the version handshake, config, and registry sync should be removed from the c namespace

Generic packet splitting is something that fabric doesn't do either and as such isn't "common"

[errant summit]

I can see a lot of confusion here

1. we already decided to make registry sync optional
2. confusion between protocol version and channel version:

• protocol version is not something mods control but something the server/client implementation does (so fabric/neo/paper/etc.)
• channel version is what a mod controls (currently on a per channel level) what technici4n proposes is that we get rid of that version and only have one network version per mod that is strictly equality compared on negotiation

[errant summit]

I found a good example of a cross loader content mod for which the per channel versions would be useful: quark
let's say the server has only module A enabled in quark and a channel for module B recently updated on the server, with per channel versioning an older client could still connect to that server since module B is disabled anyway

also since I think many of you have not read the analysis and requirements part of the document I'll post the main things here:

[errant summit]

it is a spec like USB even USB has optional stuff in its spec that not all that do USB implement

[fair sandal]

I think that most of the optional bits should be under the neoforge namespace

[errant summit]

the thing here is that if fabric ever wants to do it they just can but if we put it under our own namespace they never will

[fair sandal]

No, we can just agree to a standard protocol later on

[errant summit]

imo we can remove config syncing yes but reg id sync and packet splitting should be an optional feature of the spec

[errant summit]

that would mean bumping the protocol version

[iron ivy]

I agree, registry sync and packet splitting should be part of the spec.

[fair sandal]

The reg sync makes a lot of assumptions, and fabric does things differently. Why don't we use their spec instead

[iron ivy]

That was literally the convostaion earlier in the channel