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

reconnect MMIO constraints #1388

Open
wants to merge 14 commits into
base: arith-dev
Choose a base branch
from

Conversation

letypequividelespoubelles
Copy link
Collaborator

@letypequividelespoubelles letypequividelespoubelles commented Oct 8, 2024

Partial debugging of reconnecting the MMIO constraints (inner + all lookup touching the MMIO)

Signed-off-by: Francois Bojarski <[email protected]>
@letypequividelespoubelles letypequividelespoubelles linked an issue Oct 8, 2024 that may be closed by this pull request
@letypequividelespoubelles letypequividelespoubelles marked this pull request as ready for review October 10, 2024 13:09
@@ -445,7 +445,7 @@ public static MmuCall forIdentityExtractCallData(
final Hub hub, PrecompileSubsection precompileSubsection) {

return new MmuCall(hub, MMU_INST_RAM_TO_RAM_SANS_PADDING)
.sourceId(precompileSubsection.callSection.hubStamp())
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 and following are real fixes

.sourceRamBytes(Optional.of(modexpSubsection.callerMemorySnapshot))
.targetId(modexpSubsection.exoModuleOperationId())
.exoBytes(Optional.of(modExpMetadata.base()))
.exoBytes(Optional.of(leftPadTo(modExpMetadata.base(), MODEXP_COMPONENT_BYTE_SIZE)))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

real fix too

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is modExpMetadata.base() also right padded (for short call data) ?

@@ -38,7 +38,7 @@ public CodeCopy(final Hub hub) {
this.hub = hub;
this.contract = hub.currentFrame().metadata();

this.exoBytes(Optional.of(hub.romLex().getCodeByMetadata(contract)))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

equivalent, but requires less computation

@@ -60,17 +60,17 @@ public ModexpSubsection(final Hub hub, final CallSection callSection) {
if (modexpMetaData
.bbs()
.toUnsignedBigInteger()
.compareTo(BigInteger.valueOf(PROVER_MAX_INPUT_BYTE_SIZE))
.compareTo(BigInteger.valueOf(MODEXP_COMPONENT_BYTE_SIZE))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we had a duplicate for this constant

warmth,
hub.deploymentNumberOf(codeAddress),
hub.deploymentStatusOf(codeAddress));
AccountSnapshot.canonical(hub, hub.messageFrame().getContractAddress());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not a fix, just use the canonical way when possible

@@ -87,26 +87,23 @@ public void commit(List<MappedByteBuffer> buffers) {
Trace trace = new Trace(buffers);
int stamp = 0;
for (MmuOperation mmuOperation : mmu.operations().getAll()) {
if (mmuOperation.traceMe()) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not a fix

@@ -59,14 +59,10 @@ public void commit(List<MappedByteBuffer> buffers) {

for (MmuOperation mmuOperation : operations.getAll()) {
Preconditions.checkState(mmuOperation.traceMe(), "Cannot compute if traceMe is false");
if (mmuOperation.traceMe()) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not a fix

final int sizeToExtract = mmioInst.size() == 0 ? LLARGE : mmioInst.size();
final Bytes16 exoLimb = readBytes(mmuData.exoBytes(), offset, sizeToExtract);
mmioInst.limb(exoLimb);
for (MmuToMmioInstruction mmioInst : mmuData.mmuToMmioInstructions()) {
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 is a fix

Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -263,7 +263,9 @@ public MmuData setMicroInstructions(MmuData mmuData) {
middleMicroInstruction(mmuData, sourceLimbOffset, targetLimbOffset);
}

lastMicroInstruction(mmuData);
if (initialTotalNonTrivial > 1) {
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 is a fix

Copy link
Collaborator

Choose a reason for hiding this comment

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

import org.apache.tuweni.bytes.Bytes;

public class Bytecodes {

public static Bytes16 readBytes(final Bytes data, final long offset, final int sizeToRead) {
if (offset >= data.size()) {
public static Bytes16 readBytes(
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 is a fix

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.

just update to the latest master. MMIO constraints are still not plugged

@@ -445,7 +445,7 @@ public static MmuCall forIdentityExtractCallData(
final Hub hub, PrecompileSubsection precompileSubsection) {

return new MmuCall(hub, MMU_INST_RAM_TO_RAM_SANS_PADDING)
.sourceId(precompileSubsection.callSection.hubStamp())
.sourceId(hub.currentFrame().contextNumber()) // called at ContextReEntry
Copy link
Collaborator

@OlivierBBB OlivierBBB Oct 11, 2024

Choose a reason for hiding this comment

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

👍

@@ -706,10 +706,10 @@ public static MmuCall forModexpExtractBase(
final Hub hub, final ModexpSubsection modexpSubsection, final ModexpMetadata modExpMetadata) {
if (modExpMetadata.extractBase()) {
return new MmuCall(hub, MMU_INST_MODEXP_DATA)
.sourceId(modexpSubsection.callSection.hubStamp())
.sourceId(hub.currentFrame().contextNumber()) // called at ContextReEntry
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

@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.

I still have to understand the changes labeled with ⭐, could you please give a short description of the intended effect ?

.sourceRamBytes(Optional.of(modexpSubsection.callerMemorySnapshot))
.targetId(modexpSubsection.exoModuleOperationId())
.exoBytes(Optional.of(modExpMetadata.base()))
.exoBytes(Optional.of(leftPadTo(modExpMetadata.base(), MODEXP_COMPONENT_BYTE_SIZE)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is modExpMetadata.base() also right padded (for short call data) ?

@@ -38,7 +38,7 @@ public CodeCopy(final Hub hub) {
this.hub = hub;
this.contract = hub.currentFrame().metadata();

this.exoBytes(Optional.of(hub.romLex().getCodeByMetadata(contract)))
this.exoBytes(Optional.of(hub.currentFrame().code().bytecode()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

import org.apache.tuweni.bytes.Bytes;

public class Bytecodes {

public static Bytes16 readBytes(final Bytes data, final long offset, final int sizeToRead) {
if (offset >= data.size()) {
public static Bytes16 readBytes(
Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -263,7 +263,9 @@ public MmuData setMicroInstructions(MmuData mmuData) {
middleMicroInstruction(mmuData, sourceLimbOffset, targetLimbOffset);
}

lastMicroInstruction(mmuData);
if (initialTotalNonTrivial > 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

final int sizeToExtract = mmioInst.size() == 0 ? LLARGE : mmioInst.size();
final Bytes16 exoLimb = readBytes(mmuData.exoBytes(), offset, sizeToExtract);
mmioInst.limb(exoLimb);
for (MmuToMmioInstruction mmioInst : mmuData.mmuToMmioInstructions()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Re-connect RAM (MMU/MMIO)
2 participants