feat: Improved event syscall API #1807

fridrik01 · 2023-07-05T12:54:43Z

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.

codecov-commenter · 2023-07-05T13:00:33Z

Codecov Report

Merging #1807 (912558d) into next (9c16cf5) will decrease coverage by 18.89%.
The diff coverage is 61.29%.

Additional details and impacted files

@@             Coverage Diff             @@
##             next    #1807       +/-   ##
===========================================
- Coverage   74.98%   56.10%   -18.89%     
===========================================
  Files         148      149        +1     
  Lines       14285    14632      +347     
===========================================
- Hits        10712     8209     -2503     
- Misses       3573     6423     +2850

Impacted Files	Coverage Δ
fvm/src/call_manager/default.rs	`0.00% <0.00%> (-88.58%)`	⬇️
fvm/src/gas/price_list.rs	`32.93% <ø> (-45.37%)`	⬇️
fvm/src/kernel/default.rs	`13.14% <0.00%> (-39.97%)`	⬇️
fvm/src/machine/default.rs	`50.54% <ø> (-21.33%)`	⬇️
fvm/src/syscalls/bind.rs	`0.00% <ø> (-94.88%)`	⬇️
fvm/src/syscalls/event.rs	`0.00% <0.00%> (-100.00%)`	⬇️
ipld/amt/src/lib.rs	`94.44% <ø> (ø)`
sdk/src/event.rs	`0.00% <0.00%> (ø)`
shared/src/event/mod.rs	`0.00% <ø> (-36.85%)`	⬇️
fvm/src/blockstore/buffered.rs	`80.58% <71.42%> (+6.30%)`	⬆️
... and 2 more

... and 53 files with indirect coverage changes

Stebalien · 2023-07-12T04:02:31Z

fvm/src/kernel/default.rs

+            let flags = match Flags::from_bits(BigEndian::read_u64(&view[..8])) {
+                Some(f) => f,
+                None => return Err(syscall_error!(IllegalArgument, "invalid flag").into()),
            };
-            t.stop();
-            res
-        }?;
-        validate_actor_event(&actor_evt)?;
+            let key_len = BigEndian::read_u32(&view[8..12]);
+            let codec = BigEndian::read_u64(&view[12..20]);
+            let val_len = BigEndian::read_u32(&view[20..24]);


Given that we use little-endian for return values, we should likely do the same here. I.e., we can literally transmute the input array into an array of packed structs.

// define this in shared #[repr(C, packed)] struct EventMeta { flags: Flags, // repr ut64 codec: u64, // order matters for packing. key_len: u32, value_len: u32, }

Although... we'll likely need to transmute one-by-one and copy as the input slice may not be aligned.

This is less critical on the FVM side, but it'll save time and avoid swapping around bytes in wasm.

Note: Flags will have to be decorated with #[repr(transparent)] for this to work.

Stebalien · 2023-07-12T04:22:23Z

fvm/src/kernel/default.rs

