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.

Sign up for GitHub

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

Jump to bottom

Add attestation simulator #4880

Merged
merged 48 commits into from
Dec 14, 2023
Merged

Add attestation simulator #4880

merged 48 commits into from
Dec 14, 2023

Conversation

v4lproik
Copy link
Contributor

@v4lproik v4lproik commented Oct 25, 2023 •
edited
Loading

@v4lproik v4lproik changed the title basic scaffold wip: add attestation simulator Oct 25, 2023
@chong-he chong-he added the work-in-progress PR is a work-in-progress label Oct 26, 2023
paulhauner
paulhauner reviewed Nov 2, 2023
View reviewed changes
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Heyo, just providing some initial feedback in response to #4526 (comment) ☺️

Looking good so far, I don't think there's that much left to do!

beacon_node/beacon_chain/src/attester_per_slot_service.rs Outdated Show resolved Hide resolved
beacon_node/beacon_chain/src/attester_per_slot_service.rs Outdated Show resolved Hide resolved
beacon_node/beacon_chain/src/attester_per_slot_service.rs Outdated Show resolved Hide resolved
beacon_node/beacon_chain/src/attester_per_slot_service.rs Outdated Show resolved Hide resolved
beacon_node/beacon_chain/src/validator_monitor.rs Outdated Show resolved Hide resolved
beacon_node/beacon_chain/src/validator_monitor.rs Outdated Show resolved Hide resolved
@v4lproik v4lproik marked this pull request as ready for review November 10, 2023 01:53
@v4lproik
Copy link
Contributor Author

v4lproik commented Dec 12, 2023 •
edited
Loading

fmt, lint and test-beacon-chain are green on my side! 🚀 🙏🏼

@paulhauner paulhauner added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Dec 12, 2023
@paulhauner paulhauner changed the base branch from stable to unstable December 13, 2023 00:20
paulhauner
paulhauner requested changes Dec 13, 2023
View reviewed changes
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

I'm running this on our Holesky nodes and it's working great! I have another suggestion please 🙏

Also, I noticed it was targeting stable when it should target unstable (I wish we could make unstable the default PR target, but alas, we cannot 😔). There are some minor conflicts, but I fixed them here during my testing.

Also, it's not an issue but I wanted to point out that ff9b6b5 does not dereference. |var| *var and |&var| var are equivalent, you can read more about them here. It's the .cloned() you used that does the dereferencing.

beacon_node/beacon_chain/src/attestation_simulator.rs Outdated Show resolved Hide resolved
@paulhauner
Copy link
Member

Looking good! I'll let the tests run before I slap an approval on it.

I've been running this on a Holesky node overnight, it's working well. Here's the last 24hrs:

Screenshot 2023-12-14 at 9 22 18 am

paulhauner
paulhauner approved these changes Dec 14, 2023
View reviewed changes
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

All tests passed 🎉 Let's get this merged!

@paulhauner paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Dec 14, 2023
@paulhauner paulhauner merged commit 189430a into sigp:unstable Dec 14, 2023
27 checks passed
This was referenced Jan 9, 2024
Closed
Closed
@chenrui333 chenrui333 mentioned this pull request Jan 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulhauner paulhauner paulhauner approved these changes

Assignees

@paulhauner paulhauner

Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@v4lproik @paulhauner @chong-he