[vm] Fix and test zero-copy BCS serialization (supersedes #20179, builds on #18535)#20190
Draft
gregnazario wants to merge 12 commits into
Draft
[vm] Fix and test zero-copy BCS serialization (supersedes #20179, builds on #18535)#20190gregnazario wants to merge 12 commits into
gregnazario wants to merge 12 commits into
Conversation
…izationReadyValue instance.
…iners during value serialization.
The new SerializationReadyValue<MoveTypeLayout, Container> impl passed the Arc<MoveStructLayout> from L::Struct directly where a &MoveStructLayout is required, so move-vm-types failed to compile. Use .as_ref() to match the existing Value serializer. Co-authored-by: Greg Nazario <greg@gnazar.io>
IndexedRef::read_ref enforces check_valid_for_indexed_ref, which rejects a stale/malformed indexed reference that points at a nested container or another reference. The zero-copy serialization path dropped this guard; restore it so serialize_ref has the same safety behavior as the copy-based path. Co-authored-by: Greg Nazario <greg@gnazar.io>
Verify serialize_ref / serialized_size_ref produce byte-identical output and identical sizes to the copy-based serialize / serialized_size across scalars, specialized and nested vectors, structs, enum variants, signers (legacy and non-legacy), delayed fields, indexed field/element refs, and depth-limit enforcement. Co-authored-by: Greg Nazario <greg@gnazar.io>
IndexedRef::read_ref copies the referenced element at depth + 1, so the copy-based path enforces the nesting-depth limit one level deeper than the reference. Serialize the indexed element at self.depth + 1 so the zero-copy path rejects at the same depth boundary (relevant for indexed refs to closures, whose captured arguments add nesting). Co-authored-by: Greg Nazario <greg@gnazar.io>
Add a test asserting serialize_ref of an indexed reference to a closure enforces the depth limit exactly one level deeper than serializing the owned closure, matching IndexedRef::read_ref's copy_value(depth + 1). Co-authored-by: Greg Nazario <greg@gnazar.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Recreates the work from #20179 (which was accidentally closed after its branch was overwritten by unrelated commits). This carries #18535's zero-copy BCS serialization commits (
serialize_ref/serialized_size_ref, resolving #14175) plus the fixups below. Because #18535 comes from a fork, its commits cannot be used as a PR base in this repo, so they are included here and this PR targetsmain.Fixups on top of #18535:
Fix compile error (blocker). The new
SerializationReadyValue<MoveTypeLayout, Container>impl passed theArc<MoveStructLayout>fromL::Structdirectly where a&MoveStructLayoutis expected, somove-vm-typesdid not compile. Added.as_ref()to match the existingValueserializer. (Not caught on [vm] Avoid deep copies in BCS serialization by using serialize_ref (#14175) #18535 because CI there is gated behind a failingpermission-check, so all Rust build/test jobs were skipped.)Restore the indexed-ref invariant guard.
IndexedRef::read_refenforcescheck_valid_for_indexed_ref, which rejects a stale/malformed indexed reference pointing at a nested container or another reference. The zero-copy path dropped this guard; it is restored soserialize_refhas the same safety behavior as the copy-based path.Match
read_refdepth accounting for indexed refs.IndexedRef::read_refcopies the referenced element atdepth + 1, so the copy-based path enforces the nesting-depth limit one level deeper than the reference. The indexed-ref serialization arm now visits the element atself.depth + 1, sobcs::to_bytes/serialized_sizereject at the same depth boundary as before (relevant for indexed refs to closures, whose captured arguments add nesting).Add differential and depth-parity tests. Since the correctness requirement is that the reference path produces byte-identical output to the copy-based path (BCS output is consensus-sensitive), added tests asserting parity of bytes, size, and depth-limit behavior.
Disposition of automated review comments (from #20179)
serialize_ref_indexed_closure_enforces_depth_one_level_deeper.read_referrors now map toNone→NFE_BCS_SERIALIZATION_FAILURE): intentional and consistent with the existing ownedserializepath in [vm] Avoid deep copies in BCS serialization by using serialize_ref (#14175) #18535, which already maps every serializer error (includingVM_MAX_VALUE_DEPTH_REACHEDand invariant/tag failures) toNone. Routing reference-structural failures through the same fail-closed abort keeps the reference and owned paths consistent; these are invariant violations that should not occur under a correct verifier. Left as-is by design.How Has This Been Tested?
New tests in
third_party/move/move-vm/types/src/values/serialization_tests.rsassertserialize_ref/serialized_size_refmatchserialize/serialized_sizeacross scalars, specialized/nested/general vectors, structs, nested structs, enum variants, signers (master/permissioned, legacy/non-legacy), delayed fields, indexed field/element refs, and depth-limit enforcement (incl. the indexed-ref-to-closure+1parity).cargo test -p move-vm-types --libpasses.cargo +nightly fmtandcargo clippy -p move-vm-types --lib --testsare clean for the changed code.aptos-move-stdlib(the natives caller) compiles.Type of Change
Which Components or Systems Does This Change Impact?
Checklist