updated inner_call for hashing fn to follow generic approach #33128

sanjsingh07 · 2023-09-02T10:51:33Z

Problem
inner_call() function for hashing syscalls (sha256, keccak and blake3) are nearly identical

Summary of Changes
de-replicated the hashing syscalls to follow generic and trait approach, ref #33046.

dmakarov

LGTM

programs/bpf_loader/src/syscalls/mod.rs

t-nelson

now we're headed in the right direction! thanks for the changes

so I went with the default values of sha256 in compute_budget variables for the other hashes as well since I didn't know which values they are supposed to use as previously they were using the same ones as sha256 so if they need to be updated lmk.

hrm... wouldn't have expected that. we should definitely properly measure them, but that's out of scope fore this change set. i'd expect at leaset blake3 to be cheaper than sha256

program-runtime/src/compute_budget.rs

programs/bpf_loader/src/syscalls/mod.rs

t-nelson

almost there!

programs/bpf_loader/src/syscalls/mod.rs

t-nelson

ok. looks good to me know. let's see what the robot says!

programs/bpf_loader/src/syscalls/mod.rs

t-nelson · 2023-09-14T02:10:08Z

ok. looks good to me know. let's see what the robot says!

just realized you may not be able to view the ci output... looks like it's not liking trailing whitespace at programs/bpf_loader/src/syscalls/mod.rs:232. if you want to iterate faster, get ./ci/test-sanity.sh and ./ci/test-checks.sh passing locally. those are typically the bulk of the issues with refactors like this and pretty easy to run locally.

sanjsingh07 · 2023-09-14T06:18:59Z

ok. looks good to me know. let's see what the robot says!

just realized you may not be able to view the ci output... looks like it's not liking trailing whitespace at programs/bpf_loader/src/syscalls/mod.rs:232. if you want to iterate faster, get ./ci/test-sanity.sh and ./ci/test-checks.sh passing locally. those are typically the bulk of the issues with refactors like this and pretty easy to run locally.

Hey, I was able to see the ci output, and I ran cargo fmt -all to deal with the white space issue, cargo clippy to look for unused and reference related issue, which I've fixed/updated in new commits and also ./ci/test-sanity.sh ran completely fine but ./ci/test-checks.sh failed with below code snippet, and I tried I updating the hack sub-cmd with check which led to different error so you also might wanna check that out

+ exec cargo +nightly-2023-08-25 hack --version --verbose
error: no such command: `hack`

        Did you mean `check`?

        View all installed commands with `cargo --list`

t-nelson · 2023-09-14T20:47:58Z

ok. looks good to me know. let's see what the robot says!

just realized you may not be able to view the ci output... looks like it's not liking trailing whitespace at programs/bpf_loader/src/syscalls/mod.rs:232. if you want to iterate faster, get ./ci/test-sanity.sh and ./ci/test-checks.sh passing locally. those are typically the bulk of the issues with refactors like this and pretty easy to run locally.

Hey, I was able to see the ci output, and I ran cargo fmt -all to deal with the white space issue, cargo clippy to look for unused and reference related issue, which I've fixed/updated in new commits and also ./ci/test-sanity.sh ran completely fine but ./ci/test-checks.sh failed with below code snippet, and I tried I updating the hack sub-cmd with check which led to different error so you also might wanna check that out
+ exec cargo +nightly-2023-08-25 hack --version --verbose
error: no such command: `hack`

        Did you mean `check`?

        View all installed commands with `cargo --list`

ah you need to cargo install cargo-hack, i believe

codecov · 2023-09-14T21:47:11Z

Codecov Report

Merging #33128 (5ea1865) into master (4751199) will increase coverage by 0.0%.
Report is 2 commits behind head on master.
The diff coverage is 61.3%.

@@           Coverage Diff           @@
##           master   #33128   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         806      806           
  Lines      217676   217674    -2     
=======================================
+ Hits       178197   178202    +5     
+ Misses      39479    39472    -7

t-nelson · 2023-09-15T14:51:26Z

@sanjsingh07 you don't need to worry about the "this branch is outdated..." warning, so long as it doesn't escalate to "this branch conflicts...". rebasing clears ci, so we'll need to rerun to merge

@Lichtso @alessandrod can i get one of you to sign off on this again? it's ready to merge otherwise

sanjsingh07 · 2023-09-15T15:02:25Z

