Skip to content

Improve Filter API#295

Merged
rustaceanrob merged 1 commit intomasterfrom
filter-2-13
Feb 14, 2025
Merged

Improve Filter API#295
rustaceanrob merged 1 commit intomasterfrom
filter-2-13

Conversation

@rustaceanrob
Copy link
Collaborator

@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
Collaborator 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
Collaborator

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
Collaborator 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
Collaborator

nyonson commented Feb 14, 2025

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

Copy link
Collaborator

@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
Collaborator 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
Collaborator 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