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 modexp record gas #1290

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

zjb0807
Copy link
Contributor

@zjb0807 zjb0807 commented Jan 25, 2024

if mod_len == 0 {
return Ok(PrecompileOutput {
exit_status: ExitSucceed::Returned,
output: vec![],
});
}
// Gas formula allows arbitrary large exp_len when base and modulus are empty, so we need to handle empty base first.
let r = if base_len == 0 && mod_len == 0 {
handle.record_cost(MIN_GAS_COST)?;
BigUint::zero()
} else {

Line 185 will not be executed because it will return at Line 176

@zjb0807 zjb0807 requested a review from sorpaas as a code owner January 25, 2024 15:26
@zjb0807 zjb0807 marked this pull request as draft January 27, 2024 05:20
@zjb0807 zjb0807 marked this pull request as ready for review January 28, 2024 12:29
@zjb0807 zjb0807 requested a review from sorpaas February 19, 2024 02:37
@boundless-forest
Copy link
Collaborator

@nanocryk @notlesh Would you like to take a review about this?

Comment on lines 229 to 233
} else {
Err(PrecompileFailure::Error {
exit_status: ExitError::Other("failed".into()),
Ok(PrecompileOutput {
exit_status: ExitSucceed::Returned,
output: vec![],
})
Copy link
Contributor

Choose a reason for hiding this comment

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

This case seems unclear to me, it's basically when bytes.len() > mod_len, which shouldn't be possible except for this case of mod_len == 0. Maybe this is better written by handling that case specifically rather than assume it is the case in the final catch-all } else { block.

Also see the comment above this (line ~ 215) which is relevant:

// always true except in the case of zero-length modulus, which leads to
// output of length and value 1.

This seems to contradict the expectation of returning an empty vec.

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.

4 participants