Implement transient get/set bytes32 hostios #185

joshuacolvin0 · 2023-12-07T05:04:03Z

Depends on:
OffchainLabs/stylus-sdk-c#15
OffchainLabs/stylus-sdk-rs#92

codecov · 2023-12-12T02:43:39Z

Codecov Report

Merging #185 (e7a6287) into stylus (87f358c) will decrease coverage by 0.27%.
Report is 1 commits behind head on stylus.
The diff coverage is 35.25%.

Additional details and impacted files

@@            Coverage Diff             @@
##           stylus     #185      +/-   ##
==========================================
- Coverage   58.13%   57.87%   -0.27%     
==========================================
  Files         282      282              
  Lines       43331    43443     +112     
==========================================
- Hits        25190    25141      -49     
- Misses      15694    15832     +138     
- Partials     2447     2470      +23

rauljordan · 2023-12-12T20:11:02Z

arbitrator/stylus/src/host.rs

+    dest: u32,
+) -> MaybeEscape {
+    let mut env = WasmEnv::start(&mut env, 2 * PTR_INK + EVM_API_INK)?;
+    env.buy_gas(TRANSIENT_BYTES32_GAS)?;


Would be nice to map errors here to be more informative. To know if it failed at buying gas, at reading, or at transient get

the error messages bubbled up are already informative enough

rauljordan · 2023-12-12T20:12:04Z

arbitrator/wasm-libraries/user-host/src/host.rs

    let gas_cost = api.set_bytes32(key, value).unwrap();
    program.buy_gas(gas_cost).unwrap();
 }

