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

bank: do not remove trailing 0 bytes from return data #33639

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

seanyoung
Copy link
Contributor

@seanyoung seanyoung commented Oct 10, 2023

This is creating havoc for Solang, as the return data is borsh encoded and therefore u64 values like 0x100 get truncated.

Problem

When Solidity returns a borsh encoded return values which end in trailing zeros, then this gets mangled. For example,
a single bool value false should be a single 0 byte but this get removed. 0x100u64 has its last byte removed.

Summary of Changes

Do not remove trailing 0 bytes. There is no need for this.

Fixes #31391

@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Merging #33639 (6b9421a) into master (2f090a5) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #33639   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         806      806           
  Lines      217477   217473    -4     
=======================================
+ Hits       177999   178006    +7     
+ Misses      39478    39467   -11     

This is creating havoc for Solang, as the return data is borsh encoded
and therefore `u64` values like 0x100 get truncated.
@seanyoung seanyoung requested a review from joncinque October 11, 2023 11:36
@seanyoung seanyoung marked this pull request as ready for review October 11, 2023 11:36
@seanyoung seanyoung added the v1.17 PRs that should be backported to v1.17 label Oct 11, 2023
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

This change looks good to me. Since it only affects nodes that record return data, it shouldn't have any impact on consensus, which makes it safe as is.

I honestly can't remember why I thought this was a good idea. Maybe I thought everything was just arrays, so it's impossible to tell how much has been written. But when I look at the originating PR #23688, everything is Vec there too. So the truncation behavior is only destructive.

I also requested a review from @CriesofCarrots to double-check that this makes sense.

@seanyoung
Copy link
Contributor Author

Thank for the review @joncinque!

How do you feel about merging it to v1.16?

@joncinque
Copy link
Contributor

How do you feel about merging it to v1.16?

I would prefer to keep it out of 1.16, since that's running mainnet. With 1.17, we give it some time to soak and let people adapt to the upcoming change.

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

I honestly can't remember why I thought this was a good idea.
Maybe concern with space over the wire for RPC calls? 🤷‍♀️

Anyway, I'm fine with this.

@seanyoung
Copy link
Contributor Author

How do you feel about merging it to v1.16?

I would prefer to keep it out of 1.16, since that's running mainnet. With 1.17, we give it some time to soak and let people adapt to the upcoming change.

Makes sense.

@seanyoung seanyoung merged commit 4751199 into solana-labs:master Oct 13, 2023
16 checks passed
@seanyoung seanyoung deleted the rpc branch October 13, 2023 07:00
mergify bot pushed a commit that referenced this pull request Oct 13, 2023
This is creating havoc for Solang, as the return data is borsh encoded
and therefore `u64` values like 0x100 get truncated.

(cherry picked from commit 4751199)
seanyoung added a commit that referenced this pull request Oct 13, 2023
…t of #33639) (#33685)

bank: do not remove trailing 0 bytes from return data (#33639)

This is creating havoc for Solang, as the return data is borsh encoded
and therefore `u64` values like 0x100 get truncated.

(cherry picked from commit 4751199)

Co-authored-by: Sean Young <[email protected]>
@tkporter
Copy link

Very happy to see this change, thank you @seanyoung :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.17 PRs that should be backported to v1.17
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] simulate transaction - return data truncates 0 trailing zero bytes
4 participants