Skip to content

Charge value-graph walks so BCS/resource deserialize cannot amplify for free - #428

Open
sausagee wants to merge 3 commits into
m1from
cursor/meter-value-nodes-deserialize-e989
Open

Charge value-graph walks so BCS/resource deserialize cannot amplify for free#428
sausagee wants to merge 3 commits into
m1from
cursor/meter-value-nodes-deserialize-e989

Conversation

@sausagee

@sausagee sausagee commented Sep 6, 2026

Copy link
Copy Markdown

Description

Clean-room rewrite (new author, new names, new control flow). This is not a cherry-pick, patch port, or mechanical transcription of any aptos-labs change. Intent only: compact BCS / resource bytes can expand into a huge Move value-node graph while paying only for blob length.

Invariant: materializing or walking a Move value must not be free relative to that value’s abstract node size. A compact encoding that expands into many nodes must cost more once the timed flag is on. Arithmetic saturates so a huge graph over-charges rather than wrapping cheap.

MeterValueNodesOnDeserialize gates both deserialize and serialize-side walks (serialize had the same hole: output-byte gas alone underprices a wide graph with a tiny encoding).

Pricing is 3× m1 cmp::compare (base = 1101, per_abs_val_unit = 42), implemented as value_graph_walk_cost — not on-chain params yet. StandardGasMeter::new and make_prod_gas_meter signatures are unchanged; the VM opts in via enable_value_graph_load_billing.

Charge sites (cache miss only where a session/table cache exists):

  • interpreter resource load (charge_load_resource)
  • util::from_bytes
  • table borrow / load
  • object exists_at
  • BCS to_bytes / serialized_size

TESTING activation is 1970-01-01 03:00 UTC so one new_epoch() (2h) leaves the flag off. Movement dates precede the 2025-08-11 catch-all so the flag is not live immediately.

How Has This Been Tested?

All of the following passed locally:

  • cargo test -p aptos-gas-schedule --lib — includes value_graph (base cost, linear growth, saturate at u64::MAX)
  • cargo test -p aptos-types --lib -- value_graph_flag — genesis / 2h off, 4h on; Movement 2026-09-06 still off
  • cargo test -p aptos-gas-meter --lib — walk unbilled when flag off; billed size matches formula; missing resource = 0 execution
  • cargo test -p aptos-framework -p aptos-native-interface -p aptos-table-natives -p aptos-move-stdlib -p aptos-memory-usage-tracker -p aptos-gas-profiling --lib
  • cargo test -p e2e-move-tests --lib -- value_graph — 7 tests: bushy Cell (W1–W7 + 4-field Leaf). unpack_loop / serialize_loop / peek / peek_shelf. Enable via two new_epoch(). Assert gas_on > gas_off. Large values still succeed (charged, not rejected).
  • Clippy (-D warnings with repo allow-list) on the touched gas / native / types crates.

Key Areas to Review

  • Timed-feature match order: Movement arms must precede (_, MOVEMAINNET | MOVETESTNET).
  • Table / object charges fire only on cache miss (loaded.is_some() / num_bytes.is_some()).
  • Heap + graph size reads on exists_at keep layout params and session cache borrows disjoint.
  • No unsafe. Saturating InternalGas add/mul. Charge before/with materialization where possible (to_bytes before serialize; from_bytes after deserialize because abstract size is only known then).

Type of Change

  • New feature
  • Bug fix
  • Tests

Which Components or Systems Does This Change Impact?

  • Move/Aptos Virtual Machine
  • Aptos Framework

Security (RUST_SECURE_CODING.md / SECURITY.md)

Reviewed new code against the repo guidelines:

  • No unsafe.
  • Integer overflow: GasQuantity add/mul already saturate; walk cost uses those ops so a huge graph cannot wrap to a cheap bill.
  • No new third-party crates (only workspace aptos-gas-algebra on table-natives).
  • Error paths use Result / ?; no unwrap on charge paths.
  • Deterministic: BTree-backed table internals unchanged.
  • Timed flag default-off in TESTING until two epochs, so existing gas-sensitive tests keep historical costs.

Aikido CLI is not available in this environment; the review above is the scan of the new code. Please run the project Aikido workflow on the PR if that is part of CI.

Checklist

  • I have read and followed the contributing guidelines
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I tested both happy and unhappy path of the functionality
Open in Web Open in Cursor 

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

cursoragent and others added 3 commits September 6, 2026 05:18
BCS and resource loads could materialize a large Move value graph while
only paying for blob bytes. Bill a compare-class walk (3x cmp::compare)
behind MeterValueNodesOnDeserialize so a compact encoding cannot amplify
for free. Cache hits stay unbilled; arithmetic saturates.

Co-authored-by: Young Yang Liauw <sausagee@users.noreply.github.com>
The single nested struct literal was deep enough to overflow the
Move compiler stack while publishing the e2e package.

Co-authored-by: Young Yang Liauw <sausagee@users.noreply.github.com>
value is already a &Value after the deserialize-side walk charge.

Co-authored-by: Young Yang Liauw <sausagee@users.noreply.github.com>
@sausagee
sausagee marked this pull request as ready for review September 6, 2026 05:29
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