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

masp: move to native VP #2051

Merged
merged 8 commits into from
Nov 21, 2023
Merged

masp: move to native VP #2051

merged 8 commits into from
Nov 21, 2023

Conversation

juped
Copy link
Member

@juped juped commented Oct 26, 2023

Describe your changes

Closes #1924.

Moves the masp vp to native. Removes the now useless host env for masp validation and updates gas accounting and cost. Updated the masp vp benchmarks.

Indicate on which release or other PRs this topic is based on

commit 3a0ecd8 (v0.25.0)

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@juped juped force-pushed the ray/native-masp branch 7 times, most recently from 3510fcf to 78cd5f0 Compare October 27, 2023 10:48
@juped juped marked this pull request as ready for review October 27, 2023 10:49
@juped juped requested a review from murisi October 27, 2023 10:52
@cwgoes cwgoes mentioned this pull request Nov 5, 2023
7 tasks
@grarco grarco self-requested a review November 9, 2023 10:50
@@ -466,94 +463,5 @@ fn vp_validator(c: &mut Criterion) {
group.finish();
}

fn vp_masp(c: &mut Criterion) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move this benchmark to benches/native_vps

_ => {}
}
// Verify the proofs.
Ok(verify_shielded_tx(&shielded_tx))
Copy link
Contributor

@grarco grarco Nov 9, 2023

Choose a reason for hiding this comment

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

It would be nice to benchmark this function (I think this is the most computationally expensive one) and charge some gas before calling it in here

@grarco
Copy link
Contributor

grarco commented Nov 9, 2023

"namada_vp_verify_masp" => Function::new_native_with_env(wasm_store, env.clone(), host_env::vp_verify_masp),

I believe we don't need this host fn anymore

return Err(Error::from(TxError::InvalidAccount(owner.encode())));
Some(owner @ Address::Internal(internal)) => {
match internal {
// TODO use sentinel?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need a signature on the transaction at all if the source is the MASP cause no VP would check it anyway

grarco added a commit that referenced this pull request Nov 10, 2023
@grarco grarco requested a review from cwgoes November 10, 2023 13:07
@grarco grarco mentioned this pull request Nov 10, 2023
grarco added a commit that referenced this pull request Nov 10, 2023
grarco added a commit that referenced this pull request Nov 10, 2023
grarco added a commit that referenced this pull request Nov 10, 2023
@grarco grarco self-requested a review November 10, 2023 18:51
@adrianbrink adrianbrink mentioned this pull request Nov 11, 2023
adrianbrink added a commit that referenced this pull request Nov 14, 2023
* ray/native-masp:
  Changelog #2051
  Updates masp test proofs
  Removes unused masp check from `vp_user`
  Updates benches with masp native vp
  Removes masp vp from genesis files
  Moves masp vp to native
adrianbrink added a commit that referenced this pull request Nov 14, 2023
* ray/native-masp:
  Changelog #2051
  Updates masp test proofs
  Removes unused masp check from `vp_user`
  Updates benches with masp native vp
  Removes masp vp from genesis files
  Moves masp vp to native
@adrianbrink adrianbrink mentioned this pull request Nov 14, 2023
grarco added a commit that referenced this pull request Nov 14, 2023
Fraccaman pushed a commit that referenced this pull request Nov 20, 2023
* origin/ray/native-masp:
  Updates masp proofs for testing
  Updates alias for masp native vp
  Changelog #2051
  Updates masp test proofs
  Removes unused masp check from `vp_user`
  Updates benches with masp native vp
  Removes masp vp from genesis files
  Moves masp vp to native
Fraccaman pushed a commit that referenced this pull request Nov 20, 2023
* origin/ray/native-masp:
  Updates masp proofs for testing
  Updates alias for masp native vp
  Changelog #2051
  Updates masp test proofs
  Removes unused masp check from `vp_user`
  Updates benches with masp native vp
  Removes masp vp from genesis files
  Moves masp vp to native
Fraccaman pushed a commit that referenced this pull request Nov 20, 2023
* origin/ray/native-masp:
  Updates masp proofs for testing
  Updates alias for masp native vp
  Changelog #2051
  Updates masp test proofs
  Removes unused masp check from `vp_user`
  Updates benches with masp native vp
  Removes masp vp from genesis files
  Moves masp vp to native
Fraccaman pushed a commit that referenced this pull request Nov 20, 2023
* origin/ray/native-masp:
  Updates masp proofs for testing
  Updates alias for masp native vp
  Changelog #2051
  Updates masp test proofs
  Removes unused masp check from `vp_user`
  Updates benches with masp native vp
  Removes masp vp from genesis files
  Moves masp vp to native
@tzemanovic tzemanovic merged commit ad6a789 into main Nov 21, 2023
12 of 14 checks passed
@tzemanovic tzemanovic deleted the ray/native-masp branch November 21, 2023 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Native VPs should account for cpu cycles spent during validation
3 participants