perf: encode ed25519 signatures as CBOR byte strings, not 64-element arrays - #576
perf: encode ed25519 signatures as CBOR byte strings, not 64-element arrays#576sanity wants to merge 3 commits into
Conversation
## Problem `ed25519_dalek::Signature`'s derived `Serialize` calls `serialize_tuple(64)`, which ciborium maps to `serialize_seq` — so every signature in River's room state goes on the wire as a CBOR array of 64 integers. Any byte >= 0x18 costs two bytes in that encoding, so a 64-byte signature occupies ~122 bytes instead of the 66 a CBOR byte string needs. River already knew this trap for its own `Vec<u8>` fields (`content.rs` documents it; #443 fixed `ActionContentV1::payload`). What was missed is that it also applies to imported types whose `Serialize` impl River does not control, and it is invisible at the call site because River's own `SignatureBytes` newtype already uses `serialize_bytes`. Ten room-state fields carry a signature. On a 200-member members + member_info state the overhead measures 22,870 B, 24.8% of the encoded size. ## Approach One `serde(with = ...)` helper, `river_core::util::sig_serde`: * `serialize` writes a CBOR byte string; * `deserialize` accepts BOTH a byte string and the legacy 64-element integer array, because existing rooms hold the array form and contract migration re-PUTs that state — and because `?invitation=…` links are base58(CBOR) of a struct containing an `AuthorizedMember`, so outstanding links must keep working. Applied to all ten room-state signature fields. `WebContainerMetadata:: signature` is deliberately excluded and the reason is documented at the field: its reader is the separately deployed web-container contract, which is not re-keyed by the room-contract migration. Every field was checked against the signed representation before being touched: `sign_struct`/`verify_struct` serialize the INNER struct only, so an `AuthorizedX { x, signature }` wrapper's signature is outside the signed bytes. ## Testing New `common/tests/signature_encoding_test.rs` (13 tests): golden vectors produced with Python `cryptography` and hand-derived CBOR headers, per-type round-trip, legacy acceptance via shadow structs that use the plain derive, signature verification after legacy decode, canonicalization (old bytes in, new bytes out), rejection of malformed inputs, and an in-test size baseline that rebuilds the old encoding from the same records. Two frozen fixtures were regenerated and their pre-change values preserved as permanent legacy-decode fixtures: `common/tests/direct_messages_wire_format_legacy_sig_array.hex` and `INVITATION_LEGACY_SIG_ARRAY_FIXTURE` (UI + CLI). Refs #575
Armored identity tokens embed an `AuthorizedMember` and an optional `AuthorizedMemberInfo`, and users save them to disk and paste them into other devices — so they are a third long-lived artifact (alongside `?invitation=…` links and DM-carried invites) that carries the pre-#575 array-encoded signature and must stay importable. The fixture is built through shadow structs using the plain `Signature` derive, and the test asserts it is genuinely distinct from the current encoding before asserting it imports, so it cannot pass vacuously.
Clippy's field_reassign_with_default fired on the new signature-encoding test. Clippy is disabled repo-wide (#573), but code this PR adds should not add to that backlog.
ClippyClippy is disabled repo-wide (#573), so it was run manually as the brief asked: 84 warnings, none in any file this PR touches. The file-scoped check: returns nothing. The 84 that remain are the pre-existing #573 backlog and were left alone. One warning WAS introduced by this PR and is fixed in 0d694a1:
Note for anyone reproducing locally: [AI-assisted - Claude] |
Important
Draft — blocked behind #572, and must NOT merge in its current shape.
This PR carries the source change and tests only. The contract re-key
(rebuilt
room_contract.wasm+legacy_room_contracts.tomlentry +migration.rscount/fingerprint pin + riverctl bump) lands as a latercommit on this same PR, after rebasing onto post-#572 main, per the repo's
five-precedent single-commit pattern. See Sequencing — merging
the source without the bundle would leave the deployed contract unable to
parse what the UI and CLI write.
Problem
ed25519_dalek::Signature's derivedSerializecallsserialize_tuple(64),ciborium maps a tuple to
serialize_seq, and every byte >=0x18then coststwo bytes on the wire. A 64-byte signature occupies ~122 CBOR bytes where a
byte string needs 66.
River already documents this trap for its own
Vec<u8>fields(
content.rs:80-92; #443 fixedActionContentV1::payload). What was missed isthat it also applies to imported types whose
Serializeimpl we do notcontrol, and it is invisible at the call site because River's own
SignatureBytesnewtype already usesserialize_bytes— so the naturalassumption is that all 64-byte signatures cost 66.
Ten room-state fields carry a signature. Measured on a 200-member
MembersV1+MemberInfoV1state built in-test with real keys:The direct-message wire-format fixture shrank from 501 B to 390 B (2
signatures, 111 B, 55.5 B each).
Correction to the issue's figure. #575 quotes a flat 55 B per signature.
The saving is exactly the number of bytes in that signature that are >=
0x18, so it is signature-dependent: ~58 B on average over uniformlydistributed signature bytes, 55 B for the issue's single sample, 61 B for the
golden vector in these tests. The tests assert the exact per-signature
population rather than a constant, so the number cannot rot. The 23.9%-of-state
headline in the issue reproduces (24.8% on the members+member_info subset).
Approach
One
serde(with = ...)helper,river_core::util::sig_serde(
common/src/util.rs):serializewrites a CBOR byte string.deserializeaccepts both a byte string and the legacy 64-elementinteger array, via
deserialize_any. This is required, not cosmetic:existing rooms hold the array form and contract migration re-PUTs that state,
and three long-lived user-facing artifacts embed an
AuthorizedMember—?invitation=…links, DM-carried invites, and armored identity-exporttokens. Without the legacy arm all of them would go dead.
Shape and documentation follow
content.rs'spayload_bytes(#443), includingits notes on
deserialize_anyvsHeader::Tagand on keepingcore::fmtoutof the contract WASM. The unbounded-preallocation hazard that helper guards
against does not arise here — the destination is a fixed
[u8; 64]and theloop refuses a 65th element — but that is stated at the code rather than left
implicit.
Safety verification — per field, not assumed
sign_struct/verify_struct(common/src/util.rs:6-20) ciborium-serializethe inner struct only, so in an
AuthorizedX { x, signature }wrapper thesignature sits outside the signed bytes. Every field was checked individually
against its own verify path before being touched:
configuration.rs:18signatureAuthorizedConfigurationV1 { configuration, signature }verify_signaturere-serializesself.configurationonly (configuration.rs:157-161)member.rs:642signatureAuthorizedMember { member, signature }verify_struct(&self.member, …)(member.rs:665)member_info.rs:354signatureAuthorizedMemberInfo { member_info, signature }verify_struct(&self.member_info, …)(member_info.rs:388)message.rs:1000signatureAuthorizedMessageV1 { message, signature }verify_struct(&self.message, …)(message.rs:1042)ban.rs:446signatureAuthorizedUserBan { ban, banned_by, signature }verify_struct(&self.ban, …)(ban.rs:491) —banned_byis outside too, and untouchedupgrade.rs:20signatureAuthorizedUpgradeV1 { upgrade, signature }verify_struct(&self.upgrade, …)(upgrade.rs:108)secret.rs:286owner_signatureAuthorizedSecretVersionRecord { record, owner_signature }verify_struct(&self.record, …)(secret.rs:308)secret.rs:331owner_signatureAuthorizedEncryptedSecretForMember { secret, owner_signature }verify_struct(&self.secret, …)(secret.rs:350)direct_messages.rs:270sender_signatureAuthorizedDirectMessage { message, sender_signature }build_direct_message_signed_bytes) — not CBOR at alldirect_messages.rs:305recipient_signatureAuthorizedRecipientPurges { recipient_id, state, recipient_signature }build_recipient_purges_signed_bytesweb_container.rs:26signatureWebContainerMetadata { version, signature }Two further checks behind that table:
eight signed inner structs (
Configuration,Member,MemberInfo,MessageV1,UserBan,UpgradeV1,SecretVersionRecordV1,EncryptedSecretForMemberV1) were read field by field; none contains aSignature. So nosign_structcall site has its input re-encoded.message.rs:577hazard class is untouched.RoomMessageBody'sdata/ciphertextlive insideMessageV1, whichverify_structre-serializes. They are not changed, and the helper's doc comment names them
as the boundary not to cross.
Excluded:
WebContainerMetadata::signatureNot excluded for the signed-representation reason — that signature is outside
the signed bytes like the rest. It is excluded because its reader is a
separately deployed contract:
contracts/web-container-contractdeserializes this metadata inside the already-published web-container WASM,
which lives at its own fixed address and is not re-keyed by the room-contract
migration. Emitting the byte-string form from the publish tool would produce
metadata the live contract cannot parse, and every River UI publish would be
rejected. Moving it requires deploying an accept-both contract first and only
then switching the writer. The prize is ~58 bytes on one signature per UI
publish. The reason is recorded at the field so the next person doesn't
"finish the job".
Not touched: the
member_infosummarymember_info.rs'stype Summary = BTreeMap<MemberId, (u32, Signature)>usesthe bare
Signaturetype, whoseSerializeimpl is untouched by this PR. Thatis deliberate: #572 is replacing that tuple with a digest, and shrinking it here
would collide.
Stability properties worth stating
MessageId,BanId,PurgeTokenandAuthorizedConfigurationV1::id()all hashsignature.to_bytes()— the raw 64 bytes, never the CBOR encoding. Dedup,retention ordering and DM purge tombstones do not shift.
serde_jsonwritesserialize_bytesasan array of numbers, so
riverctl … --jsonandcli/src/storage.rs'son-disk format are unchanged, and old JSON storage files round-trip through
the legacy
visit_seqarm. Two pre-existing tests(
json_round_trip_of_full_chat_room_state_preserves_dms,storage::tests::invitation_secrets_round_trip_via_disk) turn out to guardthis and fail when the legacy arm is removed.
update_statedeserializes the incoming state/delta intoChatRoomStateV1and re-serializes with
into_writer(contracts/room-contract/src/lib.rs:180)— there is no path that stores received bytes verbatim. Verified by reading
the contract, and pinned in tests: for every changed type, decoding the legacy
form and re-encoding produces the new form byte-for-byte. This is the
freenet-core #4857 byte-compare divergence class, so it is checked rather
than assumed.
Consequences that are real but accepted
DM-carried invites and identity tokens minted after this change use the byte
string; an older River UI or
riverctlcannot decode them. This is the samecut-over the contract re-key already forces, and it is why the riverctl bump
belongs in the migration bundle.
Invitation::to_encoded_string(), so it changes with the encoding. A user whoaccepted a link on an old build and clicks the same link after upgrading may
see the modal once more. Within a version the fingerprint is consistent
(both URL-time and dismiss-time go through the canonical re-encode).
Testing
cargo test --workspacegreen. Newcommon/tests/signature_encoding_test.rs(13 tests) plus a legacy-import test in
identity.rsand one indirect_messages_test.rs, and legacy-invite tests in both the UI and the CLI.with Python's
cryptography(Ed25519 is deterministic per RFC 8032) and bothCBOR framings were hand-derived from RFC 8949 — 66 B for the byte string,
127 B for the array (this signature has 61 bytes >=
0x18).use the plain
Signaturederive, plus two frozen historical artifacts whosepre-change values are preserved verbatim and must never be regenerated:
common/tests/direct_messages_wire_format_legacy_sig_array.hexandINVITATION_LEGACY_SIG_ARRAY_FIXTURE(kept in step in the UI and the CLI).every_case_has_a_genuinely_distinct_legacy_encodingfails if any "legacy" fixture is byte-equal to its new form, and asserts the
size delta equals the count of signature bytes >=
0x18. Without it a shadowstruct drifting into the new encoding would make the whole file pass while
testing nothing.
is not the same as the signature still verifying.
above 255, and a lying
0x9B FF..FFlength header (error, not panic).member_info_summary_stays_small_per_entrydiscipline from perf(room-state): carry a signature digest in the member_info summary, not the signature #572.Mutation checks — each toggled and run, not reasoned about
visit_seqlegacy armsignature_encoding_test, 3 indirect_messages_test(incl. 2 pre-existing JSON tests); CLI + UI legacy-invite tests fail;storage::tests::invitation_secrets_round_trip_via_diskfailsserializeto the derivesignature_encoding_test, 2 indirect_messages_testvisit_seq's length checksshort_legacy_array_is_rejected,long_legacy_array_is_rejectedfail> u8::MAXelement checklegacy_array_element_above_255_is_rejectedfails#[serde(with)]fromAuthorizedMemberevery_case_has_a_genuinely_distinct_legacy_encoding,state_size_saving_matches_the_signature_populationfailgolden_legacy_form_is_the_hand_derived_int_array,every_case_has_a_genuinely_distinct_legacy_encodingfailcargo fmtclean. Clippy is disabled repo-wide (#573);cargo clippy --workspace -- -D warningswas run anyway and its result is reported in acomment below — no pre-existing lints were fixed.
Sequencing
This change re-keys the room contract, and its migration must not share a
re-key with #572's — two independent encoding changes in one migration is an
unscopeable rollback. #572 merges and publishes first.
So this PR deliberately does not rebuild or commit
room_contract.wasm,does not add a
legacy_room_contracts.tomlentry, and does not bump riverctl.Both migration CI jobs stay green in that state (
check-cli-wasmcompares thetwo committed WASM blobs against each other and only demands a riverctl bump
when a WASM file changes;
check-room-contract-migrationonly fires on acommitted WASM change).
The consequence, and the reason for the banner at the top: while this PR sits
source-only, the committed WASM no longer matches the source. Merging it
alone would put a UI and CLI that write byte-string signatures in front of a
deployed contract that can only parse arrays. The migration bundle must land on
this branch, after a rebase onto post-#572 main, before merge.
Refs #575
[AI-assisted - Claude]