[block-stm] Refactor mvhashmaps to not use layouts#20196
Draft
georgemitenkov wants to merge 5 commits into
Draft
[block-stm] Refactor mvhashmaps to not use layouts#20196georgemitenkov wants to merge 5 commits into
georgemitenkov wants to merge 5 commits into
Conversation
Contributor
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
16 tasks
georgemitenkov
force-pushed
the
george/refactor-records
branch
4 times, most recently
from
b19e1c1 to
1398a63
Compare
[block-stm] Refactor committed output Decouple the committed (materialized) transaction output from the speculative output produced during execution. Add a CommittedOutput associated type to the block-executor TransactionOutput trait, bounded by a new CommittedTransactionOutput trait in aptos-types. Materialization now returns the owned committed output instead of stashing it in a OnceCell, and final_results stores committed outputs directly. Make TransactionCommitHook generic over the output type so the hook sees the executor's committed output directly. This drops the concrete-type accessor that previously forced every committed output to contain an aptos_types::transaction::TransactionOutput. Remove the now-unused AfterMaterializationOutput trait and its guard, along with committed_output(), after_materialization(), is_materialized_and_success(), and check_materialization(). This pre-stages the records / record-output split for the new VM. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> [block-stm] Fold ValueWithLayout into outputs [block-stm] Add materializer and refactor events Replace the single Transaction::Event associated type with SpeculativeEvent and CommittedEvent. The V1 VM sets SpeculativeEvent = (ContractEvent, Option<MoveTypeLayout>) and CommittedEvent = ContractEvent, so MoveTypeLayout no longer leaks through the output's event accessor. Add a Materializer trait with a per-event materialize_event, implemented by LatestView. The output's new materialize_events drives iteration and calls the materializer, so the V1 VM borrows its event slice directly while a record-based VM can dispatch over its own storage. This replaces the free function map_id_to_values_events and the Box<dyn Iterator> events accessor. The TransactionEvent bound moves from the Transaction trait onto the V1 Materializer impl, where the byte-level event rewriting actually happens. The executor carries a matching event-shape bound while it still constructs LatestView as its materializer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> [block-stm] Abstract key-value data model for VMs Prepares Block-STM for a second VM (mono) with a different value representation. The pipeline (apply, validation, commit sequencing, materialization orchestration) stays in the executor, written once; what varies per VM is the data model: - BlockExecutableTransaction gains SpeculativeValue, the multi-version map value (legacy: ValueWithLayout<WriteOp>). MVValue and ValueWithLayout move into aptos-types so the bound is declarable; MVHashMap/UnsyncMap are instantiated with it uniformly. - Record trait: the per-txn artifact bundling reads, writes and status. Write sets (typed by the speculative value) drive the unchanged apply loops, reads self-validate against the map, commit facts feed block limits, and one materialize(&self) hook produces the committed output. The legacy record wraps CapturedReads/UnsyncReadSet plus the TransactionOutput, keeping all layout and delayed-field exchange logic internal to it. - Records<T, R>: the store of the latest records (replaces TxnLastInputOutput), with Arc'd slots so validation reads lock-free while newer incarnations record, and a store-level status kind since commit flips Success to SkipRest when the block is cut. - TransactionExecutor: the VM-facing seam. A blanket impl covers every legacy ExecutorTask by building LatestView from VM-neutral ViewArgs, so AptosExecutorTask, the mocks and all call sites stay untouched. The executor, store and scheduler wrapper no longer mention ValueWithLayout, MoveTypeLayout or event shapes; those bounds now live only on the legacy record and the blanket impl. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
incorporate_materialized_txn_output no longer takes patched resource writes. The output patches its own writes in place, pulling the map-dependent data (serialized group bytes, exchanged-read bytes) through a widened Materializer built by the record; value-level id replacement stays behind the same trait. This removes the Vec<(Key, Value)> merge step (extend_resource_write_set) and the event copy-out/copy-in, and keeps output-representation knowledge on the VM side. Groundwork for dropping the value types from BlockExecutableTransaction. Also fixes a stale 4-arg TExecutorView usage in the mock executor (test-only build breakage predating this change). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
pre_write_values is a VM concern (the value is produced in the VM's speculative representation), not a transaction property. Move it from BlockExecutableTransaction to TransactionExecutor / ExecutorTask, with the timestamp pre-write logic implemented by AptosExecutorTask. Prepares for removing the value types from the transaction trait. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
georgemitenkov
force-pushed
the
george/aggregator-v1-refactor
branch
from
5f0d7b6 to
4b9d2b7
Compare
georgemitenkov
force-pushed
the
george/refactor-records
branch
3 times, most recently
from
b3fca01 to
1fed011
Compare
BlockExecutableTransaction carried Value (committed) and SpeculativeValue (multi-version) types, forcing one value representation per transaction type across all VMs. These are VM concerns: the multi-version value now lives on Record (type Value: MVValue; the legacy record uses ValueWithLayout over the output's value), and the committed value on TransactionOutput (type Value: TransactionWrite). The transaction trait keeps only Key and Tag. The generic executor is typed on the record's value; the legacy view stack (LatestView, CapturedReads, value exchange) is parameterized by the output's value. Module publishing is concretized to (module id, state value), so no value type crosses that boundary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
georgemitenkov
force-pushed
the
george/refactor-records
branch
from
1fed011 to
379be51
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Comment on lines
+300
to
+314
| fn materialize<S: TStateView<Key = <Self::Txn as Transaction>::Key> + Sync>( | ||
| &self, | ||
| _args: &ViewArgs<'_, Self, S>, | ||
| _txn_idx: TxnIndex, | ||
| _environment: &AptosEnvironment, | ||
| ) -> Result<Self::CommittedOutput, PanicOr<ResourceGroupSerializationError>> { | ||
| // P0 stub: committed outputs are empty. The goal is executing blocks | ||
| // end-to-end; commit-time serialization of the write set comes later. | ||
| Ok(TransactionOutput::new( | ||
| WriteSet::default(), | ||
| vec![], | ||
| 0, | ||
| TransactionStatus::Keep(ExecutionStatus::Success), | ||
| TransactionAuxiliaryData::None, | ||
| )) |
Contributor
There was a problem hiding this comment.
High severity: MONO_MOVE is now an on-chain execution switch, but this materialization path still hardcodes every committed mono output to Keep(Success) with an empty write set, no events, and zero gas. build_record() records speculative writes that later transactions can read, yet commit drops those effects here, so the block can execute txn N+1 against state written by txn N and then finalize a ledger that omits txn N's changes. The new e2e test in aptos-move/e2e-move-tests/src/tests/mono_move.rs even asserts that a transfer block succeeds with empty writes/events and gas_used() == 0, so once governance enables MONO_MOVE, ordinary user transactions become successful no-ops and block-output accounting is bypassed.
Comment on lines
+108
to
+112
| // Only entry-function user transactions execute on the mono path; | ||
| // everything else commits an empty success record. | ||
| let Some((signed_txn, entry)) = entry_function(txn) else { | ||
| return Ok(MonoRecord::empty_success(record_incarnation)); | ||
| }; |
Contributor
There was a problem hiding this comment.
High severity: this branch turns every transaction the mono path does not actively support into MonoRecord::empty_success(), and the later failure paths in this file do the same for loader, argument, and runtime errors before materialize() reports Keep(Success). Under the same MONO_MOVE flag that now selects MonoExecutorTask for live block execution, that means BlockMetadata, BlockEpilogue, validator transactions, unsupported user payloads, and publish-wrapper entry functions can all silently no-op instead of running their required semantics or failing. A validator enabling the feature would therefore accept blocks that skip mandatory system effects while still marking those transactions successful.
Wires the mono VM into Block-STM as a second TransactionExecutor implementation, selected by FeatureFlag::MONO_MOVE. The goal of this step is executing whole blocks end-to-end (scheduling, speculative reads, validation, re-execution); committed outputs are stubs, gas and prologue/epilogue are out of scope, and only entry-function user transactions execute (everything else commits an empty success record). Parallel execution only. The legacy path is untouched apart from additive accessors and the dispatch branch. New crate mono-move-block-executor: - MonoValue: a pointer into a frozen execution heap plus the Arc that keeps the heap alive. Values are never compared (eq_value is false); a fresh heap per incarnation means identity never matches anyway. - MonoRecord: reads (key -> version) and writes extracted from the interpreter's read-write set in one walk. Validation is version-only against VersionedData; speculative failures fail their own validation so BlockSTMv1 is guaranteed to re-execute them. - BlockStmResourceProvider: reads consult the multi-version map first (v1 without dependency recording, v2 with); on a miss the base value is deserialized (BCS -> flat) into its own heap and published into the map at the storage version, so each key is deserialized once per block. A dependency hit aborts the incarnation via a provider flag. - StateViewModuleProvider, a production-only natives registry, and MonoExecutorTask mirroring the replay-benchmark harness. Supporting changes: - StorageRead now carries the observed version on both variants and, for ExternalHeap, the Arc of the heap the pointer targets: the read-write set doubles as the captured read set, and each read pins the memory it points into for the duration of the execution (a concurrent abort may drop the map entry that also held the heap). This moves the provider interface from mono core to the runtime crate, next to Heap; the error type stays in core (vm_error embeds it). - Runtime exposes ResourceReadWriteSet::entries() and InterpreterContext::into_storage_effects() to extract the heap and read-write set after a run. - Block-executor exposes ViewArgs accessors and the limit_processor module so records can be implemented outside the crate. - Replay-benchmark's provider switches to per-value heaps to satisfy the new StorageRead contract. An e2e test runs a block of aptos_account::transfer transactions through the parallel executor with the flag enabled; zero gas in the stub outputs is what distinguishes the mono path from the legacy one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
georgemitenkov
force-pushed
the
george/refactor-records
branch
from
54ece95 to
c8c593c
Compare
Comment on lines
+115
to
+141
| let mut args = entry_args.iter(); | ||
| for (slot, kind) in func.param_slots.iter().zip(params) { | ||
| let offset = slot.offset.0; | ||
| match kind { | ||
| ParamKind::Signer { by_ref: false } => interp.set_root_arg(offset, signer_bytes), | ||
| ParamKind::Signer { by_ref: true } => { | ||
| // A reference is a 16-byte fat pointer (base, byte_offset) | ||
| // pointing at the signer buffer. The base is outside the VM | ||
| // heap, so the GC leaves it alone. | ||
| let mut fat = [0u8; 16]; | ||
| fat[..8].copy_from_slice(&(signer_bytes.as_ptr() as u64).to_le_bytes()); | ||
| interp.set_root_arg(offset, &fat); | ||
| }, | ||
| ParamKind::Value { ty } => { | ||
| let arg = args | ||
| .next() | ||
| .ok_or_else(|| anyhow!("not enough arguments for the entry function"))?; | ||
| // SAFETY: `offset`/`ty` come from this function's own | ||
| // signature, so the slot is valid for the type's in-memory | ||
| // size. | ||
| unsafe { interp.deserialize_root_arg(offset, *ty, arg) }.map_err(|e| { | ||
| anyhow!("failed to place argument at frame offset {}: {}", offset, e) | ||
| })?; | ||
| }, | ||
| } | ||
| } | ||
| Ok(()) |
Contributor
There was a problem hiding this comment.
High severity: this argument placer reconstructs entry-call parameters without preserving the legacy AptosVM signer/argument semantics. Every signer/&signer slot is filled from the primary sender bytes only, so a multi-agent entry function sees the primary sender in every signer position, and trailing serialized arguments are silently accepted because the iterator is never exhaustively checked. On the live MONO_MOVE path that changes authenticated user-transaction behavior: supported entry functions can execute with the wrong signer binding, and malformed transactions that legacy AptosVM would reject for argument-count mismatch are accepted instead.
| use move_core_types::{ident_str, vm_status::VMStatus}; | ||
| use std::{collections::HashMap, sync::Arc}; | ||
|
|
||
| /// Effectively unbounded gas budget (no gas metering on the mono path). |
Contributor
There was a problem hiding this comment.
High severity: the mono entry path now executes supported user transactions with an effectively unbounded gas budget while explicitly skipping the normal prologue/epilogue path. Under the live MONO_MOVE routing, a transaction's usual gas and execution-limit enforcement is never applied before interpreter execution, so a user transaction that legacy AptosVM would cut off can consume arbitrarily more validator work on this feature-gated path and create a concrete execution-time DoS vector.
georgemitenkov
changed the base branch from
george/aggregator-v1-refactor
to
graphite-base/20196
16 tasks
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
1 participant
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
How Has This Been Tested?
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist