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

Add new process pending deposits test #4101

Merged
merged 3 commits into from
Jan 28, 2025

Conversation

jtraglia
Copy link
Member

@jtraglia jtraglia commented Jan 27, 2025

This PR adds a new test which would have caught a bug in Prysm. From @terencechain:

I have a good spec test case that was triggered by devnet5 this morning and caused Prysm to fork off. The test case is relatively simple:

  • There are two pending deposits in the state, both pointing to the same public key
  • The public key does not exist in the beacon state
  • The first pending deposit has an invalid signature, while the second has a valid signature

Pretty simple. This test should do exactly this.

I've also added a third deposit on Terence's advice:

If we want, we could add [third pending deposit] with the same amount or different amount and a make sure balance amount is correct

Copy link
Contributor

@mkalinin mkalinin left a comment

Choose a reason for hiding this comment

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

LGTM!

@jtraglia jtraglia merged commit 663f2d3 into ethereum:dev Jan 28, 2025
23 checks passed
@jtraglia jtraglia deleted the new-pending-deposits-test branch January 28, 2025 14:16
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.

3 participants