-
Notifications
You must be signed in to change notification settings - Fork 165
zero-copy slot hashes sysvar (with checked alternatives) #152
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
zero-copy slot hashes sysvar (with checked alternatives) #152
Conversation
Update from upstream
…topian/pinocchio into rustopian/slot-hashes-sysvar
…topian/pinocchio into rustopian/slot-hashes-sysvar
| // Reject oversized entry counts to prevent surprises. | ||
| if num_entries > MAX_ENTRIES { | ||
| return Err(ProgramError::InvalidArgument); | ||
| } | ||
|
|
||
| let required_len = NUM_ENTRIES_SIZE + num_entries * ENTRY_SIZE; | ||
| if buffer.len() < required_len { | ||
| return Err(ProgramError::InvalidArgument); | ||
| } |
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.
I am not sure the need for this checks. I would assume that the syscall did the right thing, so num_entries would never be greater than MAX_ENTRIES and if the length was invalid, the syscall would fail.
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.
removed num_entries > MAX_ENTRIES check in 551b0f9
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.
Do we need the if buffer.len() < required_le check? This is after the syscall was called, so if the buffer length was invalid, the syscall would have failed.
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.
Removed. More importantly, as I double-checked the context here, I saw that cases involving offset would happily return an incorrect number of entries to the caller (reading slot number data as a num_entries header).
fetch_into and validate_buffer_size (now named validate_and_get_buffer_capacity) have been rewritten to fix this, plus some test updates and a new test test_fetch_into_offset_avoids_incorrect_entry_count
febo
left a comment
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.
Looks great, just very minor things.
Co-authored-by: Fernando Otero <[email protected]>
Co-authored-by: Fernando Otero <[email protected]>
Update from upstream
12ce68d to
99eb7da
Compare
Minor things addressed (and hunspell satisfied). |
6b6aca4 to
6ed4829
Compare
6ed4829 to
1d9cab0
Compare
|
Still hoping this will be merged... It looks done, does @febo need to review this one last time before merging? |
febo
left a comment
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.
Looks great, thanks!
Got your wish, it's in! (thanks febo) |
Adds SlotHashes sysvar support. Checked and unchecked zero-copy paths are both included. Intended to replace 121.
Binary search is used, but zerocopy
Iteratorand copyfetch/fetch_into/fetch_into_uncheckedare also implemented for those with different SlotHashes access needs.