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

Unusually long padding of vector output of to_le_bytes. #916

Closed
2 of 4 tasks
Ethan-000 opened this issue Feb 25, 2023 · 10 comments
Closed
2 of 4 tasks

Unusually long padding of vector output of to_le_bytes. #916

Ethan-000 opened this issue Feb 25, 2023 · 10 comments
Assignees
Labels
bug Something isn't working

Comments

@Ethan-000
Copy link
Contributor

Ethan-000 commented Feb 25, 2023

Description

Unusually long padding of vector output of to_le_bytes.

Aim

Expected behavior

I'm not sure if this is intended or a bug. I think the expected length of the output vector should be 31 or as specified, but its254 at the moment.

Bug

To reproduce

see here

Environment

  • Using nargo:

    • Binary:
    • Compiled from source:
  • Using TypeScript:

    • @noir-lang/noir_wasm:
    • @noir-lang/barretenberg:
    • @noir-lang/aztec_backend:

Additional context

@Ethan-000 Ethan-000 added the bug Something isn't working label Feb 25, 2023
@Ethan-000 Ethan-000 changed the title The output of to_le_bytes is unusually long. The padding of vector output of to_le_bytes is unusually long. Feb 25, 2023
@Ethan-000 Ethan-000 changed the title The padding of vector output of to_le_bytes is unusually long. Unusually long padding of vector output of to_le_bytes. Feb 25, 2023
@kevaundray
Copy link
Contributor

@Ethan-000 is this still an issue?

@Ethan-000
Copy link
Contributor Author

@Ethan-000 is this still an issue?

seems so

@kevaundray
Copy link
Contributor

kevaundray commented Jul 23, 2023

@guipublic assigning this issue to you -- can you write a diagnosis and a solution before starting work?

@Ethan-000
Copy link
Contributor Author

@guipublic assigning this issue to you -- can you write a diagnosis and a solution before starting work?

wait I retried with --experimental-ssa, seems the padding looks fine now

@Ethan-000
Copy link
Contributor Author

its 32 with experiemental ssa so the program below

use dep::std;

fn main(x : Field) -> pub [u8; 32] {
    // The result of this byte array will be big-endian
    let byte_array = x.to_be_bytes(10);
    let mut bytes = [0; 32];
    for i in 0..32 {
        bytes[i] = byte_array[i];
    }
    bytes
}

will return

Circuit witness successfully solved
Circuit output: Vec([Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(31), Field(33), Field(60), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0)])

@Ethan-000
Copy link
Contributor Author

the output is in the middle I'm not sure if we want to leave it like this or remove the zeros after the answer

@kevaundray
Copy link
Contributor

As a side-task in this issue, I think we should document to_be_bytes and to_le_bytes, so users know what the 10` is meant to indicate. In general, documenting the stdlib as we go along is a good idea.

@kevaundray
Copy link
Contributor

@Ethan-000 re what we should do, can I leave that to you to create a rationale as to what you think is the most reasonable and non-suprising output?

@Ethan-000
Copy link
Contributor Author

Ethan-000 commented Jul 24, 2023

@Ethan-000 re what we should do, can I leave that to you to create a rationale as to what you think is the most reasonable and non-suprising output?

hmm i think byte_size api should probably be removed so that user can call to_be_bytes() that returns a 32 byte array.

This will be similar to the other apis but if a different kind of field element is used it may not be 32 bytes
maybe return max bytes the field element can decompose to and round to 64 or 32?

alternatively we can return the exact bytes the field element decompose to

@vezenovm
Copy link
Contributor

Following #2479 and #2350 padding has been removed and we simply return the specified byte size. I believe this issue can be closed @Ethan-000 correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants