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

Add zk opcodes #2439

Open
wants to merge 47 commits into
base: master
Choose a base branch
from
Open

Add zk opcodes #2439

wants to merge 47 commits into from

Conversation

AurelienFT
Copy link
Contributor

@AurelienFT AurelienFT commented Nov 14, 2024

Description

FuelLabs/fuel-specs#615

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

@AurelienFT AurelienFT changed the base branch from master to use-storage-read-with-offset November 14, 2024 12:50
@AurelienFT AurelienFT marked this pull request as ready for review December 4, 2024 10:49
@AurelienFT AurelienFT self-assigned this Dec 5, 2024
Base automatically changed from use-storage-read-with-offset to master December 23, 2024 18:41
@xgreenx xgreenx changed the base branch from master to feature/sync-version-with-minor-releases December 26, 2024 19:27
Base automatically changed from feature/sync-version-with-minor-releases to master December 31, 2024 00:08
AurelienFT and others added 3 commits January 2, 2025 01:36
## Linked Issues/PRs
#2237

## Description
This PR is on top of `add_zk_opcodes` so that the change of gas cost
will be efective with the new `GasCostsValueV5`. The benchmark was
incorrect because of
https://github.com/FuelLabs/fuel-core/blob/afbfef20fb78921cc6dc365878fb5b3a790ee769/crates/storage/src/vm_storage.rs#L291
. In benches the block_height was `0`. If we use the `DBBench` it works
because the block height used is 1. However, if we want to add blocks in
the db we also need the `DBBench` to use an higher block height. This PR
solves all of this.
This PR also simplify and comment the preparation code.

## Checklist
- [x] Breaking changes are clearly marked as such in the PR description
and changelog
- [x] New behavior is reflected in tests
- [x] [The specification](https://github.com/FuelLabs/fuel-specs/)
matches the implemented behavior (link update PR if changes are needed)

### Before requesting review
- [x] I have reviewed the code myself
- [x] I have created follow-up issues caused by this PR and linked them
here

### After merging, notify other teams

[Add or remove entries as needed]

- [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/)
- [ ] [Sway compiler](https://github.com/FuelLabs/sway/)
- [ ] [Platform
documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+)
(for out-of-organization contributors, the person merging the PR will do
this)
- [ ] Someone else?

---------

Co-authored-by: Green Baneling <[email protected]>
@AurelienFT AurelienFT requested a review from a team January 3, 2025 08:58
run_group_ref(
&mut c.benchmark_group("ecop"),
"ecop",
VmBench::new(op::ecop(0x10, 0x00, 0x01, 0x10))
Copy link
Member

Choose a reason for hiding this comment

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

can perhaps use RegId::ZERO instead of 0x00 here

run_group_ref(
&mut bench_epar,
format!("{i}"),
VmBench::new(op::epar(0x12, 0x00, 0x11, 0x10))
Copy link
Member

Choose a reason for hiding this comment

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

same, as prev comment

.with_data(points_bytearray)
.with_prepare_script(vec![
op::movi(0x11, i),
op::gtf_args(0x10, 0x00, GTFArgs::ScriptData),
Copy link
Member

Choose a reason for hiding this comment

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

same as prev comment

);
consensus_params.set_fee_params(FeeParameters::default().with_gas_per_byte(0));
consensus_params.set_gas_costs(GasCosts::free());
consensus_params.set_script_params(
ScriptParameters::default()
.with_max_script_length(64 * 1024 * 1024)
Copy link
Member

Choose a reason for hiding this comment

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

maybe 64*1024*1024 should be defined as a constant above and reused in this params

// Add block height to 0x11
op::movi(0x11, 0xc9),
])
.with_height(0xca.into())
Copy link
Member

Choose a reason for hiding this comment

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

random height chosen here? should it have been the latest height?

@@ -48,6 +48,19 @@ fn aloc_bytearray<const S: usize>(reg: u8, v: [u8; S]) -> Vec<Instruction> {
ops
}

/// Allocates a byte array of a variable size from heap and initializes it. Then points `reg` to it.
pub fn aloc_bytearray_vec(reg: u8, v: Vec<u8>) -> Vec<Instruction> {
Copy link
Member

Choose a reason for hiding this comment

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

couldn't find a usage of this function, should it be removed?

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.

4 participants