Dynamic storage support#982
Conversation
PR SummaryHigh Risk Overview Makes a backwards-compatible instruction format change to Reworks Written by Cursor Bugbot for commit 0fb12cf. This will update automatically on new commits. Configure here. |
| self, | ||
| interpreter: &mut Interpreter<M, S, Tx, Ecal, V>, | ||
| ) -> IoResult<ExecuteState, S::DataError> { | ||
| let (a, b, c, d) = self.unpack(); |
There was a problem hiding this comment.
It would be nice if we used readable names here like in hte opcode definition
| .storage | ||
| .contract_state(&contract_id, &key) | ||
| .map_err(RuntimeError::Storage)? | ||
| .map(|v| v.as_ref().as_ref().to_vec()) |
There was a problem hiding this comment.
You can convert the value into owned type (because, under the hood, it is already owned). Overwise you create vector two times(one when we create value, and another here).
| let dst = self.memory.as_mut().write(owner, dst_ptr, len)?; | ||
| let value = self | ||
| .storage | ||
| .contract_state(&contract_id, &key) |
There was a problem hiding this comment.
Maybe it is better to use read_bytes here? In this case we will avoud creation of the vector on the fuel-core side
There was a problem hiding this comment.
Not sure which read_bytes you're talking about?
There was a problem hiding this comment.
Sorry, I thought it was the name=D I meant StorageRead::read
There was a problem hiding this comment.
I don't think that works. We need to charge gas based on the full size of the storage slot, and StorageRead::read doesn't expose that info.
There was a problem hiding this comment.
Decided on call: lets change the API of StorageRead then
There was a problem hiding this comment.
That required quite a bit of refactoring due to zero filling that was already going against the documentation. Since we need to handle the returned error properly, the trait had to be reworked quite a bit.
| let Some(value) = value else { | ||
| self.registers[RegId::ERR] = 1; | ||
| return Ok(0); | ||
| }; |
There was a problem hiding this comment.
Stale data in preload area after failed SPLD
Medium Severity
When storage_preload doesn't find the requested key, the ERR register is set and 0 is returned, but the preload area is not cleared. If a previous successful SPLD loaded data, that stale data remains in the preload area. A subsequent SPCP call that doesn't properly check ERR could copy this stale data, potentially leading to unexpected behavior or information leakage between different storage keys within the same call frame.
There was a problem hiding this comment.
The is the intended behavior from the specification.
| srdd: DependentCost::LightOperation { | ||
| base: 50, | ||
| units_per_gas: 5, | ||
| base: 2513, |
There was a problem hiding this comment.
why is this so much more expensive than the original SRWQ?
There was a problem hiding this comment.
The values in default_gas_costs.rs aren't related to reality in any way at all. I'll be getting rid of this file in a follow-up PR, see issue #985
The real value from fuel-core that you could compare with is:
srwq: DependentCost::HeavyOperation {
base: 520,
gas_per_unit: 522,
},which is still cheaper than this, but not unreasonably so
MitchTurner
left a comment
There was a problem hiding this comment.
I'd like to understand what the "preload" is doing.
I had Codex walk me through all the tests because they are inscrutable for me to read unfortunately. Seems good to me, but I wish the could be a little more human-friendly. Also, we use rstest on a lot of them in a way that obscures logic, i.e. we use the cases to create logical differences in the test, rather than helping get better coverage. I think of rstest as a manual prop test solution, but we are using it as a DRYing tool, at the cost of clarity.
| /// Returns `Ok(Ok(length))` if the value does exist, where | ||
| /// the `length` is the total length of the value stored at the key. | ||
| /// | ||
| /// Note that inner error `Ok(Err(_))` is used to communicate errors that the |
There was a problem hiding this comment.
This isn't very intuitive. I'll see how it works later in the code and readdress.
There was a problem hiding this comment.
We could instead flatten the error variants into an enum, but that would make it way less ergonomic to call. The idea here is that the outer error you'll always want to propagate using ?, as there's no way you can handle it in any meaningful way at the call site. The inner error, however, you'll always handle using a match statement, because there will be logic to handle missing keys, at least.
| pub const DEFAULT: Self = Self::V2(ScriptParametersV2::DEFAULT); | ||
|
|
||
| /// Replace the max script length with the given argument | ||
| #[cfg(feature = "test-helpers")] |
There was a problem hiding this comment.
Why are these test only now?
There was a problem hiding this comment.
Since the new operation with_max_storage_slot_length will panic if used with V1 variant. I don't want to have that kind of thing happen outside tests, and since these are not used anywhere else, it seems prudent to make it test only. The pre-existing functions are marked as test-only for consistency.
| fn test_state_read_qword( | ||
| input: SRWQInput, | ||
| ) -> Result<(MemoryInstance, bool), RuntimeError<MemoryStorageError>> { | ||
| ) -> Result<(MemoryInstance, bool), RuntimeError<Infallible>> { |
There was a problem hiding this comment.
Why even keep the Result?
There was a problem hiding this comment.
Because some cases return an error. It's just that the storage error type nested inside RuntimeError is now infallible, but there are other error types there too, including RuntimeError::Recoverable which is used here.
| #[test_case(false, 1, 29, 32, 0 => Ok((0, 0)); "Wrong contract id")] | ||
| #[test_case(false, 0, 29, 33, 0 => Ok((0, 0)); "Wrong key")] | ||
| #[test_case(true, 0, None, Word::MAX, 0 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Overflowing key")] | ||
| #[test_case(true, 0, None, VM_MAX_RAM, 0 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Overflowing key ram")] |
There was a problem hiding this comment.
Wait. How can we expect a RuntimeError if it's Infallible?
There was a problem hiding this comment.
The Infallible is a generic parameter for StorageError, see the definition of RuntimeError:
pub enum RuntimeError<StorageError> {
/// Specified error with well-formed fallback strategy, i.e. vm panics.
Recoverable(PanicReason),
/// Invalid interpreter state reached unexpectedly, this is a bug
Bug(Bug),
/// Storage io error
Storage(StorageError),
}| } | ||
|
|
||
| /// Fetch a mapping from the contract state. | ||
| pub fn contract_state( |
There was a problem hiding this comment.
This just isn't used anymore?
| } | ||
|
|
||
| #[test] | ||
| fn sww_writes_32_bytes() { |
There was a problem hiding this comment.
This test is breaking my brain. I can't keep track of what thing are slots keys and what things are values while reading it.
The spec says:
This is useful because reading length of a slot requires internally reading the whole slot contents. The primary function of the preload area is to avoid having to load the same slot twice when fetching variable length data. It also allows reading parts of a slot into memory without placing it all continously, which is sometimes useful when i.e. traversing tree structures.
I attempted abstracting some of the setup code into reusable and well-named functions, but the result was even less readable.
I'm indeed using it as a DRY tool. I think duplicating the functions would hurt clarity quite a bit more than the We could of course do the same thing without rstest and instead move entire test to a separate function, passing in a closure that performs the actual operation-under-test. That would feel rather pointless, and we would lose the nice DX rstest gives us on test failure. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| self.storage | ||
| .contract_state_insert(&contract_id, &key, &value) | ||
| .map_err(RuntimeError::Storage)?; | ||
| Ok(len_after as u64) |
There was a problem hiding this comment.
SUPD/SUPI gas undercharged for writes to large slots
High Severity
storage_update_from_memory returns len_after (offset + write_len) instead of value.len() (the full slot size). When a small range is updated in a large existing slot, the gas charge is based on the write endpoint, not the total I/O. For example, writing 5 bytes at offset 10 of a 1000-byte slot charges gas for 15 bytes while the storage backend reads and rewrites all 1000 bytes. The return value for gas metering needs to reflect the actual slot size, i.e., value.len(), which equals max(original_len, offset + write_len).
Additional Locations (1)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| value.len() | ||
| } else { | ||
| convert::to_usize(offset).ok_or(PanicReason::MemoryOverflow)? | ||
| }; |
There was a problem hiding this comment.
Undocumented u64::MAX sentinel enables silent append behavior
Medium Severity
In storage_update_from_memory, offset == u64::MAX is treated as a sentinel meaning "append to end of slot." This implicit behavior is not documented in the SUPD/SUPI opcode descriptions and can be triggered by any contract that sets its offset register to u64::MAX. Instead of getting a StorageOutOfBounds panic (which would naturally occur for such a large offset), the contract silently gets append semantics.
There was a problem hiding this comment.
It's documented in the specification
| )?; | ||
| interpreter.dependent_gas_charge( | ||
| interpreter.gas_costs().srdd().map_err(PanicReason::from)?, | ||
| len, |
There was a problem hiding this comment.
SRDI charges gas using wrong cost accessor name
Low Severity
SRDI charges gas using gas_costs().srdd() and SWRI charges gas using gas_costs().swrd(). While sharing costs between immediate/register variants may be intentional, there are no corresponding srdi or swri fields in GasCostsValuesV7, so these opcodes can never be independently costed. If future gas benchmarking reveals the immediate variants deserve different costs, a new gas cost version will be needed.
Additional Locations (1)
There was a problem hiding this comment.
This is intentional and not a real concern
|
Gas cost comparison from the latest benchmark run versus existing cost of non-dynamic operations. I'm only comparing read operations here, because the database read cost completely dominates the costs; writing is almost free except that we need to read the value anyway to charge for new bytes written. Read costs: Using Using Meaning that if you're able to combine at least six slots, dynamic ops are cheaper for reading. |
## Linked Issues/PRs FuelLabs/fuel-vm#982 ## Description Adds support and benchmarks for fuel-vm dynamic storage opcodes. ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog (None!) - [x] New behavior is reflected in tests - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] I have reviewed the code myself - [x] I have created follow-up issues caused by this PR and linked them here --------- Co-authored-by: Green Baneling <XgreenX9999@gmail.com>
Discussed in #517. VM impl PR: FuelLabs/fuel-vm#982 Adds a minimal set of opcodes required to operate with dynamic (variable-sized) storage slots. More instructions can be added later to speed up common operations, as discussed in #517. This PR marks sequential bulk read and write opcodes as deprecated. There's no process for actually removing opcodes, but simply including DEPRECATED in the heading informs readers that they ought to be careful. Most imporatantly, the sequential read opcode now panics if attempting to read slots with size other than 32 bytes. The sequential clear instuction still works as expected and is kept as-is. The PR also clarifies how the old storarge instructions interact with variable sized slots. ### Before requesting review - [x] I have reviewed the code myself --------- Co-authored-by: Brandon Kite <brandonkite92@gmail.com> Co-authored-by: Igor Rončević <ironcev@hotmail.com>



Implements dynamic storage instructions as described in FuelLabs/fuel-specs#640
The storage initialization in
Createtransactions is left as-is, i.e. it only allows 32 byte slots. This feature isn't used much, and I don't think it's worth it to continue supporting.The only breaking change introduced here is adding an immediate offset operand to
SRWinstruction. Backwards compatibility is retained as the field as previously required to be zeroed.Checklist
Before requesting review