Skip to content

feat(beacon_chain): Added Box to some fields in BeaconChainError #7623

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

Open
wants to merge 6 commits into
base: unstable
Choose a base branch
from

Conversation

PoulavBhowmick03
Copy link

Issue Addressed

Fixes #7473

  • Box Hash256 fields in variants like InconsistentPayloadReconstructed, BadPreState, RevertedFinalizedEpoch, etc.
  • Kept frequently accessed small fields (like Slot) unboxed for performance

@PoulavBhowmick03
Copy link
Author

@eserilev can you PTAL and let me know if there is more to be done in this

@PoulavBhowmick03 PoulavBhowmick03 requested a review from jxs as a code owner June 21, 2025 08:11
@eserilev
Copy link
Member

Hi @PoulavBhowmick03 In order to address this issue you'll need to figure out which variants inside BeaconChainError are too large and box those specific variants. Right now your changes wont pass clippy, you can run make lint locally to see the errors. Here's an example error message

error: the `Err`-variant returned from this function is very large
    --> beacon_node/beacon_chain/src/test_utils.rs:3096:10
     |
3096 |     ) -> Result<(), SyncCommitteeError> {
     |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
    ::: beacon_node/beacon_chain/src/sync_committee_verification.rs:192:5
     |
192  |     BeaconChainError(BeaconChainError),
     |     ---------------------------------- the largest variant contains at least 128 bytes
     |
     = help: try reducing the size of `sync_committee_verification::Error`, for example by boxing large elements or replacing it with `Box<sync_committee_verification::Error>`
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err

@eserilev eserilev added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Jun 21, 2025
@PoulavBhowmick03
Copy link
Author

Hi @PoulavBhowmick03 In order to address this issue you'll need to figure out which variants inside BeaconChainError are too large and box those specific variants. Right now your changes wont pass clippy, you can run make lint locally to see the errors. Here's an example error message

error: the `Err`-variant returned from this function is very large
    --> beacon_node/beacon_chain/src/test_utils.rs:3096:10
     |
3096 |     ) -> Result<(), SyncCommitteeError> {
     |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
    ::: beacon_node/beacon_chain/src/sync_committee_verification.rs:192:5
     |
192  |     BeaconChainError(BeaconChainError),
     |     ---------------------------------- the largest variant contains at least 128 bytes
     |
     = help: try reducing the size of `sync_committee_verification::Error`, for example by boxing large elements or replacing it with `Box<sync_committee_verification::Error>`
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err

Got it, looking into it

@PoulavBhowmick03
Copy link
Author

@eserilev
image

PTAL, it passes clippy now, do let me know if i need to make any more changes

@eserilev
Copy link
Member

Thanks for your changes @PoulavBhowmick03 . It does pass Clippy now, but I'm not sure the Hash256 values need Box. For example I just removed Box from the Hash256 inside the MissingBeaconBlock error variant and it passes clippy. The goal here is to minimize Box usage as much as we can so you might need to dig a bit deeper to find which variants absolutely need Box and which don't.

@mergify mergify bot added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jun 22, 2025
@eserilev eserilev added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jun 22, 2025
@mergify mergify bot added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jun 22, 2025
Copy link
Member

@eserilev eserilev left a comment

Choose a reason for hiding this comment

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

see requested changes above

@eserilev eserilev added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jun 22, 2025
@PoulavBhowmick03
Copy link
Author

@eserilev Removed Box from String, Hash256 and Checkpoint. Passes clippy as well, PTAL now.

@@ -507,17 +506,20 @@ fn build_gossip_verified_blobs<T: BeaconChainTypes>(
fn publish_blob_sidecars<T: BeaconChainTypes>(
sender_clone: &UnboundedSender<NetworkMessage<T::EthSpec>>,
blob: &GossipVerifiedBlob<T>,
) -> Result<(), BlockError> {
) -> Result<(), Box<BlockError>> {
Copy link
Author

Choose a reason for hiding this comment

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

I had to box this since it was throwing error. Do i need to try something else?

crate::publish_pubsub_message(sender_clone, pubsub_message)
.map_err(|_| BlockError::BeaconChainError(Box::new(BeaconChainError::UnableToPublish)))
crate::publish_pubsub_message(sender_clone, pubsub_message).map_err(|_| {
Box::new(BlockError::BeaconChainError(
Copy link
Author

Choose a reason for hiding this comment

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

And this

@PoulavBhowmick03 PoulavBhowmick03 force-pushed the box_beacon_chain_error branch from 0fa7ca5 to 16a348b Compare June 24, 2025 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author The reviewer has suggested changes and awaits thier implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants