Update SlotHashesSysvar API for more flexibility
#1948
Conversation
buffalojoec
force-pushed
the
slot-hashes-api-v2
branch
from
0cfa0bb to
eda2322
Compare
| // Finally, convert to a vector of `PodSlotHash`. | ||
| data.get(start..end) | ||
| .and_then(|data| bytemuck::try_cast_slice(data).ok()) | ||
| .map(|pod_hashes| pod_hashes.to_vec()) |
There was a problem hiding this comment.
I might be missing something, but I believe to_vec() will redo the huge allocation -- we should instead find a way to reuse the existing buffer that was already fetched to avoid blowing up the heap.
I initially thought that SlotHashesSysvar would hold onto the underlying Vec<u8> and that it would also expose a way to get the underlying PodSlotHash slice using something like as_slice(), which would do the bytemuck cast.
Otherwise, we'll need to do some unsafe stuff here to lop off the first 8 bytes and return the rest as Vec<PodSlotHash> without doing another allocation. Although, looking at the Vec interface, I think it'll be pretty gross do that unfortunately.
Any better ideas for this?
There was a problem hiding this comment.
How about doing a slice from raw parts for the initial mutable buffer, since we know it's a fixed size, and then that call .to_vec() should do just one heap allocation, since the original slice will be stack?
There was a problem hiding this comment.
Sorry, I might be misunderstanding -- we can't have a 20kb variable on the stack until dynamic stack frames are added, so what would the "initial mutable buffer" be?
There was a problem hiding this comment.
Ahh darn, yeah that probably won't work then. I think I see what you're getting at in your first message. I've added a commit that hangs onto the initial buffer, and then the commit right after uses as_slice. Let me know if this is what you had in mind.
| /// Get a value from the sysvar entries by its key. | ||
| /// Returns `None` if the key is not found. | ||
| pub fn get(&self, slot: &Slot) -> Result<Option<Hash>, ProgramError> { | ||
| self.get_pod_slot_hashes().map(|pod_hashes| { | ||
| pod_hashes | ||
| .binary_search_by(|PodSlotHash { slot: this, .. }| slot.cmp(this)) | ||
| .map(|idx| pod_hashes[idx].hash) | ||
| .ok() | ||
| }) | ||
| } | ||
|
|
||
| /// Get the position of an entry in the sysvar by its key. | ||
| /// Returns `None` if the key is not found. | ||
| pub fn position(&self, slot: &Slot) -> Result<Option<usize>, ProgramError> { | ||
| self.get_pod_slot_hashes().map(|pod_hashes| { | ||
| pod_hashes | ||
| .binary_search_by(|PodSlotHash { slot: this, .. }| slot.cmp(this)) | ||
| .ok() | ||
| }) | ||
| } |
There was a problem hiding this comment.
These, along with the addition of fields to the struct, are breaking changes. Do you plan to break and backport the breaking change? We're still doing a little bit of that, and since it's a new API, it should be OK if that's how you want to go.
If that's the case, how about making SlotHashesSysvar implement the Sysvar trait for get(), and then add the other functions as before?
Or, we could avoid a breaking change by creating a new PodSlotHashes struct and deprecate the old one, which would make a nice analogue to the normal SlotHashes. What do you think?
There was a problem hiding this comment.
Or, we could avoid a breaking change by creating a new
PodSlotHashesstruct and deprecate the old one, which would make a nice analogue to the normalSlotHashes. What do you think?
I think this is probably the better way to go. We can deprecate SlotHashesSysvar and add a PodSlotHashes that implements Sysvar and offers a similar API to SlotHashes. Nice idea!
I think it maybe makes more sense at this point to open a new PR where I can add the deprecation and new struct/API. Work for you?
Problem
The new
SlotHashesSysvarAPI - used to query slot hashes using the newsol_get_sysvarsyscall - works well, but there are a few limitations that can arise when testing on-chain
programs.
For starters, the creation of a
Vec<PodSlotHash>in the API's internals doesn't play wellwith the bump allocator. On-chain programs who wish to run multiple queries will easily
max out their heap size, since the vector won't deallocate. They'd benefit from being able
to access this created vector directly.
Secondly, when the slot hashes sysvar is smaller than its typical fixed-size - such as in
program-test scenarios - the new API won't work properly, since it expects the full
size.
And lastly, the previous implementation was initializing the
Vec<PodSlotHash>withPodSlotHash::default(), which yields a slot0for each entry,. This means that no matterwhat was in the slot hashes sysvar data, a query for slot
0would succeed and yield youthe entry from the middle of the vector.
Summary of Changes
First make
get_pod_slot_hashespublic, returning direct access to the vector.Then, redesign the
get_pod_slot_hashesmethod to first query the existing length ofthe sysvar data, before making another call to
get_sysvarto initiate a properly-sizedVec<PodSlotHash>.Finally, add some more test coverage.