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: Use Share PublicKey for Share Removal #1140

Merged
merged 4 commits into from
Sep 13, 2023
Merged

Conversation

olegshmuelov
Copy link
Contributor

@olegshmuelov olegshmuelov commented Sep 10, 2023

Problem

Previously, shares were being removed based on the validator's public key, which was incorrect.

Solution

This fix introduces the proper way to delete a share by using the share's public key instead of the validator's public key. This ensures that the correct share is removed from the EKM (ETH Key Manager) storage.

UPD: based on @moshe-blox comment
added a logic that interact with database to be as part of the tx in eventhandler.

@olegshmuelov olegshmuelov self-assigned this Sep 10, 2023
@olegshmuelov olegshmuelov changed the title Fix: Share Removal by Validator PubKey Fix: Use Share PublicKey for Share Removal Sep 10, 2023
@olegshmuelov olegshmuelov marked this pull request as ready for review September 10, 2023 13:03
Copy link
Contributor

@moshe-blox moshe-blox left a comment

Choose a reason for hiding this comment

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

Code looks good 💪

Unrelated to this fix, I think there's another bug: RemoveShare is called from a task and not from event handler. This means that it's not part of the DB transaction and task failures are ignored (whereas event handling failures are retried).

I think we should fix it in this PR because this would make the primary fix testable in TestHandleBlockEventsStream (which doesn't run tasks) by adding a check on the SP data in the already existing test that removes a validator.

Wdyt? @y0sher @olegshmuelov

@y0sher
Copy link
Contributor

y0sher commented Sep 10, 2023

Code looks good 💪

Unrelated to this fix, I think there's another bug: RemoveShare is called from a task and not from event handler. This means that it's not part of the DB transaction and task failures are ignored (whereas event handling failures are retried).

I think we should do this in this PR because this would make the primary fix testable in TestHandleBlockEventsStream (which doesn't run tasks) by adding a check on the SP data in the already existing test that removes a validator.

Wdyt? @y0sher @olegshmuelov

I strongly agree that any code that plays with database should be as part of the tx in eventhandler.

@olegshmuelov
Copy link
Contributor Author

olegshmuelov commented Sep 11, 2023

Concern: Migration for Existing Shares

While this PR fixes the issue of shares and SP data being incorrectly removed by the validator's public key, it's worth noting that there may be existing shares in the ETH Key Manager storage that were not properly deleted due to this bug.

I recommend considering the creation of a migration script that scans for and deletes these improperly removed shares, particularly those that were not deleted by the ValidatorRemove event. This will ensure data integrity and consistency in the ETH Key Manager storage moving forward.

@moshe-blox @y0sher @lior-blox

y0sher
y0sher previously approved these changes Sep 12, 2023
Copy link
Contributor

@y0sher y0sher left a comment

Choose a reason for hiding this comment

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

@olegshmuelov I agree about the migration point, though I think even without the migration there is no hard of having old shares in your DB. so we should make it a point to release the new version with this migration but it dosen't have to be a part of this task. I can create a task for it.
@moshe-blox wdyt?

@olegshmuelov olegshmuelov merged commit caf3829 into stage Sep 13, 2023
5 checks passed
@olegshmuelov olegshmuelov deleted the on-share-remove-fix branch September 13, 2023 13:08
@olegshmuelov olegshmuelov restored the on-share-remove-fix branch September 13, 2023 13:13
@olegshmuelov olegshmuelov deleted the on-share-remove-fix branch September 13, 2023 13:14
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.

3 participants