bmresearch / Solnet

Solana's .NET SDK and integration library.
https://blockmountain.io/Solnet
MIT License
316 stars 128 forks source link

[Bug] DecodeRustString throw OutOfRangeException #276

Closed nazbrok closed 2 years ago

nazbrok commented 2 years ago

Describe the bug The latest release, within the Stake Program feature, introduced a change in the DecodeRustString method.

The code used to do the calculation versus the sizeof of a uint and not it's a ulong.

https://github.com/bmresearch/Solnet/blob/6a1f0d8a82c041b5db50082373aca569762c69b5/src/Solnet.Programs/Utilities/Deserialization.cs#L226-L235

I am since not able to decode properly a Rust String, I get a out of range exception.

If I use the old version, I can decode the string as expected

Any reason for this change ?

To Reproduce

Expected behavior Be able to decode a Rust string

tiago18c commented 2 years ago

@martinwhitman this was changed during your PR #251 . Was there any specific case 8 bytes were used for the string length instead of the 4?

I remember this was why I created a second function that reads the string size as a uint (4bytes). Maybe I missed this during code review.

hoakbuilds commented 2 years ago

This is also related to the latest discussion in #262 I believe. Borsh specifies the string encoding as being a u32 for the length and not a u64.

tiago18c commented 2 years ago

So, this is a case of borsh vs no borsh encoded strings?

If so, we need to make clear where each is used and rename functions accordingly.

tiago18c commented 2 years ago

@nazbrok I've been digging a bit now. So, the Solana runtime uses bincode as the binary encoder, which encodes a string as (u64,[u8]). Borsh does (u32,[u8]). What this means is that the changes @martinwhitman did are correct, and the previous code was actually wrong.

I'm going to make some changes in the code to make clear which read/write function does what, and make sure we have read & write overloads for both serialization mechanisms.

hoakbuilds commented 2 years ago

@tiago18c Because of both bincode and borsh we should end up with Encode/DecodeBorshString() and Encode/DecodeBincodeString(), seems like the logical way to go here?

martinwhitman commented 2 years ago

Seems correct in my caved-in-head wojak view.

On Mon, Feb 7, 2022 at 7:56 AM hoak @.***> wrote:

@tiago18c https://github.com/tiago18c Because of both bincode and borsh we should end up with Encode/DecodeBorshString() and Encode/DecodeBincodeString(), seems like the logical way to go here?

— Reply to this email directly, view it on GitHub https://github.com/bmresearch/Solnet/issues/276#issuecomment-1031556381, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKJIJUMHW4NOUBKMJ7PRXLTUZ7MSVANCNFSM5KVS2T4Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>