Skip to content

[vm] Fix script hot-state promotion determinism#20182

Draft
wqfish wants to merge 2 commits into
mainfrom
pr20182
Draft

[vm] Fix script hot-state promotion determinism#20182
wqfish wants to merge 2 commits into
mainfrom
pr20182

Conversation

@wqfish

@wqfish wqfish commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Hot-state promotions (to_make_hot in the block epilogue) are part of
consensus, so a transaction's recorded read set must be a pure function of the
transaction and the pre-state.

validate_and_execute_script recorded a script's declared module dependencies
only after load_script and the post-load unstable-bytecode / event-emission
checks. A script that is kept-and-charged while failing after it loads — out of
gas partway through dependency charging, or rejected by those checks (both kept
as MiscellaneousError) — skipped that recording, leaving the dependencies to
load_script's cache-gated fetches. On a warm verified-script cache those
fetches only charge the immediate dependencies and skip special-address
framework modules, whereas a cold cache loads and records them. So a kept-failed
script promoted e.g. 0x1::coin on a node whose script cache was cold but not on
one whose cache was warm — a divergence in a consensus artifact (gated behind the
hotness feature, so not yet live).

Record the dependencies up front from a fresh deserialization of the script
bytecode, before any fallible step, so the set no longer depends on where the
script aborts or on cache warmth. Deserialization failure is a no-op —
load_script fails identically just below.

Adds a regression test that drives one script transaction through
execute_single_transaction and asserts its recorded module reads are identical
under a warm and a cold verified-script cache. The block-level to_make_hot
helpers can't cover this: they aggregate reads across a block (masking one txn's
under-recording), and the verified-script cache is per-block, so a warm
committing incarnation is only reachable via a racy parallel schedule.

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com

read_recording.rs gates its impl of the fuzzing-only trait method
`unmetered_get_module_skip_verification` behind bare `#[cfg(fuzzing)]`
(the cfg cargo-fuzz passes), which trips the `unexpected_cfgs` lint
unless the crate declares it. Add the same `[lints.rust]` declaration
already used by move-vm-runtime, block-executor, and e2e-tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aptos Security Bugbot has reviewed your changes and found 1 potential issue.

Open in Web View Automation 

Sent by Cursor Automation: Security Review Bot

.map(|script| code_storage.insert_deserialized_script(hash, script)),
};
if let Some(compiled_script) = compiled_script {
for (address, module_name) in compiled_script.immediate_dependencies_iter() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Medium severity — unverified dependencies can now enter to_make_hot on lazy-loading verifier failures.

This block records compiled_script.immediate_dependencies_iter() before the lazy loader has run local bytecode verification. immediate_dependencies_iter() walks raw module handles from the deserialized script, so a script that deserializes but later fails verification still contributes attacker-chosen module keys to the recorded read set.

With the default ENABLE_LAZY_LOADING and HOTNESS_IN_EPILOGUE feature set, those verifier failures are kept rather than discarded, and the read set is copied into the block epilogue write set. A malformed script can therefore steer the consensus-visible promotion set and crowd out legitimate promotions under the per-block cap, even though the script never passes verification.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aptos Security Bugbot has reviewed your changes, there is still 1 issue that needs to be addressed from previous scan.

Open findings:

Open in Web View Automation 

Sent by Cursor Automation: Security Review Bot

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aptos Security Bugbot has reviewed your changes, there are still 1 issues that need to be addressed from previous scan.

Open findings:

Open in Web View Automation 

Sent by Cursor Automation: Security Review Bot

Hot-state promotions (`to_make_hot` in the block epilogue) are part of
consensus, so a transaction's recorded read set must be a pure function of the
transaction and the pre-state.

`validate_and_execute_script` recorded a script's declared module dependencies
only *after* `load_script` and the post-load unstable-bytecode / event-emission
checks. A script that is kept-and-charged while failing after it loads — out of
gas partway through dependency charging, or rejected by those checks (both kept
as `MiscellaneousError`) — skipped that recording, leaving the dependencies to
`load_script`'s cache-gated fetches. On a warm verified-script cache those
fetches only *charge* the immediate dependencies and skip special-address
framework modules, whereas a cold cache loads and records them. So a kept-failed
script promoted e.g. `0x1::coin` on a node whose script cache was cold but not on
one whose cache was warm — a divergence in a consensus artifact (gated behind the
hotness feature, so not yet live).

Record the dependencies up front from a fresh deserialization of the script
bytecode, before any fallible step, so the set no longer depends on where the
script aborts or on cache warmth. Deserialization failure is a no-op —
`load_script` fails identically just below.

Adds a regression test that drives one script transaction through
`execute_single_transaction` and asserts its recorded module reads are identical
under a warm and a cold verified-script cache. The block-level `to_make_hot`
helpers can't cover this: they aggregate reads across a block (masking one txn's
under-recording), and the verified-script cache is per-block, so a warm
committing incarnation is only reachable via a racy parallel schedule.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aptos Security Bugbot has reviewed your changes and found 1 potential issue.

Open in Web View Automation 

Sent by Cursor Automation: Security Review Bot

}

dispatch_loader!(code_storage, loader, {
// Record the script's declared dependencies before any fallible step: a

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Medium severity — the determinism fix is still incomplete for package publishes.

resolve_pending_code_publish_and_finish_user_session() still charges old modules and immediate dependencies from merely deserialized CompiledModule metadata before later checks reject the bundle. The pre-validation loops at aptos_vm.rs:1632-1684 call unmetered_get_module_size / unmetered_get_existing_module_size, and ReadRecordingCodeStorage records those module keys into the transaction read set even when the lookup misses.

A malformed publish transaction that later fails the sender-address check in third_party/move/move-vm/runtime/src/storage/publishing.rs:158-172 (or any later verification-status publish check) is kept rather than discarded, so those attacker-chosen module keys still flow through aptos-move/aptos-vm/src/block_executor/vm_wrapper.rs:76-88 into the block's to_make_hot accumulator when HOTNESS_IN_EPILOGUE is enabled. That leaves the same consensus-visible promotion-poisoning primitive alive for package publishes even though the script path is being hardened here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aptos Security Bugbot has reviewed your changes and found 1 potential issue.

Open in Web View Automation 

Sent by Cursor Automation: Security Review Bot

.runtime_environment()
.deserialize_into_script(serialized_script.code())
.ok()
.map(|script| code_storage.insert_deserialized_script(hash, script)),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Medium severity — this now lets kept-failing scripts accumulate in the shared per-block script cache.

On a cache miss, this new block inserts the deserialized CompiledScript into the underlying ScriptCache before load_script has verified it. ReadRecordingCodeStorage delegates insert_deserialized_script straight into LatestView's block-scoped cache, and that cache has no bound or eviction (aptos-move/aptos-vm-types/src/module_and_script_storage/read_recording.rs:210-221, aptos-move/block-executor/src/code_cache.rs:235-244, third_party/move/move-vm/types/src/code/cache/script_cache.rs:20-42,45-49,149-199). Before this PR, lazy script loading only called insert_verified_script, so verifier-failing scripts were not retained.

A block full of distinct scripts that deserialize but later fail verification or the later script-only checks will now leave one cached entry per hash until block end. validate_transaction() already admits this class of script and execution keeps it as MiscellaneousError rather than discarding it (aptos-move/e2e-testsuite/src/tests/verify_txn.rs:664-683), so this change creates a validator memory / CPU amplification path that was not present before.

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.

1 participant