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 [iu](128|256) BSATN implementations in C# #1582

Merged
merged 7 commits into from
Aug 13, 2024
Merged

Conversation

RReverser
Copy link
Contributor

@RReverser RReverser commented Aug 13, 2024

Description of Changes

128- and, as of recently, 256-bit integers were implemented incorrectly in C#. They would write upper half of the number first, followed by the lower part, making the resulting encoding mixed-endian.

Unfortunately, this flew under the radar because SDK tests only verify round-trip: reducer arguments are passed in via BSATN encoding, and the table events are also retrieved in BSATN encoding.

As a result, if any encoding implementation is incorrect in an idempotent way - that is, you can only observe the bug from inside a module via stringification or arithmetic operations, but roundtrip restores the correct value - the SDK test doesn't see anything wrong with it, as what happened here.

I added one more test that, instead of round-tripping, sends values to the module and expects it to stringify them and insert strings into a table. This allows us to see the deserialized values as the module does, without going back via the original BSATN.

To implement this, I added proper stringification of large integers to C#, going via BigInteger conversion, which can be somewhat costly, but unlikely to matter in ToString().

I also extended sample integers in the primitive struct example to fill all bytes, as previously one half of int128/int256 would be unused and stay zero, which can also hide potential issues.

I want to extract common parts later and perhaps extend tests to do real property testing in C# instead of simply hardcoding values, but for now I'm satisfied this particular issue is fixed so wanted to submit as a separate PR.

API and ABI breaking changes

If this is an API or ABI breaking change, please apply the
corresponding GitHub label.

Expected complexity level and risk

How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.

This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.

If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!

  • Write a test you've completed here.
  • Write a test you want a reviewer to do here, so they can check it off when they're satisfied.

@RReverser RReverser added release-any To be landed in any release window bugfix Fixes something that was expected to work differently labels Aug 13, 2024
@RReverser RReverser requested review from Centril, lcodes and gefjon August 13, 2024 11:10
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

Have looked at only the tests, but those look good.

@RReverser RReverser added this pull request to the merge queue Aug 13, 2024
Merged via the queue into master with commit 01d4ed1 Aug 13, 2024
8 of 9 checks passed
@RReverser RReverser deleted the ingvar/256-fixups branch August 13, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes something that was expected to work differently release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants