fix(precompile-storage): clear stale dynamic tail chunks after Cobalt#3987
Conversation
🟡 Heimdall Review Status
|
c9b7b7f to
c7259cc
Compare
Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
c7259cc to
588372f
Compare
Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
…k-overwrites-leave-stale-tail-slots
|
|
||
| let $calldata: ::alloy_primitives::Bytes = input.data.to_vec().into(); | ||
| let mut provider = ::base_precompile_storage::EvmPrecompileStorageProvider::new( | ||
| let mut provider = ::base_precompile_storage::EvmPrecompileStorageProvider::new_with_storage_semantics( |
There was a problem hiding this comment.
The third macro arm (line 34, the |$input, $ctx, $calldata| variant) was not updated in this PR and still calls EvmPrecompileStorageProvider::new($input) — which hardcodes Legacy semantics and also passes only one argument when new requires two (input, gas_params). It would fail to compile if instantiated.
Since this PR adds storage_semantics support to the first two arms, consider either updating the third arm to match or removing it if it's dead code.
…k-overwrites-leave-stale-tail-slots
| if T::BYTES <= 16 { | ||
| let first_tail_slot = calc_packed_slot_count(new_len, T::BYTES); | ||
| let old_slot_count = calc_packed_slot_count(old_len, T::BYTES); | ||
| for slot_idx in first_tail_slot..old_slot_count { | ||
| let slot_addr = data_start | ||
| .checked_add(U256::from(slot_idx)) | ||
| .ok_or(BasePrecompileError::SlotOverflow)?; | ||
| storage.store(slot_addr, U256::ZERO)?; | ||
| } |
There was a problem hiding this comment.
For packed types, clear_stale_elements only zeros fully-retired slots (those with index >= calc_packed_slot_count(new_len)). It does not clear individual stale elements that share a "boundary" slot with kept elements.
This is correct today because store_packed_elements rebuilds each slot from PackedSlot(U256::ZERO), so any stale bytes in the boundary slot are overwritten. However, this invariant is non-obvious and fragile — if someone later changes store_packed_elements to do read-modify-write (to avoid re-writing unchanged elements), stale bytes would leak.
Consider adding a comment documenting this dependency, e.g.:
// NOTE: Only fully-retired slots are cleared here. Stale bytes in the
// boundary slot (the last slot occupied by the *new* array) are handled
// by `store_packed_elements`, which rebuilds each slot from zero.| /// Checks whether writes are allowed before any preparatory reads occur. | ||
| fn ensure_writable(&self) -> Result<()> { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The default ensure_writable implementation returns Ok(()) unconditionally, meaning any StorageOps implementor that forgets to override it will silently permit writes in static contexts when Cobalt cleanup runs.
TransientOps (in slot.rs) inherits this default, which is fine since TSTORE in a static context is caught at the EVM level. But if a new StorageOps impl is added for persistent storage and forgets to override ensure_writable, cleanup SLOADs would execute in static calls before the eventual SSTORE catches the violation — subtly violating the "reject before preparatory reads" contract that the test static_write_fails_before_cobalt_cleanup_load enforces.
Consider making ensure_writable a required method (no default) so new implementors are forced to handle it, or at minimum add a doc comment warning that persistent-storage implementations must override this.
Review SummaryThis PR adds fork-dependent storage cleanup semantics ( Test coverage is thorough — tests cover long→short, long→shorter-long, packed/unpacked vector shrink, boundary slot behavior, nested dynamic struct cleanup, push-after-shrink, malformed length rejection, and static-call rejection ordering. Findings
The default
Previously raised (from earlier review)
|
Summary
Bytes,String, orVec<T>values overwrite longer valuesImplementation
StorageSemantics::{Legacy, Cobalt}selected byBasePrecompilesScope
This prevents new stale tails at and after Cobalt and cleans legacy tails when their metadata remains reachable. Historical orphaned slots whose old length metadata was already overwritten cannot be discovered without a separate state migration or high-water metadata.
Testing
cargo +nightly fmt --all -- --checkcargo test -p base-precompile-storage --all-featurescargo test -p base-common-precompilescargo test -p base-common-evmcargo check -p base-precompile-storage --no-default-featuresRISC0_SKIP_BUILD_KERNELS=1 BASE_SUCCINCT_ELF_STUB=1 cargo clippy -p base-precompile-storage -p base-common-precompiles -p base-common-evm --all-targets -- -D warningsReferences