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

Improve Filter API #295

Merged
merged 1 commit into from
Feb 14, 2025
Merged

Improve Filter API #295

merged 1 commit into from
Feb 14, 2025

Conversation

rustaceanrob
Copy link
Owner

@rustaceanrob rustaceanrob commented Feb 13, 2025

There is a bit of trait hell going on within the bip158 module. Something in the BlockFilter type is generic over Read, but the type itself concretely uses Vec. The Read implementation of Vec doesn't do any I/O so the result is infallible. We can just add an expect instead of propagating this error everywhere.

ref: Use of Vec followed by the trait hell

cc @nyonson

@rustaceanrob rustaceanrob changed the title Make filter reading infallible Improve Filter API Feb 13, 2025
@rustaceanrob
Copy link
Owner Author

While I was at it, I changed the API signature of match_any to be more generic. All the collections implement Iterator, so limiting to HashSet is more of an internal implementation detail. I caught the impl bug in method signatures over the past couple months.

@nyonson
Copy link
Contributor

nyonson commented Feb 14, 2025

There is a bit of trait hell going on within the bip158 module. Something in the BlockFilter type is generic over Read, but the type itself concretely uses Vec. The Read implementation of Vec doesn't do any I/O so the result is infallible. We can just add an expect instead of propagating this error everywhere.

ref: Use of Vec followed by the trait hell

cc @nyonson

Hm, the links you provided are the same? Not sure if intended cause I don't see the Read trait usage. But in any case, is the idea that there will definitely not be I/O errors since no I/O is actually happening?

@rustaceanrob
Copy link
Owner Author

Oops, here is the line showing the Vec. Yeah, for this particular use case, the in-memory readers will be infallible since the size of the data is known and there's no I/O

@nyonson
Copy link
Contributor

nyonson commented Feb 14, 2025

Feels a little bad ignoring the API, but yea, makes sense.

Copy link
Contributor

@nyonson nyonson left a comment

Choose a reason for hiding this comment

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

ACK 081b38c

@rustaceanrob
Copy link
Owner Author

rustaceanrob commented Feb 14, 2025

Yeah... the impl Iterator method signature might be the better of the improvements here. Tomorrow I may just update to change the method signature and do away with the expect?

I take a lot of pride in the lack of panic conditions, even though this one should never trigger. It is a tiny amount of insurance to any changes to std::io::Result as well.

@rustaceanrob
Copy link
Owner Author

self-ACK 87a9403

@rustaceanrob rustaceanrob merged commit 82b7506 into master Feb 14, 2025
14 checks passed
@rustaceanrob rustaceanrob deleted the filter-2-13 branch February 14, 2025 21:52
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.

2 participants