@sanjsingh07 you don't need to worry about the "this branch is outdated..." warning, so long as it doesn't escalate to "this branch conflicts...". rebasing clears ci, so we'll need to rerun to merge

@Lichtso @alessandrod can I get one of you to sign off on this again? it's ready to merge otherwise

Oh ok, Thanks for letting me know about that cuz I wasn't sure if it was mergeable with the branch being outdated.

t-nelson · 2023-09-15T19:51:18Z

Oh ok, Thanks for letting me know about that cuz I wasn't sure if it was mergeable with the branch being outdated.

no worries, github doesn't exactly make it obvious!

once we get another sign-off from the runtime team, this will be ready to go

t-nelson · 2023-10-10T20:24:44Z

@Lichtso @alessandrod @dmakarov can we get a final signoff from runtime on this? should be net better than the last time it was approved 😉

t-nelson · 2023-10-12T19:48:14Z

@sanjsingh07 looks like we picked up some trailing whitespace somewhere. ci sanity tests are failing

sanjsingh07 · 2023-10-12T20:03:17Z

@sanjsingh07 looks like we picked up some trailing whitespace somewhere. ci sanity tests are failing

yeah my bad, I might have left that while resolving the branch conflict, so do you want me to push the updated code or you can take care of it while merging?

alessandrod

lgtm! Please remove the whitespace at line 1951 then it's good to go

…et compute budget values

…omment form compute_budget

sanjsingh07 · 2023-10-13T09:57:47Z

lgtm! Please remove the whitespace at line 1951 then it's good to go

done!

t-nelson

we're r+ ci here. merge when green

t-nelson · 2023-10-13T21:03:05Z

finally made it! thanks for the contribution and patience with review responses!

* updated inner_call for hashing fn to follow generic approach * different hash compute budget values for all digests * fixed conflicts * reverted changes to compute_budget.rs and added 3method to trait to get compute budget values * updated type for result fn for HasherImpl * using Hash directly in result fn, got rid of HASH_BYTES and removed comment form compute_budget * updated import statement * cargo fmt -all * removed unused import and reference related warning * oops forgot semicolon * removed trailing white space (cherry picked from commit a60d185)

