Skip to content

Conversation

famouswizard
Copy link

the marker for big numbers was applied at the wrong place, which could mess up the most significant bit.
now it’s always set on the first element, so 256-bit numbers match Cairo’s little-endian format.
removed the unused start variable and updated test_encode_felts_to_u32s_big to check the marker.

@tdelabro
Copy link
Collaborator

tdelabro commented Sep 3, 2025

hey @famouswizard this is great!
Can you provide a test that prove that there was a bug. I confess that I struggle to get what is going on haha

@famouswizard
Copy link
Author

Can you provide a test that prove that there was a bug. I confess that I struggle to get what is going on haha

sure! the bug happens if the marker bit is applied to the wrong limb.
here’s a minimal failing case before the fix:

def test_encode_marker_position():
    # 256-bit number with only the highest bit set
    x = 1 << 255
    felts = [x]
    encoded = encode_felts_to_u32s_big(felts)

    # Expect the marker to be on the first u32 limb (little-endian format).
    # Before the fix, it was applied to the wrong one.
    assert encoded[0] >> 31 == 1
  • before the fix: this test fails, because the marker is applied to the wrong element.
  • after the fix: it passes, since the marker is always placed on the first limb, matching Cairo’s encoding.

@tdelabro
Copy link
Collaborator

tdelabro commented Sep 3, 2025

@famouswizard the CI is broken following your change. Maybe you need to patch other tests too

@gabrielbosio
Copy link
Collaborator

@famouswizard I have some comments on this PR:

I'm following the implementation from the Cairo VM in Python as a reference: Link. If you are following another implementation, please let me know about it.

I've found some differences between this implementation and the reference I used:

  • Endianness is different. This difference is also on main so it might be on purpose and therefore not important.
  • The marker should be added in the first byte (or last, depending on the endianness) of each encoded felt, not at the beginning of the entire vector: Link to diff.

Again, maybe this PR is using another implementation as reference, so this change might be necessary if the implementation used supersedes the one from the Python VM.

Also, I'm not sure why the comments in those tests are being removed instead of being rephrased (If changing the marker forces rephrasing those comments).

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.

3 participants