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 my sloppy oversight of asUint8 missing unchecked suffix in asBool… #25

Merged
merged 3 commits into from
Jan 16, 2024

Conversation

nonergodic
Copy link
Collaborator

@nonergodic nonergodic commented Jan 11, 2024

I made sloppy mistakes in my proposal here by using asUint8 instead of asUint8Unchecked.

Additionally, as it turns out, the inline assembly is not safe because solc is actually no cleaning up the upper 31 bytes of val (as I expected it would), so there's no way to avoid the cost of the two ISZERO opcodes after all. I missed that because it happened to insert cleanup instructions in my Playground example, which I believed to be equivalent but then it wasn't...

@nonergodic nonergodic mentioned this pull request Jan 11, 2024
@nonergodic nonergodic requested a review from nik-suri January 11, 2024 10:19
@kcsongor
Copy link
Contributor

kcsongor commented Jan 11, 2024

if val is not cleaned up then val != 0 (assuming it only operates on the last byte, otherwise it would be incorrect) involves a mask in addition to the ISZERO check right? we might as well do the cleaning mask and keep the assembly assignment to at least save the ISZERO check

@kcsongor
Copy link
Contributor

kcsongor commented Jan 11, 2024

also, if solc doesn't assume that the variables are cleaned up anyway, then what's wrong with returning a bool that's not cleaned up?
(maybe there's an assumption that functions return cleaned words? I need to look this up)

@nonergodic
Copy link
Collaborator Author

if val is not cleaned up then val != 0 (assuming it only operates on the last byte, otherwise it would be incorrect) involves a mask in addition to the ISZERO check right? we might as well do the cleaning mask and keep the assembly assignment to at least save the ISZERO check

That's a good point, I hadn't considered that the cleaning will also require masking. My first way to fix it was

uint valAsUint = uint(val);
bool ret;
assembly ("memory-safe") {
  ret := valAsUint
}

But I figured not only was it less readable (and more error prone) but also 6 gas (1 PUSH, 1 AND) just like 2 ISZERO (and 3 bytes in deployed opcode (PUSH 0xFF AND) vs just 2).

But given what you're saying, perhaps this is the preferred impl after all?

@nonergodic
Copy link
Collaborator Author

Ok, pushed the above implementation (tested it to make sure it works as intended, but didn't look at the assembly)

@nonergodic nonergodic merged commit d3afb99 into main Jan 16, 2024
1 check passed
@nonergodic nonergodic deleted the fix-asBoolUnchecked-oversight branch January 16, 2024 15:38
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.

2 participants