Allow filterclear messages for enabling TX relay only. #8709
Conversation
An example of where this might be useful is allowing a node to connect blocksonly during IBD but then becoming a full-node once caught up with the latest block. This might also even want to be the default behaviour since during IBD most TXs appear to be orphans, and are routinely dropped (for example when a node disconnects). Therefore, this can waste a lot of bandwidth. Additionally, another pull could be written to stop relaying of TXs to nodes that are clearly far behind the latest block and are running a node that doesn't store many orphan TXs, such as recent versions of Bitcoin Core.
|
Concept ACK, though might it be better to just introduce a new, trivial, P2P message for this purpose instead of overloading filterclear? |
|
Maybe I'm missing something, but how is that accomplished by just this PR? How are you starting blocksonly? I think avoiding txs during catch up could be already accomplished using the feefilter message or as @gmaxwell points out by a new mechanism specifically for that purpose if that was an optimization we really needed. I believe it mostly only saves inv traffic though as we don't request txs if we're in IBD. |
|
@morcos This is a necessary first step prior to being able to code that - if the code mentioned was to be added to the network now without this pull merged and running on the network then the current behaviour would be to ban the nodes that start blocksonly and later request the TXs. Am I making sense? Yes, you are correct in that we are talking about savnig the bandwidth from inv messages. Feefilter is a nice idea but it would only be understood by the very latest versions of Bitcoin Core whereas filterclear is understood by most versions in the field - we just need to be careful not to send them to nodes not offering BLOOM whose protocol versions are between 70011 up to the protocol version where filterclear stops being banned - which leads me to think, should protocol version be raised as part of the pull request? @gmaxwell filterclear has always done exactly what is required, i.e. sets RelayTXs to true. I'm not sure what other functionality you are thinking about that would need a new message. |
|
Its kind of a shame that we didnt catch this at the time of the BIP, but this isnt really sufficient to use as a feature, now that there are nodes which will disconnect/ban you for sending a filterclear. A new P2P message would certainly require a new BIP and some kind of version bump/signaling capability before we could use this. Alternatively, we could update the BIP and assume no one uses -nodebloom (or its a small enough set that we'd be OK to use filterclear in the future). |
|
@TheBlueMatt This pull is safe but the pull that uses filterclear will need to be careful due to the ban-happy nodes you mention. As far as I know only bitcoin core nodes currently do this, and they are easy to spot as they have protocol version 70011 to 70014 and don't advertise BLOOM services. These nodes are a small minority (since BLOOM is by default on), so it might be so few as to not need to worry about them. The sooner this pull is merged the less the impact will be of the ban-happiness. The only question remaining for me is do I need to modify this pull to increase the protocol version; if it's not raised, people/nodes won't be sure whether nodes running protocol 70014 without BLOOM will ban upon receipt of a filterclear message; but as these are such a small minority, I think raising the protocol version isn't that necessary. What is involved in updating the BIP? |
|
Sure, but ban-happy nodes won't be identifiable unless we also bump the protocol version with this pull (which i don't think we should do), so it may be useless without thinking about it further. On September 15, 2016 2:33:08 AM EDT, R E Broadley [email protected] wrote:
|
|
I misunderstood how filterclear and fRelayTxes interact, and now I think this change makes sense. utACK |
|
@rebroad you can achieve the effect you desire by simply reconnecting after IBD |
|
@pstratem Sorry, I don't understand what you mean - what effect are you referring to? Without this patch, my node was banning my SPV wallet. I do agree that to address the overloading of whitelist that I need to change this to require an additional command line option. Perhaps -bloomwhite ? |
|
ACK. |
|
Is making this optional via a command line option what is needed to get more ACKs? |
1f951c6 Allow filterclear messages for enabling TX relay only. (R E Broadley)
…only. 1f951c6 Allow filterclear messages for enabling TX relay only. (R E Broadley)
Add NODE_BLOOM service bit Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6579 - Zcash equivalent of BIP 111 - bitcoin/bitcoin#6652 - Docs for BIP 111 - bitcoin/bitcoin#7087 - bitcoin/bitcoin#7174 - bitcoin/bitcoin#8709 Part of #2074. Closes #2738.
…only. 1f951c6 Allow filterclear messages for enabling TX relay only. (R E Broadley)
…only. 1f951c6 Allow filterclear messages for enabling TX relay only. (R E Broadley)
…isting filter msg disconnect logic 3a10d93 [p2p/refactor] move disconnect logic and remove misbehaving (gzhao408) ff8c430 [test] test disconnect for filterclear (gzhao408) 1c6b787 [netprocessing] disconnect node that sends filterclear (gzhao408) Pull request description: Nodes that don't have bloomfilters turned on (i.e. no `NODE_BLOOM` service) should disconnect peers that send them `filterclear` P2P messages. Non-bloomfilter nodes already disconnect peers for [`filteradd` and `filterload`](https://github.com/bitcoin/bitcoin/blob/19e919217e6d62e3640525e4149de1a4ae04e74f/src/net_processing.cpp#L2218), but #8709 removed `filterclear` so it could be used to reset tx relay. This isn't needed now because using `feefilter` message is much better for this purpose (See #19204). Also refactors existing disconnect logic for `filteradd` and `filterload` into respective message handlers and removes banning for them. ACKs for top commit: jnewbery: Code review ACK 3a10d93 naumenkogs: utACK 3a10d93 gillichu: tested ACK: quick test_runner on macOS [`3a10d93`](3a10d93) MarcoFalke: re-ACK 3a10d93 only change is replacing false with true 🚝 Tree-SHA512: 7aad8b3c0b0e776a47ad52544f0c1250feb242320f9a2962542f5905042f77e297a1486f8cdc3bf0fb93cd00c1ab66a67b2ec426eb6da3fe4cda56b5e623620f
Following my comment just now at https://github.com/bitcoin/bitcoin/pull/6579/files#r78485962
An example of where this might be useful is allowing a node to connect blocksonly during IBD but then becoming a full-node once caught up with the latest block. This might also even want to be the default behaviour since during IBD most TXs appear to be orphans, and are routinely dropped (for example when a node disconnects). Therefore, this can waste a lot of bandwidth.
Additionally, another pull could be written to stop relaying of TXs to nodes that are clearly far behind the latest block and are running a node that doesn't store many orphan TXs, such as recent versions of Bitcoin Core.