Skip to content

Bugfix: QA: Run tests with UPnP disabled #16560

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
Closed
luke-jr wants to merge 1 commit into from
Closed

Bugfix: QA: Run tests with UPnP disabled #16560

luke-jr wants to merge 1 commit into from

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Aug 6, 2019

Needed for builds configured with --enable-upnp-default

@luke-jr
Bugfix: QA: Run tests with UPnP disabled
be192e6 
Needed for builds configured with --enable-upnp-default
@fanquake fanquake added the Tests label Aug 6, 2019
@promag
Copy link
Contributor

promag commented Aug 7, 2019

Not quite a bugfix?

Anyway, without this change:

./configure  ... --enable-upnp-default
...
checking whether to build with support for UPnP... yes
checking whether to build with UPnP enabled by default... yes
...

make
...

And the following fails:

test/functional/feature_config_args.py

Tested ACK be192e6

@practicalswift
Copy link
Contributor

practicalswift commented Aug 7, 2019 •
edited
Loading

Thanks for finding and addressing this issue @luke-jr -- we should take the security of our users seriously.

Concept ACK

@promag If running the tests opens up ports in our users' firewalls needlessly then that definitely sounds like a bug to me :-)

maflcko
maflcko reviewed Aug 7, 2019
View reviewed changes
test/functional/test_framework/test_node.py
"-debugexclude=libevent",
"-debugexclude=leveldb",
"-uacomment=testnode%d" % i,
"-upnp=0",
Copy link
Member

@maflcko maflcko Aug 7, 2019

Choose a reason for hiding this comment

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

See the comment in line 84

common args are set in the config file (see initialize_datadir). The args here are only to set the datadir and debug log options.

Copy link
Contributor

@promag promag Aug 19, 2019

Choose a reason for hiding this comment

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

Agree, looks like all tests use the generated bitcoin.conf.

@promag
Copy link
Contributor

promag commented Aug 7, 2019

@practicalswift I mean it's not a bug to the end user.

@laanwj
Copy link
Member

laanwj commented Aug 8, 2019 •
edited
Loading

Yes, this seems the correct fix
ACK be192e6
(though yeah, better specify it in the configuration file instead)

@hebasto
Copy link
Member

hebasto commented Aug 10, 2019

ACK be192e6

Verified that after ./configure --enable-upnp-default && make
the test/functional/feature_config_args.py fails on master (e47e36c).

With this PR the test/functional/feature_config_args.py passes.

Nit: agree with the suggestion to move -upnp option to the bitcoin.conf:

bitcoin/test/functional/test_framework/util.py

Lines 289 to 305 in e47e36c

def initialize_datadir(dirname, n, chain):
datadir = get_datadir_path(dirname, n)
if not os.path.isdir(datadir):
os.makedirs(datadir)
with open(os.path.join(datadir, "bitcoin.conf"), 'w', encoding='utf8') as f:
f.write("{}=1\n".format(chain))
f.write("[{}]\n".format(chain))
f.write("port=" + str(p2p_port(n)) + "\n")
f.write("rpcport=" + str(rpc_port(n)) + "\n")
f.write("server=1\n")
f.write("keypool=1\n")
f.write("discover=0\n")
f.write("listenonion=0\n")
f.write("printtoconsole=0\n")
os.makedirs(os.path.join(datadir, 'stderr'), exist_ok=True)
os.makedirs(os.path.join(datadir, 'stdout'), exist_ok=True)
return datadir

@fanquake fanquake mentioned this pull request Aug 19, 2019
Merged
@fanquake
Copy link
Member

fanquake commented Aug 19, 2019

I've cherry-picked this change into #16646, but modified it to use bitcoin.conf.

@fanquake fanquake closed this Aug 19, 2019
laanwj added a commit that referenced this pull request Aug 19, 2019
@laanwj
Merge #16646: qa: Run tests with UPnP disabled
27ee0cc 
b168dd3 Bugfix: QA: Run tests with UPnP disabled (Luke Dashjr)

Pull request description:

  This replaces #16560 by adding `upnp=0` to `bitcoin.conf` rather than passing it to nodes.

  > Needed for builds configured with --enable-upnp-default

  You can test this change using:
  ```bash
  ./configure --enable-upnp-default && make -j6 && test/functional/test_runner.py feature_config_args.py
  ```

  on master the test will fail without this change.

ACKs for top commit:
  practicalswift:
    ACK b168dd3 -- diff looks correct

Tree-SHA512: e639dd480dda2cffa19a679018c4bd7e4bd4d0f5e3001d6b407b833e3c166bde98b201063e267b8e45f8a20b0d53ec8bc028bec806b2357f9a7ba314cc4e2d07
linuxsh2 pushed a commit to linuxsh2/dash that referenced this pull request Aug 11, 2021
@laanwj @linuxsh2
Merge bitcoin#16646: qa: Run tests with UPnP disabled
b7e4be7 
b168dd3 Bugfix: QA: Run tests with UPnP disabled (Luke Dashjr)

Pull request description:

  This replaces bitcoin#16560 by adding `upnp=0` to `bitcoin.conf` rather than passing it to nodes.

  > Needed for builds configured with --enable-upnp-default

  You can test this change using:
  ```bash
  ./configure --enable-upnp-default && make -j6 && test/functional/test_runner.py feature_config_args.py
  ```

  on master the test will fail without this change.

ACKs for top commit:
  practicalswift:
    ACK b168dd3 -- diff looks correct

Tree-SHA512: e639dd480dda2cffa19a679018c4bd7e4bd4d0f5e3001d6b407b833e3c166bde98b201063e267b8e45f8a20b0d53ec8bc028bec806b2357f9a7ba314cc4e2d07
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@promag promag promag left review comments

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@luke-jr @promag @practicalswift @laanwj @hebasto @fanquake @maflcko