-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Improved event syscall API #1807
Conversation
Codecov Report
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
|
9babe6b
to
819c200
Compare
819c200
to
b5cfb97
Compare
b5cfb97
to
fc91b20
Compare
fvm/src/kernel/default.rs
Outdated
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]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is less critical on the FVM side, but it'll save time and avoid swapping around bytes in wasm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Flags
will have to be decorated with #[repr(transparent)]
for this to work.
fvm/src/kernel/default.rs
Outdated
let _t = self.call_manager.charge_gas( | ||
self.call_manager | ||
.price_list() | ||
.on_actor_event_validate(total_buf_len), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll likely need to revisit the costs here, but we can do that in a followup PR.
fvm/src/kernel/default.rs
Outdated
&raw_key[key_offset..key_offset + entry_fixed.key_len as usize], | ||
) { | ||
Ok(s) => s, | ||
Err(_) => return Err(syscall_error!(IllegalArgument, "event key").into()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
sdk/src/event.rs
Outdated
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
e376831
to
f43049f
Compare
sdk/src/event.rs
Outdated
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(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same below.
cc3263f
to
e003ef5
Compare
e003ef5
to
912558d
Compare
* 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
3690728
to
7cc50fa
Compare
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.
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.
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.
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.
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.
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.
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.
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.
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.
Fixes: #1637
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.