Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Dec 24, 2013

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
Copy link
Member Author

laanwj commented Mar 5, 2014

Rebased again.

@bardiharborow
Copy link
Contributor

Nice!

@laanwj
Copy link
Member Author

laanwj commented Mar 12, 2014

@bardiharborow did you test yet?

@bardiharborow
Copy link
Contributor

@laanwj oh no, i don't have a copy of the blockchain. Far too large. =P

@int03h
Copy link

int03h commented Mar 12, 2014

Give me your address I will mail it to you.. its quicker.

@bardiharborow
Copy link
Contributor

@int03h, i'm referring to the fact that not everyone has Internet quotas that are big enough.

@int03h
Copy link

int03h commented Mar 12, 2014

@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
Copy link
Member Author

laanwj commented Apr 4, 2014

This pull needs to be reorganized so that IsInitialBlockDownload is no longer moved to checkpoints (see @sipa's comments in #3999)

@laanwj
Copy link
Member Author

laanwj commented May 28, 2014

Rebased, moved IsInitialBlockDownload back to main.cpp

@@ -12,6 +12,7 @@
#include "addrman.h"
#include "chainparams.h"
#include "core.h"
#include "main.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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: a851bf8
Rebased-By: Wladimir J. van der Laan <laanwj@gmail.com>
@BitcoinPullTester
Copy link

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
Copy link

Diapolo commented Jul 1, 2014

I'm sad my comments don't get answered...

@laanwj
Copy link
Member Author

laanwj commented Jul 1, 2014

@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 laanwj closed this Jul 3, 2014
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants