8000 Add Bloom filter network messages by benthecarman · Pull Request #580 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add Bloom filter network messages #580

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 13, 2021
Merged

Add Bloom filter network messages #580

merged 1 commit into from
Sep 13, 2021

Conversation

benthecarman
Copy link
Contributor
@benthecarman benthecarman commented Mar 21, 2021 •
edited
Loading

Adds initial support for parsing the merkleblock, filterload, filteradd and filterclear network messages.

https://developer.bitcoin.org/reference/p2p_networking.html#filteradd

https://developer.bitcoin.org/reference/p2p_networking.html#filterclear

@jrawsthorne
Copy link
Contributor

Looks like other implementations represent the element in filteradd as raw bytes. The example you linked shows a txid being sent as an element which isn't a script. I worked on this a while ago but never created a pr. https://github.com/jrawsthorne/rust-bitcoin/blob/master/src/network/message_bloom.rs contains filterload as well if you want to include that as well

@benthecarman
Copy link
Contributor Author

Looks like other implementations represent the element in filteradd as raw bytes. The example you linked shows a txid being sent as an element which isn't a script. I worked on this a while ago but never created a pr. https://github.com/jrawsthorne/rust-bitcoin/blob/master/src/network/message_bloom.rs contains filterload as well if you want to include that as well

You're right, thanks

@benthecarman benthecarman force-pushed the filter-add-clear-p2p-msgs branch from 86a4ce3 to 9b4dc28 Compare May 18, 2021 06:01
@benthecarman
Copy link
Contributor Author

Implemented @jrawsthorne's suggestions, added him as a co-author too :)

@benthecarman benthecarman changed the title Add filteradd & filterclear network messages Add Bloom filter network messages May 18, 2021
@benthecarman benthecarman force-pushed the filter-add-clear-p2p-msgs branch from 9b4dc28 to 0cf61ad Compare May 19, 2021 08:08
jrawsthorne
jrawsthorne previously approved these changes May 19, 2021
View reviewed changes
Copy link
Contributor
@jrawsthorne jrawsthorne left a comment

Choose a reason for hiding this comment

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

Followed https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki to double check everything is correct.

@sgeisler sgeisler added the API break This PR requires a version bump for the next release label May 20, 2021
@sgeisler sgeisler added this to the 0.27.0 milestone May 20, 2021
sgeisler
sgeisler previously approved these changes May 20, 2021
View reviewed changes
Copy link
Contributor
@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

utACK 0cf61adfd48c197ed1f6ede2986bd714dcfb258b

I'm operating under the assumption that the MerkleBlock implementation is correct, it looked ok at least. If that's the case this PR looks good.

@benthecarman benthecarman dismissed stale reviews from sgeisler and jrawsthorne via c2135c9 May 21, 2021 17:46
@benthecarman benthecarman force-pushed the filter-add-clear-p2p-msgs branch from 0cf61ad to c2135c9 Compare May 21, 2021 17:46
sgeisler
sgeisler previously approved these changes May 21, 2021
View reviewed changes
Copy link
Contributor
@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

utACK c2135c976e0dbe798e357fd4264dea4f941a3b84

apoelstra
apoelstra previously approved these changes Jun 29, 2021
View reviewed changes
Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ack c2135c976e0dbe798e357fd4264dea4f941a3b84

Reviewed the code and ran tests. Did not verify that the new structures match the spec.

Note that this is a breaking change.

@benthecarman benthecarman dismissed stale reviews from apoelstra and sgeisler via e8904df July 28, 2021 17:05
@benthecarman benthecarman force-pushed the filter-add-clear-p2p-msgs branch from c2135c9 to e8904df Compare July 28, 2021 17:05
@benthecarman
Copy link
Contributor Author

Rebased

apoelstra
apoelstra previously approved these changes Sep 8, 2021
View reviewed changes
Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK e8904df5f36139ed9cea2725164a16b6fa0a96ef

Don't know why CI is not running. It passes locally (although I did not run the examples/ directory, and I have only access to x86_64, and not wasm)
8000

@benthecarman
Copy link
Contributor Author

Don't know why CI is not running. It passes locally (although I did not run the examples/ directory, and I have only access to x86_64, and not wasm)

Github is weird with first time contributors and sometimes doesn't run CI

dr-orlovsky
dr-orlovsky previously approved these changes Sep 13, 2021
View reviewed changes
Copy link
Collaborator
@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

utACK e8904df5f36139ed9cea2725164a16b6fa0a96ef

Compared correspondence of data structures and serialization to the spec https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki#New_messages

@dr-orlovsky
Copy link
Collaborator

@benthecarman I tried everything to get the tests running so I can merge this PR. No success :( Seems like the only way is to force-push, which will discard all reviews however :( I will ask one of maintainers to add his confirmation ASAP if you will chose this path (I actually see no other route...)

@benthecarman @jrawsthorne
Add Bloom filter network messages
894f0f0 
Co-authored-by: jrawsthorne <jake@jakerawsthorne.co.uk>
@benthecarman benthecarman dismissed stale reviews from dr-orlovsky and apoelstra via 894f0f0 September 13, 2021 20:08
@benthecarman benthecarman force-pushed the filter-add-clear-p2p-msgs branch from e8904df to 894f0f0 Compare September 13, 2021 20:08
@benthecarman
Copy link
Contributor Author

@benthecarman I tried everything to get the tests running so I can merge this PR. No success :( Seems like the only way is to force-push, which will discard all reviews however :( I will ask one of maintainers to add his confirmation ASAP if you will chose this path (I actually see no other route...)

Rebased

dr-orlovsky
dr-orlovsky approved these changes Sep 13, 2021
View reviewed changes
Copy link
Collaborator
@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

ACK 894f0f0

apoelstra
apoelstra approved these changes Sep 13, 2021
View reviewed changes
Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 894f0f0

@apoelstra apoelstra merged commit 57d7baf into rust-bitcoin:master Sep 13, 2021
@benthecarman benthecarman deleted the filter-add-clear-p2p-msgs branch September 13, 2021 22:00
@dr-orlovsky dr-orlovsky mentioned this pull request Jan 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dr-orlovsky dr-orlovsky dr-orlovsky approved these changes

@apoelstra apoelstra apoelstra approved these changes

@jrawsthorne jrawsthorne jrawsthorne left review comments

@sgeisler sgeisler Awaiting requested review from sgeisler

Assignees
No one assigned
Labels
API break This PR requires a version bump for the next release
Projects
None yet
Milestone
0.28.0
Development

Successfully merging this pull request may close these issues.
5 participants
@benthecarman @jrawsthorne @dr-orlovsky @apoelstra @sgeisler
0