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

Required quorums glitch #253

Conversation

gastonponti
Copy link
Contributor

@gastonponti gastonponti commented Jan 22, 2025

Fixes Issue

Closes #252

Depends on #241 (merge only after the 241 is part of main)

Changes proposed

As is stated in the issue, it avoids the quorum 1 check for the range of blocks [2950000, 2960000) for Holesky

Note to reviewers

Instead of using a flag passed through the cli, I'm checking the svc address, which will change for the v2, but I'm assuming that this issue will be only for the v1 users.
This check avoids cascading a flag in a lot of places just because of this particular bug. With this approach the custom changes live only in the verifier.

@samlaf samlaf changed the base branch from main to chore--modify-verifier-to-not-require-eth-archive-node January 22, 2025 23:09
Copy link
Collaborator

@samlaf samlaf left a comment

Choose a reason for hiding this comment

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

This approach is fine with me. @gastonponti did you want to try an archive sync using this branch just to double check that it works on your end before we merge it?

verify/verifier.go Outdated Show resolved Hide resolved
verify/verifier.go Outdated Show resolved Hide resolved
verify/verifier.go Outdated Show resolved Hide resolved
verify/verifier.go Outdated Show resolved Hide resolved
gastonponti and others added 4 commits January 23, 2025 14:26
Co-authored-by: Samuel Laferriere <samlaf92@gmail.com>
Co-authored-by: Samuel Laferriere <samlaf92@gmail.com>
Copy link
Collaborator

@EthenNotEthan EthenNotEthan left a comment

Choose a reason for hiding this comment

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

LGTM

@samlaf samlaf deleted the branch Layr-Labs:chore--modify-verifier-to-not-require-eth-archive-node January 23, 2025 18:17
@samlaf samlaf closed this Jan 23, 2025
@samlaf
Copy link
Collaborator

samlaf commented Jan 23, 2025

@gastonponti oops I had modified this PR to merge into the archive branch so that reviewing would only show the diff with it. We just merged that PR so this PR was automatically closed. Are you able to reopen this?
Otherwise might need to just create a new one on top of the current main branch.

@gastonponti
Copy link
Contributor Author

I'm not able to reopen it, but I can create a new one

@gastonponti
Copy link
Contributor Author

Created it here: #255

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.

bug: requiredQuorums glitch on holesky
3 participants