Skip to content

fix: encode msgpack structs as named maps so app info and kv values can gain fields - #1030

Open
kvinwang wants to merge 4 commits into
nextfrom
fix/msgpack-named-encoding
Open

fix: encode msgpack structs as named maps so app info and kv values can gain fields#1030
kvinwang wants to merge 4 commits into
nextfrom
fix/msgpack-named-encoding

Conversation

@kvinwang

@kvinwang kvinwang commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Problem

AppInfo is serialized into the PHALA_RATLS_APP_INFO certificate extension with
rmp_serde::to_vec, which encodes structs as positional MessagePack arrays. Field
names never reach the wire, so the compatibility contract is the field order and count.
Under that contract a struct cannot gain a field: the array length changes and readers
built against the previous definition reject it outright.

c4ea8110d ("feat(compose): support multiple init scripts") appended
init_script_hashes to AppInfo, taking it from 8 fields to 9. #[serde(default)] on
that field makes an old certificate readable by a new binary, but nothing makes a
new certificate readable by an old one:

0.5.11 cert -> current reader : ok
current cert -> 0.5.11 reader : Err("array had incorrect length, expected 8")

The failure is not confined to app info. ra-rpc decodes the extension for every prpc
call that carries a client certificate and propagates the error:

// ra-rpc/src/rocket_helper.rs:519-523 — identical in v0.5.11
let info = request.certificate.as_ref().map(|cert| -> Result<_> {
    let app_id = RocketCertificate(cert).get_app_id()?;
    let app_info = RocketCertificate(cert).get_app_info()?;   // <-- fails the whole call
    Ok((app_id, app_info))
}).transpose()?;

So a CVM built from next registering against a v0.5.11 gateway would have every prpc
call rejected, not just lose app info. dstack-util/src/system_setup.rs:502,521 sets
ext_app_info: true, so that is the normal registration path.

The same encoding is used for gateway KV values (gateway/src/kv/mod.rs), where the 11
value structs replicate between gateways and are read back after restart — the same
freeze applies there, across a mixed-version cluster.

Scope of exposure

Nothing broken has shipped. The extension has carried exactly one layout since it was
introduced:

Version AppInfo in cert Fields
≤ v0.5.5 not embedded (APP_INFO absent from cert.rs)
v0.5.6 … v0.5.11 positional 8
v0.6.0.a1 / a2 (pins dstack f1ba0a22) positional 8
next (unreleased) positional 9

The earlier 14 → 13 field churn at v0.5.0 → v0.5.1 predates embedding and produced no wire
format. So there is a single legacy layout to stay compatible with, and the window to fix
this closes when 0.6.0 ships.

Fix

Encode as a MessagePack map keyed by field name (to_vec_named) at both sites. This
moves the contract from field order to field names, which readers can skip when unknown
and default when absent.

The decode side needs no change and no version negotiation: serde's derived
Deserialize implements both visit_seq and visit_map, so a single reader accepts
either form. That is what makes this a one-way migration rather than a flag day —
already-deployed v0.5.6+ binaries can read the new encoding without being rebuilt.

Verification

All four directions across the v0.5.11 boundary, exercised by the new tests:

reader: 8-field (v0.5.6–v0.5.11) reader: 9-field (this build)
cert: positional, 8 fields (legacy) ok (unchanged) ok
cert: named, 9 fields (this build) ok (was: array had incorrect length) ok

Tests added:

  • ra-tls: legacy_positional_app_info_still_decodes pins a golden 94-byte fixture of the
    v0.5.6–v0.5.11 positional layout and asserts it decodes, with init_script_hashes
    defaulting to None. named_app_info_decodes_against_legacy_field_set decodes this
    build's output into a struct that reproduces the 8-field layout, standing in for an
    unrebuilt peer. app_info_survives_a_certificate_round_trip goes through a real
    CertRequestget_app_info().
  • gateway: equivalent legacy/reduced-reader pair plus
    nested_tagged_enums_and_custom_codecs_survive_both_encodings, which covers
    DnsCredential — the only value type nesting an internally tagged enum
    (#[serde(tag = "type")]) and a serde(with) codec.

Confirmed serde_human_bytes is unaffected: to_vec_named does not flip
is_human_readable, so #[serde(with = "hex_bytes")] fields still emit MessagePack bin
(0xC4) rather than hex strings, and JSON output is unchanged.

cargo test -p ra-tls -p dstack-gateway -p ra-rpc -p dstack-attest -p dstack-util passes;
cargo fmt --all --check clean; clippy clean apart from a pre-existing
manual_repeat_n warning in gateway/src/pp.rs:254.

Cost

AppInfo grows from 86 to 203 bytes inside the certificate extension. Gateway KV values
are small structs with a similar constant factor.

Not in this PR

wavekv's snapshot (node.rs:161) also uses positional MessagePack, but it lives in a
separate repository and already has magic + version with an enforced check, so it can
migrate explicitly by bumping SnapshotFile::VERSION. Its WAL uses bincode, which has no
named mode at all; it is protected instead by a magic/version header, length-prefixed
records, and a CRC32 over the canonical encoding that turns a format drift into a loud
startup failure.

Copilot AI lite review requested due to automatic review settings August 8, 2026 09:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes cross-version compatibility for RA-TLS certificate AppInfo and gateway KV value serialization by switching MessagePack struct encoding from positional arrays to named maps, allowing fields to be added without breaking older readers.

Changes:

  • Encode RA-TLS AppInfo extension using rmp_serde::to_vec_named and update decode error context to correctly reference msgpack.
  • Encode gateway KV values using rmp_serde::encode::to_vec_named to make KV replication/restart data forwards-compatible.
  • Add regression tests in both crates to validate mixed-version decoding across positional (legacy) and named-map (new) encodings.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
dstack/ra-tls/src/traits.rs Fixes the decode error context to accurately describe msgpack decoding.
dstack/ra-tls/src/cert.rs Switches RA-TLS app info extension encoding to named maps and adds cross-version encoding tests.
dstack/gateway/src/kv/mod.rs Switches KV value encoding to named maps and adds legacy/new roundtrip and compatibility tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread dstack/gateway/src/kv/mod.rs Outdated
Comment on lines +1255 to +1259
assert_eq!(
encoded[0] & 0xf0,
0x80,
"values must encode as MessagePack maps, not positional arrays"
);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — addressed in 93f738f.

Same helper added here. I also fixed the mirror-image bug in legacy_positional_records_are_still_readable, which asserted & 0xf0 == 0x90 and would likewise miss array16/array32 (0xdc/0xdd) once a value type exceeds 15 fields.

Comment thread dstack/ra-tls/src/cert.rs Outdated
Comment on lines +852 to +856
assert_eq!(
encoded[0] & 0xf0,
0x80,
"app info must encode as a MessagePack map, not a positional array"
);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — addressed in 93f738f.

Replaced the & 0xf0 mask with a starts_with_msgpack_map helper matching fixmap, map16 and map32 (0x80..=0x8f | 0xde | 0xdf). Verified the boundary: a 15-field struct encodes to 0x8F, a 16-field one to 0xDE — so the old assertion would have started failing exactly when AppInfo grew past 15 fields, which is the case this PR exists to enable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants