Add UPnP support. #810

2015-08-22T10:55:36+02:00

lightrabbit commented

2015-08-22 10:55:36 +02:00

(Migrated from github.com)

This feature request had been put out https://github.com/Bitmessage/PyBitmessage/issues/79#1 but never implemented before, then I tried to implement it.

peterwang1996 commented

2015-09-11 16:27:15 +02:00

(Migrated from github.com)

The product of that broadcasting?

ghost commented

2015-09-16 10:16:48 +02:00

(Migrated from github.com)

This might be a good idea, although I don't know enough about Upnp or the security implications to be sure.

lightrabbit commented

2015-09-17 11:18:22 +02:00

(Migrated from github.com)

I think there's no need to use 3rd-party UPnP library if we only create a port mapping on router with UPnP. We can simplely make a simple module only for creating port mapping.

PeterSurda commented

2015-10-16 09:36:26 +02:00

(Migrated from github.com)

Hello @lightrabbit ,

I have reviewed the patch, I haven't run it yet though. It looks like it may be working. Have you tried it yourself?

I don't like that if the port mapping fails, it will randomise it, and that it assumes the external and internal port are the same. I think that the user should be more in control of their own machine and would prefer if it only randomised the external port and kept the internal port fixed. I would add a new variable representing the external port to the shared.py, which initialises to the internal port and createPortMapping would set it once the mapping is established. Then you also need to change the assembleVersionMessage to use this external port instead of the one in the config file. Based on my understanding of the code, that should be sufficient to get it working.
I would also like to have some check for stack depth in createPortMapping, so that it cleanly fails if there's something wrong.
I would also prefer the ability to configure this option. It should default to off. So, I'd like you to add a variable to the config file that defaults to false, and a change to the QT Gui for this (preferably in the Listening Port section of the Network Settings tab you'll add a checkbox).

Once this change is in, I will review it again and test it as well.

Hello @lightrabbit , I have reviewed the patch, I haven't run it yet though. It looks like it may be working. Have you tried it yourself? - [ ] I don't like that if the port mapping fails, it will randomise it, and that it assumes the external and internal port are the same. I think that the user should be more in control of their own machine and would prefer if it only randomised the external port and kept the internal port fixed. I would add a new variable representing the external port to the shared.py, which initialises to the internal port and createPortMapping would set it once the mapping is established. Then you also need to change the assembleVersionMessage to use this external port instead of the one in the config file. Based on my understanding of the code, that should be sufficient to get it working. - [ ] I would also like to have some check for stack depth in createPortMapping, so that it cleanly fails if there's something wrong. - [ ] I would also prefer the ability to configure this option. It should default to off. So, I'd like you to add a variable to the config file that defaults to false, and a change to the QT Gui for this (preferably in the Listening Port section of the Network Settings tab you'll add a checkbox). Once this change is in, I will review it again and test it as well.

PeterSurda commented

2015-11-10 12:20:54 +01:00

(Migrated from github.com)

@lightrabbit Any updates?

PeterSurda commented

2015-11-20 23:11:50 +01:00

(Migrated from github.com)

Hey @lightrabbit

I tested it, fixed and merged it, it's now in mailchuck@b93308d7abedce21ae8ea4a4f5808be282f7abba

Hey @lightrabbit I tested it, fixed and merged it, it's now in mailchuck@b93308d7abedce21ae8ea4a4f5808be282f7abba

This repo is archived. You cannot comment on pull requests.

No reviewers