De-neuter NODE_BLOOM #6641

pull TheBlueMatt wants to merge 2 commits into bitcoin:master from TheBlueMatt:bloom-disable changing 3 files +2 −4
  1. TheBlueMatt commented at 6:37 am on September 6, 2015: member
    I forget things easily, so I’m gonna get a few months ahead of myself and PR this now…to be merged a full release cycle after #6579.
  2. laanwj added this to the milestone Future on Sep 8, 2015
  3. laanwj commented at 3:35 pm on September 8, 2015: member
    Assigned ‘future’ milestone accordingly.
  4. laanwj added the label P2P on Sep 8, 2015
  5. dcousens commented at 2:09 am on September 9, 2015: contributor
    @TheBlueMatt can this be re-based?
  6. Diapolo commented at 3:09 pm on September 9, 2015: none
    Yeah a rebase would make it clear that the base pull has been merged.
  7. TheBlueMatt force-pushed on Sep 9, 2015
  8. TheBlueMatt commented at 8:48 pm on September 9, 2015: member

    Rebased.

    On 09/09/15 15:09, P. Kaufmann wrote:

    Yeah a rebase would make it clear that the base pull has been merged.

    — Reply to this email directly or view it on GitHub #6641 (comment).

  9. MarcoFalke commented at 12:11 pm on September 22, 2015: member
    Note for future: Don’t forget to adjust https://github.com/bitcoin/bitcoin/blob/e59d2a80f9167031521d882394a08b02fa9d0343/doc/bips.md
  10. TheBlueMatt force-pushed on Sep 25, 2015
  11. TheBlueMatt commented at 7:58 pm on September 25, 2015: member
    Updated to include doc/bips.md update (needed to rebase to do so)
  12. in doc/bips.md: in 91b2858cf3 outdated 
    15@@ -16,4 +16,4 @@ BIPs that are implemented by Bitcoin Core (up-to-date up to **v0.12.0**):
