steb's review changes

(we're going to squash anyways so there's little point in breaking this
into multiple commits)

- Pass event slice length instead of slice of byte buffer.
- Rework gas costs for the new call and remove the indexing costs
  because we don't actually charge anything anyways. This lets us charge
  once at the top.
- Add an additional "deferred" memory allocation charge per-event which
  likely should have been in the initial spec (will need to go into the
  FIP).
- Be extra careful about overflows. Unecessary, but we do this _just_ in
  case.
- _Do_ cast the event entry slice but assert that the alignment is the
  same as u8 (bytes). Given `repr(packed)`, this is guaranteed.
- Don't cap the total value length at 8kib. The total cap is 8kib * 255.
- Reduce the max key length & max number of entries by one each so we
  can better reason about estimated CBOR sizes. I.e., 255 fits in 2
  bytes (major type plus one byte for the integer), 256 does not.
- Isolate the unsafe logic into the syscall API.

fvm/Cargo.toml

@@ -38,6 +38,7 @@ rand = "0.8.5"
 quickcheck = { version = "1", optional = true }
 once_cell = "1.18"
 minstant = "0.1.2"
+static_assertions = "1.1.0"
 
 [dev-dependencies]
 pretty_assertions = "1.3.0"

fvm/src/gas/price_list.rs

@@ -7,7 +7,6 @@ use std::ops::Mul;
 
 use anyhow::Context;
 use fvm_shared::crypto::signature::SignatureType;
