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

fix!: Change serialization of struct field order to match the user defined order #1166

Merged
merged 25 commits into from
May 25, 2023

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Apr 17, 2023

Related issue(s)

Resolves #1110

Description

Summary of changes

When serializing contracts, struct fields were previously serialized alphabetically. This change will serialize them in the order defined by users when declaring the struct instead.

Dependency additions / changes

Test additions / changes

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Documentation needs

  • This PR requires documentation updates when merged.

Additional context

@guipublic
Copy link
Contributor

Using a fixed serialisation pattern, like the alphabetical order for instance is more reliable and avoids user errors based on field struct ordering. I wonder if we could not find a way to keep a fix ordering?

@jfecher
Copy link
Contributor Author

jfecher commented Apr 19, 2023

Reassigning this to @kevaundray for the remaining errors.
Locally the poseidenperm test is failing, and a different test is failing on the CI.

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

Some comments from when I was looking through this earlier.

crates/noirc_abi/src/input_parser/mod.rs Outdated Show resolved Hide resolved
@kevaundray
Copy link
Contributor

For more context:

I added another example with a simple struct which also fails:

use dep::std;

 struct myStruct {
     foo: u32,
     bar: Field,
 }

 fn main(y : pub myStruct) {
     constrain y.foo == 5;
     constrain y.bar == 7;
 }

But if we re-order the struct to be in alphabetical order, it will pass like so:

 struct myStruct {
    bar: Field,
    foo: u32,
}

@jfecher
Copy link
Contributor Author

jfecher commented Apr 19, 2023

There must be a case I missed regarding the ordering. Perhaps during verification

@kevaundray
Copy link
Contributor

There must be a case I missed regarding the ordering. Perhaps during verification

Yeah looking to see if we forgot to change something from a BTreeMap

* master:
  chore: update flake version to match current release (#1204)
  feat!: Switch to aztec_backend that uses upstream BB & UltraPlonk (#1114)
  chore(ssa refactor): Add Context structs and start ssa gen pass (#1196)
  chore(ssa): Replace JmpIf with BrIf (#1193)
  chore(noir): Release 0.4.1 (#1164)
  chore(ssa refactor): Add DenseMap and SparseMap types (#1184)
  feat: bump noir-source-resolver version (#1182)
  chore(deps): bump h2 from 0.3.16 to 0.3.18 (#1186)
  fix(nargo): restore `nargo codegen-verifier` functionality (#1185)
  chore: simplify setup code in `noir_integration` test (#1180)
  feat: Add Poseidon-BN254 hash functions (#1176)
@TomAFrench
Copy link
Member

You haven't adjusted the ABI encoding code to use this new ordering logic. This means when constructing the initial witness you're reversing the two fields.

InputValue::Struct(object) => {
for value in object.into_values() {
encoded_value.extend(Self::encode_value(value)?);
}
}

* master: (66 commits)
  feat(nargo)!: retire print-acir in favour of flag (#1328)
  chore(ssa): enable cse for assert (#1350)
  chore(ssa refactor): Add basic instruction simplification (#1329)
  chore(noir): Release 0.6.0 (#1279)
  feat: enable to_radix for any field element (#1343)
  chore(ssa refactor): Simplify inlining pass and fix inlining failure (#1337)
  chore!: Update to acvm 0.11.0 (#1322)
  feat: Add ECDSA secp256k1 builtin test (#1294)
  chore: add support for encoding/decoding inputs from JSON (#1325)
  feat: Issue an error when attempting to use a `return` expression (#1330)
  chore(ssa refactor): Fix inlining bug (#1335)
  fix: to-bits and to-radix for > 128 bits (#1312)
  chore(parser): Parser error optimisation (#1292)
  chore(ssa refactor): Implement function inlining (#1293)
  chore: fix installation link in readme (#1326)
  chore: fix installation link in readme (#1326)
  feat(stdlib): Add keccak (#1249)
  fix: Parsing nested generics (#1319)
  chore(ssa refactor): Document some SSA-gen functions (#1321)
  fix: Assigning to tuple fields (#1318)
  ...
@TomAFrench
Copy link
Member

Should be good to go now.

@TomAFrench TomAFrench changed the title fix: change serialization of struct field order to match the user defined order fix: Change serialization of struct field order to match the user defined order May 17, 2023
@TomAFrench
Copy link
Member

Just noticed the panic in CI. Looking into it now

@TomAFrench
Copy link
Member

This may take some larger refactoring. It's pretty gross.

@kevaundray
Copy link
Contributor

Could you open up an issue re the hack and the larger refactoring needed

Copy link
Contributor

@kevaundray kevaundray left a comment

Choose a reason for hiding this comment

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

One comment re tracking the issues/hacks we have added, so we can fix them in the future

@TomAFrench TomAFrench changed the title fix: Change serialization of struct field order to match the user defined order fix!: Change serialization of struct field order to match the user defined order May 25, 2023
@TomAFrench TomAFrench added this pull request to the merge queue May 25, 2023
TomAFrench added a commit to noir-lang/docs that referenced this pull request May 25, 2023
Merged via the queue into master with commit 809aa3a May 25, 2023
@TomAFrench TomAFrench deleted the jf/fix-1110 branch May 25, 2023 04:37
critesjosh added a commit to noir-lang/docs that referenced this pull request May 25, 2023
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.

Make structs use the user defined order for mapping to witness indices
4 participants