Skip to content

Conversation

@Oscar-Pepper
Copy link
Contributor

@Oscar-Pepper Oscar-Pepper commented Oct 30, 2025

This bug was found during the chain index integration (#596 and #625) and this work allowed failing tests to then pass.

Motivation

Solution

Tests

Specifications & References

Follow-up Work

PR Checklist

  • The PR name is suitable for the release notes.
  • The solution is tested.
  • The documentation is up to date.

@Oscar-Pepper Oscar-Pepper added the Top Priority Current objectives and issues label Oct 31, 2025
Copy link
Member

@zancas zancas left a comment

Choose a reason for hiding this comment

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

I'm concerned about these byte order bugs. This is not the first time they've come up.

I don't understand how removing this explicit method, and directly exposing the .0 internal field PROVES that the bytes are in the correct order for the possible use contexts.

I don't know the correct solution to this issue, but it's a subtle and persistent issue, we should approach it with care.

@Oscar-Pepper
Copy link
Contributor Author

I'm concerned about these byte order bugs. This is not the first time they've come up.

I don't understand how removing this explicit method, and directly exposing the .0 internal field PROVES that the bytes are in the correct order for the possible use contexts.

I don't know the correct solution to this issue, but it's a subtle and persistent issue, we should approach it with care.

my understanding here (also based on responses from ECC engineers on this topic) is that we should only be reversing bytes during hex string encoding or decoding. I can not find anywhere else in the code base other than the places fixed in this PR that reverse bytes without immediately encoding/deciding to/from hex strings and this is the way it should be. IMO it is unecessary to add additional bloat to further complicate the reversal interface... we should just only use the provided exisitng trait methods for hex encoding/decoding with include the reversal and never reverse bytes directly. this may be secured by actually removing the byte reversal methods or making them private or less public.

This PR removes the only case of a direct reversal AFAIK so when this lands we will have consistency here and of course fix a bug blocking chain index integration

@Oscar-Pepper Oscar-Pepper requested a review from zancas November 5, 2025 12:54
@dorianvp dorianvp merged commit 7279237 into zingolabs:dev Nov 5, 2025
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Top Priority Current objectives and issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants