Skip to content
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

implement SyscallGetSysvar #1307

Merged
merged 3 commits into from
May 16, 2024
Merged

Conversation

2501babe
Copy link
Member

@2501babe 2501babe commented May 13, 2024

Problem

we have proposed a syscall, sol_get_sysvar/SyscallGetSysvar, a generic mechanism to fetch arbitrary slices of account data for a specially selected list of sysvars. this is intended primarily to enable the bpf versions of the stake and vote programs to access individual parts of the stake history and slot hashes sysvars, but it also functions as a drop-in replacement for all existing non-deprecated sysvar syscalls

see discussion in SIMD-0127 for (much) more context

Summary of Changes

necessary SysvarCache changes for this pr are in #560

this pr introduces a new feature-gated syscall, SyscallGetSysvar, which accepts a sysvar id, length, offset, and destination buffer, and:

  • consumes compute units as defined by spec
  • gets the sysvar corresponding to that id if available
  • reads the bytes from the (cached) account data from length to length+offset into the destination

nothing presently uses this syscall. forthcoming prs intended for the same agave version will use it to implement fetching parts of stake history and slot hashes, though nothing will use them until the feature is active on all networks. also, in a future version, all existing sysvar syscalls can be deprecated, and the existing Sysvar::get macro can use this syscall instead

this pr has been made as simple as possible, in line with the simd that defines it, hence deferring the usage of the syscall into its own, third, pr (which should not require a featuregate itself)

Feature Gate Issue: #615

@2501babe 2501babe marked this pull request as draft May 13, 2024 09:07
@2501babe 2501babe added the feature-gate Pull Request adds or modifies a runtime feature gate label May 13, 2024
@2501babe 2501babe force-pushed the 20240513_sysvarget4 branch from 5a41911 to aab4e6e Compare May 13, 2024 16:03
@codecov-commenter
Copy link

codecov-commenter commented May 13, 2024

Codecov Report

Attention: Patch coverage is 84.95575% with 85 lines in your changes are missing coverage. Please review.

Project coverage is 81.6%. Comparing base (e3b55cf) to head (15d46c2).
Report is 38 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #1307    +/-   ##
========================================
  Coverage    81.5%    81.6%            
========================================
  Files         867      867            
  Lines      368866   369395   +529     
========================================
+ Hits       300978   301442   +464     
- Misses      67888    67953    +65     

@2501babe 2501babe marked this pull request as ready for review May 14, 2024 14:17
@2501babe 2501babe requested a review from buffalojoec May 14, 2024 14:17
Copy link

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, just one suggestion on the compute calculation.

I also feel like we should isolate the two syscall getter tests. Not only would this make it much clearer which syscall might be failing in a test, etc.s, but it also makes it much easier to deprecate the old syscalls and cut those tests out.

Comment on lines 193 to 197
.saturating_add(32_u64.div_ceil(cpi_bytes_per_unit))
.saturating_add(std::cmp::max(
length.div_ceil(cpi_bytes_per_unit),
mem_op_base_cost,
)),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These divisions should be floor. The SIMD just uses / (🙃) but FD previously mentioned floor was preferred.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated! as for the tests, i prefered to keep them together to test that both syscall results match the same "clean" object (and thus match each other). maybe i should keep them together but use separate memory mappings tho?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. Maybe the tests are ok as-is. I don't feel strongly about it, so up to you!

buffalojoec
buffalojoec previously approved these changes May 16, 2024
Copy link

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, nice work!

Comment on lines 3577 to 3587
// clone is to zero the alignment padding
let epochschedule_from_buf =
bincode::deserialize::<EpochSchedule>(&got_epochschedule_buf)
.unwrap()
.clone();

assert_eq!(epochschedule_from_buf, src_epochschedule);
assert!(are_bytes_equal(
&epochschedule_from_buf,
&clean_epochschedule
));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking: This first assertion would pass without the clone, right? It's only to have the bytes equal? Just trying to think how we need to handle padding in the API that we expose to programs.

Copy link
Member Author

@2501babe 2501babe May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes thats right, the objects are equal by the rules of rust, and the clone is just for comparing the memory mapping using unsafe. ill move the clone down so thats explicit

everything presently exposed to programs goes through get_sysvar, so theres no change:

    let var = translate_type_mut::<T>(memory_mapping, var_addr, check_aligned)?;

    // this clone looks unecessary now, but it exists to zero out trailing alignment bytes
    // it is unclear whether this should ever matter
    // but there are tests using MemoryMapping that expect to see this
    // we preserve the previous behavior out of an abundance of caution
    let sysvar: Arc<T> = sysvar?;
    *var = T::clone(sysvar.as_ref());

    Ok(SUCCESS)

when we transition to the new syscall we just need to preserve that behavior (or decide we dont need to carry it over. its very likely not to matter)

@2501babe 2501babe merged commit d8e9da5 into anza-xyz:master May 16, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-gate Pull Request adds or modifies a runtime feature gate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants