Add Bloom filter network messages#580
Conversation
|
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 |
86a4ce3 to
9b4dc28
Compare
|
Implemented @jrawsthorne's suggestions, added him as a co-author too :) |
filteradd & filterclear network messages9b4dc28 to
0cf61ad
Compare
jrawsthorne
left a comment
There was a problem hiding this comment.
Followed https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki to double check everything is correct.
sgeisler
left a comment
There was a problem hiding this comment.
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.
0cf61ad to
c2135c9
Compare
sgeisler
left a comment
There was a problem hiding this comment.
utACK c2135c976e0dbe798e357fd4264dea4f941a3b84
apoelstra
left a comment
There was a problem hiding this comment.
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.
c2135c9 to
e8904df
Compare
|
Rebased |
apoelstra
left a comment
There was a problem hiding this comment.
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)
Github is weird with first time contributors and sometimes doesn't run CI |
dr-orlovsky
left a comment
There was a problem hiding this comment.
utACK e8904df5f36139ed9cea2725164a16b6fa0a96ef
Compared correspondence of data structures and serialization to the spec https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki#New_messages
|
@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...) |
Co-authored-by: jrawsthorne <jake@jakerawsthorne.co.uk>
894f0f0
e8904df to
894f0f0
Compare
Rebased |
This commit confirms that there is not a single untouched script instance across the project; every one has been converted to a tagged script. In bitcoin/src/network/params.rs I changed the signet example to use a WitnessScript. I wasn't sure if this script is interpreted as a witness script or a scriptpubkey on Signet itself so I guessed. In p2p/src/message.rs there is a "script" that is actually just a blob of raw bytes (that happens to be a script) used in a "test" of the bloom filter messages. See rust-bitcoin#580 where this was added for discussion about script vs bytes. (BTW why did I let a bloom filtering PR into this project? This whole system was a bad idea and a DoS vector. Is it even used?)
This commit confirms that there is not a single untouched script instance across the project; every one has been converted to a tagged script. In bitcoin/src/network/params.rs I changed the signet example to use a WitnessScript. I wasn't sure if this script is interpreted as a witness script or a scriptpubkey on Signet itself so I guessed. In p2p/src/message.rs there is a "script" that is actually just a blob of raw bytes (that happens to be a script) used in a "test" of the bloom filter messages. See rust-bitcoin#580 where this was added for discussion about script vs bytes. (BTW why did I let a bloom filtering PR into this project? This whole system was a bad idea and a DoS vector. Is it even used?)
This commit confirms that there is not a single untouched script instance across the project; every one has been converted to a tagged script. This triggers a number of doc changes which should have gone into previous commits. I apologize for that. But it's a PITA to go back and locate exactly which commit each doc change was supposed to go into. In bitcoin/src/network/params.rs I changed the signet example to use a WitnessScript. I wasn't sure if this script is interpreted as a witness script or a scriptpubkey on Signet itself so I guessed. In p2p/src/message.rs there is a "script" that is actually just a blob of raw bytes (that happens to be a script) used in a "test" of the bloom filter messages. See rust-bitcoin#580 where this was added for discussion about script vs bytes. (BTW why did I let a bloom filtering PR into this project? This whole system was a bad idea and a DoS vector. Is it even used?)
Adds initial support for parsing the
merkleblock,filterload,filteraddandfilterclearnetwork messages.https://developer.bitcoin.org/reference/p2p_networking.html#filteradd
https://developer.bitcoin.org/reference/p2p_networking.html#filterclear