#velocity

2096 messages · Page 3 of 3 (latest)

hexed flickerBOT
#
WouterGritter
[PaperMC/Velocity] Pull request opened: #1800 Cap pre-join plugin-message queue size (prevents arbitrary growth/OOM)

A client that stalls its FML/login handshake could enqueue plugin messages (each up to ~32 KiB serverbound) without bound in ClientPlaySessionHandler#loginPluginMessages, growing per-connection heap until an OOM kill. This PR caps the queue by both bytes (4 MiB) and count (1024), configurable via velocity.max-queued-login-plugin-message-bytes and velocity.max-queued-login-plugin-messages, and disconnects on overflow.

#
WouterGritter
[PaperMC/Velocity] Pull request opened: #1801 Fix build-time GraalVmProcessor warning

Building the project shows the following warning:

> Task :velocity-proxy:compileJava
warning: The `GraalVmProcessor` annotation processor is missing the recommended `log4j.graalvm.groupId` and `log4j.graalvm.artifactId` options.
  To follow the GraalVM recommendations, please add the following options to your build tool:
    -Alog4j.graalvm.groupId=<groupId>
    -Alog4j.graalvm.artifactId=<artifactId>

This PR fixes this by adding the required options.

#
WouterGritter
[PaperMC/Velocity] New comment on pull request: #1800 Cap pre-join plugin-message queue size (prevents arbitrary growth/OOM)

Depending on how long a client is actually able to stall the login phase for, this may be a scary OOM once again.

Without this fix, and at the default rate limit of 500 pps, a client would be able to fill this queue with 160MB of plugin messages every second (at 32kb/PM). That's 10GB within a minute, currently exploitable.

Of course this PR fixes that, and I'm sorry to keep bumping this but, #1786 would have greatly crippled this attack. At a limit of 5MB/s of decompressed packets per second, it would take around half an hour to fill the same 10GB in this queue. Still an OOM, if the client is actually able to stall the login phase for 30 minutes, but way less instant.

hexed flickerBOT
#
bluegreensea
[PaperMC/Velocity] New comment on pull request: #1734 Fix dimension reading in JoinGamePacket

I think this just hack the allowed range from -128 ~ 127 to -1~254 (255 is -1).
If modded servers register a dimension in -2 ~ -128, it may still broken.

