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.

Sign up for GitHub

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

Jump to bottom

refactor: random fixes and readability improvements #11129

Merged
merged 3 commits into from
Jan 17, 2025
Merged

refactor: random fixes and readability improvements #11129

merged 3 commits into from
Jan 17, 2025

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Jan 9, 2025 •
edited
Loading

In a PR down the stack I fixed safety warnings. When doing that I stumbled upon a random hodgepodge of issues. I am fixing them in this PR to not clutter the original one.

@benesjan Graphite App
Copy link
Contributor Author

benesjan commented Jan 9, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@benesjan benesjan mentioned this pull request Jan 9, 2025
Merged
@benesjan benesjan force-pushed the 01-07-chore_fixing_aztec-nr_warnings branch from 35f4b12 to b7b7b66 Compare January 9, 2025 15:27
@benesjan benesjan force-pushed the 01-09-refactor_random_fixes_and_readability_improvements branch from 8a4cad3 to 8f11ff7 Compare January 9, 2025 15:29
benesjan
benesjan commented Jan 9, 2025
View reviewed changes
noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/scheduled_delay_change.nr Outdated
+ (U128::from_integer(self.post.unwrap_unchecked()) << 32)
+ (U128::from_integer(self.block_of_change) << 64)
+ (U128::from_integer(self.post.is_some()) << 96)
+ (U128::from_integer(self.pre.is_some()) << 97);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Warnings fix. As discussed in a PR down the stack now I don't work with u64 limbs but I just pack directly. I needed to use U128 here to avoid overflows.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use Field directly? If Field doesn't support shifts then lets just multiply by the corresponding power of two. U128 will make this unnecessarily expensive.

Copy link
Contributor Author

@benesjan benesjan Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Decided to not tackle this for now because I will need access the the low and high limbs in my other PR..

For this reason this seemed like a waste of time and I have just reverted changes done to this file.

Asked Tom how to proceed here in DMs:

image

@benesjan benesjan added the e2e-all CI: Enables this CI job. label Jan 9, 2025
@benesjan benesjan marked this pull request as ready for review January 9, 2025 17:30
@benesjan benesjan requested a review from nventuro January 9, 2025 18:17
nventuro
nventuro reviewed Jan 9, 2025
View reviewed changes
noir-projects/aztec-nr/aztec/src/context/private_context.nr Show resolved Hide resolved
noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/scheduled_delay_change.nr Outdated
+ (U128::from_integer(self.post.unwrap_unchecked()) << 32)
+ (U128::from_integer(self.block_of_change) << 64)
+ (U128::from_integer(self.post.is_some()) << 96)
+ (U128::from_integer(self.pre.is_some()) << 97);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use Field directly? If Field doesn't support shifts then lets just multiply by the corresponding power of two. U128 will make this unnecessarily expensive.

