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

chore(conductor): send boxed objects over channels #1865

Merged
merged 3 commits into from
Jan 6, 2025

Conversation

ethanoroshiba
Copy link
Contributor

@ethanoroshiba ethanoroshiba commented Dec 6, 2024

Summary

Changed firm block channel to use boxed ReconstructedBlock.

Background

A large error variant lint was triggered when running clippy with Rust 1.83.0 in #1857 due to send errors returning the whole block. Large enum variants should be avoided because enums are only as small as their largest variant: https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant. Changing the channel to consist of a boxed block instead solves this problem at its source.

Changes

  • Changed firm block channel to use boxed ReconstructedBlock instead of it being unboxed.

Testing

Passing all tests.

Changelogs

No updates needed.

Breaking Changes

Overridden code freeze since this is a very small, non breaking change that shouldn't have any bearing since our previous audit.

Related Issues

closes #1858

@ethanoroshiba ethanoroshiba added conductor pertaining to the astria-conductor crate code-quality override-freeze labels Dec 6, 2024
@ethanoroshiba ethanoroshiba marked this pull request as ready for review December 6, 2024 21:12
@ethanoroshiba ethanoroshiba requested a review from a team as a code owner December 6, 2024 21:12
@SuperFluffy
Copy link
Member

Interesting that this showed up now - I remember clippy flagging this at some point, and I was almost certain that I did add an exception attribute, but maybe I am hallucinating.

Instead of boxing the problematic error variant, can you change the mpsc channel to be Box<T> instead? You'd then fix the error variants "for free" so to say.

@SuperFluffy
Copy link
Member

Neither PR text nor PR title match what the changes.

@ethanoroshiba ethanoroshiba changed the title chore(conductor): box large enum variants chore(conductor): use boxed block in firm channel Dec 10, 2024
@SuperFluffy SuperFluffy changed the title chore(conductor): use boxed block in firm channel chore(conductor): send boxed objects over channels Dec 10, 2024
Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

We can tighten this a bit to avoid the boxing/unboxing everywhere. Remeber to remove the asterisk/de-referencing operator * in various places to not accidentally unbox.

crates/astria-conductor/src/celestia/mod.rs Outdated Show resolved Hide resolved
crates/astria-conductor/src/celestia/mod.rs Outdated Show resolved Hide resolved
crates/astria-conductor/src/executor/mod.rs Outdated Show resolved Hide resolved
crates/astria-conductor/src/executor/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

Nice, this is an easy win. Thank you.

@ethanoroshiba ethanoroshiba added this pull request to the merge queue Jan 6, 2025
Merged via the queue into main with commit 8be7af8 Jan 6, 2025
46 checks passed
@ethanoroshiba ethanoroshiba deleted the ENG-1039/conductor_large_enum_variant branch January 6, 2025 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-quality conductor pertaining to the astria-conductor crate override-freeze
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore(conductor): avoid large enum variants
2 participants