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

Bugfix/eip 196 edge case #185

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

garyschulte
Copy link
Contributor

@garyschulte garyschulte commented Jun 27, 2024

fix for a weird (imo) EIP-196 expectation that a malformed/incomplete g1 point should return an error if the X value is readable, but not in the field; whereas a malformed/incomplete g1 point will otherwise return 0

Copy link

@pinges pinges left a comment

Choose a reason for hiding this comment

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

I'm not sure about the edge case. For me it does not make sense and we should try to find out whether that is the right behaviour.

@@ -100,7 +121,17 @@ func eip196altbn128G1Mul(javaInputBuf, javaOutputBuf, javaErrorBuf *C.char, cInp
errorBuf := castErrorBuffer(javaErrorBuf, errorLen)

if inputLen < EIP196PreallocateForG1 {
// if we do not have complete input, return 0

// check the X point and return an error if it is in the field, edge case
Copy link

Choose a reason for hiding this comment

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

Suggested change
// check the X point and return an error if it is in the field, edge case
// check the X point and return an error if it is not in the field, edge case

// check we have input size sufficient for a G1Affine
if inputLen < EIP196PreallocateForG1 {

// if not, check the X point and return an error if it is in the field, edge case
Copy link

Choose a reason for hiding this comment

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

Suggested change
// if not, check the X point and return an error if it is in the field, edge case
// if not, check the X point and return an error if it is not in the field, edge case

@garyschulte
Copy link
Contributor Author

garyschulte commented Jun 28, 2024

I'm not sure about the edge case. For me it does not make sense and we should try to find out whether that is the right behaviour.

This PR to resolve a reference test failure. I agree it is weird behavior. IMO all malformed inputs should return an error.

reference test fail:
https://github.com/hyperledger/besu/actions/runs/9686194628/job/26728072192?pr=7262

…t case, make tests pickier about expected errors

Signed-off-by: garyschulte <garyschulte@gmail.com>
Copy link

@pinges pinges left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@jflo jflo left a comment

Choose a reason for hiding this comment

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

I don't understand the math, but the change in error case handling is sound.

@garyschulte garyschulte merged commit b108fd3 into hyperledger:main Jul 1, 2024
11 checks passed
@garyschulte garyschulte deleted the bugfix/eip-196-edge-case branch July 1, 2024 14:39
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.

3 participants