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

feat: make the trace deterministic #1346

Conversation

letypequividelespoubelles
Copy link
Collaborator

No description provided.

Signed-off-by: Francois Bojarski <francois.bojarski@consensys.net>
Signed-off-by: Francois Bojarski <francois.bojarski@consensys.net>
@letypequividelespoubelles letypequividelespoubelles marked this pull request as ready for review October 2, 2024 06:19
Copy link
Collaborator

@DavePearce DavePearce left a comment

Choose a reason for hiding this comment

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

Notes:

  • ExpOperation needs a ExpOperationComparator as Exp implements OperationSetModule<ExpOperation>.

  • StpOperation needs a StpOperationComparator as Stp implements OperationSetModule<StpOperation>.

  • Does ExpOperation need the annotation @EqualsAndHashCode(onlyExplicitlyIncluded = true, callSuper = false) ?

  • There is some inconsistency, in that e.g. BlockHash does things differently. But perhaps it has to?

@@ -0,0 +1,35 @@
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems odd that you've implemented all the Comparators separately from the class itself. You can have e.g. done class AddOperation extends ModuleOperation implements Comparable<AddOperation> then added a compareTo() method as required for the interface. Just saves lines I guess --- not a big deal though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose the reason for doing it this way is to avoid implemented a Comparator for all Operation classes? I think you still could do it though by modifying the signature of OperationSetModule.sortOperations(). I'd have to check it worked though --- so not worth bothering!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still, one reason for having AddOperation implement Comparable<AddOperation> is that you can then modify ModuleOperationStackedSet to require that its elements must be sortable like so:

public class ModuleOperationStackedSet<E extends ModuleOperation & Comparable<E>>
    extends StackedSet<E> {

That just helps reduce the chance of a problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is some inconsistency, in that e.g. BlockHash does things differently. But perhaps it has to?

made more consistent. For BlockHash we have to sort before commit time, as we have to trigger WCP.

Does ExpOperation need the annotation @EqualsAndHashCode(onlyExplicitlyIncluded = true, callSuper = false)

added

Signed-off-by: Francois Bojarski <francois.bojarski@consensys.net>
DavePearce
DavePearce previously approved these changes Oct 2, 2024
Copy link
Collaborator

@DavePearce DavePearce left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM

return instructionComp;
}

if (op1.expCall.expInstruction() == EXP_INST_EXPLOG) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem ideal. I would have thought it makes more sense to add compareTo to the ExpCall interface and, hence, its implementations (ExplogExpCall and ModexpLogExpCall). Otherwise, this is a bit fragile is someone adds a new implementation of ExpCall ... they might forget to update this bit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do agree.

Copy link
Collaborator

@DavePearce DavePearce left a comment

Choose a reason for hiding this comment

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

I think there is still a problem with ext module.

@DavePearce DavePearce dismissed their stale review October 2, 2024 10:42

I think there is still a problem with ext module. TBC

Copy link
Collaborator

@DavePearce DavePearce left a comment

Choose a reason for hiding this comment

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

Minor typo in ext -- found via testing :)

@DavePearce
Copy link
Collaborator

Yeah, with that final typo fixed ... I think it looks good.

Signed-off-by: Francois Bojarski <francois.bojarski@consensys.net>
@letypequividelespoubelles letypequividelespoubelles enabled auto-merge (squash) October 3, 2024 05:38
Copy link
Collaborator

@DavePearce DavePearce left a comment

Choose a reason for hiding this comment

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

Nice job. LGTM.

@letypequividelespoubelles letypequividelespoubelles merged commit 9301772 into arith-dev Oct 3, 2024
5 checks passed
@letypequividelespoubelles letypequividelespoubelles deleted the 1345-make-stateless-modules-produce-deterministic-trace branch October 3, 2024 07:33
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.

Make stateless modules produce deterministic trace
2 participants