write tests for bloomfiltered connection

MarcoFalke commented at 7:35 pm on March 30, 2020: member

There are some independent tests that can be added:

Fix the TODO https://github.com/bitcoin/bitcoin/pull/18334/files#diff-a77567bbc57dc1bbc033560e5cdd3b7dR400 :heavy_check_mark: Done in #18593
Add a test for msg_mempool. (Create a tx before the peer connects, then connect, then send msg_mempool)
Check that a new peer get’s disconnected when Bitcoin Core doesn’t have -peerbloomfilter set
Run the FilterTest functional test with a second node that has fRelay set to false in the version message, see https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki#extensions-to-existing-messages
…

Useful skills:

Basic understanding of BIP 37 and the Bitcoin Core functional test framework

Want to work on this issue?

The purpose of the good first issue label is to highlight which issues are suitable for a new contributor without a deep understanding of the codebase.

You do not need to request permission to start working on this. You are encouraged to comment on the issue if you are planning to work on it. This will help other contributors monitor which issues are actively being addressed and is also an effective way to request assistance if and when you need it.

For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

MarcoFalke added the label good first issue on Mar 30, 2020

chanhosuh commented at 2:07 am on April 1, 2020: none

Hi Marco, I’d like to work on this.

glozow commented at 9:05 pm on April 26, 2020: member

@chanhosuh are you still working on this?

chanhosuh commented at 10:26 pm on April 30, 2020: none

@gzhao408 Nope, feel free to pick it up!

glozow commented at 6:41 pm on May 1, 2020: member

Thanks! I’m going to try to tackle this!

glozow commented at 10:44 pm on May 15, 2020: member

Made progress on Add a test for msg_mempool, working the issue here: https://github.com/gzhao408/bitcoin/tree/test-bloomfilter I’ll make a PR when I have more to show :)

glozow commented at 10:12 pm on May 27, 2020: member

#19083

Check that a new peer get’s disconnected when Bitcoin Core doesn’t have -peerbloomfilter set @MarcoFalke I’m not 100% sure I understand this correctly, what does it mean for Bitcoin Core to have this set? I grepped for fDisconnect = true looking for things related to peerbloomfilters and wrote tests for what I found. I’m not sure if that’s what you’re referring to though.

MarcoFalke commented at 11:31 am on May 28, 2020: member

-peerbloomfilter is a command line setting to enable or disable the NODE_BLOOM service bit. If the NODE_BLOOM bit is not set, any light client will be disconnected immediately.

glozow commented at 0:15 am on June 3, 2020: member

If the NODE_BLOOM bit is not set, any light client will be disconnected immediately.

I think I got somewhere - So far, I have a test with peerbloomfilters=0 and am adding a peer using node.add_p2p_connection. I’m making the peer a light client by omitting NODE_NETWORK. I’m able to verify through getpeerinfo() that services are getting set correctly, but it’s not disconnecting as expected.

I think the logic I’m testing is this line (am I looking in the right place?) and noticed that it checks that the node is an outbound peer. Is it possible that that’s what I’m missing? I’m having trouble figuring out how inbound/outbound is distinguished in a functional test… am I going in the wrong direction?

NicolasDorier commented at 1:26 am on June 3, 2020: contributor

Check that a new peer get’s disconnected when Bitcoin Core doesn’t have -peerbloomfilter set

Of the peer is whitelisted, it should work

MarcoFalke commented at 11:07 am on June 3, 2020: member

I think the logic I’m testing is this line

I believe the disconnect happens here https://github.com/bitcoin/bitcoin/blob/3657aee2d25ce1ffefc6817af3eead7120b1d755/src/net_processing.cpp#L2222

glozow commented at 10:27 pm on June 3, 2020: member

Ah okay thank you! So it’s only when they send a msg_filteradd or msg_filterload. It looks like they get disconnected no matter what, and additionally banned if they’re on NO_BLOOM_VERSION or higher. I already test disconnect, but my trouble now is that I’m not sure how to test banning (I need to somehow non-manually connect a node and have it send a filter message)?

Additionally, I think that filterclear should also be one of the messages that we would disconnect/ban in this situation. It was removed in #8709 but I don’t think it’s still applicable?

MarcoFalke commented at 10:30 pm on June 3, 2020: member

Don’t worry about banning. I don’t think that needs to be tested because an attacker can circumvent it in any case.

Longer term banning might even go away completely.

glozow commented at 11:48 pm on June 14, 2020: member

Er… how do I close this issue? 😅

fanquake closed this on Jun 14, 2020

DrahtBot locked this on Feb 15, 2022

write tests for bloomfiltered connection #18473

Useful skills:

Want to work on this issue?