feat: Improved event syscall API (#1807)

This PR changes the internal emit_event syscall and removes the CBOR formatting made between SDK and FVM. It does so by splitting the ActorEvent entries manually into three buffers that we know the exact size of and allows us to perform validation of certain cases (e.g check for max values) before doing any parsing.

Author: fridrik01

Cargo.lock

@@ -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/Cargo.toml

@@ -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.
@@ -921,59 +924,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 + some bytes
-        // for CBOR framing.
-        const STAMP_EXTRA_SIZE: usize = 12;
-        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/gas/price_list.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;
+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};
@@ -1004,42 +1004,99 @@ impl<C> EventOps for DefaultKernel<C>
 where
     C: CallManager,
 {
-    fn emit_event(&mut self, raw_evt: &[u8]) -> Result<()> {
+    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_TOTAL_VALUES_LEN: usize = 8 << 10;
+
         if self.read_only {
             return Err(syscall_error!(ReadOnly; "cannot emit events while read-only").into());
         }
-        let len = raw_evt.len();
+
         let t = self
             .call_manager
-            .charge_gas(self.call_manager.price_list().on_actor_event_validate(len))?;
+            .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());
+        }
 
-        // This is an over-estimation of the maximum event size, for safety. No valid event can even
-        // get close to this. We check this first so we don't try to decode a large event.
-        const MAX_ENCODED_SIZE: usize = 1 << 20;
-        if raw_evt.len() > MAX_ENCODED_SIZE {
-            return Err(syscall_error!(IllegalArgument; "event WAY too large").into());
+        // We check this here purely to detect/prevent integer overflows.
+        if event_values.len() > MAX_TOTAL_VALUES_LEN {
+            return Err(syscall_error!(IllegalArgument; "total event value lengths exceeded the max size: {} > {MAX_TOTAL_VALUES_LEN}", event_values.len()).into());
         }
 
-        let actor_evt = {
-            let res = match panic::catch_unwind(|| {
-                fvm_ipld_encoding::from_slice(raw_evt).or_error(ErrorNumber::IllegalArgument)
-            }) {
-                Ok(v) => v,
-                Err(e) => {
-                    log::error!("panic when decoding event cbor from actor: {:?}", e);
-                    Err(syscall_error!(IllegalArgument; "panic when decoding event cbor from actor").into())
-                }
+        let mut key_offset: usize = 0;
+        let mut val_offset: usize = 0;
+
+        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 = header.flags;
+            if Flags::from_bits(flags.bits()).is_none() {
+                return Err(
+                    syscall_error!(IllegalArgument; "event flags are invalid: {}", flags.bits())
+                        .into(),
+                );
+            }
+            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());
+            }
+            // We check this here purely to detect/prevent integer overflows.
+            if header.val_len > MAX_TOTAL_VALUES_LEN as u32 {
+                return Err(
+                    syscall_error!(IllegalArgument; "event entry value out of range").into(),
+                );
+            }
+            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 = &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: header.flags,
+                key: key.to_string(),
+                codec: header.codec,
+                value: value.to_vec(),
             };
-            t.stop();
-            res
-        }?;
-        validate_actor_event(&actor_evt)?;
 
-        let t = self.call_manager.charge_gas(
-            self.call_manager
-                .price_list()
-                .on_actor_event_accept(&actor_evt, len),
-        )?;
+            // shift the key/value offsets
+            key_offset += header.key_len as usize;
+            val_offset += header.val_len as usize;
+
+            entries.push(entry);
+        }
+
+        let actor_evt = ActorEvent::from(entries);
 
         let stamped_evt = StampedEvent::new(self.actor_id, actor_evt);
         self.call_manager.append_event(stamped_evt);
@@ -1060,35 +1117,6 @@ fn catch_and_log_panic<F: FnOnce() -> Result<R> + UnwindSafe, R>(context: &str,
     }
 }
 
-fn validate_actor_event(evt: &ActorEvent) -> Result<()> {
-    const MAX_ENTRIES: usize = 256;
-    const MAX_DATA: usize = 8 << 10;
-    const MAX_KEY_LEN: usize = 32;
-
-    if evt.entries.len() > MAX_ENTRIES {
-        return Err(syscall_error!(IllegalArgument; "event exceeded max entries: {} > {MAX_ENTRIES}", evt.entries.len()).into());
-    }
-    let mut total_value_size: usize = 0;
-    for entry in &evt.entries {
-        if entry.key.len() > MAX_KEY_LEN {
-            return Err(syscall_error!(IllegalArgument; "event key exceeded max size: {} > {MAX_KEY_LEN}", entry.key.len()).into());
-        }
-        if entry.codec != IPLD_RAW {
-            return Err(
-                syscall_error!(IllegalCodec; "event codec must be IPLD_RAW, was: {}", entry.codec)
-                    .into(),
-            );
-        }
-        total_value_size += entry.value.len();
-    }
-    if total_value_size > MAX_DATA {
-        return Err(
-            syscall_error!(IllegalArgument; "event total values exceeded max size: {total_value_size} > {MAX_DATA}").into(),
-        );
-    }
-    Ok(())
-}
-
 fn prover_id_from_u64(id: u64) -> ProverId {
     let mut prover_id = ProverId::default();
     let prover_bytes = Address::new_id(id).payload().to_raw_bytes();

fvm/src/kernel/default.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]) -> Result<()>;
+    fn emit_event(
+        &mut self,
+        event_headers: &[fvm_shared::sys::EventEntry],
+        raw_key: &[u8],
+        raw_val: &[u8],
+    ) -> Result<()>;
 }