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

Qt: Add network port input box to GUI settings #7107

Closed
wants to merge 8 commits into from

Conversation

@hsjoberg
Copy link
Contributor

@hsjoberg hsjoberg commented Nov 26, 2015

Related issue #7039

@tulip0
Copy link

@tulip0 tulip0 commented Nov 27, 2015

It's counterintuitive to most people that Bitcoin Core will refuse to use non-standard ports as anything but a last resort, does the option label need a GUI label or a confirmation popup to reflect this? I suspect a lot of people will be mystified otherwise that they don't get any incoming connections after changing a fairly innocuous looking option.

@@ -45,6 +45,8 @@ OptionsDialog::OptionsDialog(QWidget *parent, bool enableWallet) :
ui->threadsScriptVerif->setMinimum(-GetNumCores());
ui->threadsScriptVerif->setMaximum(MAX_SCRIPTCHECK_THREADS);

ui->networkPort->setValidator(new QIntValidator(1, 65535, this));

This comment has been minimized.

@jonasschnelli

jonasschnelli Nov 27, 2015
Member

We need a better protection to not run into a startup termination if the user did enter a invalid port. IIRC the QIntValidator still allows one to enter 0 as port number which will very likely result in a locked Bitcoin-QT (startup will stop because of a invalid port). Haven't tested although.
Maybe check: #7025

This comment has been minimized.

@tulip0

tulip0 Nov 27, 2015

This should probably see 1-1023 as invalid as well.

This comment has been minimized.

@luke-jr

luke-jr Nov 27, 2015
Member

@tulip0 That is not universally true. I think Windows allows non-admin to bind them, and on Linux there are capabilities to allow binding them.

@jonasschnelli jonasschnelli added the GUI label Nov 27, 2015
@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Nov 27, 2015

Concept ACK.

