Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

fix modexp bug: return 0 if base is zero #6424

Merged
merged 1 commit into from
Sep 2, 2017
Merged

fix modexp bug: return 0 if base is zero #6424

merged 1 commit into from
Sep 2, 2017

Conversation

Hawstein
Copy link
Contributor

@Hawstein Hawstein commented Aug 31, 2017 •
edited
Loading

It currently returns result which is BigUint::one().

@parity-cla-bot
Copy link

It looks like @Hawstein signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@NikVolf NikVolf added the A8-looksgood 🦄 Pull request is reviewed well. label Aug 31, 2017
@NikVolf
Copy link
Contributor

NikVolf commented Aug 31, 2017

indeed

@NikVolf NikVolf added the M4-core ⛓ Core client code / Rust. label Aug 31, 2017
rphmeier
rphmeier reviewed Aug 31, 2017
View reviewed changes
ethcore/src/builtin.rs Outdated
@@ -309,7 +309,7 @@ impl Impl for ModexpImpl {
base = base % &modulus;

// fast path for base divisible by modulus.
if base.is_zero() { return result }
if base.is_zero() { return BigUint::zero() }
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this done here?

https://github.com/paritytech/parity/pull/6424/files#diff-f01cea79b5ea6391c260249a4cf801a5R303

and tested here

https://github.com/paritytech/parity/pull/6424/files#diff-f01cea79b5ea6391c260249a4cf801a5L660

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. the place you mentioned is the original base. But here the base is calculated via base % &modulus. See:

https://github.com/paritytech/parity/pull/6424/files#diff-f01cea79b5ea6391c260249a4cf801a5R309

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right. A test would be good

Copy link
Contributor Author

@Hawstein Hawstein Aug 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

But I think we can just move base = base % &modulus; up to the first line in this function. and remove the test here. All the condition will be tested at:

https://github.com/paritytech/parity/pull/6424/files#diff-f01cea79b5ea6391c260249a4cf801a5R303

WDYT? @rphmeier @NikVolf

Copy link
Contributor

Choose a reason for hiding this comment

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

Then it would panic if the modulo was zero, which is protected at the given point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, right. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am struggling for the test. There are more stuffs besides the modexp function. I can easily test the modexp function, however since it is just a inner function, I have to test the ModexpImpl::execute. I will be really appreciated if there are some instructions.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh just move the function outside to the mod level and add a test, there are plenty of support functions there already

@gavofyork
Copy link
Contributor

gavofyork commented Sep 2, 2017 •
edited
Loading

waiting for a test. @rphmeier note the end of the conversation:

I will be really appreciated if there are some instructions.

@gavofyork gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Sep 2, 2017
@Hawstein
Copy link
Contributor Author

Hawstein commented Sep 2, 2017

@gavofyork @NikVolf The PR is updated according to your suggestion.

@NikVolf NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Sep 2, 2017
@gavofyork gavofyork merged commit 1d95fe4 into openethereum:master Sep 2, 2017
@Hawstein Hawstein deleted the fix-modexp-bug branch September 3, 2017 02:36
arkpar pushed a commit that referenced this pull request Sep 16, 2017
@Hawstein @arkpar
fix modexp bug: return 0 if base=0 (#6424)
084ddd1
arkpar added a commit that referenced this pull request Sep 16, 2017
@arkpar
[beta] Byzantium updates (#6529)
bb311e8 
* fix modexp bug: return 0 if base=0 (#6424)

* Running state test using parity-evm (#6355)

* Initial version of state tests.

* Refactor state to support tracing.

* Unify TransactResult.

* Add test.

* Byzantium updates
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rphmeier rphmeier rphmeier left review comments

@NikVolf NikVolf NikVolf left review comments

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Hawstein @parity-cla-bot @NikVolf @gavofyork @rphmeier