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

[Malleability] Event #7068

Open
wants to merge 6 commits into
base: yurii/malleability-check-test
Choose a base branch
from

Conversation

UlyanaAndrukhiv
Copy link
Contributor

@UlyanaAndrukhiv UlyanaAndrukhiv commented Feb 19, 2025

Close: #6651

Context

This pull request addresses an issue related to the malleability of the ID() method in Event.

  • The ID() method has been fixed by using MakeID. These changes have no impact on tests.
  • Removed eventWrapper because it is essentially equivalent to Event.
  • Removed eventIDWrapper because it is unused after these changes.
  • Removed the unused Encode method.
  • Removed the unused getEvents test helper function.
  • Added an extra unit test to ensure that Event is not malleable (replacing the existing test, which only partially verified non-malleability).

@UlyanaAndrukhiv UlyanaAndrukhiv self-assigned this Feb 19, 2025
@UlyanaAndrukhiv UlyanaAndrukhiv linked an issue Feb 19, 2025 that may be closed by this pull request
@UlyanaAndrukhiv UlyanaAndrukhiv marked this pull request as ready for review February 19, 2025 18:54
@UlyanaAndrukhiv UlyanaAndrukhiv requested a review from a team as a code owner February 19, 2025 18:54
Copy link
Member

@durkmurder durkmurder left a comment

Choose a reason for hiding this comment

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

Looks good but I think we can simplify the implementation more by removing the Fingerprint.

TxID: e.TransactionID[:],
Index: e.EventIndex,
}
return fingerprint.Fingerprint(struct {
Copy link
Member

Choose a reason for hiding this comment

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

I believe it's redundant, we don't need to implement the Fingerprint for this type, it essentially has the same fields as the original type. I would propose to get rid of it and in place where we need to fingerprint we use: fingerprint.Fingerprint(event)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@durkmurder, thank you for this note.The Fingerprint method defines a specific field order that differs from the Event struct, so we can't easily remove it.

Copy link
Contributor Author

@UlyanaAndrukhiv UlyanaAndrukhiv Feb 21, 2025

Choose a reason for hiding this comment

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

There was a discussion about this, and the conclusion is that we can change the field order for generating the RLP-encoded representation of the Event type. This simplifies the code and improves readability. However, this change will affect the Rosetta side. Since all these changes will be introduced in a new spork, we can do this, but we must also update the field order on the Rosetta side. Changing this order in Rosetta is crucial.

Based on @durkmurder ’s comment, I removed the Fingerprint implementation for the Event type and updated the tests accordingly (see commit).

@franklywatson , I want to bring this to your attention — we will need to create an issue in flow/rosetta to track this update. I’ve also described the previous and current field order for Event in the issue for this task (#6651).

@durkmurder
Copy link
Member

@jordanschalm can we get your review?

Copy link
Member

@durkmurder durkmurder left a comment

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Malleability B] Event
3 participants