-use fvm_shared::event::{ActorEvent, Flags};
 use fvm_shared::piece::PieceInfo;
 use fvm_shared::sector::{
     AggregateSealVerifyProofAndInfos, RegisteredPoStProof, RegisteredSealProof, ReplicaUpdateInfo,
@@ -27,6 +26,21 @@ use crate::kernel::SupportedHashes;
 // https://docs.rs/wasmtime/2.0.2/wasmtime/struct.InstanceLimits.html#structfield.table_elements
 const TABLE_ELEMENT_SIZE: u32 = 8;
 
+// The maximum overhead (in bytes) of a single event when encoded into CBOR.
+//
+// 1: CBOR tuple with 2 fields (StampedEvent)
+//   9: Emitter ID
+//   2: Entry array overhead (max size 255)
+const EVENT_OVERHEAD: u64 = 12;
+// The maximum overhead (in bytes) of a single event entry when encoded into CBOR.
+//
+// 1: CBOR tuple with 4 fields
+//   1: Flags (will adjust as more flags are added)
+//   2: Key major type + length (up to 255 bytes)
+//   2: Codec major type + value (codec should be <= 255)
+//   3: Value major type + length (up to 8192 bytes)
+const EVENT_ENTRY_OVERHEAD: u64 = 9;
+
 /// Create a mapping from enum items to values in a way that guarantees at compile
 /// time that we did not miss any member, in any of the prices, even if the enum
 /// gets a new member later.
@@ -293,26 +307,15 @@ lazy_static! {
             memory_fill_per_byte_cost: Gas::from_milligas(400),
         },
 
-        // These parameters are specifically sized for EVM events. They will need
-        // to be revisited before Wasm actors are able to emit arbitrary events.
-        //
-        // Validation costs are dependent on encoded length, but also
-        // co-dependent on the number of entries. The latter is a chicken-and-egg
-        // situation because we can only learn how many entries were emitted once we
-        // decode the CBOR event.
-        //
-        // We will likely need to revisit the ABI of emit_event to remove CBOR
-        // as the exchange format.
-        event_validation_cost: ScalingCost {
+        // TODO(#1817): Per-entry event validation cost. These parameters were benchmarked for the
+        // EVM but haven't been revisited since revising the API.
+        event_per_entry: ScalingCost {
             flat: Gas::new(1750),
             scale: Gas::new(25),
         },
 
-        // The protocol does not currently mandate indexing work, so these are
-        // left at zero. Once we start populating and committing indexing data
-        // structures, these costs will need to reflect the computation and
-        // storage costs of such actions.
-        event_accept_per_index_element: ScalingCost {
+        // TODO(#1817): Cost of validating utf8 (used in event parsing).
+        utf8_validation: ScalingCost {
             flat: Zero::zero(),
             scale: Zero::zero(),
         },
@@ -468,14 +471,14 @@ pub struct PriceList {
 
     /// Gas cost to validate an ActorEvent as soon as it's received from the actor, and prior
     /// to it being parsed.
-    pub(crate) event_validation_cost: ScalingCost,
-
-    /// Gas cost of every indexed element, scaling per number of bytes indexed.
-    pub(crate) event_accept_per_index_element: ScalingCost,
+    pub(crate) event_per_entry: ScalingCost,
 
     /// Gas cost of doing lookups in the builtin actor mappings.
     pub(crate) builtin_actor_manifest_lookup: Gas,
 
+    /// Gas cost of utf8 parsing.
+    pub(crate) utf8_validation: ScalingCost,
+
     /// Gas cost of accessing the network context.
     pub(crate) network_context: Gas,
     /// Gas cost of accessing the message context.
@@ -919,58 +922,33 @@ impl PriceList {
     }
 
     #[inline]
-    pub fn on_actor_event_validate(&self, data_size: usize) -> GasCharge {
-        let memcpy = self.block_memcpy.apply(data_size);
-        let alloc = self.block_allocate.apply(data_size);
-        let validate = self.event_validation_cost.apply(data_size);
-
-        GasCharge::new(
-            "OnActorEventValidate",
-            memcpy + alloc + validate,
-            Zero::zero(),
-        )
-    }
-
-    #[inline]
-    pub fn on_actor_event_accept(&self, evt: &ActorEvent, serialized_len: usize) -> GasCharge {
-        let (mut indexed_bytes, mut indexed_elements) = (0usize, 0u32);
-        for evt in evt.entries.iter() {
-            if evt.flags.contains(Flags::FLAG_INDEXED_KEY) {
-                indexed_bytes += evt.key.len();
-                indexed_elements += 1;
-            }
-            if evt.flags.contains(Flags::FLAG_INDEXED_VALUE) {
-                indexed_bytes += evt.value.len();
-                indexed_elements += 1;
-            }
-        }
+    pub fn on_actor_event(&self, entries: usize, keysize: usize, valuesize: usize) -> GasCharge {
+        // Here we estimate per-event overhead given the constraints on event values.
 
-        // The estimated size of the serialized StampedEvent event, which
-        // includes the ActorEvent + 8 bytes for the actor ID
-        const STAMP_EXTRA_SIZE: usize = 8;
-        let stamped_event_size = serialized_len + STAMP_EXTRA_SIZE;
+        let validate_entries = self.event_per_entry.apply(entries);
+        let validate_utf8 = self.utf8_validation.apply(keysize);
 
-        // Charge for 3 memory copy operations.
-        // This includes the cost of forming a StampedEvent, copying into the
-        // AMT's buffer on finish, and returning to the client.
-        let memcpy = self.block_memcpy.apply(stamped_event_size);
+        // Estimate the size, saturating at max-u64. Given how we calculate gas, this will saturate
+        // the gas maximum at max-u64 milligas.
+        let estimated_size = EVENT_OVERHEAD
+            .saturating_add(EVENT_ENTRY_OVERHEAD.saturating_mul(entries as u64))
+            .saturating_add(keysize as u64)
+            .saturating_add(valuesize as u64);
 
-        // Charge for 2 memory allocations.
-        // This includes the cost of retaining the StampedEvent in the call manager,
-        // and allocaing into the AMT's buffer on finish.
-        let alloc = self.block_allocate.apply(stamped_event_size);
+        // Calculate the cost per copy (one memcpy + one allocation).
+        let mem =
+            self.block_memcpy.apply(estimated_size) + self.block_allocate.apply(estimated_size);
 
         // Charge for the hashing on AMT insertion.
-        let hash = self.hashing_cost[&SupportedHashes::Blake2b256].apply(stamped_event_size);
+        let hash = self.hashing_cost[&SupportedHashes::Blake2b256].apply(estimated_size);
 
         GasCharge::new(
-            "OnActorEventAccept",
-            memcpy + alloc,
-            self.event_accept_per_index_element.flat * indexed_elements
-                + self.event_accept_per_index_element.scale * indexed_bytes
-                + memcpy * 2u32 // deferred cost, placing here to hide from benchmark
-                + alloc // deferred cost, placing here to hide from benchmark
-                + hash, // deferred cost, placing here to hide from benchmark
+            "OnActorEvent",
+            // Charge for validation/storing events.
+            mem + validate_entries + validate_utf8,
+            // Charge for forming the AMT and returning the events to the client.
+            // one copy into the AMT, one copy to the client.
+            hash + (mem * 2u32),
         )
     }
 }

fvm/src/kernel/default.rs

@@ -17,7 +17,7 @@ use fvm_shared::consensus::ConsensusFault;
 use fvm_shared::crypto::signature;
 use fvm_shared::econ::TokenAmount;
 use fvm_shared::error::ErrorNumber;
-use fvm_shared::event::{ActorEvent, Entry, EntryFixed, Flags};
+use fvm_shared::event::{ActorEvent, Entry, Flags};
 use fvm_shared::piece::{zero_piece_commitment, PaddedPieceSize};
 use fvm_shared::sector::RegisteredPoStProof::{StackedDRGWindow32GiBV1, StackedDRGWindow32GiBV1P1};
 use fvm_shared::sector::{RegisteredPoStProof, SectorInfo};
@@ -1006,113 +1006,93 @@ impl<C> EventOps for DefaultKernel<C>
 where
     C: CallManager,
 {
-    fn emit_event(&mut self, raw_evt: &[u8], raw_key: &[u8], raw_val: &[u8]) -> Result<()> {
-        const MAX_NR_ENTRIES: usize = 256;
-        const MAX_KEY_LEN: usize = 32;
+    fn emit_event(
+        &mut self,
+        event_headers: &[fvm_shared::sys::EventEntry],
+        event_keys: &[u8],
+        event_values: &[u8],
+    ) -> Result<()> {
+        const MAX_NR_ENTRIES: usize = 255;
+        const MAX_KEY_LEN: usize = 31;
         const MAX_VALUE_LEN: usize = 8 << 10;
 
         if self.read_only {
             return Err(syscall_error!(ReadOnly; "cannot emit events while read-only").into());
         }
 
-        if raw_val.len() > MAX_VALUE_LEN {
-            return Err(
-                syscall_error!(IllegalArgument; "event total values exceeded max size: {} > {MAX_VALUE_LEN}", raw_val.len()).into(),
-            );
-        }
-
-        let total_buf_len = raw_evt.len() + raw_key.len() + raw_val.len();
-
-        let _t = self.call_manager.charge_gas(
-            self.call_manager
-                .price_list()
-                .on_actor_event_validate(total_buf_len),
-        )?;
-
-        const BYTES_PER_ENTRY: usize = 24;
-        if raw_evt.len() % BYTES_PER_ENTRY != 0 {
-            return Err(syscall_error!(IllegalArgument; "event not divisible by 24 bytes").into());
-        }
-        let nr_entries = raw_evt.len() / BYTES_PER_ENTRY;
-
-        if nr_entries > MAX_NR_ENTRIES {
-            return Err(syscall_error!(IllegalArgument; "event exceeded max entries: {} > {MAX_NR_ENTRIES}", nr_entries).into());
+        let t = self
+            .call_manager
+            .charge_gas(self.call_manager.price_list().on_actor_event(
+                event_headers.len(),
+                event_keys.len(),
+                event_values.len(),
+            ))?;
+
+        if event_headers.len() > MAX_NR_ENTRIES {
+            return Err(syscall_error!(IllegalArgument; "event exceeded max entries: {} > {MAX_NR_ENTRIES}", event_headers.len()).into());
         }
 
         let mut key_offset: usize = 0;
         let mut val_offset: usize = 0;
 
-        let mut entries: Vec<Entry> = Vec::with_capacity(nr_entries);
-        for i in 0..nr_entries {
-            let offset = i * BYTES_PER_ENTRY;
-            let view = &raw_evt[offset..offset + BYTES_PER_ENTRY];
-
-            // parse the fixed sized fields from the raw_evt buffer
-            //
-            let entry_fixed: EntryFixed = unsafe {
-                std::mem::transmute::<[u8; BYTES_PER_ENTRY], EntryFixed>(view.try_into().unwrap())
-            };
-
+        let mut entries: Vec<Entry> = Vec::with_capacity(event_headers.len());
+        for header in event_headers {
             // make sure that the fixed parsed values are within bounds before we do any allocation
-            //
-            let flags = entry_fixed.flags;
+            let flags = header.flags;
             if Flags::from_bits(flags.bits()).is_none() {
                 return Err(
                     syscall_error!(IllegalArgument; "event flags are invalid: {}", flags.bits())
                         .into(),
                 );
             }
-            if entry_fixed.key_len > MAX_KEY_LEN as u32 {
-                let tmp = entry_fixed.key_len;
+            if header.key_len > MAX_KEY_LEN as u32 {
+                let tmp = header.key_len;
                 return Err(syscall_error!(IllegalArgument; "event key exceeded max size: {} > {MAX_KEY_LEN}", tmp).into());
             }
-            if entry_fixed.val_len > MAX_VALUE_LEN as u32 {
-                let tmp = entry_fixed.val_len;
+            if header.val_len > MAX_VALUE_LEN as u32 {
+                let tmp = header.val_len;
                 return Err(syscall_error!(IllegalArgument; "event value exceeded max size: {} > {MAX_VALUE_LEN}", tmp).into());
             }
-            if entry_fixed.codec != IPLD_RAW {
-                let tmp = entry_fixed.codec;
+            if header.codec != IPLD_RAW {
+                let tmp = header.codec;
                 return Err(
                     syscall_error!(IllegalCodec; "event codec must be IPLD_RAW, was: {}", tmp)
                         .into(),
                 );
             }
 
             // parse the variable sized fields from the raw_key/raw_val buffers
-            //
-            let key = match std::str::from_utf8(
-                &raw_key[key_offset..key_offset + entry_fixed.key_len as usize],
-            ) {
-                Ok(s) => s,
-                Err(e) => {
-                    return Err(syscall_error!(IllegalArgument, "invalid event key: {}", e).into())
-                }
-            };
-            let value = &raw_val[val_offset..val_offset + entry_fixed.val_len as usize];
+            let key = &event_keys
+                .get(key_offset..key_offset + header.key_len as usize)
+                .context("event entry key out of range")
+                .or_illegal_argument()?;
+
+            let key = std::str::from_utf8(key)
+                .context("invalid event key")
+                .or_illegal_argument()?;
+
+            let value = &event_values
+                .get(val_offset..val_offset + header.val_len as usize)
+                .context("event entry value out of range")
+                .or_illegal_argument()?;
 
             // we have all we need to construct a new Entry
             let entry = Entry {
-                flags: entry_fixed.flags,
+                flags: header.flags,
                 key: key.to_string(),
-                codec: entry_fixed.codec,
+                codec: header.codec,
                 value: value.to_vec(),
             };
 
             // shift the key/value offsets
-            key_offset += entry_fixed.key_len as usize;
-            val_offset += entry_fixed.val_len as usize;
+            key_offset += header.key_len as usize;
+            val_offset += header.val_len as usize;
 
             entries.push(entry);
         }
 
         let actor_evt = ActorEvent::from(entries);
 
-        let t = self.call_manager.charge_gas(
-            self.call_manager
-                .price_list()
-                .on_actor_event_accept(&actor_evt, total_buf_len),
-        )?;
-
         let stamped_evt = StampedEvent::new(self.actor_id, actor_evt);
         self.call_manager.append_event(stamped_evt);
 

fvm/src/kernel/mod.rs

@@ -373,5 +373,10 @@ pub trait LimiterOps {
 /// Eventing APIs.
 pub trait EventOps {
     /// Records an event emitted throughout execution.
-    fn emit_event(&mut self, raw_evt: &[u8], raw_key: &[u8], raw_val: &[u8]) -> Result<()>;
+    fn emit_event(
+        &mut self,
+        event_headers: &[fvm_shared::sys::EventEntry],
+        raw_key: &[u8],
+        raw_val: &[u8],
+    ) -> Result<()>;
 }

fvm/src/syscalls/event.rs

@@ -1,14 +1,15 @@
 // Copyright 2021-2023 Protocol Labs
 // SPDX-License-Identifier: Apache-2.0, MIT
 
+use anyhow::Context as _;
+
 use super::Context;
-use crate::kernel::Result;
+use crate::kernel::{ClassifyResult, Result};
 use crate::Kernel;
 
 /// Emits an actor event. The event is split into three raw byte buffers that have
 /// been written to Wasm memory. This is done so that the FVM can accurately charge
 /// for gas without needing to parse anything inside the FVM.
-///
 /// The buffers are serialized as follows:
 ///  - event_off/event_len: The offset and length tuple of all the event entries
 ///       flags:u64,key_len:u32,codec:u64,value_len:u32)
@@ -32,8 +33,21 @@ pub fn emit_event(
     val_off: u32,
     val_len: u32,
 ) -> Result<()> {
-    let raw_event = context.memory.try_slice(event_off, event_len)?;
+    let event_headers = unsafe {
+        const EVENT_SIZE: u32 = std::mem::size_of::<fvm_shared::sys::EventEntry>() as u32;
+        // assert the alignment so we can safely cast from a byte-slice
+        static_assertions::assert_eq_align!(fvm_shared::sys::EventEntry, u8);
+        let size = event_len
+            .checked_mul(EVENT_SIZE)
+            .context("events index out of bounds")
+            .or_illegal_argument()?;
+        let buf = context.memory.try_slice(event_off, size)?;
+        std::slice::from_raw_parts(
+            buf.as_ptr() as *const fvm_shared::sys::EventEntry,
+            event_len as usize,
+        )
+    };
     let raw_key = context.memory.try_slice(key_off, key_len)?;
     let raw_val = context.memory.try_slice(val_off, val_len)?;
-    context.kernel.emit_event(raw_event, raw_key, raw_val)
+    context.kernel.emit_event(event_headers, raw_key, raw_val)
 }