chore: handling of zero point in bls point mapping G2 #4000
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| @@ -47,6 +47,18 @@ export async function precompile11(opts: PrecompileInput): Promise<ExecResult> { | |||
| if (opts._debug !== undefined) { | |||
| opts._debug(`${pName} failed: ${e.message}`) | |||
| } | |||
| // noble‑curves throws this for inputs that map to the point at infinity | |||
| if (e.message === 'bad point: ZERO') { | |||
There was a problem hiding this comment.
This string is hardcoded, can we read this specific string from an export from BLS? Can a comment be added here that this lacks the necessary test input? Can we somehow help here to generate this input? We know which error string we should hit, so can we somehow reverse-engineer the input to trigger this?
There was a problem hiding this comment.
From the chat with the testing team, I understood that they will publish updated test vectors that will include the zero point test for G1 and G2. We have reverse engineered the G1 scalar from the BigInt value that Paul used in noble-curves (which from I understand he obtained from someone at the EF), but he mentioned having no idea how to generate the equivalent scalar for G2. I would just wait for the testing team to share the test vectors (this doesn't have to be merged before if we feel strongly about this)
| } | ||
| return { | ||
| executionGasUsed: gasUsed, | ||
| returnValue: zeroPoint, |
There was a problem hiding this comment.
Can it be explicitly checked that this should return a value and not "throw" at the EVM level? If it would throw at the EVM level the return buffer is cleared (RETURNDATASIZE would push 0 to stack) and the CALL to this precompile would push 0 to stack (instead of 1, signalling no-error for the call)
There was a problem hiding this comment.
Will come back to this. This is only throwing from within noble-curves (not for the wasm library), and aside from hardcoding results and pre-returning (therefore bypassing noble instead of waiting for it to throw) I don't see what we could do differently.
|
Hi there, just realized something, we added the error check to the BLS precompile directly. However, this should definitely move into the wrapper. So for G1 this error check ( Could you address that here, and also for G1? Also see #3979 for some extra discussion and some insights on how this test value input was found to lead to this consensus bug (if we use Noble). (If unclear, send another message there or let me know on discord 😄 ) |
Agreed. Since Paul is making some changes to his library (see paulmillr/noble-curves@d245d94), I'll wait for the new imminent release and adjust everything in one go. |
This addresses the issue that was flared up in #3979 and addressed in #3985 for bls point mapping G1 but for the G2 mapping.