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 blocklist struct #325

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

Add blocklist struct #325

wants to merge 6 commits into from

Conversation

ferranbt
Copy link
Contributor

@ferranbt ferranbt commented Dec 31, 2024

📝 Summary

This PR replaces raw HashSet<Address> usages throughout the codebase with a dedicated BlockList struct that encapsulates the blocklist functionality in a single location. The struct implements a custom deserialize operation that maintains backward compatibility: if a blocklist file exists it parses the addresses, otherwise it falls back to an empty blocklist. This behavior is fully unit tested.


✅ I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

Copy link

github-actions bot commented Dec 31, 2024

Benchmark results for 81ccfc0

Report: https://flashbots-rbuilder-ci-stats.s3.us-east-2.amazonaws.com/benchmark/81ccfc0-026df71/report/index.html

Date (UTC) 2025-01-06T08:43:09+00:00
Commit 81ccfc0be832d395899feaef7fe4b137958cc929
Base SHA 026df71fdf2c8c6f6740682b6358d02f0e14bc09

Significant changes

Benchmark Mean Status
cloning_3000_branch_node_size_elements -2.08% Performance has improved.
gather_nodes_shared_cache 2.48% Performance has degraded.
gather_nodes_storage_tries 2.22% Performance has degraded.
MEV-Boost SubmitBlock serialization/JSON encoding -36.51% Performance has improved.
gather_nodes_empty_account 2.74% Performance has degraded.

@ferranbt ferranbt requested a review from metachris as a code owner December 31, 2024 15:22
Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

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

Not super clear to me if the abstraction is needed, but if there is agreement it is then looks generally good!

Comment on lines 29 to 35
pub fn len(&self) -> usize {
self.list.len()
}

pub fn contains(&self, address: &Address) -> bool {
self.list.contains(address)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to impl

impl Deref for BlockList {
    type Target = HashSet<Address>;

    fn deref(&self) -> &Self::Target {
        &self.list
    }
}

instead of the inner methods individually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added Deref but not sure what changes I have to make to the len and contains functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove them now :)

Comment on lines 44 to 46
fn from(path: PathBuf) -> Self {
Self::from_file(path).unwrap_or_else(|_| Self::new())
}
Copy link
Contributor

@liamaharon liamaharon Jan 2, 2025

Choose a reason for hiding this comment

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

I would expect this type of method to return an error if the path doesn't exist instead of an empty list.

Suggested change
fn from(path: PathBuf) -> Self {
Self::from_file(path).unwrap_or_else(|_| Self::new())
}
fn try_from(path: PathBuf) -> Result<Self, ...> {
Self::from_file(path).unwrap_or_else(|_| Self::new())
}

@ferranbt
Copy link
Contributor Author

ferranbt commented Jan 2, 2025

Not super clear to me if the abstraction is needed, but if there is agreement it is then looks generally good!

There are three benefits for this abstraction:

  • It eliminates the need to repeatedly instantiate HashSet throughout the codebase when working with blocklists
  • It provides a dedicated location for potential future enhancements, such as implementing hot reloading of the blocklist
  • It handles the blocklist deserialization at config load time rather than requiring a runtime blocklist() function.

@ZanCorDX
Copy link
Contributor

ZanCorDX commented Jan 9, 2025

Not sure if it's needed right now, but it's done and might be useful in the future.
I think DerefMut may be too much. I can give you Deref as syntax sugar for accessing the HashSet, but DerefMut allows changing it...

Copy link
Contributor

@ZanCorDX ZanCorDX left a comment

Choose a reason for hiding this comment

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

Do we really need DerefMut?

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.

3 participants