noir-projects/aztec-nr/value-note/src/value_note.nr Outdated Show resolved Hide resolved
noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr Outdated Show resolved Hide resolved
noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/variable_merkle_tree.nr Outdated Show resolved Hide resolved
@benesjan benesjan marked this pull request as draft January 10, 2025 01:37
@benesjan benesjan mentioned this pull request Jan 10, 2025
Merged
@benesjan benesjan force-pushed the 01-07-chore_fixing_aztec-nr_warnings branch from c9df6c8 to 7e6e87e Compare January 10, 2025 21:43
@benesjan benesjan force-pushed the 01-09-refactor_random_fixes_and_readability_improvements branch from 2cd801b to 30efc51 Compare January 10, 2025 21:43
benesjan added a commit that referenced this pull request Jan 13, 2025
@benesjan
fix: underconstrained bug (#11174)
0b3088b 
Fixes underconstrained bug Nico spotted [here](#11129 (comment)).

### Note on severity
The bug could not be exploited because the `VariableMerkleTree` is only used when computing an `out_hash` [here](https://github.com/AztecProtocol/aztec-packages/blob/ccf28f56c408381867a4ac9435c5f0cc46690271/noir-projects/noir-protocol-circuits/crates/rollup-lib/src/components.nr#L128) and `out_hash` is checked on L1 against a hash computed on L1. For this reason the severity of the bug is low. But it makes sense to fix because it could become an issue in case the implementation would get used somewhere else later on.

The bug is explained in comments.
@benesjan benesjan force-pushed the 01-07-chore_fixing_aztec-nr_warnings branch from 7e6e87e to cbae56a Compare January 13, 2025 17:53
@benesjan benesjan force-pushed the 01-09-refactor_random_fixes_and_readability_improvements branch 2 times, most recently from e07e245 to b2d3266 Compare January 13, 2025 17:56
@benesjan benesjan force-pushed the 01-07-chore_fixing_aztec-nr_warnings branch from e1f8440 to fc91f1f Compare January 13, 2025 18:12
@benesjan benesjan force-pushed the 01-09-refactor_random_fixes_and_readability_improvements branch from e1f0f1e to 8349c06 Compare January 13, 2025 18:12
@benesjan benesjan force-pushed the 01-09-refactor_random_fixes_and_readability_improvements branch 2 times, most recently from 5f03dfa to 2676ce0 Compare January 13, 2025 18:27
@benesjan benesjan force-pushed the 01-07-chore_fixing_aztec-nr_warnings branch from 3e094bc to 80d9ab4 Compare January 13, 2025 19:14
@benesjan benesjan force-pushed the 01-09-refactor_random_fixes_and_readability_improvements branch from 2676ce0 to ad05ac9 Compare January 13, 2025 19:14
@benesjan benesjan force-pushed the 01-07-chore_fixing_aztec-nr_warnings branch 2 times, most recently from 609e9ad to daea821 Compare January 15, 2025 18:03
Base automatically changed from 01-07-chore_fixing_aztec-nr_warnings to master January 15, 2025 19:07
@benesjan benesjan force-pushed the 01-09-refactor_random_fixes_and_readability_improvements branch 2 times, most recently from ecb70d1 to 7acb1e4 Compare January 15, 2025 19:48
@benesjan benesjan marked this pull request as ready for review January 15, 2025 19:49
benesjan
benesjan commented Jan 15, 2025
View reviewed changes
noir-projects/noir-contracts/contracts/ecdsa_k_account_contract/src/main.nr
@@ -59,6 +59,8 @@ contract EcdsaKAccount {
let public_key = storage.public_key.get_note();

// Load auth witness
/// Safety: The witness is only used as a "magical value" that makes the signature verification below pass.
/// Hence it's safe.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to push this fix in a PR down the stack before it got merged.

benesjan
benesjan commented Jan 15, 2025
View reviewed changes
noir-projects/noir-contracts/contracts/ecdsa_r_account_contract/src/main.nr
@@ -57,6 +57,8 @@ contract EcdsaRAccount {
let public_key = storage.public_key.get_note();

// Load auth witness
/// Safety: The witness is only used as a "magical value" that makes the signature verification below pass.
/// Hence it's safe.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to push this fix in a PR down the stack before it got merged.

@benesjan benesjan requested review from nventuro and sklppy88 January 15, 2025 20:35
sklppy88
sklppy88 approved these changes Jan 17, 2025
View reviewed changes
Copy link
Contributor

@sklppy88 sklppy88 left a comment

Choose a reason for hiding this comment

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

👌

@benesjan benesjan force-pushed the 01-09-refactor_random_fixes_and_readability_improvements branch from 7acb1e4 to 4684eff Compare January 17, 2025 14:38
@benesjan benesjan enabled auto-merge (squash) January 17, 2025 14:39
@benesjan benesjan merged commit efab709 into master Jan 17, 2025
81 checks passed
@benesjan benesjan deleted the 01-09-refactor_random_fixes_and_readability_improvements branch January 17, 2025 15:28
@AztecBot AztecBot mentioned this pull request Jan 17, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomAFrench TomAFrench TomAFrench left review comments

@sklppy88 sklppy88 sklppy88 approved these changes

@nventuro nventuro Awaiting requested review from nventuro

Assignees
No one assigned
Labels
e2e-all CI: Enables this CI job.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@benesjan @nventuro @TomAFrench @sklppy88