Don't use third-party "what is my IP" services (rebase) #3461

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2013_12_rebase_thirdparty_ip changing 4 files +47 −148

laanwj commented at 1:11 PM on December 24, 2013: member

Rebased version of #3088. I've split the changes into two commits.

Note: this is not ready for merging yet, there are some known problems, see #3088 for discussion
laanwj commented at 10:21 AM on March 5, 2014: member

Rebased again.
bardiharborow commented at 1:18 AM on March 12, 2014: contributor

Nice!
laanwj commented at 7:10 AM on March 12, 2014: member

@bardiharborow did you test yet?
bardiharborow commented at 10:12 PM on March 12, 2014: contributor

@laanwj oh no, i don't have a copy of the blockchain. Far too large. =P
int03h commented at 10:19 PM on March 12, 2014: none

Give me your address I will mail it to you.. its quicker.
bardiharborow commented at 10:21 PM on March 12, 2014: contributor

@int03h, i'm referring to the fact that not everyone has Internet quotas that are big enough.
int03h commented at 10:28 PM on March 12, 2014: none

@bardiharborow I understand the pain. I am not fighting with you. The offer stands. I will mail it to you if you need it. It seems tarded but I bet I could get you the blockchain on a key drive faster than you could pull it down.
laanwj commented at 6:39 AM on April 4, 2014: member

This pull needs to be reorganized so that IsInitialBlockDownload is no longer moved to checkpoints (see @sipa's comments in #3999)
laanwj commented at 12:58 PM on May 28, 2014: member

Rebased, moved IsInitialBlockDownload back to main.cpp
in src/net.cpp:None in 09be9973a1 outdated
```
  11 | @@ -12,6 +12,7 @@
  12 |  #include "addrman.h"
  13 |  #include "chainparams.h"
  14 |  #include "core.h"
  15 | +#include "main.h"
```
sipa commented at 11:32 AM on June 1, 2014:

I took a ton of work to break this dependency; if at all possible, I'd like to keep main purely a client of net (and not the other way around).

laanwj commented at 2:12 PM on June 1, 2014:

I think that was the reason for moving IsInitialBlockdownload to checkpoints.cpp. I reverted that move because you didn't want it. I'm sure there is some other way of not reintroducing this dependency, but it looks like no one cares about this pull so it may be better to stop maintaining it.

Diapolo commented at 3:45 PM on June 2, 2014:

I think this is a fine and good pull, I didn't yet care to test it. Please don't close it :).

in src/net.cpp:None in 09be9973a1 outdated

1637 | @@ -1735,7 +1638,7 @@ void StartNode(boost::thread_group& threadGroup)
1638 |  
1639 |  #ifdef USE_UPNP
1640 |      // Map ports with UPnP
1641 | -    MapPort(GetBoolArg("-upnp", USE_UPNP));
1642 | +    if (!fNoListen && !IsLimited(NET_IPV4)) MapPort(GetBoolArg("-upnp", USE_UPNP));

Diapolo commented at 3:49 PM on June 2, 2014:

Is this the fix for "when using Tor we still map ports"?

in src/net.cpp:None in 09be9973a1 outdated

 198 | +    // us on a network we support and we are open to discovery and
 199 | +    // are listening on the default port, and we either don't know
 200 | +    // our address or seems to not be working we'll tell just that
 201 | +    // peer the address it sees for us.
 202 | +    CAddress addrLocal = GetLocalAddress(&pnode->addr);
 203 | +    if (fDiscover && pnode->addr.IsRoutable() && pnode->addrMe.IsRoutable() && pnode->addrMe != addrLocal &&

Diapolo commented at 3:52 PM on June 2, 2014:

That if-clause seems to do more than what you've written in the comment. Mind explaining it in a little more words to me? Why the limit to just the default port for example?

in src/net.h:None in 09be9973a1 outdated

  45 |  inline unsigned int SendBufferSize() { return 1000*GetArg("-maxsendbuffer", 1*1000); }
  46 |  
  47 |  void AddOneShot(std::string strDest);
  48 |  bool RecvLine(SOCKET hSocket, std::string& strLine);
  49 | -bool GetMyExternalIP(CNetAddr& ipRet);
  50 | +void AdvertizeLocalNode(CNode* pnode, bool fForce=false);

Diapolo commented at 3:53 PM on June 2, 2014:

Why is fForce needed and should we just leave out that "silly" default parameter (which I personally don't like ^^)?

[net] Don't use third-party "what is my IP" services.

This patch eliminates the privacy and reliability problematic use
of centralized web services for discovering the node's addresses
for advertisement.

The Bitcoin protocol already allows your peers to tell you what
IP they think you have, but this data isn't trustworthy since
they could lie. So the challenge is using it without creating a
DOS vector.

To accomplish this we adopt an approach similar to the one used
by P2Pool:  If we're announcing and don't have a better address
discovered (e.g. via UPNP) or configured we just announce to
each peer the address that peer told us.  Since peers could
already replace, forge, or drop our address messages this cannot
create a new vulnerability... but if even one of our peers is
giving us a good address we'll eventually make a useful
advertisement.

Rebased-From: a851bf84f6a2ff95bd86b23e55bb0647f5f47188
Rebased-By: Wladimir J. van der Laan <laanwj@gmail.com>

2672e726da

in src/net.cpp:None in 09be9973a1 outdated

 330 | -        return error("GetMyExternalIP() : connection to %s failed", addrConnect.ToString());
 331 | -
 332 | -    send(hSocket, pszGet, strlen(pszGet), MSG_NOSIGNAL);
 333 | -
 334 | -    string strLine;
 335 | -    while (RecvLine(hSocket, strLine))

Diapolo commented at 3:22 PM on June 12, 2014:

@laanwj As far as I see, you could remove RecvLine() also.

BitcoinPullTester commented at 8:33 AM on June 25, 2014: none
Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/p3461_2672e726dab23a5a19d2685f113c7096592156e7/ for binaries and test log.

This could happen for one of several reasons:
1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
4. The test suite fails on either Linux i386 or Win32
5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)
If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
Diapolo commented at 7:26 AM on July 1, 2014: none

I'm sad my comments don't get answered...
laanwj commented at 7:38 AM on July 1, 2014: member

@diapolo Right, I'm going to close this pull for now. There are lots of problems with it, and it doesn't rank very high on my priority list.
laanwj closed this on Jul 3, 2014
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#3088 [net] Don't use third-party "what is my IP" services.#3999 Move IsInitialBlockDownload from main to checkpoints #4682 Don't poll showmyip.com, it doesn't exist anymore