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

Use BN reduction context in modexp precompile #463

Merged
merged 1 commit into from
Mar 11, 2019
Merged

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Mar 5, 2019

This PR attempts to fix #214. I tried to use BN.mont explicitly, but got some weird result:

new BN(1).toRed(BN.mont(new BN('64', 16))).fromRed() which I thought should return 0x1, actually returns 0x18! Not sure if this is a bug or intended?

Should I do benchmarks to make sure this is actually more performant?

Update: It's running out of heap memory on modexp_37120_37111_1_1000000

@s1na s1na changed the title Use BN reduction context in modexp precompile [WIP] Use BN reduction context in modexp precompile Mar 5, 2019
@s1na s1na force-pushed the modexp-reduct branch 2 times, most recently from 73e5087 to 8453ed8 Compare March 5, 2019 21:46
@coveralls
Copy link

coveralls commented Mar 5, 2019

Coverage Status

Coverage remained the same at 94.657% when pulling e70d0c0 on modexp-reduct into 9b1b24a on master.

if (E.mod(new BN(2)).isZero()) return R
return (R.mul(BM)).mod(M)
// Red asserts M > 1
if (M.cmpn(1) === 0) return new BN(0)
Copy link
Member

Choose a reason for hiding this comment

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

Use .lten?

@axic
Copy link
Member

axic commented Mar 5, 2019

Should I do benchmarks to make sure this is actually more performant?

If you could that would be interesting to see.

@cdetrio
Copy link
Contributor

cdetrio commented Mar 6, 2019

new BN(1).toRed(BN.mont(new BN('64', 16))).fromRed() which I thought should return 0x1, actually returns 0x18! Not sure if this is a bug or intended?

I read here that if r is a power of 2, then the modulus n needs to be odd and greater than 3. Looking at lib/bn.js#L3408, r is being calculated as a power of 2 that's greater than n. If you pass any odd number > 1 to BN.mont(), then your line returns what you expect (it also seems to work with 3). Maybe bn.js should warn if mont() is called with an even number.

@s1na
Copy link
Contributor Author

s1na commented Mar 11, 2019

Thanks @cdetrio, that seems to be it.

To get an initial idea of how they compare, I ran the state tests on master and this branch, and measured the time it took to execute the expmod(B, E, M) line in the precompile in both branches. The tests were only run once. It's very inaccurate probably, but there doesn't seem to be much difference in execution times. Here are the measurements.

Add checks for E == 0 and M == 1

Fix M == 1 check

Check M lten 1 instead of eq
@holgerd77
Copy link
Member

Rebased this.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Looks good.

@holgerd77 holgerd77 merged commit f76a19d into master Mar 11, 2019
@holgerd77 holgerd77 deleted the modexp-reduct branch March 11, 2019 21:18
@holgerd77 holgerd77 changed the title [WIP] Use BN reduction context in modexp precompile Use BN reduction context in modexp precompile Mar 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modexp should use Montgomery multiplication from bn.js
6 participants