xdr: add views API document for team review#5928
xdr: add views API document for team review#5928tamirms wants to merge 8 commits intostellar:mainfrom
Conversation
Preliminary document describing the XDR zero-copy views API: typed, read-only windows into raw XDR bytes that parse lazily on access. Covers usage patterns, navigation (structs, unions, arrays, optionals), leaf types, validation, error handling, performance benchmarks across 1,000 pubnet ledgers, and security properties. This document is for early feedback on the API design before the full implementation lands. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds an initial design/usage document for a proposed “XDR zero-copy views” API, intended to enable lazy navigation of XDR-encoded ledger data without full decoding.
Changes:
- Introduces
xdr/views_api.mddescribing the views concept, navigation patterns, validation, errors, performance benchmarks, and security properties. - Documents proposed API surface (view types, accessors,
Raw(),Copy(),Valid(), iterators, and error kinds).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Unify decode metrics consistently - Fix depth limit description inconsistency - Clarify wire count cap precision Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
leighmcculloch
left a comment
There was a problem hiding this comment.
This looks really good to me. I have a couple thoughts inline.
| view := LedgerCloseMetaView(data) | ||
| ``` | ||
|
|
||
| Every XDR struct, union, enum, and typedef has a corresponding view type. Each field of a struct becomes a method that returns a sub-view — a `[]byte` slice starting at that field's byte offset. Sub-views are "fat slices": they extend to the end of the parent buffer, not just the field's own extent. This avoids computing the field's size during navigation. The exact bytes for a view can be extracted later with `Raw()`. |
There was a problem hiding this comment.
"fat slices": they extend to the end of the parent buffer
Is this optimisation a significant win? Reason being that if an application does mutate one view it could write into the backing array range of other views. It would be safer when slicing to also cap the new slice at its length so that corruption across views is not possible.
There was a problem hiding this comment.
I think this concern goes away once I correct the design to make it clear that mutation of the byte slice is not safe.
There was a problem hiding this comment.
this is where i really miss Scala's val if these were immutable byte arrays by type, the compilre would enforce the "dont mutate" contract for us and this woudl be a non-issue.
Go doesnt give us that, so the immutability here is a documentation contract rather than a type-level gaurantee(right?)
which brings me back to @leighmcculloch's point here..
I want to dissect this a bit further (for my own edification, more than anything else)
Since views are byte[] named arrays, every byte[] operation works on them - including append().
With fat slices, a sub-view's capacity extends to the end of the 1.5MB buffer.
Meaning... if someone accidentally does this:
subView = append(subView, extraBytes...)
the, Go sees plenty of capacity, reueses the OG backing array, and siltenly overwrites everything after that field
no allocation, no error, no panic - just silent corruption.
Now, with capped slices (i.e the 3-tuple variety - buf[start:end:end], the same append() would exceed the capacity, forcing a new alloc, and thus leave the OF buffer untouched (as in, mutation safe)
I get that the doc says "all mutation is unsafe" and that capping requiers computing field sizes (defeating lazy navigation for variable-size types).
this is probably fine in practice — nobody should be appending to a view. but since Go cant enforce that at the type level, a couple things would help:
1/
a brief note on WHY fat slices were chosen over capped slices. It's a slightly non-obvious design choice, so perhaps it is worth documenting.
Something like:
Why fat slices? The alternative — capped slices via Go's three-index syntax buf[start:end:end] — would prevent accidental writes from corrupting sibling fields.
However, capping requires knowing the field's byte size at navigation time.
For fixed-size types (int32, Hash, etc) the size is a compile-time constant so capping is free.
For variable-size types however(strings, variable-length arrays), computing the size requires scanning the fields contents — same work as full decoding, defeating lazy navigation.
Fat slices avoid this cost by deferring size computation toRaw(), which is only called when the caller explicitly needs exact bytes.
2/
maybe an explicit callout that append() on views is particualrly dangerous — its the most natural way a Go dev would accidentally violate the immutability contract.
unlike view[i] = x (which at least looks like mutation), append dosnt feel like its mutating someone elses data 🤷
Dont get me wrong. I am not contesting the design.
the design is sound, just thinking about what would make the "here be dragons" section more concrete for go devs who wont have a Scala compiler slapping thier hands.
Sync doc from protocol-next branch. Key updates: - ValidateFull() replaces Valid() with options - Single fixed depth limit (1500) replacing two independent limits - Must methods (MustField(), MustValue()) replacing Must() wrapper - Try/TryVoid for panic recovery with Must methods - MustIter() returning iter.Seq[T] - Updated benchmarks - Simplified security section Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Updated the doc to address review feedback:
|
| | Extract all tx hashes | 5.3ms | 310µs | **17x** | | ||
| | Extract all events | 5.4ms | 383µs | **14x** | | ||
| | Extract all transactions | 7.5ms | 657µs | **11x** | | ||
| | ValidateFull | 5.7ms | 489µs | **12x** | |
There was a problem hiding this comment.
nit:
For the untrusted-input case where callers should call ValidateFull() first: it would be useful to note the combined cost. E.g., ValidateFull (489us) + extract all transactions (657us) = ~1,146us total, still 6.5x faster than full decode (7.5ms).
This helps readers evaluate whether the "validate first, then navigate" pattern is still a significant win vs. just doing a full decode.
| Views alias the original buffer. If you need an independent copy that outlives the original: | ||
|
|
||
| ```go | ||
| copied, err := view.Copy() // new allocation, safe to use after original is freed |
There was a problem hiding this comment.
this cud be just me, so take it with a pinch of salt.
after reading this doc a few times, i find that Raw() gets a lot more mention than Copy() the impresson i come away with is that Raw() is the defualt choice and Copy() is a rare edge case, when, in truth, it depends on your uscase.
might just be the way the sections are sized — not sure if thats intentional
Summary
This is just the API document for early feedback — the full implementation will follow in a separate PR.
🤖 Generated with Claude Code