And the second things, GTNH is not "hack this to an unsigned byte to allow dim IDs up to 255".
It hack to an int to completely fix this vanilla bug.
(https://github.com/GTNewHorizons/Hodgepodge/blob/master/src/main/java/com/mitchej123/hodgepodge/mixins/early/minecraft/packets/MixinS01PacketJoinGame_FixDimensionID.java)

hexed flickerBOT
#
WouterGritter
[PaperMC/Velocity] New comment on pull request: #1734 Fix dimension reading in JoinGamePacket

If modded servers register a dimension in -2 ~ -128, it may still broken.

Not sure if this is possible, in older versions anyway. JoinGamePacket handles the dimension as a byte, regardless of its sign, it will always encode/decode as the same hex value. Though in vanilla minecraft this is definitely expected to be signed, because the nether is -1. RespawnPacket just encodes this as an integer instead, which is also expected to be signed, as the vanilla protocol expects the values -1, 0 and 1.

GTNH is not "hack this to an unsigned byte to allow dim IDs up to 255".

IMO it is a hack. The nether being -1 suggests that this value should be signed. Mods using a value over 127 would have needed to patch this bug also; RespawnPacket needs to return a signed -1 only ever for the nether, otherwise it should treat the byte as unsigned (and have a range from 0 - 255). This patch makes the range for the dimension value essentially be -1 - 254 (inclusive), as 255 is effectively mapped t...

hexed flickerBOT
hexed flickerBOT
hexed flickerBOT
#
WouterGritter
[PaperMC/Velocity] Pull request opened: #1802 Fix `TitleActionbarPacket`'s default action type by setting it to `SET_ACTION_BAR`

Correct me if I'm wrong, but it seems like SET_ACTION_BAR is the right action for the TitleActionbarPacket packet.

This bug doesn't surface when creating this class from the factory:

// GenericTitlePacket
public static GenericTitlePacket constructTitlePacket(ActionType type, ProtocolVersion version) {
  GenericTitlePacket packet = null;
  if (version.noLessThan(ProtocolVersion.MINECRAFT_1_17)) {
    packet = switch (type) {
      case SET_ACTION_BAR -> new TitleActionbarPacket();
      case SET_SUBTITLE -> new TitleSubtitlePacket();
      case SET_TIMES -> new TitleTimesPacket();
      case SET_TITLE -> new TitleTextPacket();
      case HIDE, RESET -> new TitleClearPacket();
      default -> throw new IllegalArgumentException("Invalid ActionType");
    };
  } else {
    packet = new LegacyTitlePacket();
  }
  packet.setAction(type);
  return packet;
}

Namely through:

case SET_ACTION_BAR -> new TitleActionbarPacket();
packet.setAction(type); // overrides```...
#
WouterGritter
[PaperMC/Velocity] New comment on pull request: #1802 Fix `TitleActionbarPacket`'s default action type by setting it to `SET_ACTION_BAR`

We may even enforce this contract by adding this below the switch statement in the factory method:

if (packet.getAction() != type) {
  throw new AssertionError("Title packet action type mismatch!");
}

But I don't think it's worth it to assert this at runtime (and this won't even work, TitleClearPacket can be created with both HIDE and RESET).

The enforcing of this is also quite inconsistent. TitleClearPacket throws when setting its action type to anything but HIDE and RESET, but the other packets let you set the action type to anything you want. Maybe it's worth expanding this PR to enforcing this for all other packet's setAction methods as well?

hexed flickerBOT
hexed flickerBOT
hexed flickerBOT
#
VidTu
[PaperMC/Velocity] New comment on pull request: #1781 Small optimization to prevent blocking netty threads on UUID.randomUUID()

I actually got curious and wrote a small benchmark for that and ran on my (admittedly ancient) desktop PC on both Windows (everyday usage) and Linux (installation ISO without background services):

> Windows
Benchmark                    Mode  Cnt       Score      Error   Units
UuidBenchmark.uuidInsecure  thrpt    6  151409.421 ± 1504.844  ops/ms
UuidBenchmark.uuidSecure    thrpt    6     698.752 ±   10.978  ops/ms

> Linux
Benchmark                    Mode  Cnt       Score     Error   Units
UuidBenchmark.uuidInsecure  thrpt    6  156989.602 ± 593.248  ops/ms
UuidBenchmark.uuidSecure    thrpt    6    2846.491 ±  35.525  ops/ms

So yeah, there are good magnitude-order speedups (less good on Linux, probably due to better reseeding from the kernel CSRNG). However, the baseline of 700 (or 2800) UUIDs/ms is not something that needs extreme urgent optimization.

hexed flickerBOT
hexed flickerBOT
hexed flickerBOT
hexed flickerBOT
hexed flickerBOT
hexed flickerBOT
hexed flickerBOT
hexed flickerBOT
hexed flickerBOT
hexed flickerBOT
hexed flickerBOT
hexed flickerBOT
#
suerion
[PaperMC/Velocity] Issue opened: #1805 Velocity with Modded Fabric and Automodpack

Expected Behavior

Hey, i'm trying to use Velocity for Modded Fabric on 26.1.2.

I had tried to join the Server without Velo, and it work right, but i had used now, the Automodpack plugin with the Mod from Automodpack. and this works fine :)

Without Velocity:
Client
https://mclo.gs/ojlvXCv

Server Log: https://mclo.gs/HIjrh9B

Actual Behavior

But when i try to join the server i got an issue and we had used -Dvelocity.max-known-packs= for more packets.

But, on joining it does not work and it throws this error:

[15:46:18] [Netty epoll Worker #4/ERROR] [com.velocitypowered.proxy.connection.MinecraftConnection]: [server connection] Suerion -> survival: exception encountered in com.velocitypowered.proxy.connection.backend.BackendPlaySessionHandler@2367b702
io.netty.handler.codec.CorruptedFrameException: Error decoding class com.velocitypowered.proxy.protocol.packet.AvailableCommandsPacket Direction CLIENTBOUND Protocol 26.1 State PLAY ID 0x10
	at com.velocitypowered.proxy.p```...
hexed flickerBOT
#
suerion
[PaperMC/Velocity] New comment on issue: #1805 Velocity with Modded Fabric and Automodpack

@electronicboy Then the labels are not Correct on Modrinth and Curseforge...

Had added crossstich and it work, but i got this issues on Velo:

-> Caused by: java.lang.IllegalArgumentException: Argument type identifier 58 unknown.

Velocity log
https://mclo.gs/SmmCICQ

Server Log
https://mclo.gs/50YzUOq

Client Log:
https://mclo.gs/AzeEUNv

Network Protokol
https://mclo.gs/ppm8Y5j

Should i try to disable BadPackets and NetProdis?

hexed flickerBOT
#
Emilxyz
[PaperMC/Velocity] New comment on issue: #1805 Velocity with Modded Fabric and Automodpack 
[17:22:04] [main/WARN]: Error loading class: net/minecraft/class_2641$class_7232 (java.lang.ClassNotFoundException: net/minecraft/class_2641$class_7232)
[17:22:04] [main/WARN]: @Mixin target net/minecraft/class_2641$class_7232 was not found crossstitch.mixins.json:command.CommandTreeSerializationMixin from mod crossstitch

I don't really know anything about fabric/modded platforms, but this makes me guess that crossstitch is not yet compatible with 26.1, probably thanks to the removal of obfuscation. There is https://github.com/PaperMC/CrossStitch/pull/23 which looks to update it, but I'm not sure what the state of that is.

If you can, give that pr a try and let us know if that resolves your problem.

hexed flickerBOT
#
suerion
[PaperMC/Velocity] New comment on issue: #1805 Velocity with Modded Fabric and Automodpack 
[17:22:04] [main/WARN]: Error loading class: net/minecraft/class_2641$class_7232 (java.lang.ClassNotFoundException: net/minecraft/class_2641$class_7232)
[17:22:04] [main/WARN]: @Mixin target net/minecraft/class_2641$class_7232 was not found crossstitch.mixins.json:command.CommandTreeSerializationMixin from mod crossstitch

I don't really know anything about fabric/modded platforms, but this makes me guess that crossstitch is not yet compatible with 26.1, probably thanks to the removal of obfuscation. There is PaperMC/CrossStitch#23 which looks to update it, but I'm not sure what the state of that is.

If you can, give that pr a try and let us know if that resolves your problem.

@Emilxyz
With the pull, it workes! :) had build it and added it, now it work :), i could join :) it works fine, now i could test more

<img width="1920" height="1080" alt="Image" src="https://github.com/user-attachments/assets/1de33d97-50d7-4fcf-8230-5a5634fc8243" />

@electronicboy i had said, 26....

hexed flickerBOT
#
evan-goode
[PaperMC/Velocity] Draft pull request opened: #1806 Fix "Invalid signature" when not forwarding UUIDs to `online-mode=false` backends

Filter the ServerboundChatSessionUpdatePacket when player info forwarding is disabled, since the client's profile key signature will not be valid for the player's offline UUID.

See comment in ClientPlaySessionHandler.java. This is marked as draft since it's in a "works for me" state, though I'm happy to address feedback and get this merged. I'm unsure whether this behavior belongs in a separate option or whether player-info-forwarding-mode = "NONE" is a good enough heuristic for when it's needed.

My use case is a public, Mojang-authenticated gateway to an online-mode=false Vanilla server (no Paper/Fabric) that unauthenticated bots can connect to over localhost.

hexed flickerBOT
#
Timongcraft
[PaperMC/Velocity] New comment on issue: #1805 Velocity with Modded Fabric and Automodpack 
[17:22:04] [main/WARN]: Error loading class: net/minecraft/class_2641$class_7232 (java.lang.ClassNotFoundException: net/minecraft/class_2641$class_7232)
[17:22:04] [main/WARN]: @Mixin target net/minecraft/class_2641$class_7232 was not found crossstitch.mixins.json:command.CommandTreeSerializationMixin from mod crossstitch

I don't really know anything about fabric/modded platforms, but this makes me guess that crossstitch is not yet compatible with 26.1, probably thanks to the removal of obfuscation. There is PaperMC/CrossStitch#23 which looks to update it, but I'm not sure what the state of that is.

If you can, give that pr a try and let us know if that resolves your problem.

It was ready but at the time I couldn't find any real mod with a custom argument for 26.1, so I wanted to wait for that.

hexed flickerBOT
hexed flickerBOT
#
WouterGritter
[PaperMC/Velocity] Pull request opened: #1808 Store backend command tree per server connection to allow on-demand resending of command set

Inspired by:

With this PR, plugins may dynamically decide to add commands to the client's command tree by triggering Player#sendAvailableCommands and listening for the PlayerAvailableCommandsEvent that gets fired as a result of it.

How much memory does this PR consume per player?

Let's assume 1.000 commands per player. A conservative lower bound, and a complete but educated-ish guess, would be about ~64 bytes per command, giving 64kb per player (for 500 players, this is 32MB)

Take this with a giant grain of salt, but I had Claude Opus 4.7 have a go at estimating the memory footprint this PR brings with it.
Prompt: Investigate exactly how much memory 'RootCommandNode<CommandSource> backendCommandsNode' in 'BackendPlaySessionHandler' consumes per player for 1000 commands with typical str...

#
WouterGritter
[PaperMC/Velocity] Pull request review comment: #1808 Store backend command tree per server connection to allow on-demand resending of command set

I'm not too sure if we need to return a CompletableFuture here. This doesn't get returned anywhere else in the Player interface, but at the same time, when this is completed it does guarantee that all PlayerAvailableCommandsEvents have been fired and the packet is on its way to the player.

Could this be useful for plugins? Or should we just return void and rely on plugins listening for this even to figure out that the packet is (about to be) on it's way?

#
WouterGritter
[PaperMC/Velocity] New comment on pull request: #1774 Adventure 5

Velocity didn't have these type pointers before so the deprecation of adventure platform has absolutely nothing to do with this.

What do you mean by this exactly? Velocity does currently resolve FacetPointers.TYPE to FacetPointers.Type.PLAYER and CONSOLE through pointers().

It seems strange that this was ever the case, as net.kyori.adventure.platform.facet is an unsupported API that really should never have been used in the first place.

This makes the deprecation of platform-facet and the likes put Velocity in quite a predicament... Removing the dependency could break plugins relying on it, but continuing to use a deprecated library/API also doesn't seem like the right solution.

I'd rather this was included/PR'd into adventure-api.

Getting net.kyori.adventure.platform.facet into adventure-api does seem like the proper solution, but I don't know if they'll want that. Another solution would be to provide these classes in proxy/deprecated/adventure-facet, similar to con...

#
WouterGritter
[PaperMC/Velocity] New comment on pull request: #1808 Store backend command tree per server connection to allow on-demand resending of command set

My initial thought was to put this behind add a feature flag system, which would need to be enabled by plugins on init but...

I mean we could do this intuitively.

  • get rid of Player#sendAvailableCommands
  • introduce Proxy#getAvailableCommandsService
interface AvailableCommandsService {
  void sendAvailableCommands(Player player);
}

Plugins would need to obtain the AvailableCommandsService which would trigger the command tree to now be saved.

The issue is however that plugins really need to do this on startup, or they'll end up missing the backend command trees from players that are already connected.

Though I don't really like this. Seems complex and unnecessary to avoid a couple megabytes of ram, realistically.
Maybe an opt-out system flag would be better. Having memory issues? Know that no plugins make use of this feature? Disable it.

hexed flickerBOT
#
Emilxyz
[PaperMC/Velocity] New comment on pull request: #1774 Adventure 5

As you've already mentioned, these pointers are marked as internal by adventure-platform. They're also not actually provided as a dependency by velocity-api and only required by velocity-proxy, which is not even published. So plugins using these is extremely unlikely and also unsupported anyways.

I'm also assuming that this pr will come with a velocity version bump, due to the other breaking changes in adventure 5. So I think just doing a clean cut and dropping adventure-platform-facet should be fine. Maybe mention it in an update announcement.

hexed flickerBOT
#
Timongcraft
[PaperMC/Velocity] New comment on pull request: #1808 Store backend command tree per server connection to allow on-demand resending of command set

My initial thought was to put this behind add a feature flag system, which would need to be enabled by plugins on init but...

I mean we could do this intuitively.

  • get rid of Player#sendAvailableCommands
  • introduce ProxyServer#getAvailableCommandsService
interface AvailableCommandsService {
  void sendAvailableCommands(Player player);
}

Plugins would need to obtain the AvailableCommandsService which would trigger the command tree to now be saved.

The issue is however that plugins really need to do this on startup, or they'll end up missing the backend command trees from players that are already connected.

Though I don't really like this. Seems complex and unnecessary to avoid a couple megabytes of ram, realistically. Maybe an opt-out system flag would be better. Having memory issues? Know that no plugins make use of this feature? Disable it.

We could also just expose the AvailableCommandsService in the proxy init e...

#
WouterGritter
[PaperMC/Velocity] New comment on pull request: #1808 Store backend command tree per server connection to allow on-demand resending of command set

Alternatively we could keep the method as is and throw if it was not enabled by some feature system on init.

I don't think we have any feature that works like this right? If we are going to set a precedent for "resource intensive feature that must be opt-in in an elegant way" we need to think of a proper solution, one that would also work for future features like this.

An alternative for this specific PR could be to store the raw packet instead of the decoded command tree. Decode the packet only when sendAvailableCommands is called. Trade a bit of CPU time (only when this feature is used) for a lower memory footprint. I'm kinda leaning towards this; I might do some testing to see how much this would reduce memory usage in comparison.

#
Timongcraft
[PaperMC/Velocity] New comment on pull request: #1808 Store backend command tree per server connection to allow on-demand resending of command set

Alternatively we could keep the method as is and throw if it was not enabled by some feature system on init.

I don't think we have any feature that works like this right? If we are going to set a precedent for "resource intensive feature that must be opt-in in an elegant way" we need to think of a proper solution, one that would also work for future features like this.

Sure, though we also should think about the users of this API and if it is hidden behind some 'obscure' mechanism they might not find it

#
WouterGritter
[PaperMC/Velocity] New comment on pull request: #1809 Trigger resourcepack callbacks

Yea I'm not sure, one of the maintainers might have an idea

Are clients able to trigger this flow, of adding a resource pack & thus registering a callback? I don't think so but I might be wrong.

If we can only trigger a callback being registered through a plugin, I don't think this is really a cause for concern. The lifetime of the map (and its entries if the client never responds) is only as long as the client is connected, worst case we're storing a couple stale entries but that buys us having compatibility with slow clients.

#
WouterGritter
[PaperMC/Velocity] New comment on pull request: #1808 Store backend command tree per server connection to allow on-demand resending of command set

Sure, though we also should think about the users of this API and if it is hidden behind some 'obscure' mechanism they might not find it

I mean, if we don't want to hide this API, we could keep Player#sendAvailableCommands and throw with an exception saying You must call ProxyInitializeEvent#enableFeature(FeatureFlags.SEND_AVAILABLE_COMMANDS) to make use of this feature!

This should never really produce an exception at runtime, because plugin developers will (should) catch this during development.

Alternatively we could log an error or warning and have it behave like a NOP instead of throwing.

hexed flickerBOT
#
WouterGritter
[PaperMC/Velocity] New comment on pull request: #1808 Store backend command tree per server connection to allow on-demand resending of command set

An alternative for this specific PR could be to store the raw packet instead of the decoded command tree. Decode the packet only when sendAvailableCommands is called. Trade a bit of CPU time (only when this feature is used) for a lower memory footprint. I'm kinda leaning towards this; I might do some testing to see how much this would reduce memory usage in comparison.

This is really cursed. AvailableCommandsPacket needs to store a byte[] encoded and perform encoding during the constructor, decoding during the getRootNode getter. Reading/writing the bytes into this encoded field during encode() and decode(). IMO this isn't worth it, it deviates from how other packets do things so much, but it will reduce the memory footprint by a lot. Curious what other maintainers think of this approach.

hexed flickerBOT
#
Timongcraft
[PaperMC/Velocity] New comment on pull request: #1808 Store backend command tree per server connection to allow on-demand resending of command set

Sure, though we also should think about the users of this API and if it is hidden behind some 'obscure' mechanism they might not find it

I mean, if we don't want to hide this API, we could keep Player#sendAvailableCommands and throw with an exception saying You must call ProxyInitializeEvent#enableFeature(FeatureFlags.SEND_AVAILABLE_COMMANDS) to make use of this feature!

This should never really produce an exception at runtime, because plugin developers will (should) catch this during development.

Well this is what I meant.

Alternatively we could log an error or warning and have it behave like a NOP instead of throwing.

Well I think this could be annoying, and idk but I see no benefit with this option