Fix/registry and keep alive config phase#76
Conversation
Adds json_to_nbt_with_options(json, options) and reroutes json_to_nbt through it with default options. When numeric_widening is enabled: - integer JSON numbers become Int (or Long on overflow), not the smallest-fitting Byte/Short - float JSON numbers become Float (32-bit) unconditionally, matching what Mojang vanilla emits for registry data - booleans stay Byte - integer arrays become IntArray/LongArray, never ByteArray This is the foundation for emitting registry data that strict codecs (PacketEvents) can decode.
…ry JSON Routes registry JSON loading through json_to_nbt_with_options with numeric_widening enabled. The on-wire RegistryDataPacket bytes now use the same NBT tag types Mojang vanilla emits — Int instead of downcast Byte/Short for tick counts and similar fields, Float instead of Double for ease curves and similar floats. Fixes the PacketEvents 2.12.x crash on minecraft:timeline registry sync in MC 1.21.11+. Cross-references retrooper/packetevents#1482.
…NBT tags Pins the regression: loading data/generated/V26_1/data/minecraft/timeline/day.json must produce period_ticks: Int(24000), time_markers.wake_up_from_sleep: Int(0), day.ticks: Int(1000), show_in_commands: Byte(1) (boolean), and moon_angle.ease.cubic_bezier[0]: Float(~0.362) — matching what Mojang vanilla and NanoLimbo emit on the wire.
ProtocolVersion::humanize() returns the minor name (e.g. "26.1"), but the Mojang select_known_packs handshake requires the exact release tag (e.g. "26.1.2"). Sending the wrong string causes the client to reject the pack and forces the server to send every registry instead of letting the client use its bundled vanilla data — defeating the whole handshake. Adds a known_pack_version() helper with per-version overrides. Only V26_1 needs one today; older protocols where humanize() already matches the release tag fall through to humanize().
Mirrors ClientBoundKnownPacksPacket but derives PacketIn instead of PacketOut, so the server can decode the client's select_known_packs response and decide what configuration packets to send next.
Consumers of PacketIn-decoded KnownPack instances need to inspect namespace, id, and version. Matches the field visibility of other serverbound packets in this crate.
Before this commit, LoginAcknowledged handler dumped the entire configuration burst (brand + KnownPacks + tags + every registry + FinishConfiguration) in one go for protocols >= 1.20.5. The client's select_known_packs response was ignored — every client received every registry whether or not it already had the vanilla pack locally. After this commit: - LoginAcknowledged sends only brand + KnownPacks and stops. - ServerBoundKnownPacksPacket is registered as a configuration-state serverbound packet and dispatched to a new handler. - The handler decides what to send next based on whether the client accepted minecraft:core at the expected version. If yes, only tags + FinishConfiguration. If no, tags + every registry + FinishConfiguration. For protocols < 1.20.5 (no KnownPacks handshake), behavior is unchanged — the full burst is sent from LoginAcknowledged. This matches Mojang vanilla / Paper behavior and avoids shipping every registry to clients that already have them, which was the underlying cause of the PacketEvents timeline crash for non-vanilla-pack-accepting clients.
Re-uses the existing ClientBoundKeepAlivePacket struct; the wire format (long id) is identical between CONFIGURATION and PLAY. Only the protocol-id-in-state differs (id=4 in CONFIG vs id=44 in PLAY on V26_1), and the PacketReport macro pulls that per-version from the data report files.
Gives a 3x margin under Velocity's default 30s read-timeout, and matches the period the CONFIGURATION-state keep-alive will use.
Without this, the ControllableInterval stays Disabled until the player acknowledges FinishConfiguration, leaving Velocity's read-timeout (30s by default) free to kill the backend connection during long CONFIGURATION holds — breaking flows that gate the player on an async dialog (e.g. registration) for longer than the timeout.
|
|
||
| const NAMELESS_ROOT: u8 = 1 << 0; | ||
| const DYNAMIC_LISTS: u8 = 1 << 1; | ||
| const NUMERIC_WIDENING: u8 = 1 << 2; |
There was a problem hiding this comment.
Do you have any documentation about this numeric widening or have you reverse-engineered Mojang's code?
There was a problem hiding this comment.
Reverse-engineered by comparing wire output with NanoLimbo (which works with PacketEvents) and inspecting how Mojang's NbtOps emits canonical NBT for any value passed through their Codec API.
| /// whose release tag differs from `humanize()` needs an arm here. | ||
| fn known_pack_version(version: ProtocolVersion) -> &'static str { | ||
| match version { | ||
| ProtocolVersion::V26_1 => "26.1.2", |
There was a problem hiding this comment.
So what happens if I join PicoLimbo in 26.1 or 26.1.1 ?
There was a problem hiding this comment.
You're right, this shouldn't have stayed in the PR.
I added this temporarily while diagnosing the timeline issue, to confirm whether the client was rejecting our select_known_packs offer.
protocol_version.humanize() returns "26.1" but in my tests i found that the client compares the pack version strictly against its locally-stored minecraft:core@26.1.2 (or .1 / .0), so the offer never matches and the client always responds with an empty list, which forces the full registry send path. Hardcoding V26_1 -> "26.1.2" was just a quick way to confirm that hypothesis on the wire, not a real fix.
| let period = Duration::from_secs(15); | ||
| // 10 s gives a 3x safety margin under Velocity's default 30 s | ||
| // read-timeout, and matches the period used in CONFIGURATION state. | ||
| let period = Duration::from_secs(10); |
There was a problem hiding this comment.
The vanilla interval is 15 seconds, I'd prefer to match vanilla's behavior. Or perhaps the period is different during the configuration phase?
There was a problem hiding this comment.
You're right, vanilla is 15 seconds. I went with 10 seconds because most proxies (e.g. Velocity) have a default read timeout of 30 seconds, so a single miss could lead to a read timeout in the vast majority of default setups. 10 seconds was just an idea to keep some margin.
Side note: PaperMC sends this keep-alive every second, lol.
Would it be interesting to make this value configurable in server.toml, or not?
| bound = "clientbound", | ||
| name = "minecraft:keep_alive" | ||
| )] | ||
| ConfigurationClientBoundKeepAlive(ClientBoundKeepAlivePacket), |
There was a problem hiding this comment.
The packet id for this packet may need to be added for older versions, I think it is missing prior to 1.21
There was a problem hiding this comment.
Cross-checked the data reports: minecraft:keep_alive clientbound in CONFIGURATION state is present from V1_20_2 onwards (id=3 in V1_20_2/V1_20_3, id=4 from V1_20_5+). Pre-V1_20_2 versions don't have CONFIGURATION state at all, and that's already gated by supports_configuration_state() / is_after_inclusive(V1_20_5) checks in send_configuration_packets.
2 participants
Fix Velocity + PacketEvents interop issues during configuration phase
Context
We run PicoLimbo as a backend behind a Velocity proxy with PacketEvents 2.12.1. The proxy intentionally holds connecting players in the
CONFIGURATIONstate for an extended period (typically more than 30 seconds) to run a custom registration / authentication dialog flow before transitioning the player to the actual gameplay server.In this setup we hit a cluster of bugs that prevent PicoLimbo from working as a Velocity backend for modern Minecraft clients (1.21.11+ / protocol 26.1.x). This PR fixes all of them.
The root causes are independent but they interact:
minecraft:timelineregistry that PicoLimbo emits.select_known_packshandshake, so it sends every registry to every client.minecraft:corepack version string that doesn't match what 26.1.x clients have locally.keep_alivewhile the client is inCONFIGURATION, so Velocity's backendread-timeout(30 s by default) tears down the connection during the dialog flow.Bug 1 —
IllegalStateExceptionfrom PacketEvents onminecraft:timelineSymptom (Velocity logs):
Reproduces on every connection from a 26.1.x client passing through Velocity + PacketEvents 2.12.x.
Root cause:
PicoLimbo's
json_to_nbtconverter downcasts integers to the smallest-fitting NBT type (e.g.0→Byte,1000→Short,24000→Short) and downcasts floats toDoublewhen precision can't round-trip toFloat. Mojang's vanilla server emits the canonical NBT types:Int(orLongon overflow) for integers,Float(32-bit) for floats, always.The MC 26.1
minecraft:timelineregistry codec on the client (and the corresponding decoder in PacketEvents 2.12.x) expects strictly canonical tags. When aShort(24000)arrives where the codec expectsInt(24000), the decoder throws. Cross-reference: retrooper/packetevents#1482.Fix:
NbtOptions::numeric_wideningflag inpico_nbtand a newjson_to_nbt_with_options(json, options)entry point.numeric_wideningis enabled: integer JSON numbers always becomeValue::Int(orLongon overflow) — never downcast Byte/Short. Floats always becomeValue::Float(32-bit), even at the cost of f64→f32 precision. JSON booleans remainValue::Byte(0/1)(vanilla behavior). Integer arrays becomeIntArray/LongArray, neverByteArray.pico_registriesis the only call site that loads registry JSON; it now passesnumeric_widening(true).V26_1timeline/daydata report.Default behavior of
json_to_nbt(without the flag) is unchanged — any other use of the converter is unaffected.Bug 2 — Wrong
minecraft:corepack version inClientBoundKnownPacksPacketSymptom:
A 26.1.x vanilla client never accepts the pack PicoLimbo offers. The client's
select_known_packsresponse is an empty list.Root cause:
ClientBoundKnownPacksPacket::new(protocol_version.humanize())—humanize()returns the protocol's minor name, which forV26_1is"26.1". The client compares the pack version string strictly: a client running26.1.2has a locally-storedminecraft:core@26.1.2pack, notminecraft:core@26.1, so the offer is rejected and the client tells us it has no packs in common.Fix:
New
known_pack_version(version)helper that maps protocol versions whose Mojang release tag differs fromhumanize()to the right literal. Today onlyV26_1 → "26.1.2"; future protocols whose release tag matcheshumanize()fall through and don't need an arm.Note: this only optimizes for 26.1.2 clients. Clients on 26.1.0 / 26.1.1 (same protocol number, different point release) will reject the offer and the server falls back to sending full registry data — which is correct, and the fallback path is now safe thanks to Bug 1's fix.
Bug 3 —
select_known_packshandshake not implementedSymptom:
On
LoginAcknowledged, PicoLimbo dumps the entire configuration burst (brand +select_known_packsoffer + tags + every registry +FinishConfiguration) in one batch, regardless of how the client responds. Even when the client accepts the vanilla pack we offer, we still ship every registry — wasted bandwidth, and on Velocity + PacketEvents this is precisely the path that triggers Bug 1.Root cause:
There is no
PacketIndecoder forselect_known_packs, noServerBoundSelectKnownPacksvariant inPacketRegistry, and no handler. The server-bound side of the handshake is dropped on the floor.Fix:
ServerBoundKnownPacksPacket(inminecraft_packets::configuration) derivingPacketIn.ServerBoundSelectKnownPacksvariant inPacketRegistry, registered asstate = "configuration",bound = "serverbound",name = "minecraft:select_known_packs".KnownPackstruct fields madepubso the server-bound handler can compare them.send_configuration_packetsis split into two functions:send_configuration_packets: for protocols>= 1.20.5, sends only brand +select_known_packsand stops. For pre-1.20.5 (no handshake), still emits the full burst as before.send_post_known_packs_configuration_packets: sends tags + registry data (unless skipped) +FinishConfiguration. Called either directly (pre-1.20.5) or from the new handler (post-1.20.5).impl PacketHandler for ServerBoundKnownPacksPacket: when the client's response containsminecraft:core@<expected_version>, registry data is skipped — matching Mojang vanilla / Paper behavior.Pre-1.20.5 protocols are unaffected.
Bug 4 — No
keep_alivein CONFIGURATION state → Velocityread-timeoutSymptom (Velocity logs):
…fires after 30 seconds whenever the proxy holds the player in CONFIGURATION longer than that — which is exactly what our register/auth dialog does.
Root cause:
PacketRegistryonly declaresClientBoundKeepAliveinstate = "play". The CONFIGURATION variant of the packet doesn't exist on the server side.ControllableInterval) is only armed whensend_play_packetsruns, i.e. at the end ofAcknowledgeConfiguration. While the client is held in CONFIGURATION, the ticker is idle and the backend connection sits silent on the network.send_keep_aliveearly-returns unless the client is inState::Play.Velocity's default backend
read-timeoutis 30 seconds. Any CONFIGURATION hold longer than that → silent backend → connection torn down.Fix:
ConfigurationClientBoundKeepAlivevariant inPacketRegistry, reusing the existingClientBoundKeepAlivePacketstruct (the wire format is identical; only the per-state protocol ID differs and thePacketReportmacro picks the right one per version).send_keep_alivenow dispatches onstate: emitsConfigurationClientBoundKeepAliveinState::Configuration,ClientBoundKeepAliveinState::Play, no-op elsewhere.LoginAcknowledgedhandler (i.e. as soon as the client enters CONFIGURATION), not delayed until PLAY.read-timeoutinstead of a 2× margin.Why these are in one PR
Bug 1 by itself doesn't fix the Velocity backend scenario: even with canonical NBT, the client still gets timed out at 30 s if the proxy holds it in CONFIGURATION (Bug 4). Bugs 2 and 3 don't fix the crash from Bug 1 — they just avoid the code path that triggers it for vanilla clients. The four are entangled enough that they should land together.
Concrete coverage:
Tests
pico_nbt: 32 unit tests, including 14 new ones covering scalar and array widening fornumeric_widening. Default code path is preserved.pico_registries: new integration testregistry_widening.rsloads the realV26_1timeline/day.jsonfrom the data report and asserts canonical Int / Float at multiple paths (period_ticks,time_markers.wake_up_from_sleep,time_markers.day.ticks,show_in_commands,moon_angle.ease.cubic_bezier[0]).pico_limbo: refactored configuration tests into three (initial burst, vanilla-rejected post-handshake, vanilla-accepted post-handshake) and tightened theLoginAcknowledgedtest to assert the keep-alive ticker is armed.cargo test --workspace: all green.cargo clippy --workspace --all-targets -- -D warnings: clean.Compatibility
pico_nbt::json_to_nbtkeeps its previous signature and behavior; the new functionality is opt-in viajson_to_nbt_with_options(json, options).End-to-end verification
Built statically against
x86_64-unknown-linux-musland tested against the live setup: Velocity 3.5.x + PacketEvents 2.12.x + a 1.21.11+ client + a configuration dialog holding the player ~60 seconds.Before this PR:
IllegalStateException: Error while reading registry minecraft:timelineon every connection.[connected player] read timed outat the 30 s mark.After this PR:
Commits
14 commits, grouped by bug:
Bug 1 — NBT canonical types (5 commits):
feat(nbt): add NbtOptions::numeric_widening flagfeat(nbt): respect numeric_widening in json_to_nbtfeat(nbt): re-export json_to_nbt_with_optionsfix(registries): emit canonical Int/Long NBT tags when loading registry JSONtest(registries): assert timeline registry emits canonical Int+Float NBT tagsBug 2 — pack version (1 commit):
fix(login): send correct minecraft:core pack version per protocolBug 3 — KnownPacks handshake (4 commits):
feat(packets): add ServerBoundKnownPacksPacketfeat(packets): make KnownPack fields publicfeat(login): split configuration burst around the KnownPacks handshakestyle(login): backtick KnownPacks in doc comment for clippyBug 4 — keep-alive in CONFIG (4 commits):
feat(packet-registry): add ConfigurationClientBoundKeepAlive variantrefactor(keep-alive): lower interval from 15s to 10sfeat(keep-alive): emit ConfigurationClientBoundKeepAlive in CONFIG statefeat(keep-alive): start ticker when entering CONFIGURATION state