Skip to content

UPnP #114

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.

Sign up for GitHub

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

Jump to bottom
Merged
TheBlueMatt merged 0 commits into from
Mar 21, 2011
Merged

UPnP #114

TheBlueMatt merged 0 commits into from
Mar 21, 2011

Conversation

@TheBlueMatt
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented Mar 11, 2011

Finally a seemingly proper patch for UPnP Port Mapping.
Note that this one still does not have proper support in the makefiles for anything but UNIX.

@gavinandresen
Copy link
Contributor

gavinandresen commented Mar 11, 2011

Cool! Thanks for being persistent.

Two comments:

The appropriate copyright notice should be added to uiproject.fbp and uibase.cpp; I think building the next release with UpNP support compiled in would be a good idea.

Having both -noupnp and -upnp options (and tri-state USE_UPNP) is confusing and will cause problems if bitcoin.conf files are copied around. I'd suggest something like (assuming we want upnp on by default for Windows):

bool fUseUpnp = GetBoolArg("-upnp");
#ifdef _WINDOWS
if (mapArgs.count("-upnp") == 0)
    fUseUpnp = true;  // upnp on if not explicitly turned off
#endif

@TheBlueMatt
Copy link
Contributor Author

TheBlueMatt commented Mar 11, 2011

Didn't realize Luke added -upnp in his commit, sorry. The original intention was to have USE_UPNP as a compile option as to whether or not one would compile with UPnP or not and a single -noupnp where UPnP defaults to on.
Currently the Windows makefile doesn't support any UPnP (some have suggested using Window's native UPnP library instead on Windows). I think it should be always on by default. In the end that can only be better for the network as it creates more peer diversity (and maybe eventually increasing MAX_OUTBOUND_CONNECTIONS). I will do that unless you disagree.

@gavinandresen
Copy link
Contributor

gavinandresen commented Mar 12, 2011

Please start a discussion on the forums about whether or not UpNP should be on or off by default; I think you will find strong feelings that it should be off by default among Linux folks.

@TheBlueMatt
Copy link
Contributor Author

TheBlueMatt commented Mar 12, 2011

Forum thread here: http://www.bitcoin.org/smf/index.php?topic=4392.0

@jgarzik
Copy link
Contributor

jgarzik commented Mar 15, 2011

Consensus seems to be "off by default"

@TheBlueMatt
Copy link
Contributor Author

TheBlueMatt commented Mar 16, 2011

That's the interesting thing about UPnP, for an individual, having UPnP on by default is bad - potential security problems down the line, increased traffic, etc. For the network as a whole, having UPnP on by default is good as it creates more nodes which accept incoming connections ie good for network diversity in the long run. (maybe even increased MAX_OUTBOUND_CONNECTIONS in the future)

@TheBlueMatt TheBlueMatt merged commit b7ebc66 into bitcoin:master Mar 21, 2011
glv2 referenced this pull request in glv2/peercoin Sep 7, 2014
@brossi
Merge pull request peercoin#114 from glv2/prob
cd5499b 
Fix the probability calculation of the minting tab and listminting RPC
sipa referenced this pull request in sipa/bitcoin Dec 4, 2014
@sipa
Squashed 'src/secp256k1/' changes from ad2028f..b0210a9
87bddb7 
b0210a9 Merge pull request bitcoin#135
ee3eb4b Fix a memory leak and add a number of small tests.
4d879a3 Merge pull request bitcoin#134
d5e8362 Merge pull request #127
7b92cf6 Merge pull request #132
0bf70a5 Merge pull request bitcoin#133
29ae131 Make scalar_add_bit test's overflow detection exact
9048def Avoid undefined shift behaviour
efb7d4b Use constant-time conditional moves instead of byte slicing
d220062 Merge pull request #131
82f9254 Fix typo
601ca04 Merge pull request #129
35399e0 Bugfix: b is restricted, not r
c35ff1e Convert lambda splitter to pure scalar code.
cc604e9 Avoid division when decomposing scalars
ff8746d Add secp256k1_scalar_mul_shift_var
bd313f7 Merge pull request #119
276f987 Merge pull request #124
25d125e Merge pull request #126
24b3c65 Add a test case for ECDSA recomputing infinity
32600e5 Add a test for r >= order signature handling
4d4eeea Make secp256k1_fe_mul_inner use the r != property
be82e92 Require that r and b are different for field multiplication.
597128d Make num optional
659b554 Make constant initializers independent from num
0af5b47 Merge pull request #120
e2e8a36 Merge pull request #117
c76be9e Remove unused num functions
4285a98 Move lambda-splitting code to scalar.
f24041d Switch all EC/ECDSA logic from num to scalar
6794be6 Add scalar splitting functions
d1502eb Add secp256k1_scalar_inverse_var which delegates to GMP
b5c9ee7 Make test_point_times_order test meaningful again
0b73059 Switch wnaf splitting from num-based to scalar-based
1e6c77c Generalize secp256k1_scalar_get_bits
5213207 Add secp256k1_scalar_add_bit
3c0ae43 Merge pull request #122
6e05287 Do signature recovery/verification with 4 possible recid case
e3d692f Explain why no y=0 check is necessary for doubling
f7dc1c6 Optimize doubling: secp256k1 has no y=0 point
666d3b5 Merge pull request #121
2a54f9b Correct typo in comment
9d64145 Merge pull request #114
99f0728 Fix secp256k1_num_set_bin handling of 0
d907ebc Add bounds checking to field element setters
bb2cd94 Merge pull request #116
665775b Don't split the g factor when not using endomorphism
9431d6b Merge pull request #115
e2274c5 build: osx: attempt to work with homebrew keg-only packages

git-subtree-dir: src/secp256k1
git-subtree-split: b0210a9
dexX7 added a commit to dexX7/bitcoin that referenced this pull request Jul 6, 2015
@dexX7
Merge pull request bitcoin#114
78fcd28 
a3619e2 Fix OSX icon & installer background (zathras-crypto)
deadalnix pushed a commit to deadalnix/bitcoin that referenced this pull request Jan 19, 2017
@sipa
Merge pull request bitcoin#114
9d64145 
99f0728 Fix secp256k1_num_set_bin handling of 0 (Pieter Wuille)
d907ebc Add bounds checking to field element setters (Pieter Wuille)
classesjack pushed a commit to classesjack/bitcoin that referenced this pull request Jan 2, 2018
Merge pull request bitcoin#114 from qtumproject/neil/pos-fixes
215d0c7 
Fix PoS diff retarget
0xartem referenced this pull request in Crowndev/crown-core Feb 4, 2018
Fixed popup style when adding a new SN/MN (#114)
0abb43c
cryptapus added a commit to cryptapus/bitcoin that referenced this pull request Oct 2, 2018
@cryptapus
Merge pull request bitcoin#114 from cryptapus/myriadcoin.master
7dfac65 
bump version to 0.14.4.0
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Feb 23, 2019
@tohsnoom
Merge pull request bitcoin#114 from tohsnoom/version142
7dfc759 
Adjusted testnet zerocoin block and applied 1.4.2 version
rajarshimaitra pushed a commit to rajarshimaitra/bitcoin that referenced this pull request Aug 5, 2021
@bolatovumar
Replace "Gossip protocol" with "gossip protocol"
0ffeb48 
address bitcoin#114
@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.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@TheBlueMatt @gavinandresen @jgarzik