We just need to make sure, people can only enter reasonable/valid ports. Because if someone enters port 80, bitcoin-qt is very likely "bricked" (unless he start with root permissions!) until he manages to startup Bitcoin-Qt with a override port setting (through a shell with `-port=8333' or by changing bitcoin.conf).

Best would be to ask the user – during startup – if he likes to switch to the standard port if the given port can't be used (permissions or already used through another process).

@laanwj laanwj added the Feature label Nov 27, 2015
@hsjoberg
Copy link
Contributor Author

@hsjoberg hsjoberg commented Nov 28, 2015

Thanks for the feedback @jonasschnelli, I'll work on the things you pointed out.
I'll be available more and more in #bitcoin #bitcoin-dev etc as cocoBTC.

@fanquake
Copy link
Member

@fanquake fanquake commented Dec 24, 2015

@cocosoft Are you still working on this?

@hsjoberg
Copy link
Contributor Author

@hsjoberg hsjoberg commented Dec 26, 2015

@fanquake Yes, I do intend to continue working on this.

@tulip0
Copy link

@tulip0 tulip0 commented Dec 26, 2015

Again, this needs a warning that users will get essentially no incoming connections if they change the port. It's not initiative behaviour and will cause significant confusion, I'm not convinced that this option should even be in a configuration window like this to begin with.

 // do not allow non-default ports, unless after 50 invalid addresses selected already
if (addr.GetPort() != Params().GetDefaultPort() && nTries < 50)
    continue;

The address rumouring system really can't be used in conjunction with anything but a single static port.

@laanwj
Copy link
Member

@laanwj laanwj commented Jan 28, 2016

Concept ACK.

Agree that adding a warning (e.g. as mouseover) would make sense, and that ports should be restricted to valid ports (>1024 at least).

@hsjoberg
Copy link
Contributor Author

@hsjoberg hsjoberg commented Jan 28, 2016

Just FYI, I am still committed to this issue. I am still alive.

@laanwj, agreed.

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Feb 12, 2016

@cocosoft I've addressed the port number concern, and modified NetworkStyle to store regtest settings separately from testnet.

git pull https://github.com/luke-jr/bitcoin.git qtnetworkport
@hsjoberg
Copy link
Contributor Author

@hsjoberg hsjoberg commented Feb 18, 2016

@luke-jr, great, thank you for your help!

if (settings.value("nNetworkPort") != value) {
// If the port input box is empty, set to default port
if (value.toString().isEmpty()) {
settings.setValue("nNetworkPort", (quint16)Params().GetDefaultPort());

This comment has been minimized.

@hsjoberg

hsjoberg Feb 18, 2016
Author Contributor

Maybe I'm mistaken, but won't this code never be reached because of acc8f4e?

This comment has been minimized.

@luke-jr

luke-jr Apr 5, 2016
Member

You mean because the empty field is invalid input? I suppose so.

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Feb 19, 2016

Needs rebase.
I could continue this. I think it needs some additional "focus-lost" checks (also see how proxy port settings do work) and I could imaging a problem when setting the port to a port-number that is already in use (could prevent from starting Bitcoin-Qt).

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Apr 5, 2016

@ping Cocosoft. Are you interested to finish this?

@hsjoberg
Copy link
Contributor Author

@hsjoberg hsjoberg commented Apr 5, 2016

@jonasschnelli, I am interested in finishing this. I do understand however if you or someone else wants to take over because it has been idling for so long.

I'll try to be on the IRC channel this week.

@laanwj
Copy link
Member

@laanwj laanwj commented Apr 25, 2016

I could imaging a problem when setting the port to a port-number that is already in use (could prevent from starting Bitcoin-Qt).

Possibly in the case of the GUI it could be non-fatal if binding a port at startup fails. After warning the user there's no harm in continuing with the port closed.

This would make this change a lot less dangerous.

@fanquake
Copy link
Member

@fanquake fanquake commented Jun 22, 2016

@cocosoft Are you still planning on finishing this? This needs a rebase at least.

@hsjoberg
Copy link
Contributor Author

@hsjoberg hsjoberg commented Jun 22, 2016

@fanquake Yes, I haven't forgotten about this and I still plan to continue, I just need to find time. I fully understand if someone wants to finish it up though, as it has been delayed for so long.

@hsjoberg hsjoberg force-pushed the hsjoberg:qtnetworkport branch from 3d69602 Jun 26, 2016
@luke-jr
Copy link
Member

@luke-jr luke-jr commented Jun 27, 2016

Why did you add an enableOkButton and disableOkButton methods that are unused?

@hsjoberg
Copy link
Contributor Author

@hsjoberg hsjoberg commented Jun 27, 2016

I did not, actually you made them according to Git:

$ git blame -L 227,227 src/qt/optionsdialog.cpp
578064a6 (Luke Dashjr 2016-02-12 20:08:00 +0000 227) void OptionsDialog::enableOkButton()

I'll rebase again and fix that.

Or did you merge the qtnetworkport branch to the master branch of Bitcoinknots? If that's the case, I maybe could just make a new commit removed the unused code.

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Jun 27, 2016

That code wasn't part of my commit; somehow it got added in when you rebased. Perhaps an accident during a merge?

@hsjoberg
Copy link
Contributor Author

@hsjoberg hsjoberg commented Jun 27, 2016

You're right, sorry. It seems like Philip Kaufman wrote the functions back in 2013:

$ git blame -L 227,227 src/qt/optionsdialog.cpp
7e195e84 (Philip Kaufmann 2013-12-03 09:10:10 +0100 227)     setOkButtonState(false);
@hsjoberg
Copy link
Contributor Author

@hsjoberg hsjoberg commented Jun 27, 2016

I'm not sure where the functions come from, I can confirm (by a backup) that they existed before the rebase I did.

I'll remove them.

Edit: Right, I had a merge conflict regarding the functions during the rebase. My bad!

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 28, 2016
@fanquake
Copy link
Member

@fanquake fanquake commented Aug 31, 2016

This needs a rebase.

@hsjoberg hsjoberg force-pushed the hsjoberg:qtnetworkport branch to 4396a3a Sep 3, 2016
hsjoberg added 2 commits Sep 5, 2016
Lets you override an argument even if it has already been set,
in contrast to SoftSetArg/SoftSetBoolArg.
If the port (specified by -port or GUI setting) is already in use when
starting the GUI client, and it's not the standard port, ask the user if
they want to listen via the standard network port instead.
@hsjoberg hsjoberg force-pushed the hsjoberg:qtnetworkport branch to 1f37c87 Sep 5, 2016
@hsjoberg
Copy link
Contributor Author

@hsjoberg hsjoberg commented Sep 5, 2016

As per @jonasschnelli suggestion, I added a Question box on startup to ask if the user wants to use the standard network port if the specified port (whether it's from -port or in GUI) is already in use.

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Sep 20, 2016

Needs rebase and nit fixes.

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Oct 18, 2016

Closing due to inactivity. Happy to reopen once this has made progress.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
Lets you override an argument even if it has already been set,
in contrast to SoftSetArg/SoftSetBoolArg.

Github-Pull: bitcoin#7107
Rebased-From: f5267bf
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
If the port (specified by -port or GUI setting) is already in use when
starting the GUI client, and it's not the standard port, ask the user if
they want to listen via the standard network port instead.

Github-Pull: bitcoin#7107
Rebased-From: 1f37c87
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 18, 2017
Lets you override an argument even if it has already been set,
in contrast to SoftSetArg/SoftSetBoolArg.

Github-Pull: bitcoin#7107
Rebased-From: f5267bf
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 18, 2017
If the port (specified by -port or GUI setting) is already in use when
starting the GUI client, and it's not the standard port, ask the user if
they want to listen via the standard network port instead.

Github-Pull: bitcoin#7107
Rebased-From: 1f37c87
@hsjoberg
Copy link
Contributor Author

@hsjoberg hsjoberg commented Feb 23, 2017

Hi @jonasschnelli, I've rebased the branch upon master on cocosoft/bitcoin branch qtnetworkport.
I'm not sure what nits are still missing, but I would be happy to fix them.

I added a warning prompt on startup if the user tries to use a non-standard port that is already in use, and gives the option to use the standard port.

Best
Hampus

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Aug 29, 2017
If the port (specified by -port or GUI setting) is already in use when
starting the GUI client, and it's not the standard port, ask the user if
they want to listen via the standard network port instead.

Github-Pull: bitcoin#7107
Rebased-From: 1f37c87
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 13, 2018
If the port (specified by -port or GUI setting) is already in use when
starting the GUI client, and it's not the standard port, ask the user if
they want to listen via the standard network port instead.

Github-Pull: bitcoin#7107
Rebased-From: 1f37c87
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018
If the port (specified by -port or GUI setting) is already in use when
starting the GUI client, and it's not the standard port, ask the user if
they want to listen via the standard network port instead.

Github-Pull: bitcoin#7107
Rebased-From: 1f37c87
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 29, 2018
If the port (specified by -port or GUI setting) is already in use when
starting the GUI client, and it's not the standard port, ask the user if
they want to listen via the standard network port instead.

Github-Pull: bitcoin#7107
Rebased-From: 1f37c87
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Apr 22, 2019
If the port (specified by -port or GUI setting) is already in use when
starting the GUI client, and it's not the standard port, ask the user if
they want to listen via the standard network port instead.

Github-Pull: bitcoin#7107
Rebased-From: 1f37c87
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jan 5, 2020
If the port (specified by -port or GUI setting) is already in use when
starting the GUI client, and it's not the standard port, ask the user if
they want to listen via the standard network port instead.

Github-Pull: bitcoin#7107
Rebased-From: 1f37c87
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 9, 2020
If the port (specified by -port or GUI setting) is already in use when
starting the GUI client, and it's not the standard port, ask the user if
they want to listen via the standard network port instead.

Github-Pull: bitcoin#7107
Rebased-From: 1f37c87
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.