Ignore checksum on 'version' messages. #816

pull sipa wants to merge 1 commits into bitcoin:master from sipa:lameversion changing 1 files +1 −1
  1. sipa commented at 3:36 AM on February 10, 2012: member

    Februari 15th, the protocol changes and 'version' and 'verack' messages get a checksum. However, some NAT routers change the IP address inside packet's payloads, causing the version message's checksum to become invalid.

    This commit disables the checksum check, so clients behind such routers can continue to connect after februari 15th.

  2. Ignore checksum on 'version' messages.
    Februari 15th, the protocol changes and 'version' and 'verack' messages
    get a checksum. However, some NAT routers change the IP address inside
    packet's payloads, causing the version message's checksum to become invalid.
    
    This commit disables the checksum check, so clients behind such routers can
    continue to connect after februari 15th.
    c5fa286111
  3. gmaxwell commented at 3:56 AM on February 10, 2012: contributor

    We know such routers exist— they've been observed to impact p2pool users (which uses a similar protocol with checksums on the version messages). But we don't know what brands or how common they are. It's a pretty bone headed thing to do.

    Rather than just ignoring it, it would probably be better to still log it but leave the connection up. Then we'd at least get some helpful logs.

  4. laanwj commented at 7:03 AM on February 10, 2012: member

    Huh this really surprises me. The IP replacement certainly exists, but used to be only for protocols like FTP and IRC that were known by the router, not in unknown binary data.

  5. gavinandresen commented at 5:00 PM on February 10, 2012: contributor

    I'd really like to know more about the root and scope of the problem-- was this "one or two people had problems with p2pool because they misconfigured iptables/netfilter" or is it "LinkSys routers automangle the first packet if its first N bytes look like it is an FTP session" ?

  6. gmaxwell commented at 5:13 PM on February 10, 2012: contributor

    Unfortunately we don't know the scope.

    We observed checksum errors on p2pool. Forrest investigated and found this to be the cause through some lucky reasoning. It appeared to be two distinct users, out of something like 150 using P2Pool at the time. So, I'd guess we could be looking at incidents rates between something like 1:10 and 1:100000 in the general bitcoin using population. There could also be port number differences that moot it.

    If this were just some new feature in 0.6 I wouldn't even bother talking about it— if we found it to be an issue in practice we'd just issue new software. But because its a network wide synchronized hardcut its more troubling. I think the bluematt pull request is prudent— it's better behavior even if this is a non-issue, and should cleanly improve this behavior— Perhaps we should keep this particular patch in our pocket in case things go pear shaped?

    Luke suggested we send an alert 24 hour ahead of the switch saying that an automated switchover was about to take place and advising people to visit bitcoin.org for the latest information. This sounds prudent to me, since if it does go poorly (due to the nat or other issues) we may not be able to send an alert to the impacted users.

  7. jgarzik commented at 3:08 PM on February 17, 2012: contributor

    IMO consider keeping existing (feb20 switchover) behavior as default. Add a "no-version-csum" (or "fucked-router") option. I am deeply suspect of routers that randomly rewrite binary packets transited in unknown (to the router) binary protocols.

    Would rather err on the side of stronger checksum protection, and only weaken that as absolutely necessary.

  8. gavinandresen commented at 9:51 PM on February 25, 2012: contributor

    Given the lack of problems in the last 5 days (besides alternative implementations that didn't code in the change), I think this is safe to close as "unnecessary."

  9. gavinandresen closed this on Feb 25, 2012

  10. ptschip referenced this in commit 51f6a947cc on Nov 1, 2017
  11. lateminer referenced this in commit 51e7b2c4b0 on Feb 23, 2019
  12. DrahtBot locked this on Sep 8, 2021
  13. DrahtBot added the label CI failed on Apr 2, 2023
  14. DrahtBot removed the label CI failed on Apr 2, 2023
  15. DrahtBot added the label CI failed on Apr 2, 2023
  16. DrahtBot removed the label CI failed on Apr 3, 2023
  17. DrahtBot added the label CI failed on Apr 5, 2023
  18. MarcoFalke removed the label CI failed on Apr 5, 2023
  19. DrahtBot added the label CI failed on Apr 11, 2023
  20. MarcoFalke removed the label CI failed on Apr 11, 2023

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: 2026-04-19 09:16 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me