+        let _t = self.call_manager.charge_gas(
+            self.call_manager
+                .price_list()
+                .on_actor_event_validate(total_buf_len),


we'll likely need to revisit the costs here, but we can do that in a followup PR.

fvm/src/kernel/default.rs

Stebalien · 2023-07-12T14:40:16Z

fvm/src/kernel/default.rs

+                &raw_key[key_offset..key_offset + entry_fixed.key_len as usize],
+            ) {
+                Ok(s) => s,
+                Err(_) => return Err(syscall_error!(IllegalArgument, "event key").into()),


Suggested change

Err(_) => return Err(syscall_error!(IllegalArgument, "event key").into()),

Err(e) => return Err(syscall_error!(IllegalArgument, "invalid event key: {}", e).into()),

(or something like that)

Stebalien · 2023-07-12T14:44:44Z

sdk/src/event.rs

+        LittleEndian::write_u64(&mut view[..8], e.flags.bits());
+        LittleEndian::write_u64(&mut view[8..16], e.codec);
+        LittleEndian::write_u32(&mut view[16..20], e.key.len() as u32);
+        LittleEndian::write_u32(&mut view[20..24], e.value.len() as u32);


We should be able to use transmutes here as well. As a matter of fact, we should be able to just:

Make an array of EntryFixed.

Transmute by-reference.

....

Actually, we should be able to just take *mut EventFixed (pointer to an array) in the syscall ABI itself and let rust do the transmute for us. Welcome to C!

Stebalien · 2023-07-12T16:17:14Z

sdk/src/event.rs

+    let mut keys = vec![0u8; total_key_len];
+    let mut offset: usize = 0;
+    for i in 0..evt.entries.len() {
+        let e = &evt.entries[i];
+        keys[offset..offset + e.key.len()].copy_from_slice(e.key.as_bytes());
+        offset += e.key.len();
+    }


Suggested change

let mut keys = vec![0u8; total_key_len];

let mut offset: usize = 0;

for i in 0..evt.entries.len() {

let e = &evt.entries[i];

keys[offset..offset + e.key.len()].copy_from_slice(e.key.as_bytes());

offset += e.key.len();

}

let mut keys = Vec::with_capacity(total_key_len);

let mut offset: usize = 0;

for i in 0..evt.entries.len() {

let e = &evt.entries[i];

keys.extend_from_slice(e.key.as_bytes());

offset += e.key.len();

}

(saves us from zeroing first)

Same below.

* 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. * fix value length check

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.

fridrik01 force-pushed the 1637-improved-event-syscall branch 4 times, most recently from 9babe6b to 819c200 Compare July 5, 2023 15:16

fridrik01 changed the base branch from master to next July 5, 2023 16:34

fridrik01 changed the title ~~[WIP] feat: Improved event syscall API~~ feat: Improved event syscall API Jul 5, 2023

fridrik01 force-pushed the 1637-improved-event-syscall branch from 819c200 to b5cfb97 Compare July 10, 2023 13:58

fridrik01 requested a review from Stebalien July 11, 2023 13:53

fridrik01 force-pushed the 1637-improved-event-syscall branch from b5cfb97 to fc91b20 Compare July 11, 2023 13:53

fridrik01 marked this pull request as ready for review July 11, 2023 13:54

Stebalien reviewed Jul 12, 2023

View reviewed changes

fvm/src/kernel/default.rs Outdated Show resolved Hide resolved

Stebalien reviewed Jul 12, 2023

View reviewed changes

fridrik01 force-pushed the 1637-improved-event-syscall branch from e376831 to f43049f Compare July 12, 2023 15:56

Stebalien reviewed Jul 12, 2023

View reviewed changes

fridrik01 force-pushed the 1637-improved-event-syscall branch 2 times, most recently from cc3263f to e003ef5 Compare July 12, 2023 17:22

Stebalien mentioned this pull request Jul 17, 2023

Implement actor events from the verified registry filecoin-project/builtin-actors#1320

Closed

Stebalien force-pushed the 1637-improved-event-syscall branch from e003ef5 to 912558d Compare July 20, 2023 17:21

fridrik01 and others added 3 commits July 24, 2023 12:29

feat: Improved event syscall API

c343517

Perform raw copy of fixed entries between sdk and kernel

1d20080

Stebalien force-pushed the 1637-improved-event-syscall branch from 3690728 to 7cc50fa Compare July 24, 2023 19:29

Stebalien merged commit f2ad893 into next Jul 24, 2023
3 checks passed

Stebalien deleted the 1637-improved-event-syscall branch July 24, 2023 19:30

fridrik01 mentioned this pull request Aug 14, 2023

fix: use ptr::null instead of casting 0 to a pointer #1841

Merged

fridrik01 mentioned this pull request Aug 15, 2023

FIP-XXXX: Improved event syscall API filecoin-project/FIPs#784

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Improved event syscall API #1807

feat: Improved event syscall API #1807

fridrik01 commented Jul 5, 2023 •

edited

Loading

codecov-commenter commented Jul 5, 2023 •

edited

Loading

Stebalien Jul 12, 2023

Stebalien Jul 12, 2023

Stebalien Jul 12, 2023

Stebalien Jul 12, 2023

Stebalien Jul 12, 2023

Stebalien Jul 12, 2023

Stebalien Jul 12, 2023

Stebalien Jul 12, 2023

Stebalien Jul 12, 2023

	Err(_) => return Err(syscall_error!(IllegalArgument, "event key").into()),
	Err(e) => return Err(syscall_error!(IllegalArgument, "invalid event key: {}", e).into()),

feat: Improved event syscall API #1807

feat: Improved event syscall API #1807

Conversation

fridrik01 commented Jul 5, 2023 • edited Loading

codecov-commenter commented Jul 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fridrik01 commented Jul 5, 2023 •

edited

Loading

codecov-commenter commented Jul 5, 2023 •

edited

Loading