* updated inner_call for hashing fn to follow generic approach (#33128) * updated inner_call for hashing fn to follow generic approach * different hash compute budget values for all digests * fixed conflicts * reverted changes to compute_budget.rs and added 3method to trait to get compute budget values * updated type for result fn for HasherImpl * using Hash directly in result fn, got rid of HASH_BYTES and removed comment form compute_budget * updated import statement * cargo fmt -all * removed unused import and reference related warning * oops forgot semicolon * removed trailing white space (cherry picked from commit a60d185) * Bump solana_rbpf to v0.8.0 (#33679) * Bumps solana_rbpf to v0.8.0 * Adjustments: Replaces declare_syscall!() with declare_builtin_function!(). Removes Config::encrypt_runtime_environment. Simplifies error propagation. (cherry picked from commit a5c7c99) --------- Co-authored-by: Alexander Meißner <[email protected]>

mergify bot added community Community contribution need:merge-assist labels Sep 2, 2023

mergify bot requested a review from a team September 2, 2023 10:52

pgarg66 requested review from dmakarov and Lichtso September 5, 2023 15:08

dmakarov previously approved these changes Sep 5, 2023

View reviewed changes

t-nelson requested changes Sep 5, 2023

View reviewed changes

programs/bpf_loader/src/syscalls/mod.rs Outdated Show resolved Hide resolved

programs/bpf_loader/src/syscalls/mod.rs Outdated Show resolved Hide resolved

sanjsingh07 dismissed dmakarov’s stale review via 94b139c September 6, 2023 13:13

sanjsingh07 requested review from t-nelson and dmakarov September 6, 2023 13:14

t-nelson reviewed Sep 6, 2023

View reviewed changes

program-runtime/src/compute_budget.rs Outdated Show resolved Hide resolved

programs/bpf_loader/src/syscalls/mod.rs Outdated Show resolved Hide resolved

programs/bpf_loader/src/syscalls/mod.rs Outdated Show resolved Hide resolved

sanjsingh07 requested a review from t-nelson September 7, 2023 08:03

t-nelson reviewed Sep 12, 2023

View reviewed changes

programs/bpf_loader/src/syscalls/mod.rs Outdated Show resolved Hide resolved

sanjsingh07 requested a review from t-nelson September 13, 2023 06:25

t-nelson reviewed Sep 13, 2023

View reviewed changes

programs/bpf_loader/src/syscalls/mod.rs Outdated Show resolved Hide resolved

t-nelson added the CI Pull Request is ready to enter CI label Sep 13, 2023

solana-grimes removed the CI Pull Request is ready to enter CI label Sep 13, 2023

sanjsingh07 requested a review from t-nelson September 14, 2023 10:19

t-nelson added the CI Pull Request is ready to enter CI label Sep 14, 2023

solana-grimes removed the CI Pull Request is ready to enter CI label Sep 14, 2023

t-nelson added the CI Pull Request is ready to enter CI label Sep 15, 2023

solana-grimes removed the CI Pull Request is ready to enter CI label Sep 15, 2023

t-nelson closed this Sep 15, 2023

t-nelson mentioned this pull request Sep 27, 2023

replicode in hashing syscall implementations #33046

Closed

t-nelson added the CI Pull Request is ready to enter CI label Oct 10, 2023

solana-grimes removed the CI Pull Request is ready to enter CI label Oct 10, 2023

alessandrod self-requested a review October 13, 2023 07:04

alessandrod reviewed Oct 13, 2023

View reviewed changes

sanjsingh07 added 11 commits October 13, 2023 15:22

updated inner_call for hashing fn to follow generic approach

43cbe8d

different hash compute budget values for all digests

9356e31

fixed conflicts

d4df62b

reverted changes to compute_budget.rs and added 3method to trait to g…

13124c5

…et compute budget values

updated type for result fn for HasherImpl

c90b536

using Hash directly in result fn, got rid of HASH_BYTES and removed c…

5b2260d

…omment form compute_budget

updated import statement

d42d379

cargo fmt -all

551fef5

removed unused import and reference related warning

4d1dc30

oops forgot semicolon

7c0854d

removed trailing white space

5ea1865

t-nelson added the CI Pull Request is ready to enter CI label Oct 13, 2023

solana-grimes removed the CI Pull Request is ready to enter CI label Oct 13, 2023

t-nelson approved these changes Oct 13, 2023

View reviewed changes

t-nelson merged commit a60d185 into solana-labs:master Oct 13, 2023
16 checks passed

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore anza-xyz/agave#475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updated inner_call for hashing fn to follow generic approach #33128

updated inner_call for hashing fn to follow generic approach #33128

sanjsingh07 commented Sep 2, 2023

dmakarov left a comment

t-nelson left a comment

t-nelson left a comment

t-nelson left a comment

t-nelson commented Sep 14, 2023

sanjsingh07 commented Sep 14, 2023 •

edited

Loading

t-nelson commented Sep 14, 2023

codecov bot commented Sep 14, 2023 •

edited

Loading

t-nelson commented Sep 15, 2023

sanjsingh07 commented Sep 15, 2023

t-nelson commented Sep 15, 2023

t-nelson commented Oct 10, 2023

t-nelson commented Oct 12, 2023

sanjsingh07 commented Oct 12, 2023

alessandrod left a comment

sanjsingh07 commented Oct 13, 2023

t-nelson left a comment

t-nelson commented Oct 13, 2023

updated inner_call for hashing fn to follow generic approach #33128

updated inner_call for hashing fn to follow generic approach #33128

Conversation

sanjsingh07 commented Sep 2, 2023

dmakarov left a comment

Choose a reason for hiding this comment

t-nelson left a comment

Choose a reason for hiding this comment

t-nelson left a comment

Choose a reason for hiding this comment

t-nelson left a comment

Choose a reason for hiding this comment

t-nelson commented Sep 14, 2023

sanjsingh07 commented Sep 14, 2023 • edited Loading

t-nelson commented Sep 14, 2023

codecov bot commented Sep 14, 2023 • edited Loading

Codecov Report

t-nelson commented Sep 15, 2023

sanjsingh07 commented Sep 15, 2023

t-nelson commented Sep 15, 2023

t-nelson commented Oct 10, 2023

t-nelson commented Oct 12, 2023

sanjsingh07 commented Oct 12, 2023

alessandrod left a comment

Choose a reason for hiding this comment

sanjsingh07 commented Oct 13, 2023

t-nelson left a comment

Choose a reason for hiding this comment

t-nelson commented Oct 13, 2023

sanjsingh07 commented Sep 14, 2023 •

edited

Loading

codecov bot commented Sep 14, 2023 •

edited

Loading