16 * [`BIP 61`](https://github.com/bitcoin/bips/blob/master/bip-0061.mediawiki): The 'reject' protocol message (and the protocol version bump to 70002) was added in **v0.9.0** ([PR #3185](https://github.com/bitcoin/bitcoin/pull/3185)).
17 * [`BIP 66`](https://github.com/bitcoin/bips/blob/master/bip-0066.mediawiki): The strict DER rules and associated version 3 blocks have been implemented since **v0.10.0** ([PR #5713](https://github.com/bitcoin/bitcoin/pull/5713)).
18 * [`BIP 70`](https://github.com/bitcoin/bips/blob/master/bip-0070.mediawiki) [`71`](https://github.com/bitcoin/bips/blob/master/bip-0071.mediawiki) [`72`](https://github.com/bitcoin/bips/blob/master/bip-0072.mediawiki): Payment Protocol support has been available in Bitcoin Core GUI since **v0.9.0** ([PR #5216](https://github.com/bitcoin/bitcoin/pull/5216)).
19-* [`BIP 111`](https://github.com/bitcoin/bips/blob/master/bip-0111.mediawiki): `NODE_BLOOM` service bit added, but only enforced for peer versions `>=70011` as of **v0.12.0** ([PR #6579](https://github.com/bitcoin/bitcoin/pull/6579)).
20+* [`BIP 111`](https://github.com/bitcoin/bips/blob/master/bip-0111.mediawiki): `NODE_BLOOM` service bit added, and enforced for all peer versions as of **v0.13.0** ([PR #6579](https://github.com/bitcoin/bitcoin/pull/6579) and [PR #6641](https://github.com/bitcoin/bitcoin/pull/6641).
    MarcoFalke commented at 8:02 pm on September 25, 2015:
    Nit: Missing closing bracket. ‘)’
    TheBlueMatt commented at 8:16 pm on September 25, 2015:

    Fixed.

    On 09/25/15 20:02, MarcoFalke wrote:

    In doc/bips.md #6641 (review):

    @@ -16,4 +16,4 @@ BIPs that are implemented by Bitcoin Core (up-to-date up to v0.12.0):

    Nit: Missing closing bracket. ‘)’

    — Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/6641/files#r40469809.

  13. TheBlueMatt force-pushed on Sep 25, 2015
  14. laanwj commented at 10:17 am on November 13, 2015: member
    There was discussion about this in #bitcoin today: some prefer this to be an option that would already be present in 0.12 but default to off, then the option removed in 0.13. (not necessarily saying this is a good idea, just thought I’d remark it)
  15. jameshilliard commented at 10:35 am on November 13, 2015: contributor

    some prefer this to be an option that would already be present in 0.12 but default to off

    This would certainly be useful for mining pools and other high-availability services that can’t afford to waste resources serving SPV clients. I don’t see any significant usability issues with allowing this right away since SPV clients will just look for more nodes if they get disconnected(the percentage of nodes using this option is likely to be quite small due to most using default settings). This appears to be a better solution than the current method of white-listing nodes by subver(which is in use by pools such as f2pool).

  16. jonasschnelli commented at 9:27 am on November 25, 2015: contributor
    Concept ACK. Although there is one problem: Misbehaving(pfrom->GetId(), 100) results in a ban, fDisconnect=true allows a node to reconnect. My tests have shown that SPV clients tend to directly reconnect, call filter commands, get disconnected, a.s.o. Which happens in connect/disconnect loops multiple times per second. Not sure what is best here, maybe it’s reasonable to ban the node for serval minutes after not respecting the NODE_BLOOM bit.
  17. dcousens commented at 9:56 am on November 25, 2015: contributor
    Concept ACK
  18. TheBlueMatt commented at 2:18 am on November 26, 2015: member
    @jonasschnelli I think fDisconnect is the only reasonable behavior we can do (unless we implement a temporary ban, but that wouldnt add much given that either way it results in disconnect after only one or two roundtrips). We certainly cant completely ban given that we really want the “wait, why is my node not syncing? Oh, there’s an update to my software” flow to work.
  19. Always disconnect old nodes which request filtered connections. 03cef3fb8f
  20. Update doc/bips.md for full NODE_BLOOM enforcement 632454f3c5
  21. in src/main.cpp: in 17144a14e7 outdated 
    4630-              pfrom->nVersion >= NO_BLOOM_VERSION)
4631+               strCommand == "filterclear"))
4632     {
4633         if (pfrom->nVersion >= NO_BLOOM_VERSION)
4634             Misbehaving(pfrom->GetId(), 100);
4635-        //TODO: Enable this after reasonable network upgrade
    MarcoFalke commented at 7:26 am on November 27, 2015:
    Needs rebase.
  22. TheBlueMatt force-pushed on Dec 1, 2015
  23. TheBlueMatt commented at 6:18 am on December 1, 2015: member
    Rebased.
  24. laanwj added this to the milestone 0.13.0 on Dec 3, 2015
  25. laanwj removed this from the milestone Future on Dec 3, 2015
  26. laanwj commented at 11:59 am on December 3, 2015: member
    Moved milestone from ‘future’ to ‘0.13’, we can always decide to postpone further if SPV client implementations aren’t ready by then.
  27. laanwj commented at 2:39 pm on February 2, 2016: member
    Needs rebase.
  28. laanwj commented at 4:11 pm on March 17, 2016: member
    Still needs rebase
  29. pstratem commented at 0:24 am on March 18, 2016: contributor
    @laanwj #7708
  30. jonasschnelli commented at 7:15 am on March 18, 2016: contributor
    Closing in favor of #7708
  31. jonasschnelli closed this on Mar 18, 2016
  32. DrahtBot locked this on Sep 8, 2021


TheBlueMatt laanwj dcousens Diapolo MarcoFalke jameshilliard jonasschnelli pstratem

Labels
P2P

Milestone
0.13.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-06-26 16:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me