Skip to content

fix(encode)!: canonically sort major type 7 map keys - #186

Merged
rvagg merged 2 commits into
rvagg:masterfrom
spokodev:fix/map-sort-simple-keys
Jul 29, 2026
Merged

fix(encode)!: canonically sort major type 7 map keys#186
rvagg merged 2 commits into
rvagg:masterfrom
spokodev:fix/map-sort-simple-keys

Conversation

@spokodev

@spokodev spokodev commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

The default map key sorter compared key types with keyToken1.type !== keyToken2.type and fell back to Type.compare, which only looks at the major type. false, true, null, undefined and floats all share major type 7, so any pair of them compared as equal and kept their insertion order. A map keyed by these values encoded to different bytes depending on insertion order, and pairs such as true/false came out in the wrong order (true 0xf5 before false 0xf4).

This routes keys that share a major type through the type encoder's compareTokens and gives the major 7 encoder a comparator that orders the simple values by their encoded byte and floats by value, so these keys sort deterministically. Adds tests for simple value and float map keys.

@rvagg

rvagg commented Jul 22, 2026

Copy link
Copy Markdown
Owner

Thanks for picking this up (tbh I haven't cared much about non-string keys), and it's a nice start, but this still isn't RFC 7049 (3.9) ordering for float keys since we're aiming for that. The rule is: encoded length first, then bytewise lexical order; numeric value isn't equivalent. What we have here currently is:

  • [-1.5, 1.5] sorts negative first, but f93e00 (+1.5) must precede f9be00 (-1.5)
  • [1.1, Infinity] sorts 1.1 first, but three-byte f97c00 must precede nine-byte fb3ff199999999999a
  • NaN compares equal to every float because both numeric comparisons are false, so insertion order still changes the output

So we need to make the major-7 comparison equivalent to encoded length followed by encoded bytes, honouring float64, and cover negative floats, mixed widths, and NaN in both insertion orders.

Also we should ! the commit message here when I land it and signal a "breaking" change—very low liklihood of anyone actually getting bitten by this but important to be consistent.

Also fwiw RFC's 8949 restated rules end up having major-7 sorted in the same way, it's just defined differently:

  • RFC 7049 "canonical" order: encoded length first, then bytewise lexical order.
  • RFC 8949 "core deterministic" order: bytewise lexical order only.
  • RFC 8949 4.2.3: separately preserves RFC 7049 compatibility as “length-first core deterministic encoding”

For major-7 we end up in the same place anyway, so nice and consistent and easy in this case.

@spokodev
spokodev force-pushed the fix/map-sort-simple-keys branch from ff012a9 to c610e95 Compare July 23, 2026 11:48
@spokodev

Copy link
Copy Markdown
Contributor Author

Reworked to order major 7 keys by their canonical encoding rather than by value.

compareTokens now encodes each key to the bytes it would actually be written as (the four simple values to one byte, floats to 3/5/9) and compares length-first then bytewise, i.e. RFC 7049 3.9 for major 7. That covers the three cases you flagged: +1.5 (f93e00) before -1.5 (f9be00), 3-byte Infinity before a 9-byte float64, and NaN deterministic instead of insertion-order dependent.

It honours float64: mapSorter and compareTokens now receive the encode options, so a value that encodes 3-byte by default but 9-byte under float64 sorts to match the bytes actually written. Options ride along as a trailing argument, so custom (e1, e2) sorters are untouched. Left the commit prefix as-is for you to mark breaking at land time.

Tests cover negative floats, mixed widths, NaN and the float64 case in both insertion orders. Full suite green.

Comment thread lib/7float.js
@rvagg
rvagg force-pushed the fix/map-sort-simple-keys branch from c610e95 to 497e7d9 Compare July 29, 2026 08:17
@rvagg

rvagg commented Jul 29, 2026

Copy link
Copy Markdown
Owner

force pushed a rebased branch plus a caching fix over the top (497e7d9); for plain floats I get a speed up of 1.5x for a map with 10 entries, nearly 4x for a map with 10k entries (!). for float64 it's more modest, from 1.2x to 2x.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Ready to approve

The change addresses a concrete determinism bug, implements canonical bytewise ordering for major-7 keys, and includes targeted regression tests covering key edge cases.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR fixes non-deterministic CBOR map key ordering for “major type 7” keys (simple values and floats), ensuring canonical, insertion-order-independent encoding.

Changes:

  • Updates the default map sorter to route same-major comparisons through the major encoder’s compareTokens (instead of falling back to Type.compare).
  • Implements a major-7 comparator that orders keys by canonical encoded bytes (length-first, then lexicographic), honoring options.float64.
  • Adds regression tests covering simple-value and float map key ordering (including NaN, mixed widths, and float64 mode).
File summaries
File Description
lib/encode.js Fixes map sorting to compare by major type first, then defer to per-major compareTokens, and passes options into comparators.
lib/7float.js Implements canonical major-7 key comparison via encoded-byte generation + caching, honoring float64.
interface.ts Extends TokenTypeEncoder.compareTokens and MapSorter signatures to optionally accept EncodeOptions.
test/test-5map.js Adds tests asserting deterministic ordering for simple values and floats (including NaN and float64).
types/interface.d.ts Updates public type declarations to match the new comparator/sorter signatures.
types/interface.d.ts.map Regenerates sourcemap for updated interface declarations.
types/lib/7float.d.ts Updates float encoder type declarations (including compareTokens signature) and adds a helper token type for cached bytes.
types/lib/7float.d.ts.map Regenerates sourcemap for updated float declarations.
types/lib/encode.d.ts.map Regenerates sourcemap reflecting updated encode-side typing/structure.
Review details
  • Files reviewed: 4/9 changed files
  • Comments generated: 1
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Comment thread test/test-5map.js
spokodev and others added 2 commits July 29, 2026 19:05
Sort simple values and floats by their RFC 7049 canonical encodings,
honouring float64, so map encoding no longer depends on insertion order.

This changes encoded output for maps containing simple-value or
floating-point keys that previously retained insertion order.

BREAKING CHANGE:

Map encodings containing simple-value or floating-point keys may change byte
order. Hashes, signatures, and identifiers derived from affected encodings
will therefore change.
Cache encoded key bytes while sorting to avoid repeated conversion.
@rvagg
rvagg force-pushed the fix/map-sort-simple-keys branch from 497e7d9 to be82627 Compare July 29, 2026 09:09
@rvagg rvagg changed the title fix(encode): deterministically sort simple value and float map keys fix(encode)!: canonically sort major type 7 map keys Jul 29, 2026
@rvagg
rvagg merged commit e440ae2 into rvagg:master Jul 29, 2026
7 checks passed
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants