Bitmessage/PyBitmessage-2025-02-27
Archived
2
0
Fork 0
Code Issues 167 Pull Requests 56 Actions Packages Projects Releases 5 Wiki Activity

Add UPnP support. #810

Closed
lightrabbit wants to merge 1 commits from master into master
pull from: master
merge into: Bitmessage:master
Conversation 6 Commits 1 Files Changed 3 +206 -2
lightrabbit commented 2015-08-22 10:55:36 +02:00 (Migrated from github.com)
Copy Link

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.

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)
Copy Link

The product of that broadcasting?

The product of that broadcasting?
ghost commented 2015-09-16 10:16:48 +02:00 (Migrated from github.com)
Copy Link

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

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)
Copy Link

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.

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)
Copy Link

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)
Copy Link

@lightrabbit Any updates?

@lightrabbit Any updates?
PeterSurda commented 2015-11-20 23:11:50 +01:00 (Migrated from github.com)
Copy Link

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.
Reviewers
No Reviewers
Labels
Clear labels
bug
Something isn't working as it's supposed to
build
Related to building or build system
dependencies
Pull requests that update a dependency file
developers
The issue is relevant mainly for developers rather than users
documentation
duplicate
enhancement
New feature
formatting
Code or UI text formatting
invalid
legal
mobile
Frontend for mobile devices (kivy)
obsolete
packaging
performance
protocol
Protocol change
question
refactoring
regression
Some issue, fixed in the past, appears again
security
test
translation
usability
wontfix
No Label
enhancement
Milestone
No items
No Milestone v0.6.0
Projects
Clear projects
No project
Assignees
Clear assignees
PeterSurda
No Assignees
1 Participants
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Bitmessage/PyBitmessage-2025-02-27#810
No description provided.
Delete Branch "master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?