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

[BLOCKED] Add client side block filter (BIP158) #281

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@tamasblummer
Copy link
Member

commented Jun 11, 2019

Code moved here from murmel. Has been running there fore a while.

@stevenroose dependency on siphasher is temporary. Will use bitcoin_hashes if rust-bitcoin/bitcoin_hashes#46 is merged.

@tamasblummer

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

@apoelstra this fails to build on 1.22 because of missing u128 support. 1.26 would be sufficient. How long do you plan to stick at 1.22 ?

@stevenroose
Copy link
Collaborator

left a comment

I think the general API of the module is a bit strange. The test provide limited insight in the usage of the code. I'm gonna checkout your branch and see how flexible it is to improve the API.

Show resolved Hide resolved src/util/blockfilter_test.json Outdated
Show resolved Hide resolved src/util/blockfilter.rs Outdated
Show resolved Hide resolved src/util/blockfilter.rs Outdated
Show resolved Hide resolved src/util/blockfilter.rs Outdated
Show resolved Hide resolved src/util/blockfilter.rs Outdated
Show resolved Hide resolved src/util/blockfilter.rs Outdated
@apoelstra

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@tamasblummer another year or two

@tamasblummer

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

@apoelstra @stevenroose addressed all your comments except replacement of Box<dyn ScriptAccessor> with a closure or paramete. Squash or do you insist on that despite that Scripts it deals with are also Boxes and this "fat pointer" is one called through once per block? I think parametrizing is a heavy weight pattern to avoid an infrequent call through a vtable.

@tamasblummer

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

BLOCKED on rust-bitcoin/bitcoin_hashes#46 wich would replace siphasher used here which is using u128 which is not supported below rustc 1.26

@tamasblummer tamasblummer changed the title Add client side block filter (BIP158) [BLOCKED] Add client side block filter (BIP158) Jun 12, 2019

@stevenroose

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2019

rust-bitcoin/bitcoin_hashes#46 is merged but not yet released.

Your u128 method can be replaced with this, just like btcsuite does here: https://github.com/btcsuite/btcutil/blob/master/gcs/gcs.go#L54

You could perhaps even have two implementations depending on whether u128 is supported, not sure it's possible to build over that condition.

About the API, at first, I was confused with the Writer and Reader thing. I think the Writer is not really needed, but I agree the Reader can help prevent some unnecessary allocations.

Could you perhaps also add the match methods to the BlockFilter type and just pass them on to a Reader-wrapper?

Show resolved Hide resolved src/util/bip158.rs Outdated
@tamasblummer

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

parametrized BlockFilter and moved match methods into it, that is the only API now.
I prefer the u128 replacement I wrote, the compiler should do the optimization if there is any. I looked up but could not find conditional compilation on rustc version without added dev dependency. Is it worth?

@stevenroose

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2019

Ah ok cool you already wrote a non-u128 version, I didn't see that in your update. No that should do! Did you read my comment about the match api on both blockfilter and blockfilterreader?

Btw, is there any reason why BlockFilter(Reader|Writer) is split out from GCSFilter(Reader|Writer)?

@tamasblummer

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

GCSFilterRW are more generic, BlockFilterRWs are assuming that sip keys are derived from block hash. Squash?

@stevenroose

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2019

I'd first make the Reader methods public again :) Then squash.

@tamasblummer tamasblummer force-pushed the tamasblummer:client_side_block_filter branch from f7116b7 to 0b23fc2 Jun 15, 2019

@tamasblummer

This comment has been minimized.

Copy link
Member Author

commented Jun 15, 2019

@stevenroose squashed. Using patch in Cargo.toml to access master of bitcoin_hashes. @apoelstra please do a release of bitcoin_hashes, so siphash24 can be used here.

@tamasblummer tamasblummer force-pushed the tamasblummer:client_side_block_filter branch 2 times, most recently from 71d058c to 7553778 Jun 16, 2019

@tamasblummer tamasblummer force-pushed the tamasblummer:client_side_block_filter branch from bf270df to cd5b820 Jun 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.