search

btcsuite / btcutil

Provides bitcoin-specific convenience functions and types
477 stars 410 forks source link

Fix range check in bloom filter false positive rate #68

Closed ghost closed 8 years ago

ghost commented 8 years ago

Tiny fix here. If you gave an argument of 0 for the false positive rate in NewFilter(), it would make a 0-length filter, always matching. That's really a false positive rate of 1 which seems like the opposite effect. Interpreting a 0 as the lower limit of 1e-9 seems more intuitive behavior when receiving an out-of-bounds argument.

Review on Reviewable

davecgh commented 8 years ago

Nice catch. Would you please also add a test that fails the current code and passes the new code to be sure any future regressions in this area are prevented? I believe there is already a test for the maximum size filter, but there is obviously not one for the minimum case.

Also, please update the copyright year on the touched files.

Thanks!

ghost commented 8 years ago

Sure, I added a short test case and added 2016 to the copyright years at top. Let me know if there's anything else.

ghost commented 8 years ago

OK thanks, yeah I had just copied from other tests in the file without looking into them too much. I put your code as a commit in my fork, is that the best way to do it?

davecgh commented 8 years ago

Yep, that's fine. We almost always squash PRs down to a single commit anyways before merging so the commit history doesn't end up cluttered with all the working revisions during PR review. Typically the only exception to that is generally when it's clearer as separate commits such as one where a new infrastructure piece is added and then something else new is added to take advantage of it.

Anyways, this looks good to go now. Please rebase/squash it to a single commit and I'll get it merged.

Thanks for your work on this. I always appreciate your fixes and contributions.

ghost commented 8 years ago

OK thanks! Squahed into 1 commit. Also working with @Roasbeef on a segregated witness PR. Not ready yet and that's a pretty big one :)