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(MMU_BLAKE): add test + debug #1519

Conversation

letypequividelespoubelles
Copy link
Collaborator

No description provided.

Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
@letypequividelespoubelles letypequividelespoubelles linked an issue Nov 19, 2024 that may be closed by this pull request
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
@letypequividelespoubelles letypequividelespoubelles changed the title fix(MMU_BLAKE): typo fix(MMU_BLAKE): add test + debug Nov 19, 2024
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
@letypequividelespoubelles letypequividelespoubelles linked an issue Nov 20, 2024 that may be closed by this pull request
final Bytes bytecode =
BytecodeCompiler.newProgram()
.push(Bytes.fromHexString("0x0badb077")) // value, some random data to hash
.push(5) // offset
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the intent here. You are producing this in memory:

<-- 5 bytes -->    <--      32 bytes       -->
00 00 00 00 00  |  00 00 00 ... 00 0b ad b0 77

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, I just want to have something (ie not 0) in the hash input data, so really don't care of what I put inside as soon as we have a non zero between offset 4 and 212.

.push(5) // offset
.op(OpCode.MSTORE)
.push(10) // value = r * 256 ** 3 for Blake call
.push(0) // offset
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the intent here. You are producing this in memory:
And now

<-- 5 bytes -->    <--      32 bytes       -->
0a 00 00 00 00  |  00 00 00 ... 00 0b ad b0 77

So you have r = 0x 0a 00 00 00 = 10 * 256 ** 3 rounds (and gas cost.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, I want to produce an OOG (wih the MSTORE8) and just have a non zero input for the hash (same comment as before). It's true we don't care about the input of the hash as we're OOG, but still

final Bytes bytecode =
BytecodeCompiler.newProgram()
.push(Bytes.fromHexString("0x0badb077")) // value, some random data to hash
.push(5) // offset
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same issue

BytecodeCompiler.newProgram()
.push(Bytes.fromHexString("0x0badb077")) // value, some random data to hash
.push(5) // offset
.op(OpCode.MSTORE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really don't understand the 5 here. Is it to produce some gibberish value ? Don't get me wrong, that'll do the job, it's just surprising.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I put a 5 just to be sure it doesn't overlap with the 4 first bytes of the r. But yes the value will be leftPadded in the memory, but I don't care.

@@ -148,6 +149,7 @@ public MmuData setMicroInstructions(MmuData mmuData) {
.sourceLimbOffset(sourceLimbOffsetF)
.sourceByteOffset(sourceByteOffsetF)
.targetByteOffset((short) (LLARGE - 1))
.targetLimbOffset(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice.

.push(2) // value = f for Blake call
.push(212) // offset
.op(OpCode.MSTORE8)
.push(LLARGE * (INDEX_MAX_BLAKE_RESULT + 1)) // return size
Copy link
Collaborator

Choose a reason for hiding this comment

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

You actually have BLAKE2f_HASH_OUTPUT_SIZE in the code elsewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh yes, forget to make the change everywhere

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -587,7 +591,7 @@ public static MmuCall parameterExtractionForBlake(
.successBit(blakeSuccess)
.limb1(blakeR)
.limb2(blakeF)
.setBlakeModexp()
.setBlakeModexp(blakeSuccess)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this one was easy ;)

@@ -123,9 +124,9 @@ private void traceParameter(Trace trace, UnsignedByte stamp) {
}

private void traceBlakeResult(Trace trace, UnsignedByte stamp) {
final Bytes input = computeBlake2fResult();
final Bytes hash = computeBlake2fResult();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was that a bug ? or just overloading the word input ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just renaming. Everything was named input before (same name for callData, hashInput, and hashOutput ...)

Copy link
Collaborator

@OlivierBBB OlivierBBB left a comment

Choose a reason for hiding this comment

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

LGTM

OlivierBBB and others added 3 commits November 21, 2024 21:03
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
…ithub.com:Consensys/linea-tracer into 1517-wrong-calculation-for-numbermmioinstructions
Signed-off-by: F Bojarski <ceciestunepoubelle@protonmail.ch>
…ithub.com:Consensys/linea-tracer into 1517-wrong-calculation-for-numbermmioinstructions
@letypequividelespoubelles letypequividelespoubelles merged commit 892534c into arith-dev Nov 21, 2024
5 checks passed
@letypequividelespoubelles letypequividelespoubelles deleted the 1517-wrong-calculation-for-numbermmioinstructions branch November 21, 2024 15:38
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.

wrong calculation for numberMmioInstructions BlakeSubsection null
2 participants