+#[no_mangle]
+pub unsafe extern "C" fn user_host__storage_transient_load_bytes32(key: usize, dest: usize) {
+    let program = Program::start(2 * PTR_INK + EVM_API_INK);


Where does the magic number 2 come from?

reading two ptr inputs (key, dest)

rauljordan · 2023-12-12T20:12:46Z

arbitrator/wasm-libraries/user-host/src/host.rs

+    let value = wavm::read_bytes32(value);
+
+    let api = &mut program.evm_api;
+    api.transient_set_bytes32(key, value).unwrap();


Could we move wavm::read_bytes32(key) and value into the api.transient_set_bytes32(key, value).unwrap(); to avoid declaring two variables?

This code has been replaced by macro. In general, better to keep things explicit so that the amount of gas used is as clear as possible

change names and update Makefile to cleanup generated files

rachel-bousfield

Nice work! Some minor suggests as I continue reviewing

rachel-bousfield · 2023-12-14T03:12:07Z

arbitrator/stylus/src/evm_api.rs

+        let value = call!(self, transient_get_bytes32, key);
+        value


Let's drop the let value =

rachel-bousfield · 2023-12-14T03:16:01Z

arbitrator/arbutil/src/evm/api.rs

@@ -57,6 +59,17 @@ pub trait EvmApi: Send + 'static {
    /// Analogous to `vm.SSTORE`.
    fn set_bytes32(&mut self, key: Bytes32, value: Bytes32) -> Result<u64>;

+    /// Reads the 32-byte value in the EVM state trie at offset `key`.


Maybe

Reads the 32-byte value from transient storage at offset key.

And drop the next line.

rachel-bousfield · 2023-12-14T03:16:36Z

arbitrator/arbutil/src/evm/mod.rs

+// params.WarmStorageReadCostEIP2929
+pub const TRANSIENT_BYTES32_GAS: u64 = 100;


Let's call it TRANSIENT_OP_GAS or similar

rachel-bousfield · 2023-12-14T03:22:23Z

arbitrator/wasm-libraries/user-test/src/host.rs

+#[no_mangle]
+pub unsafe extern "C" fn vm_hooks__transient_storage_load_bytes32(key: u32, dest: u32) {
+    let mut program = Program::start(2 * PTR_INK + EVM_API_INK);
+    let key = wavm::read_bytes32(key);
+
+    let value = TRANSIENT_KEYS.lock().get(&key).cloned().unwrap_or_default();
+    program.buy_gas(evm::TRANSIENT_BYTES32_GAS).unwrap();
+    wavm::write_bytes32(dest, value);
+}
+
+#[no_mangle]
+pub unsafe extern "C" fn vm_hooks__transient_storage_store_bytes32(key: u32, value: u32) {
+    let mut program = Program::start(2 * PTR_INK + EVM_API_INK);
+    program.buy_gas(evm::TRANSIENT_BYTES32_GAS).unwrap(); // pretend the worst case
+
+    let key = wavm::read_bytes32(key);
+    let value = wavm::read_bytes32(value);
+    TRANSIENT_KEYS.lock().insert(key, value);
+}
+


Let's drop these since they're unused

rachel-bousfield · 2023-12-14T03:29:41Z

arbitrator/stylus/src/test/api.rs

        let api = TestEvmApi {
            contracts: Arc::new(Mutex::new(HashMap::new())),
            storage: Arc::new(Mutex::new(storage)),
+            transient_storage: Arc::new(Mutex::new(transient_storage)),


Let's drop this and related funcs until we have a test framework with a notion of transactions (all tests are currently just isolated calls, so there's no meaningful distinction between normal and transient storage)

joshuacolvin0 · 2024-07-11T21:44:38Z

implemented in different PR

cla-bot bot added the s label Dec 7, 2023

joshuacolvin0 marked this pull request as ready for review December 11, 2023 19:48

joshuacolvin0 marked this pull request as draft December 11, 2023 20:26

joshuacolvin0 marked this pull request as ready for review December 12, 2023 02:16

rauljordan reviewed Dec 12, 2023

View reviewed changes

joshuacolvin0 force-pushed the rename-memory-grow branch from 768c397 to 745a078 Compare December 13, 2023 23:38

add transient get/set bytes32 hostios

185d5bd

joshuacolvin0 force-pushed the transient-hostios branch from d80817e to 185d5bd Compare December 14, 2023 02:45

Base automatically changed from rename-memory-grow to stylus December 14, 2023 03:18

change storage_transient_* to transient_*

03a4698

change names and update Makefile to cleanup generated files

rachel-bousfield suggested changes Dec 14, 2023

View reviewed changes

joshuacolvin0 added 4 commits December 13, 2023 21:18

Addressing assorted minor code review comments

b45f0c0

Remove unused import

92c815c

Annotate placeholder methods with unimplemented!()

00cc0f0

Merge branch 'stylus' into transient-hostios

e7a6287

joshuacolvin0 closed this Jul 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement transient get/set bytes32 hostios #185

Implement transient get/set bytes32 hostios #185

joshuacolvin0 commented Dec 7, 2023 •

edited

Loading

codecov bot commented Dec 12, 2023 •

edited

Loading

rauljordan Dec 12, 2023

joshuacolvin0 Dec 15, 2023

rauljordan Dec 12, 2023

joshuacolvin0 Dec 15, 2023

rauljordan Dec 12, 2023

joshuacolvin0 Dec 15, 2023

rachel-bousfield left a comment

rachel-bousfield Dec 14, 2023

joshuacolvin0 Dec 14, 2023

rachel-bousfield Dec 14, 2023

joshuacolvin0 Dec 14, 2023

rachel-bousfield Dec 14, 2023

joshuacolvin0 Dec 14, 2023

rachel-bousfield Dec 14, 2023

joshuacolvin0 Dec 14, 2023

rachel-bousfield Dec 14, 2023

joshuacolvin0 Dec 14, 2023

joshuacolvin0 commented Jul 11, 2024

		// params.WarmStorageReadCostEIP2929
		pub const TRANSIENT_BYTES32_GAS: u64 = 100;

Implement transient get/set bytes32 hostios #185

Implement transient get/set bytes32 hostios #185

Conversation

joshuacolvin0 commented Dec 7, 2023 • edited Loading

codecov bot commented Dec 12, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rachel-bousfield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuacolvin0 commented Jul 11, 2024

joshuacolvin0 commented Dec 7, 2023 •

edited

Loading

codecov bot commented Dec 12, 2023 •

edited

Loading